Repository navigation
feat: add an experimental quarto.validate.links.enabled setting for LSP link diagnostics - #1186
nealrichardson wants to merge 6 commits into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Converting to draft: the approach needs a decision before this merges. Once enabled, the link diagnostics give two Quarto-specific false positives:
As written, this PR reuses VS Code's Options:
Maintainer input welcome on which way to go. |
|
Hmmmmm, interesting 👀 I believeI prefer option 1, a
The explicit ID fix can be small if I am understanding it correctly. Tokens in From where I sit, I think the |
ConfigurationManager.update() rebuilt the markdown section from the defaults on every update, so getDiagnosticsOptions() always returned enabled: false and the link diagnostics could never be turned on. Also request the markdown.validate section (VS Code's built-in Markdown settings) and convert it, falling back to the defaults for missing or invalid values. Validation stays off unless markdown.validate.enabled is true. Add unit tests for apps/lsp (now part of yarn test-packages) and an end-to-end test in lsp-features.test.ts. Fixes quarto-dev#1185 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Add an experimental `quarto.validate.links.enabled` setting, off by default, as the switch for link diagnostics in Quarto documents. The per-rule levels and ignored links still come from `markdown.validate.*` (falling back to VS Code's defaults when the client doesn't send them), but `markdown.validate.enabled` is ignored, so users who turned it on for `.md` files don't get new warnings in `.qmd` files. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e LSP
The table of contents only knew the identifier generated from a heading's
text, so `[see](#sec-intro)` pointing at `## Introduction {#sec-intro}` was
reported as "No header found", and completions, go to definition and
references used the wrong identifier. As in Pandoc, an explicit `{#id}` now
replaces the generated one (and is reserved so later generated identifiers
don't reuse it).
Divs with an identifier (`::: {#fig-x}`, callouts, theorems) are now valid
fragment targets too. They're kept apart from the outline entries, so the
document outline, folding and completions are unchanged.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
756113b to
f921339
Compare
…iption `#markdown.validate.*#` isn't a setting ID, so VS Code wouldn't render it as a link. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`lsp-features.qmd` linked `[the section](#first-section)` to
`## First Section {#sec-first}`. Pandoc gives that heading only the
`sec-first` identifier, so the link is broken, and now that the LSP follows
Pandoc, find references no longer counts it. Link to `#sec-first` instead,
and cover references to explicit identifiers in the LSP unit tests.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
🤖 Thanks @juliasilge, this now follows option 1:
Not covered, left for follow-ups: IDs on inline content the block parser doesn't record ( CI is green, so I'm marking this ready for review. |
| if (!id) { | ||
| return undefined; | ||
| } | ||
| const slug = pandocSlugifier.fromHeading(id); |
There was a problem hiding this comment.
pandocSlugifier.fromHeading(id) changes the explicit identifier, but Pandoc uses an explicit {#id} exactly as the author wrote it. The slugifier makes the text lowercase, removes punctuation such as :, and removes all characters before the first letter.
This causes a couple of problems, I think:
#path completion offers the wrong target. For## Results {#sec-Results}, it offers#sec-results, which is a broken link in the rendered HTML (completion-path.ts:335usesentry.slug.value).lookup()also slugifies the fragment, so links with the wrong case pass the new link diagnostics. In my manual test,[x](#sec_mixed)for## Mixed Case {#Sec_Mixed}got no diagnostic.- Different identifiers can become the same slug.
{#1-intro}becomesintroand collides with the generated identifier of## Intro. Also,{#fig:plot}matches#figplot. - If the slug is empty (for example,
::: {#123}), every fragment that also slugifies to an empty string resolves to that div.
A possible fix would be to keep the explicit identifier verbatim as the slug, and compare it to the raw fragment with no slugifier. We could use the slugifier comparison only for generated identifiers. references.ts:164 also slugifies the fragment before it compares, so it would need the same change.
Do you think this is worth fixing now? I did notice it in my manual playing around here.
| const slug = this.#slugifier.fromHeading(fragment); | ||
| return this.entries.find(entry => entry.slug.equals(slug)); | ||
| return this.entries.find(entry => entry.slug.equals(slug)) | ||
| ?? this.anchors.find(entry => entry.slug.equals(slug)); |
There was a problem hiding this comment.
lookup() now returns div anchors, so a link such as [see](#fig-x) to ::: {#fig-x} gets no diagnostic, but navigation to the div does not work. All of the navigation providers make sure that the entry is a heading with isTocHeaderEntry() before they use it, and a div entry has no headerLocation:
- Go to definition (
definitions.ts:87) returns nothing. - Cmd/Ctrl+click on the link (
document-links.ts:899) opens the file at the top, not at the div. - Find references (
references.ts:242) does not include the div as the definition. - Document highlights (
document-highlights.ts:114) do not highlight the div.
Code cell labels (#| label: fig-x) had the same gap before this PR so this is not a new kind of problem, but the CHANGELOG says that div identifiers "are also recognized as valid targets", and users would probably expect navigation from that claim.
We could let these providers use sectionLocation when the entry is not a heading, so that navigation goes to the div (and to code cells). Or alternatively, would you prefer to update the CHANGELOG with something more accurate?

Fixes #1185
The language server's link diagnostics (undefined reference links, missing headers, missing files, unused and duplicate link definitions) could never be turned on.
ConfigurationManager.update()rebuiltmarkdown.validatefrom the defaults (enabled: false) on every update and never read it from the client. This has been the case since 09dc066 (2023).Following the discussion below (option 1), the diagnostics are now turned on by a new Quarto setting, and the false positives on Quarto's explicit identifiers are fixed.
Changes
quarto.validate.links.enabled(boolean, defaultfalse, taggedexperimental). It's the only switch for link diagnostics in Quarto documents.markdown.validate.*settings:referenceLinks.enabled,fragmentLinks.enabled,fileLinks.enabled,fileLinks.markdownFragmentLinks,unusedLinkDefinitions.enabled,duplicateLinkDefinitions.enabledandignoredLinks. Missing or invalid values fall back to VS Code's defaults (warning, orhintfor unused definitions), so the setting still works if the client doesn't send them.markdown.validate.enabledis ignored. Users who turned it on for.mdfiles don't get new warnings in.qmdfiles.other.html) are reported as missing, and suggests linking to the.qmdsource or usingmarkdown.validate.ignoredLinks.apps/lsp/src/service/toc.ts). The table of contents used only the identifier generated from a heading's text, so[see](#sec-intro)pointing at## Introduction {#sec-intro}was reported as "No header found".{#id}(already parsed into the token'sattrby quarto-core) replaces the generated identifier, and is reserved so a later generated identifier doesn't reuse it. Besides diagnostics, this fixes go to definition, find references, highlights and#path completions for such headings. They used the generated identifier before, and that identifier doesn't exist in the rendered document.::: {#fig-x},::: {.callout-note #nte-x},::: {#thm-x}) are now valid fragment targets. They're kept in a separate list from the outline entries, so the outline, folding, smart select and completions don't change. Code cell labels (#| label: fig-x) were already targets.apps/lspgets its first unit tests (test/config.test.ts,test/link-targets.test.ts, helpers intest/workspace.ts) and atestscript, andyarn test-packagesnow includesquarto-lsp. They cover:markdown.validate.enabled: true;markdown.validate.*, and VS Code's defaults when those are missing or invalid;#introductionfor## Introduction {#sec-intro}is still flagged;apps/vscode/src/test/lsp-features.test.ts: an end-to-end test turns onquarto.validate.links.enabled, waits for a diagnostic on a broken fragment link in a new fixture (lsp-link-diagnostics.qmd), then turns the setting off and waits for the diagnostic to clear. The existinglsp-features.qmdfixture linked[the section](#first-section)to## First Section {#sec-first}, a link that's broken in the rendered document; it now links to#sec-first, so the "references to a header" test checks the Pandoc behavior.apps/vscode/CHANGELOG.mdlines: the new setting, and the explicit identifier fix.Not covered, for follow-ups
[..](#id): figures ({#fig-x}), tables (: Caption {#tbl-x}), equations ($$ ... $$ {#eq-x}), listings (code blocks with{#lst-x}), and spans ([text]{#id}). Quarto documents usually reference these with@fig-x, not links, so I left them out. Fixing them needs inline attribute parsing in quarto-core's markdown-it parser.other.htmlfromother.qmd) are reported as missing files. Quarto recommends linking to the.qmdsource, so this is lower priority. It's noted in the setting's description.markdown.*settings the server models (preferredMdPathExtensionStyle,suggest.paths.*) are still never read from the client.Verification
On Node 24, rebased onto
main(a83c5cc):yarn install --frozen-lockfile: lockfile unchanged.yarn build --force: 14/14 tasks.yarn test-packages: 14/14 tasks (13 onmain, plusquarto-lsp), including the 11quarto-lsptests.yarn lint --continue: 17/17 tasks, 0 errors, 17 warnings, all inquarto, the same as onmain.apps/vscodeyarn build-test: compiles.yarn test-vscodewasn't run locally: the VS Code tests open GUI windows, and no headless container was available. CI runs the newlsp-features.test.tstest.🤖 Generated with Claude Code