Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconciler doesn't leverage --release option #3168

Open
stephan-herrmann opened this issue Oct 26, 2024 · 10 comments
Open

Reconciler doesn't leverage --release option #3168

stephan-herrmann opened this issue Oct 26, 2024 · 10 comments
Assignees

Comments

@stephan-herrmann
Copy link
Contributor

In a project using a recent JDK (e.g., 23) that is configured with --release 19, the reconciler will see class files from the most recent version (here 23), despite the --release option.

Classes are read in this call stack:

ClassFileReader.<init>(byte[], char[], boolean) line: 242	
ClassFile.getJarBinaryTypeInfo() line: 228	
ClassFile.existsUsingJarTypeCache() line: 147	
NameLookup.seekTypesInBinaryPackage(String, IPackageFragment, boolean, int, IJavaElementRequestor) line: 1475	
NameLookup.seekTypes(String, IPackageFragment, boolean, int, IJavaElementRequestor, boolean) line: 1442	
NameLookup.findType(String, IPackageFragment, boolean, int, boolean, boolean) line: 988	
NameLookup.findType(String, String, boolean, int, boolean, boolean, boolean, IProgressMonitor, IPackageFragmentRoot[]) line: 815	
NameLookup.findType(String, String, boolean, int, boolean, IPackageFragmentRoot[]) line: 734	
CancelableNameEnvironment(SearchableEnvironment).find(String, String, IPackageFragmentRoot[]) line: 191	
CancelableNameEnvironment(SearchableEnvironment).findType(char[], char[][], char[]) line: 549	
LookupEnvironment.fromSplitPackageOrOracle(IModuleAwareNameEnvironment, ModuleBinding, PackageBinding, char[]) line: 473	
LookupEnvironment.lambda$1(IModuleAwareNameEnvironment, PackageBinding, char[], ModuleBinding) line: 346	
0x00007f6213aac050.apply(Object) line: not available	
LookupEnvironment.askForTypeFromModules(ModuleBinding, ModuleBinding[], Function<ModuleBinding,NameEnvironmentAnswer>) line: 440	

While the ClassFile could use its enclosing IJavaProject to detect the --release 19 option, we indirectly use JRTUtil.getClassfileContent() which constantly passes null as the release in getJrtSystem(). This, btw, seems the prevalent use of that method!

To observe this behavior, use the above project configuration and inside a method catching MalformedURLException type the following without saving:

URL.of(null, null);

You will not see an error until the file is saved. This demonstrates that in fact the 23 version is used by the reconciler, where the method exists (since 21).

Manually tweaking getJrtSystem() to use release "19" (in the debugger) does not suffice, because the logic used to locate the class file is in JrtFileSystem.getClassfileContent() / JrtFileSystem.getFileBytes(String, String), which have no override in JrtFileSystemWithOlderRelease.

I'm not even sure if its a good idea to amend the code to respect the release version, because there's a lot of caching going on, where I don't readily see if those caches are multi-version aware??

In fact, I don't readily see if there is any low hanging fruit, or whether the inconsistency is actually tolerable in face of the complexity.

@stephan-herrmann
Copy link
Contributor Author

After saving, we have this inconsistency on URL.of(null, null)

  • an error is shown "The method of(null, null) is undefined for the type URL"
  • pressing F3 will navigate to that method (in JDK 23).
  • Ctrl+Shift hover will show the javadoc of that method (in JDK 23).

@stephan-herrmann
Copy link
Contributor Author

Notifying a few people having worked in this area, @jarthana @jukzi @iloveeclipse @HannesWell @laeubi -- do any of you have an opinion they want to share? Has this perhaps been discussed before?

@laeubi
Copy link
Contributor

laeubi commented Oct 27, 2024

I think it should be consistent with compile, so at best the release option will be passed down to the JrtSystem, I'm just not 100% sure about your mentioned inconsistency, e.g. if java 23 is configured as a system library it seems correct for me to show the javadoc and/or jump to the class in question... so what I would expects is something like this:

  1. regardless of save/not save code should not be accepted and maybe show "this method is not available for the current target level", so reconcile would use configured target level
  2. I always see the javadoc / jump to code of the project JVM regardless of target setting (I can't think how it otherwise should work consistently)

Maybe the target level could/should be more prominent to the user e.g. if I would have coonfigured java 9 target for a Java 17 JVM
grafik

@stephan-herrmann
Copy link
Contributor Author

  1. regardless of save/not save code should not be accepted and maybe show "this method is not available for the current target level", so reconcile would use configured target level

The error "The method of(null, null) is undefined for the type URL" is raised by the compiler, which has no idea if --release had any impact.

With this I don't see a good option to avoid the confusion: compiler says method is undefined, but when you navigate to the class it's there. This seems to be an intrinsic property of --release.

Should the editor of class URL signal that the source code shown may not be the same as what --release indicates? Should all editors of classes where --release is in effect show a warning? ("The source code shown is from version 23, wheres version 19 is requested using --release").

@jukzi
Copy link
Contributor

jukzi commented Oct 27, 2024

As long as there is a any Problem marker, that the "JDK used does not match the requested version and that this may lead to inconsistencies" it is ok to have inconsistencies

@jarthana
Copy link
Member

Should the editor of class URL signal that the source code shown may not be the same as what --release indicates? Should all editors of classes where --release is in effect show a warning? ("The source code shown is from version 23, wheres version 19 is requested using --release").

If we want to display a warning, we might (also) want to show it on the --release option on the build path properties page?

@laeubi
Copy link
Contributor

laeubi commented Oct 28, 2024

The error "The method of(null, null) is undefined for the type URL" is raised by the compiler, which has no idea if --release had any impact.

How does compiler then knows it it undefined if it actually is there? So it could know that and at least on some places it says "X requires Java Y", maybe if its only on the UI it would be fine (for me), I'm just throwing here an idea what would be best from user perspective not that it is easily achievable or I have any clue how :-)

With this I don't see a good option to avoid the confusion: compiler says method is undefined, but when you navigate to the class it's there. This seems to be an intrinsic property of --release.

Usually I hope you have the source or javadoc there that shows a @since so there is a chance to know that, thats why I suggested to make the used release visible in the UI.

Should the editor of class URL signal that the source code shown may not be the same as what --release indicates? Should all editors of classes where --release is in effect show a warning? ("The source code shown is from version 23, wheres version 19 is requested using --release").

I don't think we need to complicate things here, if I decide to use --release option I obviously should be aware of its implications. If I need 1:1 mapping I would simply argue to use a PERFECT MATCH jre...

If we want to display a warning, we might (also) want to show it on the --release option on the build path properties page?

I don't think warnings are a good idea, the release option is to compile for a given target using a more recent JVM so there is not reason to warn about it, but JDT has the concept of "perfect match" already

grafik

So for me just showing the JRE + selected target seems sufficient. If we can mark certain things "grayed out" in the UI (e.g. if I jump to the source) that would be really great, but not mandatory for me, so maybe the UI would need some kind of "release" flag to and then can interpret @since when showing sources from the JDK.

@stephan-herrmann
Copy link
Contributor Author

I think we have consensus that the reconciler should indeed respect the --release flag. To achieve such consistency ideally all clients of org.eclipse.jdt.internal.compiler.util.JRTUtil.getJrtSystem(File, String) would pass the release version as configured in the IJavaProject.

Additionally, the logic from the builder's ClasspathJrtWithReleaseOption concerning CtSym will probably have to be copied to other implementations accessing the JRT system - or perhaps this can be factored into JrtFileSystem itself.

Fixing this should not require familiarity with the compiler, just ensure that classes are looked up from the correct folders inside ct.sym.

@stephan-herrmann
Copy link
Contributor Author

stephan-herrmann commented Oct 28, 2024

The error "The method of(null, null) is undefined for the type URL" is raised by the compiler, which has no idea if --release had any impact.

How does compiler then knows it it undefined if it actually is there?

it depends, where you look. The name environment used during build finds signatures of JDK classes solely via "ct.sym", which provides version specific information. Once the class is passed into the compiler the connection to ct.sym is lost (because the compiler has no knowledge about stuff like filesystems, archives, JDK libraries). Hence for a compiler in a --release 19 project, method URL.of() does not exist. End of story.

So it could know that and at least on some places it says "X requires Java Y", maybe if its only on the UI it would be fine (for me),

Right, since this is UI specific I created eclipse-jdt/eclipse.jdt.ui#1740

In both issues I'm available for discussion and help, but I will not drive either of them.

@stephan-herrmann
Copy link
Contributor Author

#3608 demonstrates that failure to pass --release can cause exceptions in unexpected places.

getJrtSystem(File, String) should always pass the release version where available.
Any volunteer for driving an update?

@jarthana jarthana self-assigned this Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants