Improve type resolution for duplicate names

[thejk]

When resolving types inside a ClassOrInterfaceDeclarationContext the
types found in extends and implements where searched before local
members. This caused problems for code such as:

import other.I;

class A implements I {
    interface I { }

    void foo(I i) { }
}

In the above the first parameter type for "foo" should be A.I and not
other.I.

Moving the code in ClassOrInterfaceDeclarationContext that searched
extends and implements first to JavaParserTypeDeclarationAdapter and
after searching local members showed the original problem of using
ClassOrInterfaceDeclarationContext when resolving extends and
implements as now A would instead implement A.I. Made a new fix for
that by introducing ClassOrInterfaceDeclarationExtendsContext which
only seaches in the parent context with one exception: type parameters.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@7a8796f). Click here to learn what that means.
The diff coverage is 97.058%.

@@            Coverage Diff             @@
##             master     #3012   +/-   ##
==========================================
  Coverage          ?   57.357%           
  Complexity        ?      2562           
==========================================
  Files             ?       616           
  Lines             ?     34280           
  Branches          ?      5796           
==========================================
  Hits              ?     19662           
  Misses            ?     12658           
  Partials          ?      1960           

Flag	Coverage Δ	Complexity Δ
AlsoSlowTests	57.357% <97.058%> (?)	2562.000 <4.000> (?)
javaparser-core	52.940% <ø> (?)	0.000 <ø> (?)
javaparser-symbol-solver	35.498% <97.058%> (?)	5328.000 <4.000> (?)
jdk-10	57.352% <97.058%> (?)	2562.000 <4.000> (?)
jdk-11	57.352% <97.058%> (?)	2562.000 <4.000> (?)
jdk-12	57.352% <97.058%> (?)	2562.000 <4.000> (?)
jdk-13	57.352% <97.058%> (?)	2562.000 <4.000> (?)
jdk-14	57.352% <97.058%> (?)	2562.000 <4.000> (?)
jdk-15	57.352% <97.058%> (?)	2562.000 <4.000> (?)
jdk-16	57.352% <97.058%> (?)	2561.000 <4.000> (?)
jdk-8	57.357% <97.058%> (?)	2560.000 <4.000> (?)
jdk-9	57.349% <97.058%> (?)	2562.000 <4.000> (?)
macos-latest	57.348% <97.058%> (?)	2562.000 <4.000> (?)
ubuntu-latest	57.342% <97.058%> (?)	2562.000 <4.000> (?)
windows-latest	57.348% <97.058%> (?)	2562.000 <4.000> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...l/contexts/ClassOrInterfaceDeclarationContext.java	88.461% <ø> (ø)	13.000 <0.000> (?)
...ymbolsolver/javaparsermodel/JavaParserFactory.java	85.849% <80.000%> (ø)	48.000 <0.000> (?)
...xts/ClassOrInterfaceDeclarationExtendsContext.java	100.000% <100.000%> (ø)	4.000 <4.000> (?)
...del/contexts/JavaParserTypeDeclarationAdapter.java	91.666% <100.000%> (ø)	36.000 <0.000> (?)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a8796f...15174d9. Read the comment docs.

[jlerbsc]

My opinion is that i don't think there is a need to create a new context (ClassOrInterfaceDeclarationExtendsContext) to implements this behavior.

[thejk]

Do you prefer extending ClassOrInterfaceDeclarationContext with a final field to switch behavior (ie only include type parameters or everything) instead?

[jlerbsc]

I think you are right, ClassOrInterfaceDeclarationContext must verify inner classes before implemented or extended types. How it should it be done is another question. For the moment I have no answer because there are many different cases that may arise.

[thejk]

Right. I tried a few different solutions that tried to reorder the lookup, or skip for specific case and similar.
But in the end I can't think of any case where the type in extends or implements are related to anything inside the ClassOrInterfaceDeclaration except of course type parameters. You can't do class A implements B { interface B {}} or class A implements List<B> { interface B {} } but there might of course be some case I'm not thinking of.

That's why I opted to add ClassOrInterfaceDeclarationExtendsContext to just search in the types that should be searched instead of trying to get ClassOrInterfaceDeclerationContext to return the right thing for extends and implements.

[MysterAitch]

I've not yet had time to verify the test fails before the fix then passes with it, or to otherwise look at it in detail, so don't have a strong view other way.

Would it be reasonable to add this as-is and then iterate on it to improve/polish it later?

[jlerbsc]

It's okay for me. Maybe we should then create an issue to remember that we eventually need to review this implementation

[jlerbsc]

I found another solution that also fixes the issue #2995. I'm not sure it solves all cases and maybe it will need to be improved but it covers current issues. I will push a PR for these cases.

[thejk]

Did a quick rebase on the fix for #2995.

Wondering tho, is JavaParserTypeDeclarationAdapter meant to go away or why are fixes added to ClassOrInterfaceDeclarationContext instead of fixing the adapter? I moved the #2995 fix to the adapter as part of this PR because with the lookup in extends/implements before local types moved to after local types (as needed for type lookup inside the class) the context.solveType() mainly just duplicated what javaParserTypeDeclarationAdapter.solveType() call will do again anyway.

[jlerbsc]

I'm not sure but it looks like JavaParserTypeDeclarationAdapter is set up to factorize common behavior between contexts related to enums, annotations, and classes / interfaces. It seems to me that it makes sense to keep the ClassOrInterfaceDeclarationContext class which implements the specificities related to classes / interfaces. For example, to my knowledge, there is no inheritance on enumerations, so it is inappropriate to have notions of inheritance in a class used by enumerations. Likewise it does not seem to me that the notion of inheritance on annotations has the same meaning as the inheritance of classes / interfaces (it is more a notion of applicability).

[thejk]

Ok, so JavaParserTypeDeclarationAdapter should only search common parts, I can see that but it looks like it leads to a fair amount of code duplication and it makes it trickier to keep the lookup priorities straight.
I think NodeWithTypeParameters, NodeWithImplements and so on are useful alternatives here.
Keep the priorities in one place (JavaParserTypeDeclarationAdapter) but only search for what the node has offer.

Maybe it's just me but ending up with a ClassOrInterfaceDeclarationContext#solveType implementation that does (current master):

1. Match my own name
2. Match implemented types
3. Match extended types
4. Match local types

JavaParserTypeDeclarationAdapter#solveType:
5) Match my own name
6) Match local classes (only one level and also matching in a slightly different way than (4))
7) Match type parameters
8) Match ancestor contexts

looks both errorprone and somewhat ineffective to me.
Specific points: (4) and (6) should do the same thing I think, as far as I can tell your fix for #2995 was an improvement that doesn't change behavior of enums or annotation declarations. Also I think that searching the local types like this is complicated enough that having multiple copies of it doing slightly different things in different places is a source of confusion and bugs.
(1) and (5) are obvious duplicates but trivial enough ...

But I'm new to this code, to get the right answer when resolving types inside a class I mainly just need to move (2) and (3) after (4) and add the limited context for when resolving implements and extends so if that is preferred I'll do that.

[jlerbsc]

I agree with your analysis. There are code duplications which are a potential source of bugs. I'm also not completely sure what the JavaParserTypeDeclarationAdapter class (versus ClassOrInterfaceDeclarationContext) should implement. Maybe @matozoid
@ftomassetti or @MysterAitch can clarify this point.

[MysterAitch]

@thejk - are you in a position to look at the merge conflict? 👍

If not, I'll take a look next week.

[thejk]

Sure, pushed a rebase.

[MysterAitch]

I also agree that it needs some organisation.

There are a few ways it could be done -- one is to continue using the adapter, another is to insert an AbstractTypeDeclarationContext class into the hierarchy with small granular methods that the subclasses can use (my current preferred option).

To give an example, I imagine that these small granular methods be defined (in the adapter or the abstract class):

• solveNameInInterfaceList
• solveNameInExtendsList
• solveNameInAncestors,

... then the context implementations would use those shared methods like this, making the search order explicit while also removing (some of the) duplication.

    @Override
    public SymbolReference<ResolvedTypeDeclaration> solveType(String name) {

        Optional<SymbolReference<ResolvedTypeDeclaration>> solvedType;
        
        solvedType = solveTypeInOwnName(name);
        if(solvedType.isPresent()) {
             return solvedType.get();
        }
        solvedType = solveNameInInterfaceList(name);
        if(solvedType.isPresent()) {
            return solvedType.get();
        }
        solvedType = solveNameInExtendsList(name);
        if(solvedType.isPresent()) {
            return solvedType.get();
        }
        solvedType = solveNameInAncestors(name);
        if(solvedType.isPresent()) {
            return solvedType.get();
        }


        // Symbol not found within this context
        // Search within the parent context
        return getParent()
                .orElseThrow(() -> new RuntimeException("Parent context unexpectedly empty."))
                .solveType(name);
    }

For now though, I'll merge this PR and any moves one way or another can be refactored within a new PR 👍