Implement per-source-folder module-info for multi-release compilation

[laeubi]

See issue

• MR compilation should respect release-specific module-info #4268

@stephan-herrmann this now works for the compilation part but not for the reconciler yet, where I need to check where to inject the info best.

I see two options here:

1. We merge this one and fix the reconciler in another PR (might also be easier to review)
2. We wait until i fixed that as well and delay this a bit more

[laeubi]

The reconciler case should now work as well (testcase is missing).

[stephan-herrmann]

Did you see the compile errors?

[laeubi]

Now there are API errors, I think I'll wait until the next stream opens to fix those...

[laeubi]

Now there are API errors, I think I'll wait until the next stream opens to fix those...

It seems I totally missed this - sorry for that. @stephan-herrmann should we go on with this to merge it for the next release stream? Was you able to test if this is fixing your JPMS issues you have seen?

[laeubi]

@stephan-herrmann finally build is green can you review/merge?

[laeubi]

@stephan-herrmann a friendly reminder for review, it would be good to have this in the M1 to allow extended testing period.

[stephan-herrmann]

@stephan-herrmann a friendly reminder for review, it would be good to have this in the M1 to allow extended testing period.

After a large burst of work on type inference I'm back on it now.

[stephan-herrmann]

For some call paths that are not yet release aware I filed #5149.

The following observation, however, might be good to fix in this PR:

Using the project from the new test case: Go to any of the Test classes, then select any of the Window or Element type references. Try hover, or Go To Declaration (F3 or ctrl+click) and observe the same inconsistency in all versions:

• Window is correctly resolved
• Element offers a selection of 4 types named "Element"

[stephan-herrmann]

@laeubi I tried to polish the javadoc for new API, please double check if anything is amiss now.

For the build failure I'm sure you understand better than me what is going on here (some baseline checks).

[laeubi]

Using the project from the new test case: Go to any of the Test classes, then select any of the Window or Element type references. Try hover, or Go To Declaration (F3 or ctrl+click) and observe the same inconsistency in all versions

Is this directly related to classes changed here / module support in general? If not I would prefer to better make smaller PRs than include more than the minimal required to fix one issue at a time.

[laeubi]

I tried to polish the javadoc for new API, please double check if anything is amiss now.

I added one comment to a change that looks suspicious the rest seems fine so far.

For the build failure I'm sure you understand better than me what is going on here (some baseline checks).

I think I likely need to re-base the PR if you are fine with that

[stephan-herrmann]

Using the project from the new test case: Go to any of the Test classes, then select any of the Window or Element type references. Try hover, or Go To Declaration (F3 or ctrl+click) and observe the same inconsistency in all versions

Is this directly related to classes changed here / module support in general? If not I would prefer to better make smaller PRs than include more than the minimal required to fix one issue at a time.

This problem can only be reproduced in an MR project with module-info.java, making the feature introduced here appear incomplete / broken. I haven't analyzed where in the code things go wrong.

[stephan-herrmann]

I think I likely need to re-base the PR if you are fine with that

If it's a pure rebase with no further changes hidden inside that's fine.

[laeubi]

If it's a pure rebase with no further changes hidden inside that's fine.

I triggered the rebase now, lets see if it helps already...

This problem can only be reproduced in an MR project with module-info.java, making the feature introduced here appear incomplete / broken.

I can try to take a look but strictly speaking this PR was about compilation so possibly still better to have it in an extra PR as it more seem to surface at the UI.

[laeubi]

Openable.codeSelect (the entry point for Hover / Open Declaration) created its SearchableEnvironment with NO_RELEASE, so it always built the base module (src, which requires nothing) — regardless of which folder's Test.java was open. Consequently org.w3c.dom.Element is not found in the module graph (java.xml not readable), the binding can't be resolved, and SelectionEngine falls back to a module-unaware textual findTypes("Element") that returns all 4 JDK types named Element. Window "worked" only because exactly one accessible type is named Window.

I tried to add a test and fix the issue for this case now by adding MR awareness to the Openable.codeSelect ...

[laeubi]

I now also addressed some other call sites where release awareness was missing. Let me know if you are fine with these or if I should better split up things into individual PRs.

[stephan-herrmann]

New warning in ReconcilerModuleMultiReleaseTests.java:109:

The method getWorkingCopy(String, String, WorkingCopyOwner, IProblemRequestor) from the type AbstractJavaModelTests is deprecated

[stephan-herrmann]

Changes look good to me. Only test coverage seems to be incomplete (or did I miss relevant tests?)

Of course it's good to make more call paths release-aware. So I leave it to you to choose:

• add more test cases here, or
• file a follow-up issue to improve test coverage later.

Finally the new compile warning needs to be resolved.

[stephan-herrmann]

BTW, @laeubi, did you see #5221? While it talks about similar topics, technically it seems to be unrelated, but perhaps worth a look to ensure that the different parts of JDT are consistent with each other (conceptually and at the code level)?

[laeubi]

Only test coverage seems to be incomplete (or did I miss relevant tests?)

Its always a bit hard (for me) to guess best ways to cover these especially I'm not always sure what triggers it, so first thing is that I look that it at least does not break anything. Second is that I often try to implement for what we have a non working example, but as you have explicitly asked for other places I thought giving it a try and the pattern is rather repetitive.

Finally the new compile warning needs to be resolved.

I'll take a look.

[laeubi]

@stephan-herrmann as the build is now green I think its ready for another review. Let me know if you want me to squash everything into one an/or rebase on current master.