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

Detect function body type (retake) #297

Merged
merged 84 commits into from Jun 18, 2015
Merged

Conversation

@soywiz
Copy link
Contributor

soywiz commented Jun 6, 2015

No description provided.

soywiz added 22 commits Jun 3, 2015
- 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
- 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
@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Jun 6, 2015

I have added a FixPackage test after a lot of tries. Not sure if it is done the right way, so please any comments will be welcomed.

@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Jun 7, 2015

So I have added some more tests and added some nice checks and fixes.

parameter_and_return_type_checks

… fields non being constant
@EBatTiVo

This comment has been minimized.

Copy link
Contributor

EBatTiVo commented Jun 16, 2015

@soywiz A couple of test cases are failing. Can you check and fix those while Srikanth and I finish our reviews? (Should only be a day or so longer.)

[EDIT:] I should have pointed out that the Travis-CI output is the place to look.

@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Jun 17, 2015

I though travis-ci tests were broken for this project at this point. Glad to hear that. I will check and fix them asap.

@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Jun 17, 2015

It was a case-sensitive issue. Since I'm using windows and mac, which both have case insensitive file systems, I didn't triggered the issue. It was a bit strange since I created tests cases using the intelliJ which created files with the first upper cased letter. I have seen that getTestName allows you to specify whether you want the first letter to be upper case or not. So instead of changing names which usually causes problems when pulling in case insensitive file systems I'm going to try to refer to the files as they were cased already.

List<HaxeParameter> list = parameterList.getParameterList();
if (isExtension) {
list = new ArrayList<HaxeParameter>(list);
list.remove(0);

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jun 17, 2015

Contributor

The list that comes back from parameterList.getParameterList is already an ephemeral list. It does not need to be copied to be manipulated.

final PsiElement target = ((HaxeReference)callExpression.getExpression()).resolve();
final HaxeReference expression = (HaxeReference)callExpression.getExpression();
final PsiElement target = expression.resolve();
final boolean isExtension = expression.resolveIsExtension();

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jun 17, 2015

Contributor

Hmm. Took me a little while to figure out that isExtension everywhere really means is a static extension. is_Static_Extension would be a better name. Not a big deal, though.

@@ -46,8 +46,12 @@
public static final HaxeResolver INSTANCE = new HaxeResolver();
public static final String IMPORT_EXTENSION = ".hx";

public static boolean isExtension = false;

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jun 18, 2015

Contributor

This really isn't going to work with multi-threaded code. The annotators run on background threads while the users changes occur on the UI thread.

Perhaps, we could try to add the data to the candidateInfoArray and/or change the signature of the resolve function to add an object that the code would set. The trouble there is that the resolve caches already expect the current structure and they are part of the IDEA code.

I think that the best way is to duplicate the test that determines whether the isExtension flag should be set, and move it into the models (HaxeType or HaxeTypeResolver, whichever is more appropriate).

This comment has been minimized.

Copy link
@soywiz

soywiz Jun 18, 2015

Author Contributor

As a temporal solution we can use a ThreadLocal. So each thread has its own value. That should be safe and work too.

@@ -121,6 +119,12 @@ public PsiElement resolve() {
return resolve(true);
}

public boolean resolveIsExtension() {
// @TODO: DIRTY HACK! to avoid rewriting all the code!

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jun 18, 2015

Contributor

As I noted previously, we don't need the hack if we move/duplicate the test that determines an extension to the models. Then we can get rid of this hack and the instance variable.

This comment has been minimized.

Copy link
@soywiz

soywiz Jun 18, 2015

Author Contributor

I have done it with a ThreadLocal temporal solution, as I think I don't understand completely what you mean. We can change it later.

@@ -681,7 +694,7 @@ private void addChildClassVariants(Set<HaxeComponentName> variants, HaxeClass ha
}
}

private static void addUsingVariants(Set<HaxeComponentName> variants, @Nullable HaxeClass ourClass, List<HaxeClass> classes) {
private static void addUsingVariants(Set<HaxeComponentName> variants, Set<HaxeComponentName> variantsWithExtension, @Nullable HaxeClass ourClass, List<HaxeClass> classes) {

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jun 18, 2015

Contributor

What is the point of the new variable? All of the data going to the first set is also going to the second. Or is there supposed to be an 'if/else' statement surrounding lines 704 and 705?

This comment has been minimized.

Copy link
@soywiz

soywiz Jun 18, 2015

Author Contributor

Extension methods doesn't have the first argument. But you can call it statically or as extension method. So it depends on how are you calling it. I needed to differentiate those entries in context. And that was a easy way of doing that without too much refactoring.

return null;
}

static public <T extends PsiElement> T getChild(PsiElement element, Class<T> clazz, String text) {

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jun 18, 2015

Contributor

The name of this function is far too generic for what it does. It should be named something more like getNamedAssignableChild getNamedChild.

This comment has been minimized.

Copy link
@soywiz

soywiz Jun 18, 2015

Author Contributor

I changed it to getChildWithText. Semantically it is just text. It can be a name, a keyword a random text. You can change it later if you think it is better getNamedChild.

return null;
}

static public <T extends PsiElement> List<T> getChilds(PsiElement element, Class<T> clazz) {

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jun 18, 2015

Contributor

getChildren(), not getChilds().

This comment has been minimized.

Copy link
@soywiz

soywiz Jun 18, 2015

Author Contributor

Changed

@@ -0,0 +1,84 @@
/*

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jun 18, 2015

Contributor

For this file, I'm not sure why you wanted to create a new set of utility functions, when they already exist. Most of them already exist in UsefulPsiTreeUtil and PsiTreeUtil.

This comment has been minimized.

Copy link
@soywiz

soywiz Jun 18, 2015

Author Contributor

I have moved all methods to UsefulPsiTreeUtil.

By then I didn't want to congest the namespace. And probably I though it was related to recursive stuff. But moving static methods is an easy and fast refactor so it was not a big deal.

}

static public boolean isValidClassName(String name) {
return name.substring(0, 1).equals(name.substring(0, 1).toUpperCase());

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jun 18, 2015

Contributor

Just because the first letter is capitalized doesn't mean that the class name is valid. That's only one requirement. For example, it can't start with a number or an underscore, and it can only contain letters, numbers, and underscores.

This comment has been minimized.

Copy link
@soywiz

soywiz Jun 18, 2015

Author Contributor

Yep, we will have to improve that method. And since it is a static method it is easily unit-testable. Numbers are not valid syntactically so where it is used it won't be a problem. But yeah, we should fix that anyway and that would allow to use that in other places like renaming validation and so on.

public HaxeTypeOrAnonymous getAbstractUnderlyingType() {
if (!isAbstract()) return null;
PsiElement[] children = getPsi().getChildren();
if (children.length >= 2) {

This comment has been minimized.

Copy link
@EBatTiVo

EBatTiVo Jun 18, 2015

Contributor

This seems brittle. It should be coded to use PsiTreeUtil.getChildOfType(HaxeTypeOrAnonymous.class), rather then relying on the elements exact position in the tree.

This comment has been minimized.

Copy link
@soywiz

soywiz Jun 18, 2015

Author Contributor

PsiTreeUtil.getChildOfType is iterating over all the descendants? Of just its children?

Also this code fails when you do something like:

abstract Abstract<T>(OtherClass<T>) {
}

So this will require some more work.
I examined the AST and didn't find a proper way of doing that. I will have to check if checking for HaxeTypeOrAnonymous works, but probably not the best way of doing this. I think that changing the haxe.bnf for that part would be good.

@EBatTiVo

This comment has been minimized.

Copy link
Contributor

EBatTiVo commented Jun 18, 2015

@soywiz Hi Carlos, I've looked at everything except the model code; Srikanth was reviewing those, though I have added a comment or two. Can you please either fix the outstanding issues or explain why they are correct so I can make this merge? My plan was to do it today, but it takes a while to read so much code. :)

We were see-sawing back and forth between whether we wanted to eliminate the redundant models and just merge them with the underlying PSI classes or not. I know that Srikanth has a list of those that can be merged. I think we're going to go ahead with keeping them as they are at this point. Whether we continue to expand and use them or not depends upon whether they create more of a maintenance headache than they solve. Right now we're not sure of that cost.

soywiz added 5 commits Jun 18, 2015
…tter way doing this. Probably "resolve" returning a result object that could contain more information like this one.
@soywiz

This comment has been minimized.

Copy link
Contributor Author

soywiz commented Jun 18, 2015

I have reviewed your comments and done some much more granular commits easier to follow to fix some of them. I expect those changes are enough for the merge. Obviously there will be some more things to fix. New false positive errors and so on. But with some time we will be able to fix them as we spot them.

Regarding to models... My point was to try to keep things a little more separated. It was not going to be completely DRY in the other case since I had to change a interface and at least one implementation for each method I wanted to add. Also there are some concepts that have no direct psi representation. And having a non-symmetric API would be pretty strange. I think it is not too much trouble to maintain that. But obviously that is my opinion. So I would accept and follow what you decide. If you like we can try to keep them for a while and if the feedback we obtain from the code is that it is more a burden than anything else, I would remove them by myself so it is not a burden for you.

Thanks again for all your reviewing and my apologises for all the craziness in there :)

EBatTiVo added a commit that referenced this pull request Jun 18, 2015
Detect function body type.  Annotate errors regarding language types and introduce refactorings to fix them.
Introduces Haxe language model classes.
@EBatTiVo EBatTiVo merged commit 7fa56f3 into HaxeFoundation:master Jun 18, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@EBatTiVo

This comment has been minimized.

Copy link
Contributor

EBatTiVo commented Jun 18, 2015

@soywiz Hi Carlos, Now that this has been merged, can you please go through and close all of the bugs that were fixed as a result?

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.