diff --git a/.claude/docs/plans/pandoc-list-vs-blocks-analysis.md b/.claude/docs/plans/pandoc-list-vs-blocks-analysis.md new file mode 100644 index 00000000000..f5daa300cbe --- /dev/null +++ b/.claude/docs/plans/pandoc-list-vs-blocks-analysis.md @@ -0,0 +1,256 @@ +# pandoc.List() vs pandoc.Blocks() Bug Analysis + +## Executive Summary + +The bug fixed in manuscript.lua (PR #13624) is **NOT ConditionalBlock-specific**. It's a type mismatch bug where `pandoc.List()` is used instead of `pandoc.Blocks()` when building block collections that are assigned to element `.content` properties. ConditionalBlock merely **exposed** this bug, but the bug could be triggered by any code path that walks the AST of an element with wrongly-typed content. + +## The Core Bug: Type Mismatch + +### What's the Difference? + +From the Lua type definitions in quarto-cli: + +**pandoc.Blocks** (`src/resources/pandoc/blocks.lua:27-29`): +```lua +-- List of Block elements, a special case of a pandoc.List. +-- pandoc.Blocks is a type alias for pandoc.List, but it also +-- supports a `walk` method. +``` + +**pandoc.List** (`src/resources/pandoc/List.lua`): +- Standard Lua List implementation +- Has methods like `:insert()`, `:extend()`, `:map()`, etc. +- Does NOT have a `.walk()` method in the standard implementation + +### The Bug Pattern + +```lua +-- BUGGY CODE +local blocks = pandoc.List() +for _, child in ipairs(divEl.content) do + blocks:insert(child) +end +divEl.content = blocks -- BUG: Assigns List to .content + +-- Later in the filter chain... +_quarto.ast.walk(divEl, {...}) -- CRASH: tries to walk divEl.content +``` + +When you assign a `pandoc.List()` to `.content`, any subsequent AST walking operations that expect `pandoc.Blocks` (with its `.walk()` method) will fail. + +### Why It Crashes + +The crash happens when: +1. Code assigns `pandoc.List()` to an element's `.content` property +2. That element is returned from a filter or passed to another filter +3. Some code tries to walk the AST, either: + - Directly via `_quarto.ast.walk(element, ...)` + - Indirectly when a filter returns `element.content` and Pandoc's filter system tries to process it + - When custom filters traverse the AST + +The error manifests as attempting to call a method that doesn't exist on `pandoc.List`. + +## ConditionalBlock's Role: Exposing, Not Causing + +### What ConditionalBlock Does + +From `src/resources/filters/quarto-pre/content-hidden.lua:63`: +```lua +function conditionalBlock(show) + return function(el) + if show then + return el.content -- Returns the Blocks directly + else + return {} + end + end +end +``` + +When `show` is true, ConditionalBlock returns `el.content` **as-is**. This propagates whatever type `.content` is up the filter chain. + +### How It Exposed the Bug + +1. manuscript.lua assigns `pandoc.List()` to `divEl.content` +2. That `divEl` contains a ConditionalBlock +3. ConditionalBlock's filter runs and returns `el.content` (the wrongly-typed List) +4. Later code tries to walk this returned content +5. CRASH: `pandoc.List` doesn't have the expected methods + +**But ConditionalBlock is just the messenger!** The bug was already there when `pandoc.List()` was assigned to `.content`. ConditionalBlock just exposed it by propagating the wrong type upward. + +## Other Potential Triggers (Without ConditionalBlock) + +The bug could be triggered by: + +### 1. Direct AST Walking +```lua +local blocks = pandoc.List() +divEl.content = blocks +_quarto.ast.walk(divEl, {...}) -- CRASH: even without ConditionalBlock +``` + +### 2. Custom Filters That Return .content +Any custom filter extension that does: +```lua +function MyFilter(el) + return el.content -- If content is wrongly-typed List +end +``` + +### 3. Filter Chain Interactions +When filters compose and one expects `pandoc.Blocks` but receives `pandoc.List`. + +### 4. Format-Specific Processing +Some output formats may do additional AST traversal that would trigger the crash. + +## Analysis of Each Occurrence + +### manuscript.lua (Line 68) - **BUGGY** ✗ + +**Code**: +```lua +local blocks = pandoc.List() +-- ... build blocks ... +divEl.content = blocks -- Assigns List to .content +``` + +**Why it's buggy**: +- Assigns `pandoc.List()` to `divEl.content` +- Later code walks the AST (line 138: `_quarto.ast.walk(divEl, ...)`) +- Exposed by ConditionalBlock, but would crash on any AST walk + +**Fix applied**: Changed to `pandoc.Blocks({})` + +### outputs.lua (Line 41) - **SAFE** ✓ + +**Code**: +```lua +local blocks = pandoc.List() +-- ... build blocks ... +return blocks -- Returns directly from filter +``` + +**Why it's safe**: +- Returns `pandoc.List()` directly from the filter +- Does NOT assign to `.content` +- Pandoc's filter system can handle returned Lists +- The returned blocks are used to replace content, not stored as-is + +**No fix needed**: Return pattern is safe. + +### jats.lua quarto-post (Line 57) - **SAFE** ✓ + +**Code**: +```lua +function unrollDiv(div, fnSkip) + local blocks = pandoc.List() + -- ... build blocks ... + return blocks -- Returns directly +end +``` + +**Why it's safe**: Same as outputs.lua - returns directly, never assigns to `.content`. + +**No fix needed**: Return pattern is safe. + +### jats.lua quarto-post (Line 175) - **BUGGY** ✗ + +**Code**: +```lua +local orderedContents = pandoc.List() +orderedContents:extend(otherEls) +orderedContents:extend(outputEls) +div.content = orderedContents -- Assigns List to .content +``` + +**Why it's buggy**: +- Assigns `pandoc.List()` to `div.content` +- Same pattern as manuscript.lua +- Could crash if this div is later walked or returned by a filter like ConditionalBlock + +**Needs fix**: Change line 172 to `pandoc.Blocks({})` + +### dashboard.lua (Lines 248, 296) - **SAFE** ✓ + +**Code**: +```lua +local results = pandoc.List() +-- ... build results ... +return pandoc.Blocks(results) -- Wraps in pandoc.Blocks before return +``` + +**Why it's safe**: +- Uses `pandoc.List()` internally (fine for building) +- Wraps in `pandoc.Blocks()` before returning +- Returns the correct type + +**No fix needed**: Already handles type correctly. + +## The Fix Pattern + +### When to Use pandoc.Blocks() + +Use `pandoc.Blocks({})` when: +1. Building a collection that will be assigned to `.content` +2. Building a collection that will be part of the document AST +3. Building blocks that need to support `.walk()` operations + +```lua +-- CORRECT +local blocks = pandoc.Blocks({}) +blocks:extend(childContent) +element.content = blocks +``` + +### When pandoc.List() is OK + +Use `pandoc.List()` when: +1. Building a temporary collection that will be returned directly (filter system handles it) +2. Building a collection that will be wrapped in `pandoc.Blocks()` before use +3. Building non-Block collections (like List of Inlines, table rows, etc.) + +```lua +-- CORRECT: Direct return +local blocks = pandoc.List() +blocks:insert(something) +return blocks -- Filter system handles conversion + +-- CORRECT: Explicit wrap +local blocks = pandoc.List() +blocks:insert(something) +return pandoc.Blocks(blocks) + +-- CORRECT: Non-block collection +local inlines = pandoc.List() -- Building Inlines, not Blocks +``` + +## Root Cause Summary + +The bug is fundamentally about **type safety in Lua's Pandoc AST**: + +1. `.content` properties expect `pandoc.Blocks` (which has `.walk()`) +2. Using `pandoc.List()` creates wrong type without required methods +3. Crash occurs when AST traversal tries to use `.walk()` on the List +4. ConditionalBlock exposed the bug by returning wrongly-typed content +5. But ANY AST walking or content propagation could trigger it + +## Recommended Actions + +1. **Fix jats.lua line 172**: Change `pandoc.List()` to `pandoc.Blocks({})` +2. **Test verification**: Create tests for both manuscript and JATS formats with ConditionalBlock +3. **Pattern search**: Search codebase for `.content = .*pandoc.List` pattern +4. **Documentation**: Consider adding a code comment in blocks.lua about when to use which type + +## Testing Strategy + +Tests should verify: +1. Documents with ConditionalBlock in manuscript format (already exists) +2. Documents with ConditionalBlock in JATS format (needs creation) +3. Direct AST walking on elements with `.content` assigned (stress test) +4. Filter composition scenarios + +The tests should confirm that: +- Fixed code works with ConditionalBlock +- Fixed code works with direct AST walking +- Safe patterns (outputs.lua, dashboard.lua) continue working diff --git a/src/resources/filters/layout/manuscript.lua b/src/resources/filters/layout/manuscript.lua index a0a3f18cc40..ab21d42e138 100644 --- a/src/resources/filters/layout/manuscript.lua +++ b/src/resources/filters/layout/manuscript.lua @@ -57,10 +57,10 @@ function manuscript() -- If this is a notebook embed cell, 'lift' the contents of any child divs -- up (unroll their contents), this will help us avoid -- labeling divs marked as `cells` more than once - local blocks = pandoc.List() + local blocks = pandoc.Blocks({}) for _, childBlock in ipairs(divEl.content) do if is_regular_node(childBlock, "Div") then - tappend(blocks, childBlock.content) + blocks:extend(childBlock.content) else blocks:insert(childBlock) end diff --git a/tests/docs/smoke-all/2025/10/27/13616/.gitignore b/tests/docs/smoke-all/2025/10/27/13616/.gitignore new file mode 100644 index 00000000000..ad293093b07 --- /dev/null +++ b/tests/docs/smoke-all/2025/10/27/13616/.gitignore @@ -0,0 +1,2 @@ +/.quarto/ +**/*.quarto_ipynb diff --git a/tests/docs/smoke-all/2025/10/27/13616/13616.qmd b/tests/docs/smoke-all/2025/10/27/13616/13616.qmd new file mode 100644 index 00000000000..85e7938dae9 --- /dev/null +++ b/tests/docs/smoke-all/2025/10/27/13616/13616.qmd @@ -0,0 +1,13 @@ +--- +title: "Conditional Block in manuscript article" +_quarto: + tests: + pdf: + noErrors: true +--- + +::: {.content-visible when-format="pdf"} + +Something + +::: diff --git a/tests/docs/smoke-all/2025/10/27/13616/_quarto.yml b/tests/docs/smoke-all/2025/10/27/13616/_quarto.yml new file mode 100644 index 00000000000..2539ccd82a5 --- /dev/null +++ b/tests/docs/smoke-all/2025/10/27/13616/_quarto.yml @@ -0,0 +1,9 @@ +project: + type: manuscript + +manuscript: + article: 13616.qmd + +format: + pdf: + pdf-engine: xelatex