Fix missing "Go to Symbol" results in Java projects#9327
Fix missing "Go to Symbol" results in Java projects#9327eirikbakke wants to merge 3 commits intoapache:masterfrom
Conversation
For the past years, the "Go to Symbol" feature has appeared broken for me; searching for the name of a class that exists would frequently turn up no results. Today I managed to reproduce the problem. I put Claude Opus 4.6 on the task of investigating the issue, and had it produce both a fix and a regression test.
The problem was that ".sig" files were written to the cache using a .class format with with various NetBeans enhancements, but then loaded again in a way that failed when these enhancements were encountered. Only classes with certain contents would be affected, though here is a rather trivial example class that couldn't be found with "Go to Symbol" prior to the fix in this commit:
public class DifficultClass19 {
private static final String SOMECONST_STR = "hello";
public void someMethod() { }
}
(If you try searching for this class in Go to Symbol, you may still get one hit in lowercase "difficultclass19". That's from the text-based Lucene index, which is used as a fallback when no Java symbols appear.)
The fix is in AsyncJavaSymbolDescriptor. I have confirmed manually that the new JavaSymbolProviderTest fails without the fix in AsyncJavaSymbolDescriptor, and passes after the fix has been applied.
An unrelated potential race condition in Go to Symbol's ContentProviderImpl is also fixed.
(The commit description above was written by me, Eirik. The code in this commit is by Claude Opus 4.6, written under my supervision. See the PR for Claude's summary of the investigation.)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The only failing test, "NetBeans on nb-javac", seems to be unrelated to this change. It seems the "nb-javac" label is not in active use, and I can't find a record of that job ever having run successfully (on merged PRs or on commits in the delivery branch for NetBeans 29-rc3). |
The |
|
@matthiasblaesing Thanks for that explanation! I enabled a bunch of tests since the PR touched Java indexing. Later I realized that this PR is actually quite localized. AsyncJavaSymbolDescriptor is only used by the Go to Symbol dialog. So it should be a low-risk PR. I'm curious if others have noticed the Go to Symbol dialog not working properly in Java projects for the last 11 years? It's been on my list of things to look into for all those years, but I didn't hear others mention it. |
ide/jumpto/src/org/netbeans/modules/jumpto/symbol/ContentProviderImpl.java
Show resolved
Hide resolved
…tually needed to fix the motivating bug.
…ter Eirik reviewed Claude's patch.
|
I reviewed the patch again, adjusted it a bit, and removed the "do not merged" label. I've tested it for a bit in my IDE, and its impact should be limited to the Go to Symbol dialog, which now finally seems to work properly again. |
For the past years, the "Go to Symbol" feature has appeared broken for me; searching for the name of a class that exists would frequently turn up no results. Today I managed to reproduce the problem. I put Claude Opus 4.6 on the task of investigating the issue, and had it produce both a fix and a regression test.
(There's also a more commonly used "Go to Type" feature in NetBeans, which did not have the bug, and which is not affected by this PR. Maybe I'm the only user who was pressing Ctrl+Alt+Shift+O for "Go to Symbol" instead of Ctrl+O for "Go to Type"... hence why I've never heard anyone else complaining about the bug.)
The problem was that ".sig" files were written to the cache using a .class format with various NetBeans enhancements, but then loaded again in a way that failed when these enhancements were encountered. Only classes with certain contents would be affected, though here is a rather trivial example class that couldn't be found with "Go to Symbol" prior to the fix in this commit:
(If you try searching for this class in Go to Symbol, you may still get one hit in lowercase "difficultclass19". That's from the text-based Lucene index, which is used as a fallback when no Java symbols appear. Though even that fallback was previously broken; the Lucene-based searches would appear and then quickly disappear, and open() wouldn't work on them. This fallback case is also fixed in this PR.)
The fix is in AsyncJavaSymbolDescriptor. I have confirmed manually that the new JavaSymbolProviderTest fails without the fix in AsyncJavaSymbolDescriptor, and passes after the fix has been applied.
The change in this PR should only affect the Go to Symbol functionality, not e.g. Find Usages or other parts of the IDE that uses the index. Rationale: The AsyncJavaSymbolDescriptor class is only used in JavaSymbolProvider, which is the implementation of the o.n.spi.jumpto.symbol.SymbolProvider interface. JavaSymbolProvider is not used in any other way.
The commit/PR description above was written by me, Eirik. The code in this commit is by Claude Opus 4.6, written under my supervision. I am including Claude's summary of the investigation and fix below, as it is quite informative.
Claude's Summary (AI-generated, though useful)
Fix Go to Symbol silently dropping Java results.
AsyncJavaSymbolDescriptor.resolve() is meant to enrich Lucene-indexed Java symbol hits with real javac TypeElements. Since 2015 it has silently been returning an empty collection for every Java type whose per-root .sig file used any NetBeans-specific class-file encoding, which made Go to Symbol effectively broken for Java in many real projects: Lucene-backed entries flashed briefly in the list (lowercased, no icon) and were then deleted by Models.MutableListModelImpl#descriptorChanged, which treats an empty resolve() result as "delete this row". Clicking a surviving entry also did nothing, because the old open() went through the same empty resolve().
Root cause: resolve() built its JavacTask by calling JavacTool.create().getTask(...) directly, which does not pre-register any of NetBeans' javac context enhancers. NetBeans .sig files (the per-root cached form of indexed Java classes, written by NBClassWriter) use a relaxed/extended class-file format that only NBClassReader knows how to parse correctly -- stock javac's ClassReader rejects them with BadClassFile, which propagates as CompletionFailure. ElementUtils.getTypeElementByBinaryName silently catches the CompletionFailure and returns null, so resolve() falls through to its empty-list return path. The exact rejection message varies by which NB-specific feature the .sig used; for one observed real-world case the .sig contained a ConstantValue attribute on a non-static-final String field, and stock javac rejected it with "variable of type 'java.lang.String' cannot have a constant value, but has one specified". The boot classpath, java.lang.Object loadability, and supertype walking are not involved -- the old code can perfectly well find Object via the running JDK's auto-discovered system modules; it just can't read the per-root .sig file.
History (verified against the pre-donation NetBeans repo):
2015-05-18 (pre-Apache commit jtulach/netbeans-releases@0f2c3e3d) AsyncJavaSymbolDescriptor.java introduced. resolve() goes through JavaSource.create(cpInfo).runUserActionTask(...), which routes via JavacParser and pre-registers the full NB javac context (NBClassReader, NBAttr, NBEnter, NBMemberEnter, NBResolve, NBClassFinder, NBJavaCompiler, NBClassWriter, etc.). Correct.
2015-05-19 (pre-Apache commit jtulach/netbeans-releases@445add56) "Speed up of javac resolution". Replaces the JavaSource/CompilationController call with a hand-rolled JavacTool.create().getTask(..., new RootChange(), ...) to skip the JavaSource setup overhead. NB context enhancer pre-registration is lost in the process; the resulting javac task uses stock ClassReader. This is the root-cause regression.
2015-07-28 (pre-Apache commit jtulach/netbeans-releases@6b7a248a) "#253081:NullPointerException at AsyncJavaSymbolDescriptor.resolve". An NPE in resolve() is reported in the wild only ~10 weeks after the 2015-05-19 regression -- consistent with the new code path being immediately broken on at least some configurations, although the exact trigger is not derivable from the diff alone. The fix wraps the (already-broken) reflective ClassReader.packages/classes cleanup hack so it returns Collections.emptyMap() instead of null, hiding the NPE without addressing why the reflective cleanup failed in the first place.
2016-12-12 (pre-Apache commit jtulach/netbeans-releases@499db956) "Fixed Go To Symbol does not work with new javac". The commit message itself confirms that by late 2016 Go to Symbol was known to be broken with the JDK 9 javac. The attempted fix only switches the (still-broken) reflective hack from the now-non-existent ClassReader.packages/classes fields to a Symtab parameter, but still does ClassReader.class.getDeclaredField(...), so on JDK 9+ it remains a silent no-op. The underlying defect -- using stock javac with no NB context enhancers -- is not addressed, and Go to Symbol stays broken. This is the strongest direct evidence in the history that the bug has been live and acknowledged since at least 2016.
The defect has been in place for just under eleven years.
The fix replaces the whole hand-rolled construction -- including the long-broken reflective ClassReader.packages/classes cleanup hack (a silent no-op since those fields moved to Symtab on JDK 9, and the origin of the one-time WARNING "packages"/"classes" logged from AsyncJavaSymbolDescriptor at every JDK 9+ startup) -- with a single JavacParser.createJavacTask(ClasspathInfo.create(root), ...) call, matching the idiom JavaBinaryIndexer and ModuleNames.parseModuleName already use. JavacParser.createJavacTask routes through the NB context enhancer pre-registration path, so the resulting javac task uses NBClassReader and friends and can read the per-root .sig files. About 150 lines of dead code and the startup WARNINGs go away. See the comments in the file for the defense-in-depth fallbacks retained in resolve() and open().
A new regression test, JavaSymbolProviderTest.testAsyncDescriptorResolvesForIndexedType, indexes a single-class source root whose source is intentionally rich (a lambda, generics, a static-final String constant, and a separate annotation type) so that the .sig file the indexer writes is empirically rejected by stock javac's ClassReader -- a trivial empty class produces a .sig vanilla javac reads without trouble, so the richer source is required. The test then asks JavaSymbolProvider to look up the class, triggers AsyncJavaSymbolDescriptor's async resolve() by calling getSymbolName() on the returned descriptor, and asserts that the resulting descriptor-change event carries a non-empty, enriched replacement that is not the original AsyncJavaSymbolDescriptor instance. It passes against the fix and fails against the pre-fix code path (verified end-to-end by reverting AsyncJavaSymbolDescriptor.java only and re-running the test). The exact attribute in the test class's .sig that trips stock ClassReader has not been pinned down -- it is enough to know that the differential exists and is reliably reproduced.