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

[NOT READY] solve implemented/extended types with scope #2522

Open
wants to merge 3 commits into
base: master
from

Conversation

@iTakeshi
Copy link
Contributor

iTakeshi commented Feb 11, 2020

An interface or a class can implement/extend another interface/class using their "scoped" name, but in such case, the implemented/extended one can be referred by its "simple" name, e.g.

class A { class B {} }
class Foo extends A.B {
    B field_B;
}

To resolve the type B for field_B, ClassOrInterfaceDeclarationContext should search the type with regard to its "scoped" name, which currently only uses the "simple" name.

@matozoid

This comment has been minimized.

Copy link
Member

matozoid commented Feb 11, 2020

Thanks for all this work - I'm trying to find someone to review these PR's :-)

@iTakeshi

This comment has been minimized.

Copy link
Contributor Author

iTakeshi commented Feb 11, 2020

No need to rush :D
Thanks for this great OSS!

@MysterAitch

This comment has been minimized.

Copy link
Collaborator

MysterAitch commented Feb 11, 2020

In the situation where a type isn't being found, either the query is wrong or the search results are wrong (or some odd combination of both!).

This solution fixes the query (changing the input argument to .solveType()), while I'm wondering if we should instead be fixing the search results (fixing solveType() itself).

I've been digging into this a little during my lunch break so I've not had time to come to any meaningful conclusion on this yet, but I note this down now while I think about it and would definitely appreciate some additional inputs here to help sway one way or the other!

@MysterAitch

This comment has been minimized.

Copy link
Collaborator

MysterAitch commented Feb 11, 2020

Just to expand on this as I wasn't able to put it down! :D ...

Note in the screenshot below that for the wrappedNode of CompilationUnit, wrappedNode.getTypes() returns only the types that are immediately within the cu itself (note that none of the nested classes/interfaces are included).

For this reason, the name C2_1 is never matched (thus never solved).

So this raises the question: should getTypes also return the types (classes/interfaces/enums etc.) found within the children of the compilation unit? Should those be qualified etc? Should it be filtered to only include "visible" types (public/package-private)?

(as above, I'm not yet sold one way or another)

image

image

@iTakeshi

This comment has been minimized.

Copy link
Contributor Author

iTakeshi commented Feb 14, 2020

@MysterAitch Thank you for reviewing.

I think the current behavior of CompilationUnitContext that

returns only the types that are immediately within the cu itself

is correct, and I think resolving nested types is NOT a responsibility of a cu.
If the target name is qualified (i.e. contains a dot like C2.C2_1), cu delegates the type resolution to the outermost class declaration C2 (see

// Look for member classes/interfaces of types in this compilation unit. For instance, if the given name is
// "A.B", there may be a class or interface in this compilation unit called "A" which has another member
// class or interface called "B". Since the type that we're looking for can be nested arbitrarily deeply
// ("A.B.C.D"), we look for the outermost type ("A" in the previous example) first, then recursively invoke
// this method for the remaining part of the given name.
if (name.indexOf('.') > -1) {
SymbolReference<ResolvedTypeDeclaration> ref = null;
SymbolReference<ResolvedTypeDeclaration> outerMostRef =
solveType(name.substring(0, name.indexOf(".")));
if (outerMostRef != null && outerMostRef.isSolved() &&
outerMostRef.getCorrespondingDeclaration() instanceof JavaParserClassDeclaration) {
ref = ((JavaParserClassDeclaration) outerMostRef.getCorrespondingDeclaration())
.solveType(name.substring(name.indexOf(".") + 1));
} else if (outerMostRef != null && outerMostRef.isSolved() &&
outerMostRef.getCorrespondingDeclaration() instanceof JavaParserInterfaceDeclaration) {
ref = ((JavaParserInterfaceDeclaration) outerMostRef.getCorrespondingDeclaration())
.solveType(name.substring(name.indexOf(".") + 1));
}
if (ref != null && ref.isSolved()) {
return ref;
}
}
)

Thus, in my opinion, the target name (query) should be appropriately qualified.
Then, ClassOrInterfaceDeclarationContext (or JavaParserTypeDeclarationAdapter to whom it delegates the resolution) can handle the query.

(In my understanding, the JavaParserTypeDeclarationAdapter solves types with regard to the access modifiers, but I'm not completely sure if it can interpret the visibility under any circumstances. Probably there are some cases in which some types are resolved to types which should be invisible)

@iTakeshi iTakeshi force-pushed the iTakeshi:symbolsolver-classmember branch from 2deb040 to db9f5e6 Feb 14, 2020
@iTakeshi

This comment has been minimized.

Copy link
Contributor Author

iTakeshi commented Feb 14, 2020

I just found another edge case...
When a class implements/extends a type with the same simple name, the class should be prioritized to the implemented/extended type.

package main;
class A implements another.A {
  A field_a;  // <= this should refer main.A, but not another.A
}

I added a commit to solve this issue, but I'd like you to review if it's ok to have redundant statements among ClassOrInterfaceDeclarationContext and JavaParserTypeDeclarationAdapter...
https://github.com/javaparser/javaparser/pull/2522/files#diff-35fec71f6603467347504bdd005702cbR98-R101

if (this.wrappedNode.getName().getId().equals(name)) {
return SymbolReference.solved(JavaParserFacade.get(typeSolver).getTypeDeclaration(wrappedNode));
}

@iTakeshi iTakeshi changed the title solve implemented/extended types with scope [NOT READY] solve implemented/extended types with scope Feb 14, 2020
@iTakeshi

This comment has been minimized.

Copy link
Contributor Author

iTakeshi commented Feb 14, 2020

I found yet another edge case:

package main;
import yet.another.A;
class B implements another.A {
  A field_a;
}

In the above case, the type of field_A should be yet.another.A, but not another.A (that is, imported types are prioritized to implemented/extended types)

Now I start thinking that fixing the search algorithm instead of the query might be the better option.
I'll do another couple of experiments this weekend to figure it out.
Until then, I mark this PR as [NOT READY].

@iTakeshi

This comment has been minimized.

Copy link
Contributor Author

iTakeshi commented Feb 15, 2020

Digging into the JLS, but so far I couldn't find any reference for this behavior...
This is apparently related to #2195 / #2211, but do you guys know any resources to help this situation?

@matozoid

This comment has been minimized.

Copy link
Member

matozoid commented Feb 18, 2020

My references are mostly:

  • the JLS
  • what does the compiler or intellij do?
  • the JDK test suite
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.