WIP detecting method return type from body #290
Conversation
- Now HaxeTypeUtils returns SpecificHaxeClassReference
- Detect type of new expression, if statement and ternary expression - Some cleanups
…missing anonymous types
Small HaxeTypeUtil cleanup
Check that package names starts by lower case characters
… in which package should it be fixing both package; and package <p>; forms
|
I just finished scanning all of the changes. That's a lot of code! So, can you explain to me why you need all of those new util classes? To me, it looks like most of this code already exists. Besides that, I'm don't see why you needed new models in separate classes from the ones already in place. I'm not wedded to the current structure, but I'd like to have a good reason for adding another layer on top of what's already there. I'm worried about the maintenance costs that it will entail. |
|
Ok. I have read all the comments. And here an explanation for most of them: I just have literally two days of experience with the code base. So lots of questions. Lots of unknown stuff. So I have two options to advance with my objectives:
Regarding with the structure, the classes and methods. I usually start getting things done, with small classes or sometimes more classes in a most of time random package, then when I am getting more feedback about how the code feels and the actual problem is being resolved I am able to refactor stuff, move things to a righter place, iteratively. So I don't mind too much about having a good structure at the very beginning, and let it evolve little by little. Also I have mixed this pull request: it includes method type detection with semantic errors. Regarding to HaxeSemanticAnnotator I will move Keys to a class. And will keep alll the getUserData/putUserData in a single class. Initially I had all in a single class, I was iteratively improving it. And I think doing what you said will improve it. Regarding to performance: initially I tried to do all the semantic at once. So I wanted to centralize in just one annotator. So I can do the scanning just once, and cache the results. Regarding to util functions/classes: that because what I written first. I don't have experience, so instead of being blocked by not being able to find a method, I just written it and waited for you to point it out :) Maybe even better to complete the pull request and you or @as3boyan who have much more experience to refactor it: remove repeated stuff, move things to the right place and so on. Regarding to models: I know that having the structure repeated is not too good. But I think they are separate things that are being mixed. I think that one thing is the AST and other thing the models themselves. Also there is the spacename congestion. When you access HaxeClass you get lots of random stuff not related to classes themselves, but inherited from PsiElement and derivates. Yeah, you probably will need the PsiElement, but that should be encapsulated or at least hidden after calling a getter. I usually like to have a good structure of classes instead of static methods, because it is easier for a noob to follow things (specially with java that doesn't have extension methods). If I have 100 classes with static helper methods, I probably miss some, but if I have a package entity and I press . and i see that there is a method called getAllClasses. It would be easier to follow. Also having a separate class representing the concept, I can store safely relations. Instead of iterating the AST or caching in a PsiElement node. I didn't want to deal with class names that could change while changing the bnf, or having somehow strange names MethodMixingImpl or something liek that (I don't remember) Obviously this all is a mere opinion. And since you handle this project I would do what you decide. I asked about creating this classes before, but again, I had to decide to be able to continue without waiting for replies. At this point I don't have methods that are not found in their counterparts, so it is easy to remove those models. Also I didn't do any automated testing. This is not how I usually work, but I wanted to experiment and to get experience with the codebase, and probably doing testing at this stage would being more noise for me than other thing. With a bit more experience I will do TDD as well. BTW: Could you do the merge? Then you can remove duplicate code by yourself. I think that would be much faster for both. I will check changes so I will know the existence of some code. Also you can remove Models if you wish. Also I request the merge of the pull request because I want to continue my work in other repository. I requested permission in my company, and I will continue the work here: https://github.com/Akamon/intellij-haxe Thank you for all your hard work! Your consistency made me to start collaborating :) And I expect we will get the best HaXe coding experience out there! Regards |
- Check that parent method visibility matches - Check when overriding that the parenet method doesn't have inline or static
Added fixers for visibility
Now all implemented semantic errors have fixers
…also added a HaxeDocument that will allow to modify the document easily without using static methods
| if (isPublic()) { | ||
| list.setModifierProperty(HaxePsiModifier.PUBLIC, true); | ||
| } | ||
| else { | ||
| } else { | ||
| list.setModifierProperty(HaxePsiModifier.PRIVATE, true); |
|
Lots of code, mostly ok, but some may need to be moved, I didn't get it yet, why you want to manipulate chars, HaxeCharUtils, didn't get it yet, also HaxeDocument and etc. We may also need to add additional unit tests for new functionality. (Detect type, etc...) |
|
HaxeCharsUtils is just a static helper that I named like that. It is not related with Haxe. I didn't found a method for determining if a char was an space or a tab, so I created it. I usually create/delete classes as I need them/or I find better ways or doing things or detect duplicated code which leads to refactorings. Regarding to HaxeDocument, it is a wrapper for Document. I wanted to avoid at most create static methods, because they are difficult to find. I created *Model classes that allowed me to create pretty good code in the SemanticAnnotator that can be explored accessing fields, instead of having lots of static factories or static methods scattered for all the code. So HaxeDocument is analogous to Document, but it is a non-static helper to replace text and doing editing stuff. It will probably evolve, but that was what first came in. So don't worry too much about the structure or the classes, but what is done. The how, will evolve and will be curated with time. Yeah, we need tests. But I don't know how to create them yet (it is a bit of magic, for what i have seen, like using the test method names without any annotation and so on). BTW: could you please merge, then refine a bit? As I mentioned I want to continue within another fork ( https://github.com/Akamon/intellij-haxe ) and for that it would be great for this to be completed. After merging feel free to move things, refactor, clean or change stuff. And when done, tell me and I will continue with your changes. My next objective is to check method calls signatures; I will probably evolve the type resolver yo process method blocks and to emit when neccessary errors, so that will lead to new kind of semantic error detections, and will improve type detection inside methods. So looking forward for the merge and your consolidating/fixing changes :) I will check them and I will try to do stuff as you decided. Regards |
No, I won't merge that pull request into master. Our process states that it can't come up into master until it passes all unit tests and has been reviewed. Maybe into another branch, but that's no different than you keeping it in another branch on your fork. (IMO, you shouldn't work directly on the master branch in your fork, either.) I will give a code review and point out where other code is already available so that you can fix it. I will answer questions and be as much help as I can. I won't refactor your work for you since I have my own deadlines. You will eventually have to spend time doing deep analysis any way. You are making magnificent progress (Yes!) at the expense of understanding; the most that you can hope is to amortize that expense over a longer period of time. The more we help you to delay, though, the more it costs the rest of us. We will help you to learn; we will not help you to avoid learning. I don't want you to get discouraged. You are making excellent progress. Your thinking is sound. You are going to be a great help to this team. We welcome you. As to the rest of your comments above, I understand what you are saying there. I think that you have made practical decisions. Once we remove redundant code, we will see which models need to continue to exist. I think that a package model is a good addition. Here is a bit of my architectural understanding: IDEA (well, the Java PSI and this plugin) does not store state in many places. It tries to avoid it and recalculate data when it needs it (and unfortunately, it does it a LOT). This is primarily because data can go stale quickly (the user is constantly changing the code, thus the interpretation of it). By always recalculating, they get away with fewer caches that have to be maintained and remove a lot of broadcast/listener infrastructure that would otherwise have to exist. It also makes multi-threading easier. For this same reason, it builds everything off of the PSI (which itself builds off of the AST). As for the class hierarchy being too deep, it seems that your complaint is that it primarily uses is-a relationships, rather than has-a relationships, and too many things are PsiElements. I'm not going to defend it, but I will point out that the focus of IDEA is to be an editor, and the type hierarchy reflects that; every entity is based upon a piece of text. If it really bothers you to have too many completions available, you can make a simple java completion filtering plugin that re-orders suggestions based upon their position in the class hierarchy, rather than alphabetically as it is now.
If what you are saying is that you want to move your current code between repositories, then, using the git command line, simply clone the destination repository, point to the source repository as a remote, and pull the changes. (git clone ; git checkout -b <workbranch_name>; git pull <old_repo> ) |
|
|
||
| @Override | ||
| public void annotate(PsiElement element, AnnotationHolder holder) { | ||
| PsiFile file = element.getContainingFile(); |
There was a problem hiding this comment.
This version of annotate is (obviously) called for each element in the PSI. However, you really only want to process it once, and the whole file at the same time. The file-based version of this interface, ExternalAnnotator, is more appropriate. ExternalAnnotators run immediately after the simple annotators.
There was a problem hiding this comment.
I didn't know the existence of this. But in the end I changed how I do this. I'm not caching it anymore or using userData. Because I'm not needing it yet, and maybe I won't need it. Also it prevented the normal behaviours: apply the annotator for all visible elements, then for the rest of the file.
But anyway, it is great to know that this exists. So in the case I ended requiring a full parsing (I will first try just single method parsing with normal annotators), I will use it.
| PackageChecker.check((HaxePackageStatement)element); | ||
| } | ||
| else if (element instanceof HaxeMethod) { | ||
| HaxeClass clazz = HaxePsiUtils.getAncestor(element, HaxeClass.class); |
There was a problem hiding this comment.
PsiTreeUtil.getParentOfType(this, HaxeClass.class);
There was a problem hiding this comment.
To that search for ancestors and not just its parent? I will check and change it. Thanks!
|
Ok, so I have read already all the comments. Thanks for your time! Regarding to the merge. I'm ok with that. I can just close the pullrequest, send these commit nodes to the other repository and open a new pullrequest. I wanted to avoid that, but no problem. Also my main concern was the code to getting outdated if the merge taken too much, and later having a very hard time resolving conflicts. What do you think about keeping its current features, maybe striping out some (comenting thme out so they can be enabled later), or accepting they are not covering all the cases, but being useful in some simple clases. Adding tests for all the current cases and then merging. Then I will continue making some features more generic and improve semantic analysis stuff. Is that ok for you? |
|
So I'm closing this PR, and continuing it in #297 |
Ok. So this is a first try on #288.
I'm still a noob on this codebase so probably, things are pretty bad done.