Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved semantics #322

Open
wants to merge 46 commits into
base: master
from
Open

Improved semantics #322

wants to merge 46 commits into from

Conversation

@soywiz
Copy link
Contributor

soywiz commented Jun 30, 2015

This is the PR for the issue #321. It is not finished yet and has some regressions in tests that I have not fixed yet. Also misses tests for new functionality, but since it will take for me some more days to complete , and it has zillions of changes I create the PR already so you can start reviewing this.

soywiz added 30 commits Jun 26, 2015
WIP HaxeProjectModel and HaxePackageModel
Some refactorings and cleanups
Error reporting for imports not found
Autoreformat when updating text on HaxeDocument
Implemented CreateMethodFixer
…el API

Some fixes
Added try...catch support in expression evaluator
- getMember and getMembers now works with extends and implements
- Small refactorings
- Fix importing classes
- Implement throw, break and continue in expression evaluator
- Implement EReg literals in expression evaluator
- Added HaxeSuffixExpression to expression evaluator
- Fixed Void -> XXX typetag function type
- Supported anonymous typedefs
Improved method creation fixer supporting return and parameters types
- Create method inside a body now detect arguments and return type
- break and continue checks wether they are inside a loop
- Added naming tools
- Small fixes and improvements
…eturns

Moved all else if code in expression evaluator to methods to reduce cyclomatic complexity and make it easier to read
- Not require semicolon in expression points
- More semicolon detection cases
PsiElement[] children = methodElement.getChildren();
if (children.length == 0) return null;
PsiElement child = children[children.length - 1];
if (child instanceof HaxeTypeTag) return null;

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jul 8, 2015

Contributor

This breaks when the function does not have a code block, but merely contains an expression. Against the following code, the return value of getBodyPsi is the PsiToken for the terminating semi-colon

   public function test()
      (1);

And, on a further note, it looks like we have a parsing error because the following code parses as a function with a return statement, followed by a variable declaration, though it compiles just fine on try.haxe.org.

    public function test2(size:Int)
        return var a : Array<Int> = [for (i in 0...size) i];

In which case, the body of this function returns as the "return" keyword, which I'm sure breaks the typing code. :/ I'll file the issue.

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jul 8, 2015

Contributor

#329 filed.

}

@Nullable
public HaxeClassReferenceModel getHaxeClassReference() {

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jul 8, 2015

Contributor

Doesn't work with wildcards. Neither does getHaxeClass().

classes.add(file.getProject().rootPackage.accessClass("Std"));

for (HaxeImportModel importModel : getImports()) {
HaxeClassModel importedClass = importModel.getHaxeClass();

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jul 8, 2015

Contributor

This is broken for wildcard imports.

@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Jul 9, 2015

Sorry for the delay. I have been in a business trip, and I didn't have too much time. I will read all the comments today.

Regarding to the next release: This improves the return type detection in some methods, at least when outlining due to improved resolving.
Rest of the visible improvements were related to the body semantic analyzer.
But I didn't have enough time to finish it yet, so it still generates some false positives.

We can always disable the body semantic analyzer. Or even, just enable the missing semicolon part that could provide more value to developers without false positives.

Also completing the merge would reduce conflicts that if delayed.

But anyway you can skip this one if would interfere with your release.

Regards, and thanks for the hard reviewing!

@EBatTiVo

This comment has been minimized.

Copy link
Contributor

EBatTiVo commented Jul 9, 2015

No problems. I've only gotten about a third of the way through the code, though. So, I've still got a ways to go. This PR won't make the release. We can release again, once you've got this functionality polished up the way you want it.

@EBatTiVo

This comment has been minimized.

Copy link
Contributor

EBatTiVo commented Jul 21, 2015

@soywiz Hi Carlos, I haven't forgotten about this review. I've got a lot to do at the moment. And, you know 5K lines takes quite a while to review well. 8P I'll get back to it soon, but it may be next week.

Thanks for contributing -- and don't be discouraged!

@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Aug 14, 2015

Hello again. I have returned from my holidays. I will merge my branch with master and resume the work soon.

…emantics

Conflicts:
	src/common/com/intellij/plugins/haxe/ide/annotator/HaxeSemanticAnnotator.java
	src/common/com/intellij/plugins/haxe/model/HaxeClassModel.java
	src/common/com/intellij/plugins/haxe/model/HaxeParameterModel.java
	src/common/com/intellij/plugins/haxe/model/type/HaxeExpressionEvaluator.java
@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Feb 10, 2016

@EBatTiVo hi there. I'm here again. Let's finish this. There was something that prevented from merging it already? What I have to do? It would be enough to read comments and fix issues?

@EBatTiVo

This comment has been minimized.

Copy link
Contributor

EBatTiVo commented Feb 10, 2016

@soywiz Hi Carlos! As I remember, I had read about a third of the way through these changes. I had some comments that I thought should be addressed. I never got to finish the review, though, so there may be more things to think about.

In general, I think what I cared about most was that the models were caching information that could change underneath them. (Though, I admit, that is unlikely if they are very short-lived, as they are with the annotators.) That is, I REALLY prefer that the models do not hold any state, but that they make the underlying call to the PSI to get the current state when the data is requested. My main reasoning is that I like the models that you are developing; I find them much easier to conceptualize what is going on in the code. Therefore, I want to use them for other parts of the plugin which want to keep long(er)-lived objects: Things like the hierarchy panels, project structure, and the like.

I'll have to re-read this, though. Perhaps, in the interest of time, I'll not read so deeply.

@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Feb 10, 2016

Ok. I can disable or remove all the caches, and read all the comments and address them. Also it seems that this branch already has conflicts that should be handled, so that's another thing I can do. I will start this tomorrow.

@as3boyan

This comment has been minimized.

Copy link
Contributor

as3boyan commented Feb 11, 2016

Could you please list all of implemented features?

soywiz added 2 commits Mar 13, 2016
@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Mar 13, 2016

I have merged master already and I have setup the development environment again.
There are a few tests not passing, but mostly seems to work.

It has passed a lot of time since last time and I don't remember too much about this, and I won't have too much time because now I have other priorities. After this gets merged I will be able to contribute in the future with smaller commits.

@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Mar 13, 2016

BTW. Can we use Java8 or Kotlin already?

soywiz added 10 commits Mar 13, 2016
Fixed implementing interface with methods implementeed in an ancestor class
…inal fields

Following suggestions at: #322 (comment)
…ifiers)

Created HaxeModifier interface, and split old modifiers into extra + visibility, so we can treat visibility in its own semantic enum (HaxeVisibility)
Created HaxeModifiers abstract, so we can have two implementations: one with psi elements, and other with a plain list, but have common methods (that could be extension methods in kotlin or default methods in java8)

#322 (comment)
…semantic analyzer work.
@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Mar 14, 2016

Ok. So I have already fixed tests (I have disabled a couple of them that are not too important and will require more work on the semantic analyzer).

I really think that we should merge this already. I have put a lot of work on this. And I don't know wether I will be able to invest more time on this soon. Also fixing stuff commented here will require more time for me, and for you reviewing it.

The merge was awful, and took a lot of time, and there were things that were done again because this was not merged; so duplicated work.

From a pragmatic point of view I think we should merge this. Even if it is not perfect, you can continue the work from there and I get finally freed from this. I don't think I would handle a delay of another 6 months :) I won't remember anything as it happened this time and will need some time to get used to the code again.
Also that would allow to me to contribute with smaller stuff and smaller PRs, now that there is more infrastructure than before that I can reuse.

After the merge you can get all the code you don't like, like the caching stuff, and removing or disabling it. Improve it iteratively and fixing stuff, even if there are regressions that are not covered in unittests.

I won't continue with this PR so I can go on with other stuff and free my mind (I was always thinking about this, and waiting to have time to merge and complete this; after all, this supposed a lot of work).

So if it won't get merged like it is already, please @as3boyan or @eliasku, fork Akamon/intellij-haxe/ImprovedSemantics, close this PR and create a new one. I will delete that fork and the branch next week for the sake of my mind sanity :)

@EBatTiVo

This comment has been minimized.

Copy link
Contributor

EBatTiVo commented Mar 18, 2016

@soywiz, @as3boyan, @eliasku - I've taken a snapshot of this PR. Out of an abundance of caution, I would like to release 0.9.10 (probably Monday) before I pull this in. I'll take care of any more merges that occur before I do.

@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Mar 18, 2016

Ok. Sorry. I won't remove it just yet, but I do not want to continue updating this PR. I highly prefer to contribute to fixes in small PRs after this gets merged. I know of several things that are not working or that could be improved and I was able to fix or improve them, but didn't want to make this bigger or to create another PR without being able to use the stuff I added here.
So glad you are will take care of the merges. Let me know when it's merged! And I will help with bugfixes and regressions with smaller PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.