Fixed issue #2909 + improved general test

[deadlocklogic]

Fixes #2909.

[MysterAitch]

Removing this public method is an API change -- is there a way to avoid removing it?

Preferably we would deprecate the method first, with javadoc to explain that it does not work in some cases.

[deadlocklogic]

Of course no problem with it, I thought that it has no purpose anymore.

[MysterAitch]

Where there's an alternative preferred way to do it, deprecate it and explain what the alternative is please :)

(perhaps rewrite the content of the method to use the alternative technique too)

[deadlocklogic]

Ok, no problem. So I will revert the commits with method deletion, and add javadoc to the new method.

[MysterAitch]

The javadoc for both findContainingTypeDecl and findContainingTypeDeclOrObjectCreationExpr has been removed - is that deliberate?

[deadlocklogic]

Ah I see, maybe we can keep the removed method with its java docs, and add java docs to both new methods.

[MysterAitch]

Thanks @deadlocklogic ! ❤️

Seeing so many parameters and nested if/else if/else blocks raises an eyebrow.

I'm really not sure how else it could be written though, so don't really have a suggestion of my own!

What are your thoughts? :)

[deadlocklogic]

Your welcome man! Thanks for all your efforts and the community for maintaining this awesome library.
Well I couldn't find any nicer solutions, and guess what! looks like this section is suffering from same problem and needs similar patch, need to create tests first thought.

javaparser/javaparser-symbol-solver-core/src/main/java/com/github/javaparser/symbolsolver/javaparsermodel/JavaParserFacade.java

Lines 240 to 260 in 045575a

	public SymbolReference<ResolvedTypeDeclaration> solve(ThisExpr node) {
	// If 'this' is prefixed by a class eg. MyClass.this
	if (node.getTypeName().isPresent()) {
	// Get the class name
	String className = node.getTypeName().get().asString();
	// Attempt to resolve using a typeSolver
	SymbolReference<ResolvedReferenceTypeDeclaration> clazz = typeSolver.tryToSolveType(className);
	if (clazz.isSolved()) {
	return solved(clazz.getCorrespondingDeclaration());
	}
	// Attempt to resolve locally in Compilation unit
	Optional<CompilationUnit> cu = node.findAncestor(CompilationUnit.class);
	if (cu.isPresent()) {
	Optional<ClassOrInterfaceDeclaration> classByName = cu.get().getClassByName(className);
	if (classByName.isPresent()) {
	return solved(getTypeDeclaration(classByName.get()));
	}
	}
	}
	return solved(getTypeDeclaration(findContainingTypeDeclOrObjectCreationExpr(node)));
	}

I had few questions about few things in general, is the gitter chat quite active?

1. For instance consider the expression: typeName.this. When invoking calculateResolvedType() on its node, we get the
   ResolvedType which is the class that typeName.this refers to. Sweet!
   Now invoking resolve()on its node, we get the ResolvedTypeDeclaration which is the class where typeName.this was defined right? Or is it the typeDeclaration of the class it refers to? (I guess the answer should be the former because testing it says so but still not so sure).

2. Why SuperExpr doesn't have a resolve method too then, why it doesn't implement Resolvable? Very useful too instead of using manual Navigator.demandParentNode(node) and extra redundancy?

Actually I am finding many more room for improvements, and still there is a tons of TODOS/empty stubs in the solver module.
Nevertheless, pretty good library overall. Cheers!

[MysterAitch]

On behalf of all the folk who put in MASSIVE amounts of work to get it this far, we appreciate the kind comments! ❤️

I had few question about few things in general, is the gitter chat quite active?

Gitter seems to ebb and flow. We all try to chip in when we can. Personally I struggle to keep up when things are threaded (especially on the mobile app!). For substantive questions that need a bit of investigation/thought, the issue tracker here is probably best -- and for shorter/simpler questions, try gitter 👍

As for the numbered questions:

1. I'm not quite sure what the question is. Speaking generally, resolve() points at the declaration (of the field/variable/method), and calculateResolvedType points at the return type of the method or the type of the variable. This is most evident for methods, where resolve() resolves the method declaration, and calculateResolevdType() resolves to the return type of the method. Hopefully that's enough to either confirm your thoughts or to give you ammo to come back with another question :)

2. I don't have an immediate answer to that. If it's reasonable to add in, chances are that it just hasn't been thought about / asked for! Offhand, super can refer to both an object and a method (e.g. super()), but so can this so no objections from me on that front.

Actually I am finding many more room for improvements, and still there is a tons of TODOS/empty stubs in the solver module. Nevertheless, pretty good library overall. Cheers!

Yep, the parsing side of things is pretty solid!

Symbol solving is the impressive work of @ftomassetti who built it nearly single handedly!! Since bringing it into the main project a bunch of us have helped to build it up and fill in bits here/there ( @jlerbsc has been doing a LOT of work with JavaParser recently), but it's a tricky beast with lots of moving parts -- necessarily so, given that references can be resolved via reflection/jars/source code etc, each with slightly different features/behaviours!

All help to put together test cases and fix any gaps in coverage/functionality is very much welcomed! 👍

[deadlocklogic]

Yes of course, and since I am building a java transpiler based on this library I guess I can provide huge feedback on the solving part especially because the parsing part so far is pretty robust. I can even create a dedicated folder for all my tests which will include all combinations of syntax nesting/order I will ever face probably.

[deadlocklogic]

So just to be 100% sure: following your reasoning on my first question, when invoking resolve() on ThisExpr this test should pass right? Because it refers to OuterClass.
assertEquals("test.Program.OuterClass", expr.resolve().getQualifiedName());

package test;
class Program {
    class OuterClass {
        class InnerClass {
            InnerClass() {
                OuterClass outer = OuterClass.this;
            }
        }
    }
}

In current implementation it doesn't, in fact it is as broke as calculateResolvedType() and I believe SuperExpr is even so.
assertEquals("test.Program.OuterClass.InnerClass", expr.resolve().getQualifiedName()); passes.

Maybe because using a typeName before these expressions is not so common anyway unless there are identifier names clashing and we need finer control to invoke enclosing class members.

[MysterAitch]

Short answer: I don't know!

With the disclaimer that this is all from memory (I'm on mobile)...

I think that this (when used as a field access expression like in your example - either on its own this or with a prefix Class.this) is a case where both methods resolve to the same thing -- i.e. both resolve to the class declaration. Logically, the same would apply to super.

If this or super are a method call expression referring to the constructor method (i.e. this() or super()), then resolve would resolve to the specific constructor method (i.e. determined by the parameters passed) while the calculatedReturnType would resolve to the class declaration.

Not sure how this applies to the test cases (I'll take another look in a while when I'm back on computer), but hope this helps to clarify things! :)

[deadlocklogic]

Need to bring out the big guns on this one! Maybe some other time I will ask it on gitter if I couldn't find out myself. Thanks anyway!

[MysterAitch]

I added this comment on #2931 so I'll leave it here too for comment.

I was thinking about this for #2928 too, which also uses endsWith...

Presumably SomeOtherObject would match endsWith("Object"), meaning that something more sophisticated like splitting it then iterating right to left would be needed in order to do this robustly?

[deadlocklogic]

The thing is that we use endsWith as first test and then check for equality. I first thought of splitting and testing the concatenation, but it generates too much garbage allocation for no reason. My solution is really simple and not too complicated and does the same exact task, just we need to keep in mind to reset the search if a TypeDeclaration (be it class/enum/interface...) from the bottom-up hierarchy is an ObjectCreationExpr or LocalClassDeclarationStmt because they are anonymous and can't be fully qualified before compile time, there are synthetic.
I leave it up to you of course to choose what suits the library, but for now I am happy with my patch as I can get things going because I was kinda stuck. But I deeply think it is foolproof because I couldn't find a counter-test.
Just one little remark, is that a solution doesn't have to be over engineered/sophisticated as long as it works. This simple problem took almost 5 PRs and still breaks talking about robustness 😉

[deadlocklogic]

I will close this PR hoping #2931 fixes the issue as it looks like.