fix: Respect shadowCache in JavaReflectionTreeBuilder for enclosing classes

[I-Al-Istannen]

After two way too long debugging sessions, I ended up with this one line fix for #4698. The ReflectionTreeBuilder unconditionally scanned the enclosing class, bypassing the shadow class cache. This resulted in two different CtClass instances for ArrayList, causing the role reference equality check to fail.

The problem here is, that scanning a class only results in that class to be added to the shadow cache - but no nested classes. The ImportConflictDetector, added by default as a preprocessor for the pretty printer, then caused a nested class to be scanned (which was never added to the shadow cache), which promptly re-scanned the enclosing ArrayList. This new instance was then added to the package, overwriting the original class present in the shadow cache. Role lookups were performed on the original cached instance, which now no longer was present in any package -- the lookup in the parent failed and a null role was returned.

Scanning nested classes eagerly causes FIVE (5!) instances of the ArrayList class to be created in the test from this issue (#4703). Just performing the lookup where I placed it in this PR actually removes inconsistencies in getNestedTypes as well, as the member lookup somebody inserted there now actually does what it claims to do. I am not sure why nested types aren't directly added to the shadow cache in the first place (as they are already built and added as children to the enclosing CtType), but at least it is now consistent.

Debugging for this PR highlighted that the current shadow model was a bad idea IMHO. We really should have opted for a set of lazily-caching proxy classes that delegate to reflective lookups. This would allow the model to be immutable, prevent duplicate work more easily and only perform expensive reflection lookups when they are actually needed.

The current situation can therefore IMHO be summarized as

Fixes: #4698

[I-Al-Istannen]

Apparently this is a breaking change judging from the test failure. Here is the test case:

	@Test
	public void testInnerClass() {
		// contract: JavaReflectionTreeBuilder works on internal named classes
		Launcher launcher = new Launcher();
		launcher.addInputResource("src/test/java/spoon/support/visitor/java/JavaReflectionTreeBuilderTest.java");
		launcher.buildModel();
		CtType ctType = launcher.getFactory().Type().get(Diff.class);
		assertEquals("Diff", ctType.getSimpleName());
		assertEquals(false, ctType.isAnonymous());
		assertEquals(false, ctType.isShadow());

		Class<?> klass = ctType.getActualClass();
		assertEquals("spoon.support.visitor.java.JavaReflectionTreeBuilderTest$Diff", klass.getName());
		assertEquals(false, klass.isAnonymousClass());

		CtType<?> ctClass = new JavaReflectionTreeBuilder(launcher.getFactory()).scan(klass);
		assertEquals("Diff", ctClass.getSimpleName());
		assertEquals(false, ctClass.isAnonymous());
		assertEquals(true, ctClass.isShadow());  // is actually FALSE
		assertEquals("element", ctClass.getFields().toArray(new CtField[0])[0].getSimpleName());
	}

The class added as an input resource contains the inner class Diff and scan(klass) now looks up the class in the model, finding the correct source-code-based CtType instance. It then fetches its inner class and returns a proper class that has seen the light. The test expects the builder to throw away everything and rescan the whole class as a reflective class.

I am not sure I agree that this is the behaviour we want, but if we do, this PR won't quite cut it and I am not sure how the underlying problem can be solved. Not having this behaviour forces the tree builder to re-scan everything every time, causing the problem this PR tries to solve.

[flacocobot]

flacoco flags 4 suspicious lines as the potential root cause of the test failure.

[MartinWitt]

The lookup fix appears to be fine to me, but as you stated, this is technically breaking behavior. Currently, even if we know the CtType, the JavaReflectionTreeBuilder always returns shadow classes. The scan method for JavaReflectionTreeBuilder, on the other hand, only has the contract transforms a java.lang.Class into a CtType (ie a shadow type in Spoon's parlance). In my opinion, getting a well-formed CtType if the type is known is preferable to getting a shadow CtType with missing information for all users.
As this is a breaking change @slarse and @monperrus, what are your thoughts?

[monperrus]

To me, this new behavior is strictly better and clearly fixes a bug. (recall that all fixes are breaking if some clients rely on the previous behavior)

[monperrus]

plus, a meme in a PR description 💯

[I-Al-Istannen]

To me, this new behavior is strictly better and clearly fixes a bug. (recall that all fixes are breaking if some clients rely on the previous behavior)

I wasn't sure as the test seems to explicitly test the old behavior. I adjusted the contract, test, and made a new one to test inner classes again.

Qodana failing is unfortunate, but I am not sure I can fix it. Maybe its cache is buggy again. Rerunning it fixed it...

[MartinWitt]

Thank you, @I-Al-Istannen for this valuable fix and the great meme.