eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
168 stars 133 forks source link

Reconciler doesn't leverage --release option #3168

Open stephan-herrmann opened 1 month ago

stephan-herrmann commented 1 month ago

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 commented 1 month ago

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

stephan-herrmann commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago
  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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1740

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