feat: make scaladoc/javadoc wiki links navigable in hover and go-to-definition - #8658
feat: make scaladoc/javadoc wiki links navigable in hover and go-to-definition#8658jozanek wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a marker-based protocol for scaladoc/javadoc entity links so that hover and completion responses no longer leak broken raw links. It adds import-scope-aware link resolution for both Scala and Java, a new ChangesScaladoc link resolution and navigation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant WorkspaceLspService
participant MetalsLspService
participant ScaladocDefinitionProvider
participant DefinitionProvider
Editor->>WorkspaceLspService: executeCommand(GotoScaladocLink, payload)
WorkspaceLspService->>MetalsLspService: resolveScaladocLink(params)
MetalsLspService->>MetalsLspService: parsePayload / build DocScope context
MetalsLspService->>ScaladocDefinitionProvider: resolveLinkLocations(isJava, knownDocstringFile)
ScaladocDefinitionProvider->>DefinitionProvider: resolveSymbol / fromSymbol
DefinitionProvider-->>ScaladocDefinitionProvider: candidate locations
ScaladocDefinitionProvider-->>MetalsLspService: Resolved / NotUnique / Failed
MetalsLspService-->>WorkspaceLspService: ScaladocLinkResolution
alt Resolved
WorkspaceLspService->>Editor: GotoLocation
else NotUnique or Failed
WorkspaceLspService->>Editor: show message
end
sequenceDiagram
participant Editor
participant Compilers
participant PresentationCompiler
participant MetalsSymbolLink
Editor->>Compilers: textDocument/hover
Compilers->>PresentationCompiler: hover(params)
PresentationCompiler-->>Compilers: raw hover markup with markers
Compilers->>MetalsSymbolLink: parsePayload for each marker
Compilers->>Compilers: rewriteHoverWikiLinks(commandInHtmlFormat?)
alt command links enabled
Compilers-->>Editor: hover with command:metals.goto-scaladoc-link links
else plaintext fallback
Compilers-->>Editor: hover with plain titles
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (5)
metals/src/main/scala/scala/meta/internal/metals/Compilers.scala (1)
1235-1266: 🚀 Performance & Scalability | 🔵 TrivialHover markup is rendered twice per request (
toLsp()called in both the detection path andRewrittenHover.toLsp()).
rewriteHoverWikiLinks's non-ScalaHoverbranch callshover.toLsp()just to peek at the contents, then wraps the result inRewrittenHover, whose owntoLsp()callsunderlying.toLsp()again when the client later renders the hover. IftoLsp()does non-trivial markdown generation, that's a redundant computation on every out-of-process hover. Not incorrect, just wasteful; consider caching the firstHoverresult insideRewrittenHoverinstead of recomputing.Also applies to: 2034-2056
🤖 Prompt for 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. In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala` around lines 1235 - 1266, The non-ScalaHover path in rewriteHoverWikiLinks is recomputing hover markup by calling hover.toLsp() for detection and then again later through RewrittenHover.toLsp(). Update Compilers.RewrittenHover (and its use in rewriteHoverWikiLinks) to cache the first LSP Hover/contents result so the rewritten hover can reuse it instead of regenerating markdown on every render.tests/unit/src/test/scala/tests/HoverLspSuite.scala (3)
860-866: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisplaced/orphaned comment.
This comment block ("A Java wildcard import... resolves to
a.b.Helper") describeswiki-link-navigate-java-wildcard-import(line 979), not thewiki-link-scheme-in-labeltest it actually precedes. Looks like it was left behind when the tests were reordered. Move it above the correct test to avoid confusing future readers.✏️ Suggested fix
- // A Java wildcard import (`import a.b.*;`) qualifies a relative Javadoc link - // via its prefix, so `{`@link` Helper}` resolves to `a.b.Helper` - // (scalameta/metals#3383). // A docstring whose link label itself contains the marker scheme (an escaped // `](metals-wiki-link2:…)`) must not crash hover/completion rendering — it // degrades gracefully instead (scalameta/metals#3383). test("wiki-link-scheme-in-label".tag(FlakyWindows)) {and then, before
test("wiki-link-navigate-java-wildcard-import"...):+ // A Java wildcard import (`import a.b.*;`) qualifies a relative Javadoc link + // via its prefix, so `{`@link` Helper}` resolves to `a.b.Helper` + // (scalameta/metals#3383). test("wiki-link-navigate-java-wildcard-import".tag(FlakyWindows)) {🤖 Prompt for 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. In `@tests/unit/src/test/scala/tests/HoverLspSuite.scala` around lines 860 - 866, The wildcard-import comment is attached to the wrong test and should be moved to the matching `wiki-link-navigate-java-wildcard-import` case instead of `wiki-link-scheme-in-label`. Relocate the comment block so it sits directly above the `test("wiki-link-navigate-java-wildcard-import"...` in `HoverLspSuite`, keeping the existing `wiki-link-scheme-in-label` test only with its own relevant comment.
1085-1120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAmbiguity tests don't consistently verify the "Could not uniquely resolve" message.
clickFirstLinkreturnsNoneboth when the rendered hover has no command link at all and when the command executes but resolution is genuinely ambiguous — these tests only asserturi.isEmpty, so they can't distinguish the two. Other tests in this same suite covering the same category of scenario (e.g.wiki-link-java-duplicate-static-import,wiki-link-overload-no-navigation,wiki-link-no-companion-navigation,wiki-link-ambiguous-no-navigation) do additionally assert onclient.showMessagescontaining "Could not uniquely resolve". Applying the same assertion here would make these tests actually validate that ambiguity was detected and reported, rather than merely that navigation didn't happen.Also applies to: 1350-1387, 2118-2146, 2810-2847, 2896-2940
🤖 Prompt for 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. In `@tests/unit/src/test/scala/tests/HoverLspSuite.scala` around lines 1085 - 1120, The ambiguity hover/navigation test only checks that clickFirstLink returns no URI, so it does not verify that the failure was actually reported as an ambiguous resolution. Update the affected HoverLspSuite tests, including wiki-link-ambiguous-wildcard-no-navigation and the other similar ambiguous cases, to also assert that client.showMessages contains the “Could not uniquely resolve” message after the hover link interaction, matching the pattern already used by wiki-link-java-duplicate-static-import, wiki-link-overload-no-navigation, and related tests.
3055-3081: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame weak-assertion gap as
DefinitionLspSuite's enum-case-param test.The comment states this verifies
[[r]]resolves to the case parameter rather than falling through to the enum's companion, but the assertion only checks the file suffix (/a/Color.scala), not the resolved line — both the case and any companion fallback would live in files ending the same way in this setup. Consider asserting the specific line/range to actually catch a regression toward the enum companion.🤖 Prompt for 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. In `@tests/unit/src/test/scala/tests/HoverLspSuite.scala` around lines 3055 - 3081, The test in HoverLspSuite for wiki-link-navigate-enum-case-param is too weak because it only checks that clickFirstLink returns a URI ending in Color.scala, which would also pass if the link incorrectly resolved to the enum companion. Strengthen the assertion by checking the exact target location or range returned by clickFirstLink, using the existing test setup around Color.scala and Main.scala to verify the wiki link resolves to the case parameter r rather than just any symbol in the same file.tests/unit/src/test/scala/tests/DefinitionLspSuite.scala (1)
813-842: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't verify the claimed CASE-vs-ENUM distinction.
The comment states this test confirms
[[r]]resolves against the caseMix, not the enclosingenum E, but both live in the same file (E.scala), and the assertion only checks the URI suffix, not the resolved line/range. A regression that resolves to the enum itself instead of the case parameter would still pass this test. Consider assertinglocations.head.getRange().getStart().getLine()(or similar), the way the siblingscaladoc-definition-scala3-source-orderandscaladoc-definition-scala3-toplevel-membertests already do.🤖 Prompt for 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. In `@tests/unit/src/test/scala/tests/DefinitionLspSuite.scala` around lines 813 - 842, The scaladoc definition test only checks that the result is in the same file, so it does not actually verify that `[[r]]` resolves to the enum case parameter rather than the enclosing enum. Update the `scaladoc-definition-enum-case-param` test in `DefinitionLspSuite` to assert the returned location’s range/line matches the `case Mix(r: Int)` parameter target, using the same pattern as the nearby `scaladoc-definition-scala3-source-order` and `scaladoc-definition-scala3-toplevel-member` tests.
🤖 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 `@mtags/src/main/scala/scala/meta/internal/metals/docstrings/WikiLink.scala`:
- Around line 41-53: The Scaladoc for the bracket-link offset behavior is
attached to the wrong helper: the comment above javadocAtOffset describes the
atOffset behavior instead. Move that documentation block so it sits with
atOffset, and keep javadocAtOffset’s own inline-link doc in place; use the
atOffset and javadocAtOffset symbols to relocate the orphaned comment cleanly.
---
Nitpick comments:
In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala`:
- Around line 1235-1266: The non-ScalaHover path in rewriteHoverWikiLinks is
recomputing hover markup by calling hover.toLsp() for detection and then again
later through RewrittenHover.toLsp(). Update Compilers.RewrittenHover (and its
use in rewriteHoverWikiLinks) to cache the first LSP Hover/contents result so
the rewritten hover can reuse it instead of regenerating markdown on every
render.
In `@tests/unit/src/test/scala/tests/DefinitionLspSuite.scala`:
- Around line 813-842: The scaladoc definition test only checks that the result
is in the same file, so it does not actually verify that `[[r]]` resolves to the
enum case parameter rather than the enclosing enum. Update the
`scaladoc-definition-enum-case-param` test in `DefinitionLspSuite` to assert the
returned location’s range/line matches the `case Mix(r: Int)` parameter target,
using the same pattern as the nearby `scaladoc-definition-scala3-source-order`
and `scaladoc-definition-scala3-toplevel-member` tests.
In `@tests/unit/src/test/scala/tests/HoverLspSuite.scala`:
- Around line 860-866: The wildcard-import comment is attached to the wrong test
and should be moved to the matching `wiki-link-navigate-java-wildcard-import`
case instead of `wiki-link-scheme-in-label`. Relocate the comment block so it
sits directly above the `test("wiki-link-navigate-java-wildcard-import"...` in
`HoverLspSuite`, keeping the existing `wiki-link-scheme-in-label` test only with
its own relevant comment.
- Around line 1085-1120: The ambiguity hover/navigation test only checks that
clickFirstLink returns no URI, so it does not verify that the failure was
actually reported as an ambiguous resolution. Update the affected HoverLspSuite
tests, including wiki-link-ambiguous-wildcard-no-navigation and the other
similar ambiguous cases, to also assert that client.showMessages contains the
“Could not uniquely resolve” message after the hover link interaction, matching
the pattern already used by wiki-link-java-duplicate-static-import,
wiki-link-overload-no-navigation, and related tests.
- Around line 3055-3081: The test in HoverLspSuite for
wiki-link-navigate-enum-case-param is too weak because it only checks that
clickFirstLink returns a URI ending in Color.scala, which would also pass if the
link incorrectly resolved to the enum companion. Strengthen the assertion by
checking the exact target location or range returned by clickFirstLink, using
the existing test setup around Color.scala and Main.scala to verify the wiki
link resolves to the case parameter r rather than just any symbol in the same
file.
🪄 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
Run ID: 0c941f7c-7aad-4c09-90c8-e0145ebd2389
📒 Files selected for processing (28)
metals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/ScaladocDefinitionProvider.scalametals/src/main/scala/scala/meta/internal/metals/ServerCommands.scalametals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scalamtags/src/main/scala/scala/meta/internal/metals/Docstrings.scalamtags/src/main/scala/scala/meta/internal/metals/JavadocIndexer.scalamtags/src/main/scala/scala/meta/internal/metals/ScaladocImportScope.scalamtags/src/main/scala/scala/meta/internal/metals/ScaladocIndexer.scalamtags/src/main/scala/scala/meta/internal/metals/docstrings/DocScope.scalamtags/src/main/scala/scala/meta/internal/metals/docstrings/MetalsSymbolLink.scalamtags/src/main/scala/scala/meta/internal/metals/docstrings/ScaladocParser.scalamtags/src/main/scala/scala/meta/internal/metals/docstrings/WikiLink.scalamtags/src/main/scala/scala/meta/internal/metals/docstrings/printers/MarkdownGenerator.scalamtags/src/main/scala/scala/meta/internal/mtags/JavacMtags.scalaproject/TestGroups.scalatests/mtest/src/main/scala/tests/DocstringMarkers.scalatests/mtest/src/main/scala/tests/PCSuite.scalatests/mtest/src/main/scala/tests/TestHovers.scalatests/mtest/src/main/scala/tests/TestInlayHints.scalatests/unit/src/test/scala/tests/DefinitionLspSuite.scalatests/unit/src/test/scala/tests/HoverLspSuite.scalatests/unit/src/test/scala/tests/JavadocSuite.scalatests/unit/src/test/scala/tests/MarkdownGeneratorSuite.scalatests/unit/src/test/scala/tests/MetalsSymbolLinkSuite.scalatests/unit/src/test/scala/tests/ScaladocSymbolsSuite.scalatests/unit/src/test/scala/tests/WikiLinkSuite.scala
| /** | ||
| * The target of the entity link whose brackets (`[[ ... ]]`, or any `n >= 2` | ||
| * matching brackets, mirroring the renderer) enclose `offset`, so source | ||
| * go-to-definition navigates exactly the links the renderer makes clickable | ||
| * (scalameta/metals#3383). | ||
| */ | ||
| /** | ||
| * The target of the Javadoc inline link (`{@link ... }` / `{@linkplain ... }`) | ||
| * whose braces enclose `offset`, matching what the renderer extracts so source | ||
| * clicks navigate the same links. The first token is the target; the rest is | ||
| * the label (scalameta/metals#3383). | ||
| */ | ||
| def javadocAtOffset(text: String, offset: Int): Option[String] = { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Orphaned Scaladoc comment — belongs to atOffset.
The block at Lines 41-46 documents the [[ ... ]] bracket-link-at-offset behavior, which is atOffset (Line 157), but it sits above javadocAtOffset (which already has its own doc at Lines 47-52). Meanwhile atOffset at Line 157 has no documentation. This reads as a copy/move artifact and misleads readers of this public helper.
Move the Lines 41-46 block above atOffset.
🤖 Prompt for 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.
In `@mtags/src/main/scala/scala/meta/internal/metals/docstrings/WikiLink.scala`
around lines 41 - 53, The Scaladoc for the bracket-link offset behavior is
attached to the wrong helper: the comment above javadocAtOffset describes the
atOffset behavior instead. Move that documentation block so it sits with
atOffset, and keep javadocAtOffset’s own inline-link doc in place; use the
atOffset and javadocAtOffset symbols to relocate the orphaned comment cleanly.
|
Hey @tgodzik 👋 If you were wondering where I vanished to for the last three weeks, this was it. 😄 Sorry about the size of the diff. It started as a small "make the broken hover wiki links clickable" change. Then it turned into a never-ending story of fixes. Every edge case I closed uncovered a couple more (renamed / Happy to split it into smaller PRs or walk you through any part if that makes review easier. 🙏 |
That would be quite welcome, this is a bit terrifying to review 😅 Do you have any fixes unrelated to the current PR we could split? |
|
I was thinking about this feature previously and I hoped we could just make the links become Sublime I think also has some link syntax we could use. |
| * `Resolved`; a miss or ambiguous/overloaded link is `NotUnique`; an | ||
| * unexpected (logged) error is `Failed` (scalameta/metals#3383). | ||
| */ | ||
| def resolveScaladocLink( |
There was a problem hiding this comment.
I wonder if we could reuse https://github.com/scalameta/metals/blob/main/metals/src/main/scala/scala/meta/internal/metals/ScaladocDefinitionProvider.scala
here (this might need some fixes)
…#8676) First part of #8658, paritally fixing #3383 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Scaladoc and Javadoc link parsing, including links with custom titles, nested brackets, signatures, and backticked names. * Fixed navigation so links resolve to their actual target symbols rather than displayed titles. * Correctly renders quoted `@see` text as plain text instead of a link. * **Tests** * Added coverage for complex link formats, offset detection, and definition navigation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…scalameta#8676) First part of scalameta#8658, paritally fixing scalameta#3383 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Bug Fixes** * Improved Scaladoc and Javadoc link parsing, including links with custom titles, nested brackets, signatures, and backticked names. * Fixed navigation so links resolve to their actual target symbols rather than displayed titles. * Correctly renders quoted `@see` text as plain text instead of a link. * **Tests** * Added coverage for complex link formats, offset detection, and definition navigation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit cb485be)
…scalameta#8676) First part of scalameta#8658, paritally fixing scalameta#3383 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Bug Fixes** * Improved Scaladoc and Javadoc link parsing, including links with custom titles, nested brackets, signatures, and backticked names. * Fixed navigation so links resolve to their actual target symbols rather than displayed titles. * Correctly renders quoted `@see` text as plain text instead of a link. * **Tests** * Added coverage for complex link formats, offset detection, and definition navigation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit cb485be)
2 participants
Closes #3383.
Scaladoc and Javadoc entity (wiki) links such as
[[scala.Foo]]and{@link Bar}are now navigable instead of rendering as broken links.Before / After
For
/** Returns a [[scala.Predef.String]]. */, hover used to render[String](scala.Predef.String), whose target is a bare fully-qualified name that opens an invalid location, and the link did nothing in the source comment. Now hover rendersStringas a goto-definition command link (or plain text on clients without command-link support), and clicking it in the source comment navigates to the definition.Solution
Links are resolved to their definitions, and fall back to plain text when a link is ambiguous or unresolvable or the client lacks command-link support, so a broken link is never shown. Because
mtagshas no symbol resolver or client config, the work spans two layers.mtagsmarks each link with a versioned sentinel scheme that carries a structuredDocScope(the owner symbol, per-scope import scope, language, source file, and dialect) into the rendered markdown. The server decodes the marker and rewrites each link to a goto command or plain text, and source go-to-definition rebuilds the same context from the syntax tree, so hover and source navigate identically.What's covered
[[...]]and Java{@link},{@linkplain}, and@see(inline and multi-line block) links are supported._root_, chained, and backticked-keyword aliases as well as the per-scope enclosing package.def/val/typemembers that live in the synthetic<file>$packageowner.Map[K, V]), and backtick and signature aware target/title splitting.Known limitations
[[List.apply]]or[[Child#inheritedMember]]fall back to plain text.given_<type>symbol cannot be rebuilt from syntax alone.scalaandjava.langscope, which the flat fallback tiers do not model.Summary by CodeRabbit
New Features
Bug Fixes