refactor: move the SemanticDB to binary name conversion onto Symbol - #8763
Open
damiankus-vl wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe change centralizes top-level SemanticDB symbol name conversion. Document links now resolve nested Javadoc classes. Classpath, source indexing, and virtual text document lookups use the shared conversions. Integration tests cover same-package and fully qualified nested classes. ChangesTop-level symbol name conversion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Javadoc
participant DocumentLinksProvider
participant Symbol
participant JavaSource
Javadoc->>DocumentLinksProvider: submit nested class reference
DocumentLinksProvider->>Symbol: construct candidate symbols
DocumentLinksProvider->>JavaSource: resolve candidate
JavaSource-->>DocumentLinksProvider: return Outer.java target
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@metals/src/main/scala/scala/meta/internal/parsing/DocumentLinksProvider.scala`:
- Line 299: Update classRefToSymbols so dotted Javadoc class references support
SemanticDB nested-class symbols: retain the Symbol.fromToplevelClassName
candidate for top-level classes, and additionally resolve nested segments using
the nested symbol form (for example, java/util/Map#Entry#). Return both
candidates when necessary so valid nested links can be found without breaking
top-level resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f70fbcf-2869-436a-bb20-39d9c0d9e3fd
📒 Files selected for processing (5)
metals/src/main/scala/scala/meta/internal/metals/mbt/VirtualTextDocument.scalametals/src/main/scala/scala/meta/internal/parsing/DocumentLinksProvider.scalamtags/src/main/scala/scala/meta/internal/mtags/ClasspathDefinitionIndex.scalamtags/src/main/scala/scala/meta/internal/mtags/Symbol.scalamtags/src/main/scala/scala/meta/internal/mtags/SymbolIndexBucket.scala
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@metals/src/main/scala/scala/meta/internal/parsing/DocumentLinksProvider.scala`:
- Line 304: Update the dotted-reference handling around
DocumentLinksProvider.dottedClassRefToSymbols so references beginning with a
type name also generate the current-package candidate (for example,
a/Outer#Inner#) before unqualified candidates. Preserve existing resolution
behavior and add LSP regressions covering {`@link` Outer.Inner} and {`@link`
Outer.Inner#doSomething} in the same package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ceb44c7-058d-499a-a63e-ebb52ec41fc5
📒 Files selected for processing (3)
metals/src/main/scala/scala/meta/internal/parsing/DocumentLinksProvider.scalatests/unit/src/test/scala/tests/DocumentLinkLspSuite.scalatests/unit/src/test/scala/tests/parsing/DocumentLinksProviderSuite.scala
damiankus-vl
force-pushed
the
improvement-extract-common-symbol-conversion-operations
branch
from
August 4, 2026 07:47
80a0940 to
dcc391a
Compare
Five call sites spelled the same conversion out by hand: strip the descriptor suffix off a toplevel symbol to get the name the JVM uses, and sometimes swap `/` for `.` on top of that, or the other way around to build the symbol of a class named the way `Class.forName` names it. Two of them wrote the two stripSuffix calls in the opposite order, which reads like it might matter. Symbol.toplevelBinaryName, Symbol.toplevelClassName and Symbol.fromToplevelClassName state it once, the last one being the inverse of the second. The scaladoc records why they answer for the toplevel class: a nested class is Outer$Inner in a binary name but Outer#Inner# in a symbol, and no caller needs that mapping. MetalsPasteProvider and MunitTestFinder strip the same characters for different reasons - one keeps the result in symbol form, the other must not walk up to the toplevel class - so they stay as they are. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Javadoc separates a nested class from its outer one with a dot, the same character that separates packages, so `java.util.Map.Entry` was read as class Entry in package java.util.Map and looked up as `java/util/Map/Entry#`. No such symbol exists - SemanticDB writes `java/util/Map#Entry#` - so the link resolved to nothing, and so did every member reference through it. Read the reference as a nested class first, splitting packages from types where Java's naming convention says they part, and keep the all-packages reading as the fallback for names that ignore the convention. The candidates already feed a first-one-that-resolves lookup, so an extra one costs nothing when the first answers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A nested class of the file's own package is written without any package at all, so `Outer.Inner` is dotted while naming no package of its own. Both dotted readings then miss: `Outer#Inner#` and `Outer/Inner#` describe a class in the default package, and the same-package candidate that a simple name gets was only built in the no-dots branch. A reference that starts with a type now gets the current package prefixed onto the readings, tried before the unqualified ones, so `Outer.Inner` inside `package a` is looked up as `a/Outer#Inner#` first. The unqualified candidates stay where they were, behind it, so nothing that resolved before stops. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
damiankus-vl
force-pushed
the
improvement-extract-common-symbol-conversion-operations
branch
from
August 6, 2026 11:33
dcc391a to
daced54
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are a few places where we convert SemanticDB symbols to Java binary names, for example
We could extract that logic to make it reusable
This change originated in from #8742
I've moved it to a separate PR to make the original one smaller
Summary by CodeRabbit
Bug Fixes
Tests