Skip to content

Listing diagnostics blame the wrong key: fix materialized container spans (bd-9yh3pzfu, bd-2mxo) - #459

Merged
cscheid merged 2 commits into
mainfrom
bugfix/bd-9yh3pzfu-diagnostics-blame-wrong-key
Aug 7, 2026
Merged

Listing diagnostics blame the wrong key: fix materialized container spans (bd-9yh3pzfu, bd-2mxo)#459
cscheid merged 2 commits into
mainfrom
bugfix/bd-9yh3pzfu-diagnostics-blame-wrong-key

Conversation

@cscheid

@cscheid cscheid commented Aug 6, 2026

Copy link
Copy Markdown
Member

The report

Rendering a Quarto 1 site ported to Q2 (the Posit Connect docs) produced a warning whose message and whose caret disagreed:

Warning: [Q-12-7] `template:` was set but `type:` is not `custom`; falling back to the built-in template for the declared type.
   ╭─[ cookbook/vanities/index.qmd:4:11 ]
   │
 4 │     sort: false
   │           ──┬──
   │             ╰──── `template:` was set but `type:` is not `custom`; …

The message talks about template:. The caret underlines sort: false — an unrelated sibling key.

Root cause

materialize_cursor synthesized a materialized container's span instead of preserving it:

  • map containers took their first entry's value span
  • array containers took their last item's span
  • a map whose first entry was itself a map got a programmatic_config sentinel — no location at all
  • every ConfigMapEntry.key_source was replaced with that sentinel

So every diagnostic anchored on a map, an array, or a key pointed somewhere arbitrary. Reordering the fixture's keys moved the caret, which is how this was confirmed rather than inferred.

The provenance was never lost, only discarded in flight. MergedConfig holds layers: Vec<&ConfigValue> — borrowed originals with spans and key_source intact — and MergedMap is a virtual map computed over them. keys() was already iterating the real ConfigMapEntry structs and keeping only entry.key.

What changed

Two commits, separately reviewable.

4698b85f — span assertions + blaming the right node

crates/ held 154 assertions on a diagnostic's code and roughly a dozen touching its location. That imbalance is why this survived: the existing Q-12-7 test asserts the code only and passed against the broken span for as long as the diagnostic existed.

Adds quarto_config::span_assert (behind a span-assert feature that quarto-core enables as a dev-dependency), resolving a SourceInfo to the (path, line, column, underlined text) a reader sees. It reports Original { FileId(0), 0..0 } as SuspiciousDefault rather than rendering it as "file 0, line 1" — a lenient helper would reproduce, inside the test suite, the exact failure mode the suite exists to catch.

Test fixtures have to come from real text: the existing helpers stamp SourceInfo::for_test() on everything, so a wrong span is indistinguishable from a right one. parse_from_yaml drives the real path — YAML → yaml_to_config_valueMergedConfigmaterializeparse_listings — matching what transforms/listing_generate.rs:72 reads at render time.

Call-site fixes: Q-12-7 now blames the template: entry; Q-12-4 blames the duplicate id: rather than the whole listing map.

d47b3799 — preserve spans through materialization (bd-2mxo)

Two additive MergedCursor accessors, container_source() and key_source(key), both walking layers in reverse so they follow the same layer as_value/as_scalar already pick for the winning value — a container's span and its winning contents agree on which file they came from. They return the layer's span verbatim, so a programmatically-built layer keeps saying it was generated instead of borrowing a neighbour's location.

unwrap_or_default() fallbacks became SourceInfo::generated(By::unknown()). SourceInfo::default() is Original { FileId(0), 0..0 } — a well-formed span indistinguishable downstream from a real location at the first byte of file 0. A fallback that fabricates a plausible location is worse than one that admits ignorance.

Reviewer notes

Three things that may not be obvious from the diff:

Zero .snap files changed, against a plan that predicted tree-wide churn. No existing snapshot ever captured a materialized container span — the same assertion gap that hid the bug made the fix invisible to the suite. Scalar spans were never affected, which the "winning layer" test independently confirms.

Containers nested inside arrays were never broken. The Array arm clones items verbatim rather than recursing through the cursor (array items have no path to navigate to), so only map-valued keys went through the synthesizing arm. This corrected an earlier audit of mine mid-review: Q-12-2 was fine all along (regression test added anyway), while Q-12-3 was genuinely broken and previously underlined "title".

A latent feature turned out to be dead. L5's categories_source exists solely to anchor Q-12-12 ("categories enabled but no item has any") and was receiving the sentinel — it could never point anywhere. Confirmed inert against a stashed tree; now resolves to the real categories: key.

Verification

  • Workspace: 10877 passed, 197 skipped
  • cargo xtask verify — full, including the WASM/hub-client leg — clean
  • End-to-end q2 render on the reported shape: caret moved from sort: false to template: ../template.ejs
  • Connect docs re-rendered: all 15 Q-12-7 instances now point at the template path, including the reported cookbook/vanities/index.qmd (4:11 → 5:15). Q-16-3 and Q-5-3 spans spot-checked in the same run — unaffected.

Each fix was confirmed failing before it was applied, by running the new tests against a stashed working tree.

Deliberately out of scope

The Connect docs still render poorly (9 errors, 310 warnings). That is expected and agreed: two further defects found during this investigation are filed separately, so this PR stays a source-mapping fix.

  • bd-oywyaouf — Q2 emits raw EJS into the HTML with no diagnostic when a Q1 .ejs template is used. Q2 dropped EJS deliberately (doctemplate replaced it so untrusted contexts like hub-client never execute arbitrary JS); nothing tells the user.
  • bd-lu16jgxq — Q-12-7's text asserts a type: the user never declared, and the docs page misstates Q1's behavior (Q1 makes template: imply type: custom).
  • bd-cwk7l4dl — xtask lint for unwrap_or_default() on SourceInfo; deferred because it needs type information a grep rule lacks.

Closes bd-2mxo. Plan: claude-notes/plans/2026-08-06-q12-7-listing-template-diagnostic.md

🤖 Generated with Claude Code

cscheid and others added 2 commits August 6, 2026 11:53
…pzfu)

Q-12-7's message talks about `template:` while its caret underlined an
unrelated sibling key. Reported against a Q1 site ported to Q2, where

    listing:
        sort: false
        template: ../template.ejs

produced a warning about `template:` pointing at `sort: false`.

Root cause is that a materialized map's `source_info` is synthesized from
its *first entry's value* (quarto-config/src/materialize.rs:142-158), so
blaming the enclosing map underlines whichever key happens to come first.
That defect is bd-2mxo and is fixed separately; this commit fixes the
diagnostics that should not have been blaming a container in the first
place. Both fixes are needed: quarto-yaml spans a mapping from its first
key to MappingEnd, so even a correct map span would underline the whole
`listing:` block rather than the `template:` line.

Phase 0 — make spans assertable. `crates/` held 154 assertions on a
diagnostic's `code` and roughly a dozen touching its location, which is
why this survived: the existing Q-12-7 test asserts the code only and
passed against the broken span for as long as the diagnostic existed.

Adds `quarto_config::span_assert` (behind a `span-assert` feature that
quarto-core enables as a dev-dependency) to resolve a SourceInfo to the
concrete (path, line, column, underlined text) a reader sees. It reports
`Original { FileId(0), 0..0 }` as SuspiciousDefault rather than rendering
it as "file 0, line 1" — a lenient helper would reproduce, inside the
test suite, the exact failure mode the suite exists to catch.

Test fixtures must come from real text: the existing helpers stamp every
value with `SourceInfo::for_test()`, so a wrong span is indistinguishable
from a right one. `parse_from_yaml` drives the real path — YAML ->
yaml_to_config_value -> MergedConfig -> materialize -> parse_listings —
matching what transforms/listing_generate.rs:72 reads at render time.
Going through materialize is the point; skipping it skips the defect.

Phase A — blame the right node.

- Q-12-7 now blames the `template:` entry (`template_source`), captured
  while walking the listing map.
- Q-12-4 now blames the duplicate `id:` value rather than the whole
  listing map, via a new `map_entry_value` helper.

Both were confirmed failing before their fix. Q-12-4 previously
underlined "contents: ./b.qmd\n      id: dupe\n".

Audited but deliberately unchanged: Q-12-2 (inline contents record),
Q-12-1 (non-map listing entry) and Q-12-3 (sort shape) already blame the
semantically correct value and read wrong only because that value's
container span is wrong — those are bd-2mxo's to fix.

No snapshot files changed. Full workspace suite: 10869 passed, 197
skipped. `cargo xtask verify --skip-hub-build` clean.

Plan: claude-notes/plans/2026-08-06-q12-7-listing-template-diagnostic.md

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…(bd-2mxo)

`materialize_cursor` synthesized a materialized container's span instead
of preserving it:

- map containers took their *first entry's value* span
- array containers took their *last item's* span
- a map whose first entry was itself a map got a programmatic-config
  sentinel — no location at all
- every `ConfigMapEntry.key_source` was replaced with that sentinel

Every diagnostic anchored on a map, an array, or a key therefore pointed
somewhere arbitrary. Q-12-7 underlining `sort: false` while talking about
`template:` (bd-9yh3pzfu) was the visible symptom.

The provenance was never lost, only discarded in flight. `MergedConfig`
holds `layers: Vec<&ConfigValue>` — borrowed originals with spans and
`key_source` intact — and `MergedMap` is a virtual map computed over
them. `keys()` was already iterating the real `ConfigMapEntry` structs
and keeping only `entry.key`.

Adds two accessors on `MergedCursor`:

- `container_source()` — the container's span from the highest-priority
  layer holding a value at this path
- `key_source(key)` — that key's own span, from the layer supplying the
  winning value

Both walk layers in reverse, matching the layer `as_value`/`as_scalar`
already pick, so a container's span and its winning contents agree on
which file they came from. They return the layer's span verbatim, so a
programmatically-built layer keeps saying it was generated rather than
borrowing a neighbour's location.

The `unwrap_or_default()` fallbacks are replaced with
`SourceInfo::generated(By::unknown())`. `SourceInfo::default()` is
`Original { FileId(0), 0..0 }` — a well-formed span indistinguishable
downstream from a real location at the first byte of file 0. A fallback
that fabricates a plausible location is worse than one that admits
ignorance. (Upstream fix written up for a separate session in
claude-notes/scratch/2026-08-06-memo-quarto-source-map-default-sourceinfo.md.)

Fixed as a side effect: L5's `categories_source` capture, which exists
solely to anchor Q-12-12 ("categories enabled but no item has any"), was
receiving the sentinel — the feature could never point anywhere.
Confirmed inert against a stashed tree, and now resolves to the real
`categories:` key.

Scope correction to the earlier Phase A audit, found while verifying:
the Array arm clones items verbatim rather than recursing through the
cursor, so containers nested *inside arrays* always kept correct spans.
Only map-valued keys went through the synthesizing arm. That means
Q-12-2 (inline `contents:` record) was never broken — a regression test
is added — while Q-12-3 (`sort:` shape) was, and is confirmed fixed: it
previously underlined "title".

Verification:
- Workspace: 10877 passed, 197 skipped.
- `cargo xtask verify` (full, including the WASM/hub-client leg) clean.
- End-to-end `q2 render` on the reported shape: caret moved from
  `sort: false` to `template: ../template.ejs`.
- Connect docs re-rendered: all 15 Q-12-7 instances now point at the
  template path, including the reported cookbook/vanities/index.qmd
  (4:11 -> 5:15). Q-16-3 and Q-5-3 spans spot-checked in the same run —
  unaffected.

Snapshot report: **zero .snap files changed**, against a plan that
predicted tree-wide churn. No existing snapshot ever captured a
materialized container span — the same assertion gap that hid the bug
made the fix invisible to the suite. Scalar spans were never affected.

Plan: claude-notes/plans/2026-08-06-q12-7-listing-template-diagnostic.md

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@posit-snyk-bot

posit-snyk-bot commented Aug 6, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@cscheid
cscheid merged commit c9f6177 into main Aug 7, 2026
8 of 14 checks passed
@cscheid
cscheid deleted the bugfix/bd-9yh3pzfu-diagnostics-blame-wrong-key branch August 7, 2026 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants