PackageFragmentRoot.getRawClasspathEntry seem to depend on resolved classpath

[iloveeclipse]

          I see one PDE test fail which probably could be related:

https://download.eclipse.org/eclipse/downloads/drops4/I20250108-0430/testresults/html/org.eclipse.pde.ui.tests_ep435I-unit-win32-x86_64-java21_win32.win32.x86_64_21.html

testImportLinksMultiple[Import binary]

C:\Program Files\Eclipse Adoptium\jdk-21.0.5.11-hotspot\lib\jrt-fs.jar is not on its project's build path

Java Model Exception: Error in Java Model (code 1006): C:\Program Files\Eclipse Adoptium\jdk-21.0.5.11-hotspot\lib\jrt-fs.jar is not on its project's build path
at org.eclipse.jdt.internal.core.PackageFragmentRoot.getRawClasspathEntry(PackageFragmentRoot.java:621)
at org.eclipse.pde.ui.tests.imports.BaseImportTestCase.assertSourceAttached(BaseImportTestCase.java:202)
at org.eclipse.pde.ui.tests.imports.BaseImportTestCase.verifyProject(BaseImportTestCase.java:189)
at org.eclipse.pde.ui.tests.imports.BaseImportTestCase.testImportLinksMultiple(BaseImportTestCase.java:120)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)

However these tests run fine locally. I guess MT or timing issue.

Code does basically this:

IJavaProject jProject = JavaCore.create(project);
		for (IPackageFragmentRoot root : jProject.getPackageFragmentRoots()) {
			IClasspathEntry entry = root.getRawClasspathEntry();
			if (entry.getEntryKind() == IClasspathEntry.CPE_LIBRARY
					|| (entry.getEntryKind() == IClasspathEntry.CPE_CONTAINER)
					&& !entry.getPath().equals(PDECore.REQUIRED_PLUGINS_CONTAINER_PATH)) {
				assertNotNull("Missing source attachement for entry " + entry, root.getSourceAttachmentPath());
			}
		}

Originally posted by @iloveeclipse in #3531 (comment)

[iloveeclipse]

The spaghetti code around setting project classpath hits multiple times in org.eclipse.jdt.internal.core.JavaModelManager.PerProjectInfo.setClasspath() with conditional breakpoint, setting rootPathToRawEntries to null from multiple threads.

I can't see any clear logic when it is supposed to be in consistent state and when not.

I believe we have to revert #3531.

[iloveeclipse]

Also in our product tests we saw now sporadic test fails (they success on automated repeating) with following stack:

!ENTRY org.eclipse.jdt.core 4 4 2025-01-08 19:45:14.200
!MESSAGE Error checking whether PackageFragmentRoot is on module path!
!STACK 1
Java Model Exception: Error in Java Model (code 1006): /workspaces/.../system/lib/XYZ.jar is not on its project's build path
	at org.eclipse.jdt.internal.core.PackageFragmentRoot.getRawClasspathEntry(PackageFragmentRoot.java:621)
	at org.eclipse.jdt.internal.core.search.matching.JavaSearchNameEnvironment.computeModuleFor(JavaSearchNameEnvironment.java:610)
	at org.eclipse.jdt.internal.core.search.matching.JavaSearchNameEnvironment.mapToClassPathLocation(JavaSearchNameEnvironment.java:302)
	at org.eclipse.jdt.internal.core.search.matching.JavaSearchNameEnvironment.computeClasspathLocations(JavaSearchNameEnvironment.java:215)
	at org.eclipse.jdt.internal.core.search.matching.JavaSearchNameEnvironment.computeClasspathLocations(JavaSearchNameEnvironment.java:187)
	at org.eclipse.jdt.internal.core.search.matching.JavaSearchNameEnvironment.<init>(JavaSearchNameEnvironment.java:95)
	at org.eclipse.jdt.internal.core.search.indexing.SourceIndexer.resolveDocument(SourceIndexer.java:180)
	at org.eclipse.jdt.internal.core.search.JavaSearchParticipant.resolveDocument(JavaSearchParticipant.java:125)
	at org.eclipse.jdt.internal.core.search.indexing.IndexManager.indexResolvedDocument(IndexManager.java:668)
	at org.eclipse.jdt.internal.core.search.indexing.IndexManager$2.execute(IndexManager.java:1272)
	at org.eclipse.jdt.internal.core.search.processing.JobManager.indexerLoop(JobManager.java:541)
	at java.base/java.lang.Thread.run(Thread.java:1583)

I'm going to revert the change...

[jukzi]

I'm going to revert the change...

This does not seem urgent.
Please give the contributor a chance to comment on / fix it.
@laeubi

[iloveeclipse]

i found this in my log: does it relate?

Not sure, this one seem to work on resolved path.

But we see for sure related indexer errors in different tests which were all green before the PR in question.

[jukzi]

i found this in my log: does it relate?

skip this comment- that was me testing issue 3298

[laeubi]

I guess MT or timing issue.

As mentioned in the other issues, jdt.ui has a job that is run and init al classpath container. After that of course everything is "set" and one will not find a difference. Also there is writeAndCacheClasspath so maybe a cached state is used on local execution.

Maybe the places failing now actually want to use getResolvedClasspathEntry instead of getRawClasspathEntry ? But I don't know what would be the real difference here.

[laeubi]

I now tried to limit the scope of the map here:

• Limit scope of JavaModelManager#rootPathToRawEntries field #3538

this makes the code much safer and requires less boilerplate, maybe one can even use this as an entry point to cache the data on demand if it is missing.

[szarnekow]

Knowing that it'll sound quite rude, I'll voice my concerns nevertheless.
The discussion around the classpath entries (raw + resolved) and the piecemeal patching around leave the strong impression that neither the impact of the attempted change nor the history of the code is understood well enough yet. Jumping to conclusions about even a single line of code that primes a cache prone to failure if the control- and data-flow isn't known well enough. Given the experience that I have with my Eclipse installation after upgrading to 2024-12 I'd prefer a more careful handling of the legacy code. From my experience with legacy code, an seemingly obvious improvement is exactly that - obvious and in a codebase of 15years+ of age, the engineer having the obvious idea is not the first person with that idea. There's often a reason why it wasn't done yet.

Please consider reverting the previous change for the sake of stability until a profound understanding of that piece of code was reached.

[iloveeclipse]

Knowing that it'll sound quite rude, I'll voice my concerns nevertheless.

Thanks Sebastian, that matches exactly my feelings. I've considered original PR as a quick "test" whether it would survive or not "broader" test scenarios as only Java model tests, and it did not from day one.

The code itself , as I've wrote, changes the data quite often and from multiple threads, so it wonders me that it "somehow" worked before, but obviously the original authors had much more understanding about the concept as I have.

So I would revert this line removal now, considering that "simple" experiment to remove it as failed - but @laeubi it doesn't mean we don't want to change / improve the code in general - we simply must be sure we are doing the right thing, which is not the case here.

[jukzi]

Wouldn't it help to keep the change some days to possibly find a reproducing test within jdt.core such that it can be better analyzed?

[iloveeclipse]

Wouldn't it help to keep the change some days to possibly find a reproducing test within jdt.core such that it can be better analyzed?

Seeing different unrelated tests randomly failing, I assume we have multi-threading or timing issue. Also see my comment above, debugging with conditional breakpoints shows for me lot of interactions with the corresponding map.

I assume if someone takes enough time to analyze how the state is supposed to be updated in the "right" way, it would be possible to provide a better change, but until then I would like to see SDK back to a stable working state we had.

[laeubi]

obvious and in a codebase of 15years+ of age, the engineer having the obvious idea is not the first person with that idea.

Sadly it is also true that these ideas / knowledge was not properly documented and is now los in the fog-of-time... and sad enough there seem to be no single test covering that case here in the repo.

I just tried to understand from the API the inital intend here and it says:

A raw classpath entry corresponds to a package fragment root if once resolved this entry's path is equal to the root's path.

And can't really get my head around it... how could a method know the corresponding entry "if once resolved" without resolving it?!? So for me it also seems that just removing the line won't work even though the way to fill the cache is quite suspicious, indirect and hard to understand. My expectation would be that calling the cache will fill it as needed that's why I tried to make it less exposed here

• Limit scope of JavaModelManager#rootPathToRawEntries field #3538

but to be honest it fees I'm to old (or young?) to adapt to "JDT style" to be helpful in this regard. For me improving codebase includes adapting to modern ways that are easier to understand and maintain instead of insist of a 15 year old "consistent" way of doing things.