Test case and fix for #4672

[datho7561]

What it does

Adds a (failing) test case for #4672

How to test

Through CI

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[fedejeanne]

Thank you for providing this test! I have some minor comments on it.

Also, I had to test this locally. I didn't see your test (or any tests, actually) in the CI pipeline. May be an infrastructure problem?

[fedejeanne]

Earlier Java versions should be tested too. @stephan-herrmann wasn't there a test class that ran several tests with different Java versions? I can't find it anymore.

[stephan-herrmann]

Actually this line causes error The type java.lang.Object cannot be resolved. It is indirectly referenced from required .class files because no classpath variable "JCL21_LIB" has been initialized.

It looks like the line was copied from Java21ElementTests, which shows the same bug, actually two bugs:

• compliance and classpath should use the same version
• initialized variables have one more underscore, like "JCL_25_LIB"

Interestingly, this error has no impact on the actual bug being witnessed by the test.

[fedejeanne]

Oh, thank you for the info. I didn't actually experience that error while running the test locally.

What do you recommend to use in this case? Does the test need to explicitly test for several Java versions or is it enough to test for 1 version (which one)?

[datho7561]

I'll update the library and compliance to be the same version.

initialized variables have one more underscore, like "JCL_25_LIB"

Most other places I see references to JCL*_LIB it only has one underscore

[stephan-herrmann]

Oh, thank you for the info. I didn't actually experience that error while running the test locally.

Just put a breakpoint in org.eclipse.jdt.internal.compiler.problem.ProblemHandler.handle(int, String[], int, String[], int, int, int, ReferenceContext, CompilationResult) (I constantly have one breakpoint right after case ProblemSeverities.Error), to see the error (still).

It has no obvious effect on the test, but adds unnecessary noise.

[stephan-herrmann]

Okay, I'll address this for the test case I added.
Do you think it's important to fix the other occurances (in a separate PR?)

Not a big deal, so I would actually appreciate this little cleanup even as part of the current PR. I'll make a small note about it in the final commit comment when merging.

As we are in collaboration mode here, please make any changes now as separate commits, rather than history re-writing, thanks.

You didn't see these, @datho7561 ?

[datho7561]

oops no my bad

[datho7561]

I've added the fix for Java21ElementTests, sorry about rewriting history on the PR earlier.

[stephan-herrmann]

I've added the fix for Java21ElementTests,

thanks

sorry about rewriting history on the PR earlier.

It's OK, people have different habits.

To explain my request: I personally prefer to see all incremental changes as a fix evolves. It helps my style of reviewing. And unfortunately, github is inferior (compared to gerrit, gitlab) in showing these differences when force-push is involved.

[stephan-herrmann]

Unfortunately there's more confusion regarding Java21ElementTests:

• Apparently it was introduced when Java 21 introduced instance main methods as a preview.
• Meanwhile that feature has matured (no more preview) but it requires source level 25

@datho7561 Do you mind further cleaning up in this direction?

• rename the class to Java25ElementTests
  • check if it is still included at the correct location in the suite
• configure it to use "JCL_25_LIB" and "25"

[stephan-herrmann]

from #4672 (comment) :

Some options:

1. Could LE.askForType() know about the "purpose" of why a type is being requested?
   • If that "purpose" is supertype lookup, we should not stash and restore
   • All other purposes continue to use stash and restore

This option looked simplest to me.

By integrating the new flag also in GlobalStateMemento we support arbitrary interleaving between two modes, supertype lookup, other type lookup:

• while ClassScope.connectHierarchy is active do not clear typesBeingConnected, but still use stash / restore for the sake of the new flag.
• yet, in the middle of supertype lookup, any resolving during accept() should not blindly assume we're still in hierarchy lookup, achieved by temporarily resetting the new flag.

I put the new flag into the ROOT_ONLY discipline, meaning: we don't maintain separate flags per module.

Perhaps even typesBeingConnected should always be referenced via root, but then a cycle across module boundaries would first require an illegal cycle among modules.

[datho7561]

Looks like that fix worked; I'll push the change to the variable in the test now.

[stephan-herrmann]

Thanks @datho7561 & @fedejeanne for great collaboration!