eirikbakke opened a new pull request, #9327:
URL: https://github.com/apache/netbeans/pull/9327
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 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/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.**
I'm putting a "do not merge" label on this until I've had more human
feedback on this and tested the patch in my working IDE for a while.
### 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 wal
king 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
https://github.com/jtulach/netbeans-releases/commit/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
https://github.com/jtulach/netbeans-releases/commit/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
https://github.com/jtulach/netbeans-releases/commit/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
https://github.com/jtulach/netbeans-releases/commit/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 s
tock ClassReader has not been pinned down -- it is enough to know that the
differential exists and is reliably reproduced.
Also fixes a small, unrelated race in ContentProviderImpl.Worker:
model.remove and model.add, previously issued as two separate background-thread
invokeInEDT calls, are now performed inside the existing invokeLater runnable
and guarded by !isCanceled, so they happen atomically within one EDT turn.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
