From 26caa66ff94079733dfee0999cfbccb5a262db8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Canouil?= <8896044+mcanouil@users.noreply.github.com> Date: Sat, 11 Apr 2026 21:47:52 +0200 Subject: [PATCH 1/8] fix(format): strip cell option directives before formatting embedded code Quarto code cells carry metadata on leading comment lines (`#| label: foo`, `#| echo: false`, `//| label: bar`, ...) that must survive verbatim so Quarto can parse them on the next render. Previously, `formatBlock` handed the entire cell to the language formatter (Black, autopep8, styler, ...), which treats these lines as ordinary comments and may reflow, reorder, or rewrite them. Hide the option lines from the formatter entirely by slicing them off `blockLines` before calling `virtualDocForCode`, and shift the returned edits back into the real code range via `block.range.start.line + 1 + optionLines`. Blocks that are entirely option directives short-circuit to `undefined` (no formatter invocation, no out-of-range toast). This replaces the filter-after approach that dropped any edit whose start line landed in the option region, which quietly swallowed valid multi-line edits and block-wide edits starting at virtual-doc line 0. --- apps/vscode/src/providers/format.ts | 50 ++++++++++++++++++++++------- apps/vscode/src/providers/option.ts | 2 +- 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/apps/vscode/src/providers/format.ts b/apps/vscode/src/providers/format.ts index 73cbd3b3..2f8f2249 100644 --- a/apps/vscode/src/providers/format.ts +++ b/apps/vscode/src/providers/format.ts @@ -45,6 +45,7 @@ import { virtualDocForLanguage, withVirtualDocUri, } from "../vdoc/vdoc"; +import { languageOptionComment } from "./option"; export function activateCodeFormatting(engine: MarkdownEngine) { return [new FormatCellCommand(engine)]; @@ -247,9 +248,34 @@ async function formatBlock(doc: TextDocument, block: TokenMath | TokenCodeBlock) return undefined; } - // Create virtual document containing the block const blockLines = lines(codeForExecutableLanguageBlock(block, false)); - const vdoc = virtualDocForCode(blockLines, language); + + // Count leading Quarto option directives (e.g. `#| label: foo`) so we can + // hide them from the formatter entirely. Feeding these lines to formatters + // like Black or styler risks reflowing or rewriting them, which would + // silently break the cell's behaviour on the next render. + const languageComment = languageOptionComment(language.ids[0]); + const optionPrefix = languageComment ? languageComment + "| " : undefined; + let optionLines = 0; + if (optionPrefix) { + for (const line of blockLines) { + if (line.startsWith(optionPrefix)) { + optionLines++; + } else { + break; + } + } + } + + // Nothing to format if the block is entirely option directives. + if (optionLines === blockLines.length) { + return undefined; + } + + // Create virtual document containing only the code portion of the block + // so the formatter never sees the option directives. + const codeLines = blockLines.slice(optionLines); + const vdoc = virtualDocForCode(codeLines, language); const edits = await executeFormatDocumentProvider( vdoc, @@ -265,19 +291,21 @@ async function formatBlock(doc: TextDocument, block: TokenMath | TokenCodeBlock) // Because we format with the block code copied in an empty virtual // document, we need to adjust the ranges to match the edits to the block - // cell in the original file. + // cell in the original file. The `+ 1` skips the opening fence line and + // `+ optionLines` skips the leading option directives we hid from the + // formatter. + const lineOffset = block.range.start.line + 1 + optionLines; const blockRange = new Range( new Position(block.range.start.line, block.range.start.character), new Position(block.range.end.line, block.range.end.character) ); - const adjustedEdits = edits - .map(edit => { - const range = new Range( - new Position(edit.range.start.line + block.range.start.line + 1, edit.range.start.character), - new Position(edit.range.end.line + block.range.start.line + 1, edit.range.end.character) - ); - return new TextEdit(range, edit.newText); - }); + const adjustedEdits = edits.map(edit => { + const range = new Range( + new Position(edit.range.start.line + lineOffset, edit.range.start.character), + new Position(edit.range.end.line + lineOffset, edit.range.end.character) + ); + return new TextEdit(range, edit.newText); + }); // Bail if any edit is out of range. We used to filter these edits out but // this could bork the cell. Return `[]` to indicate that we tried. diff --git a/apps/vscode/src/providers/option.ts b/apps/vscode/src/providers/option.ts index eff92603..885322ec 100644 --- a/apps/vscode/src/providers/option.ts +++ b/apps/vscode/src/providers/option.ts @@ -89,7 +89,7 @@ function handleOptionEnter(editor: TextEditor, comment: string) { } } -function languageOptionComment(language: string) { +export function languageOptionComment(language: string) { // some mappings if (language === "ojs") { language = "js"; From 515fd101c047e27d84f1096a17d78d7555ffafad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Canouil?= <8896044+mcanouil@users.noreply.github.com> Date: Sat, 11 Apr 2026 21:48:01 +0200 Subject: [PATCH 2/8] test(format): cover cell option protection with deterministic fixtures Add a `Code Block Formatting` suite that registers a fake Python formatter from a pure `string -> string` function, so each case can prove exactly what did and did not get rewritten. The fake formatters mangle `=` and `+` into spaced forms, rewrite `#| ` to `# PIPE`, and normalise comment spacing. Cover six scenarios: a single directive followed by code, multiple directives preserved in original order, a block with no directives, a directives-only block (no-op), multiline code with badly-formatted standalone and inline comments, and a hostile formatter run against a mixed block to prove the directives never reach the formatter at all. --- .../format-python-multiline-with-comments.qmd | 14 + .../format-python-multiple-options.qmd | 12 + .../examples/format-python-no-options.qmd | 9 + .../examples/format-python-only-options.qmd | 9 + .../src/test/examples/format-python.qmd | 10 + apps/vscode/src/test/formatting.test.ts | 275 ++++++++++++++++++ 6 files changed, 329 insertions(+) create mode 100644 apps/vscode/src/test/examples/format-python-multiline-with-comments.qmd create mode 100644 apps/vscode/src/test/examples/format-python-multiple-options.qmd create mode 100644 apps/vscode/src/test/examples/format-python-no-options.qmd create mode 100644 apps/vscode/src/test/examples/format-python-only-options.qmd create mode 100644 apps/vscode/src/test/examples/format-python.qmd create mode 100644 apps/vscode/src/test/formatting.test.ts diff --git a/apps/vscode/src/test/examples/format-python-multiline-with-comments.qmd b/apps/vscode/src/test/examples/format-python-multiline-with-comments.qmd new file mode 100644 index 00000000..62727821 --- /dev/null +++ b/apps/vscode/src/test/examples/format-python-multiline-with-comments.qmd @@ -0,0 +1,14 @@ +--- +title: Formatting Multiline Python Code Cells with Comments +subtitle: https://github.com/quarto-dev/quarto/pull/655 +format: html +--- + +```{python} +#| label: multiline +#| echo: false +#standalone comment +x=1 #inline comment +y=2 +z=x+y +``` diff --git a/apps/vscode/src/test/examples/format-python-multiple-options.qmd b/apps/vscode/src/test/examples/format-python-multiple-options.qmd new file mode 100644 index 00000000..d4b12878 --- /dev/null +++ b/apps/vscode/src/test/examples/format-python-multiple-options.qmd @@ -0,0 +1,12 @@ +--- +title: Formatting Python Code Cells with Multiple Options +subtitle: https://github.com/quarto-dev/quarto/pull/655 +format: html +--- + +```{python} +#| label: multi +#| echo: false +#| warning: false +x=1;y=2;z=x+y +``` diff --git a/apps/vscode/src/test/examples/format-python-no-options.qmd b/apps/vscode/src/test/examples/format-python-no-options.qmd new file mode 100644 index 00000000..e5efdcc6 --- /dev/null +++ b/apps/vscode/src/test/examples/format-python-no-options.qmd @@ -0,0 +1,9 @@ +--- +title: Formatting Python Code Cells without Options +subtitle: https://github.com/quarto-dev/quarto/pull/655 +format: html +--- + +```{python} +x=1;y=2;z=x+y +``` diff --git a/apps/vscode/src/test/examples/format-python-only-options.qmd b/apps/vscode/src/test/examples/format-python-only-options.qmd new file mode 100644 index 00000000..d07e350d --- /dev/null +++ b/apps/vscode/src/test/examples/format-python-only-options.qmd @@ -0,0 +1,9 @@ +--- +title: Formatting Python Code Cells with Only Options +subtitle: https://github.com/quarto-dev/quarto/pull/655 +format: html +--- + +```{python} +#| label: only-options +``` diff --git a/apps/vscode/src/test/examples/format-python.qmd b/apps/vscode/src/test/examples/format-python.qmd new file mode 100644 index 00000000..5c929cb0 --- /dev/null +++ b/apps/vscode/src/test/examples/format-python.qmd @@ -0,0 +1,10 @@ +--- +title: Formatting Python Code Cells +subtitle: https://github.com/quarto-dev/quarto/pull/655 +format: html +--- + +```{python} +#| label: my-code +x=1;y=2;z=x+y +``` diff --git a/apps/vscode/src/test/formatting.test.ts b/apps/vscode/src/test/formatting.test.ts new file mode 100644 index 00000000..3a596eed --- /dev/null +++ b/apps/vscode/src/test/formatting.test.ts @@ -0,0 +1,275 @@ +import * as vscode from "vscode"; +import * as assert from "assert"; +import { openAndShowExamplesTextDocument, wait } from "./test-utils"; + +/** + * Creates a document formatting provider from a formatting function. + * @param format - Function that transforms source text + * @returns Document formatting edit provider + */ +function createFormatterFromStringFunc( + format: (sourceText: string) => string +): vscode.DocumentFormattingEditProvider { + return { + provideDocumentFormattingEdits( + document: vscode.TextDocument + ): vscode.TextEdit[] { + const text = document.getText(); + const formatted = format(text); + return [ + new vscode.TextEdit( + new vscode.Range( + document.positionAt(0), + document.positionAt(text.length) + ), + formatted + ), + ]; + }, + }; +} + +/** + * Sets the cursor position in the active editor. + * @param line - Line number + * @param character - Character position + */ +function setCursorPosition(line: number, character: number): void { + const editor = vscode.window.activeTextEditor; + if (editor) { + const position = new vscode.Position(line, character); + editor.selection = new vscode.Selection(position, position); + } +} + +/** + * Tests formatter on a file at a given cursor position. + * @param filename - Name of test file + * @param position - Tuple of line and character position + * @param format - Formatting function + * @returns Formatted document text + */ +async function testFormatter( + filename: string, + [line, character]: [number, number], + format: (sourceText: string) => string +) { + const { doc } = await openAndShowExamplesTextDocument(filename); + + const formattingEditProvider = + vscode.languages.registerDocumentFormattingEditProvider( + { scheme: "file", language: "python" }, + createFormatterFromStringFunc(format) + ); + + setCursorPosition(line, character); + await wait(450); + await vscode.commands.executeCommand("quarto.formatCell"); + await wait(450); + + const result = doc.getText(); + formattingEditProvider.dispose(); + await vscode.commands.executeCommand("workbench.action.closeActiveEditor"); + + return result; +} + +/** + * Inserts spaces around `=` and `+`, and splits `;`-separated statements + * so fake formatter output is deterministic and visibly different from + * the input. + */ +function spaceAssignments(sourceText: string): string { + return spaceBinaryOperators(sourceText.replace(/;/g, "\n")); +} + +/** + * Hostile formatter that would rewrite option directives and mangle + * assignments if it were ever allowed to see them. Used to prove that + * option lines never reach the formatter. + */ +function hostileFormatter(sourceText: string): string { + return spaceBinaryOperators(sourceText.replace(/#\| /g, "# PIPE ")); +} + +/** + * Reformats assignments and adds a space after the `#` of standalone + * comments. Exists so that tests with multiline, comment-containing code + * cells can prove both the code and the comments are touched by the + * formatter, while the option directives are not. + */ +function formatCodeAndComments(sourceText: string): string { + return spaceBinaryOperators(sourceText).replace(/^#(\S)/gm, "# $1"); +} + +function spaceBinaryOperators(src: string): string { + return src + .replace(/(\w)=(\w)/g, "$1 = $2") + .replace(/(\w)\+(\w)/g, "$1 + $2"); +} + +suite("Code Block Formatting", function () { + test("single option directive is preserved while code is formatted", async function () { + const formattedResult = await testFormatter( + "format-python.qmd", + [8, 0], + spaceAssignments + ); + + assert.ok( + formattedResult.includes("x = 1"), + "Code should be reformatted with spaces around `=`" + ); + assert.ok( + formattedResult.includes("y = 2"), + "Code should be reformatted with spaces around `=`" + ); + assert.ok( + formattedResult.includes("z = x + y"), + "Code should be reformatted with spaces around `=`" + ); + assert.ok( + formattedResult.includes("#| label: my-code"), + "Option directive should be preserved" + ); + assert.ok( + !formattedResult.includes("x=1;y=2;z=x+y"), + "Original unformatted source should be gone" + ); + }); + + test("multiple option directives are all preserved in order", async function () { + const formattedResult = await testFormatter( + "format-python-multiple-options.qmd", + [10, 0], + spaceAssignments + ); + + const labelIndex = formattedResult.indexOf("#| label: multi"); + const echoIndex = formattedResult.indexOf("#| echo: false"); + const warningIndex = formattedResult.indexOf("#| warning: false"); + assert.ok(labelIndex >= 0, "`#| label: multi` should be preserved"); + assert.ok(echoIndex >= 0, "`#| echo: false` should be preserved"); + assert.ok(warningIndex >= 0, "`#| warning: false` should be preserved"); + assert.ok( + labelIndex < echoIndex && echoIndex < warningIndex, + "Option directives should keep their original order" + ); + assert.ok( + formattedResult.includes("x = 1"), + "Code should be reformatted with spaces around `=`" + ); + assert.ok( + !formattedResult.includes("x=1;y=2;z=x+y"), + "Original unformatted source should be gone" + ); + }); + + test("blocks without option directives are formatted normally", async function () { + const formattedResult = await testFormatter( + "format-python-no-options.qmd", + [7, 0], + spaceAssignments + ); + + assert.ok( + formattedResult.includes("x = 1"), + "Code should be reformatted with spaces around `=`" + ); + assert.ok( + formattedResult.includes("z = x + y"), + "Code should be reformatted with spaces around `=`" + ); + assert.ok( + !formattedResult.includes("#|"), + "No option directives should be introduced" + ); + }); + + test("blocks containing only option directives are left untouched", async function () { + const formattedResult = await testFormatter( + "format-python-only-options.qmd", + [7, 0], + (src) => src.replace(/only-options/g, "REPLACED") + ); + + assert.ok( + formattedResult.includes("#| label: only-options"), + "Option directive should be preserved verbatim when there is no code to format" + ); + assert.ok( + !formattedResult.includes("REPLACED"), + "Hostile formatter must not reach option-only blocks" + ); + }); + + test("multiline code with badly formatted comments is reformatted while options stay intact", async function () { + const formattedResult = await testFormatter( + "format-python-multiline-with-comments.qmd", + [10, 0], + formatCodeAndComments + ); + + assert.ok( + formattedResult.includes("#| label: multiline"), + "`#| label: multiline` should be preserved exactly" + ); + assert.ok( + formattedResult.includes("#| echo: false"), + "`#| echo: false` should be preserved exactly" + ); + assert.ok( + !formattedResult.includes("# | label"), + "The hashpipe must not be rewritten by the comment-normalising regex" + ); + assert.ok( + formattedResult.includes("# standalone comment"), + "Standalone code comment should be reformatted with a leading space" + ); + assert.ok( + !/^#standalone comment$/m.test(formattedResult), + "Original unformatted standalone comment should be gone" + ); + assert.ok( + formattedResult.includes("x = 1"), + "First assignment should be reformatted" + ); + assert.ok( + formattedResult.includes("y = 2"), + "Second assignment should be reformatted" + ); + assert.ok( + formattedResult.includes("z = x + y"), + "Third assignment should be reformatted" + ); + }); + + test("option directives are hidden from hostile formatters", async function () { + const formattedResult = await testFormatter( + "format-python-multiple-options.qmd", + [10, 0], + hostileFormatter + ); + + assert.ok( + formattedResult.includes("#| label: multi"), + "`#| label: multi` must not be rewritten" + ); + assert.ok( + formattedResult.includes("#| echo: false"), + "`#| echo: false` must not be rewritten" + ); + assert.ok( + formattedResult.includes("#| warning: false"), + "`#| warning: false` must not be rewritten" + ); + assert.ok( + !formattedResult.includes("# PIPE "), + "Hostile rewrite of the hashpipe must not appear anywhere" + ); + assert.ok( + formattedResult.includes("x = 1"), + "Code should still be reformatted by the hostile formatter" + ); + }); +}); From dfcc6a7ec4d03c7aa18c55430f79a3edae892162 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Canouil?= <8896044+mcanouil@users.noreply.github.com> Date: Sat, 11 Apr 2026 21:48:04 +0200 Subject: [PATCH 3/8] docs: note cell option formatting fix in CHANGELOG --- apps/vscode/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/vscode/CHANGELOG.md b/apps/vscode/CHANGELOG.md index 8a17652d..17ab2266 100644 --- a/apps/vscode/CHANGELOG.md +++ b/apps/vscode/CHANGELOG.md @@ -5,6 +5,7 @@ - Added support for Positron's statement execution feature that reports the approximate line number of the parse error (). - Fixed a bug where `Quarto: Format Cell` would notify you that no formatter was available for code cells that were already formatted (). - No longer claim `.typ` files. Typst syntax highlighting in Quarto documents is unaffected, but standalone Typst files are now left to dedicated extensions like Tinymist (). +- Preserve Quarto code cell option directives (e.g. `#| label: foo`) when formatting embedded code. The directives are now stripped from the virtual document before being handed to the language formatter, so formatters such as Black, autopep8, and styler can no longer reflow or rewrite them (). ## 1.130.0 (Release on 2026-02-18) From ed0ea97ae87718dbd4b9a47a47985f8bcbb2215a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Canouil?= <8896044+mcanouil@users.noreply.github.com> Date: Sat, 11 Apr 2026 23:08:39 +0200 Subject: [PATCH 4/8] fix(format): protect option directives via canonical regex and per-block formatting Close three gaps discovered in adversarial review of the initial fix: 1. The `languageOptionComment` helper used a private map keyed by short language ids (e.g. `python`, `r`, `js`). It didn't know about `typescript`, so `//| ...` directives in `ts` cells were never stripped and still reached the formatter. Use `language.comment` from editor-core instead, which is the canonical, language-wide mapping. 2. The option-line detection used `startsWith("| ")`, which only matched the canonical form. Quarto's own cell-option parser (`cell/options.ts`) accepts `^\s*\| ?`, so variants like `#|label`, `# | label`, and `#| label` are legal. Switch to the same canonical regex so every variant is protected. 3. `embeddedDocumentFormattingProvider` bypassed `formatBlock` entirely for languages with `canFormatDocument !== false` (R, Julia, TS, ...), handing the whole virtualised document to the formatter. Option lines inside every block leaked through. Route every target block through `formatBlock` instead, so the same strip-before-format path protects both cell-format and document-format invocations. --- apps/vscode/src/providers/format.ts | 65 ++++++++++++++++++----------- 1 file changed, 41 insertions(+), 24 deletions(-) diff --git a/apps/vscode/src/providers/format.ts b/apps/vscode/src/providers/format.ts index 2f8f2249..860c98b1 100644 --- a/apps/vscode/src/providers/format.ts +++ b/apps/vscode/src/providers/format.ts @@ -37,15 +37,14 @@ import { isQuartoDoc } from "../core/doc"; import { MarkdownEngine } from "../markdown/engine"; import { EmbeddedLanguage, languageCanFormatDocument } from "../vdoc/languages"; import { + isBlockOfLanguage, languageFromBlock, mainLanguage, unadjustedRange, VirtualDoc, virtualDocForCode, - virtualDocForLanguage, withVirtualDocUri, } from "../vdoc/vdoc"; -import { languageOptionComment } from "./option"; export function activateCodeFormatting(engine: MarkdownEngine) { return [new FormatCellCommand(engine)]; @@ -91,22 +90,25 @@ export function embeddedDocumentFormattingProvider(engine: MarkdownEngine) { return []; } - if (languageCanFormatDocument(language)) { - // Full document formatting support - const vdoc = virtualDocForLanguage(document, tokens, language); - return executeFormatDocumentProvider( - vdoc, - document, - formattingOptions(document.uri, vdoc.language, options) - ); - } else if (block) { - // Just format the selected block if there is one - const edits = await formatBlock(document, block); - return edits ? edits : []; - } else { - // Nothing we can format - return []; + // If the selected language supports whole-document formatting, format + // every block of it; otherwise, format only the cell containing the + // cursor. Either way, each block is routed through `formatBlock` so + // that Quarto option directives are protected by the same + // strip-before-format path and can't leak to the language formatter. + const targetBlocks: (TokenMath | TokenCodeBlock)[] = languageCanFormatDocument(language) + ? (tokens.filter(isBlockOfLanguage(language)) as (TokenMath | TokenCodeBlock)[]) + : block + ? [block] + : []; + + const allEdits: TextEdit[] = []; + for (const target of targetBlocks) { + const edits = await formatBlock(document, target, options); + if (edits) { + allEdits.push(...edits); + } } + return allEdits; }; } @@ -236,7 +238,11 @@ async function executeFormatDocumentProvider( } } -async function formatBlock(doc: TextDocument, block: TokenMath | TokenCodeBlock): Promise { +async function formatBlock( + doc: TextDocument, + block: TokenMath | TokenCodeBlock, + defaultOptions?: FormattingOptions +): Promise { // Extract language const language = languageFromBlock(block); if (!language) { @@ -253,13 +259,20 @@ async function formatBlock(doc: TextDocument, block: TokenMath | TokenCodeBlock) // Count leading Quarto option directives (e.g. `#| label: foo`) so we can // hide them from the formatter entirely. Feeding these lines to formatters // like Black or styler risks reflowing or rewriting them, which would - // silently break the cell's behaviour on the next render. - const languageComment = languageOptionComment(language.ids[0]); - const optionPrefix = languageComment ? languageComment + "| " : undefined; + // silently break the cell's behaviour on the next render. The pattern + // mirrors Quarto's own cell-option parser in `cell/options.ts`, so every + // variant the executor recognises (`#| label`, `#|label`, `# | label`, + // `#| label`, ...) is also protected here. `language.comment` is the + // canonical comment string from `editor-core` and covers every formatter + // language (including TypeScript, which was missing from the ad-hoc map + // the previous implementation used). + const optionPattern = language.comment + ? new RegExp("^" + escapeRegExp(language.comment) + "\\s*\\| ?") + : undefined; let optionLines = 0; - if (optionPrefix) { + if (optionPattern) { for (const line of blockLines) { - if (line.startsWith(optionPrefix)) { + if (optionPattern.test(line)) { optionLines++; } else { break; @@ -280,7 +293,7 @@ async function formatBlock(doc: TextDocument, block: TokenMath | TokenCodeBlock) const edits = await executeFormatDocumentProvider( vdoc, doc, - formattingOptions(doc.uri, vdoc.language) + formattingOptions(doc.uri, vdoc.language, defaultOptions) ); if (!edits) { @@ -327,3 +340,7 @@ function unadjustedEdits( return new TextEdit(unadjustedRange(language, edit.range), edit.newText); }); } + +function escapeRegExp(str: string): string { + return str.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} From a3914ecc89456735bfa8da12986a2287a4a3717e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Canouil?= <8896044+mcanouil@users.noreply.github.com> Date: Sat, 11 Apr 2026 23:08:58 +0200 Subject: [PATCH 5/8] test(format): cover whitespace variants, typescript, and document formatting Add three fixtures and three regression tests for the gaps fixed in the previous commit: - `format-python-option-variants.qmd` exercises every whitespace variant the Quarto cell-option parser accepts (`#|label`, `# | label`, `#| label`). The accompanying test runs an aggressive formatter that rewrites any `#\s*\|` line to `# MANGLED`; with the canonical-regex strip in place, no directive ever reaches the formatter, so `# MANGLED` must not appear. - `format-typescript.qmd` covers the `//|` directive form. Before switching to `language.comment` the `ts` language had no entry in the private comment map and the directive was unprotected; this test would have silently allowed the hostile rewrite. - `format-r-multiple-blocks.qmd` drives the document-formatting path (formerly bypassed `formatBlock` for languages with `canFormatDocument !== false`). The test invokes `embeddedDocumentFormattingProvider` directly since the LSP middleware isn't wired up in the vscode-test host, and asserts that every block's directives survive while the code itself is reformatted. The two new helpers `hostileRFormatter` and `hostileTypeScriptFormatter` use the same canonical regex pattern to decide what to mangle, so they are realistic stand-ins for any formatter that treats `#|`/`//|` lines as ordinary comments. --- .../format-python-option-variants.qmd | 12 ++ .../examples/format-r-multiple-blocks.qmd | 18 ++ .../src/test/examples/format-typescript.qmd | 13 ++ apps/vscode/src/test/formatting.test.ts | 186 +++++++++++++++++- 4 files changed, 227 insertions(+), 2 deletions(-) create mode 100644 apps/vscode/src/test/examples/format-python-option-variants.qmd create mode 100644 apps/vscode/src/test/examples/format-r-multiple-blocks.qmd create mode 100644 apps/vscode/src/test/examples/format-typescript.qmd diff --git a/apps/vscode/src/test/examples/format-python-option-variants.qmd b/apps/vscode/src/test/examples/format-python-option-variants.qmd new file mode 100644 index 00000000..e3371420 --- /dev/null +++ b/apps/vscode/src/test/examples/format-python-option-variants.qmd @@ -0,0 +1,12 @@ +--- +title: Formatting Python Code Cells with Option Directive Variants +subtitle: https://github.com/quarto-dev/quarto/pull/655 +format: html +--- + +```{python} +#|label: no-space +# | space-before-pipe +#| extra-space-after +x=1;y=2;z=x+y +``` diff --git a/apps/vscode/src/test/examples/format-r-multiple-blocks.qmd b/apps/vscode/src/test/examples/format-r-multiple-blocks.qmd new file mode 100644 index 00000000..8dd60f8b --- /dev/null +++ b/apps/vscode/src/test/examples/format-r-multiple-blocks.qmd @@ -0,0 +1,18 @@ +--- +title: Formatting an R Document with Multiple Blocks +subtitle: https://github.com/quarto-dev/quarto/pull/655 +format: html +--- + +```{r} +#| label: first +x<-1 +``` + +Some prose between the cells. + +```{r} +#| label: second +#| echo: false +y<-2 +``` diff --git a/apps/vscode/src/test/examples/format-typescript.qmd b/apps/vscode/src/test/examples/format-typescript.qmd new file mode 100644 index 00000000..311a61f3 --- /dev/null +++ b/apps/vscode/src/test/examples/format-typescript.qmd @@ -0,0 +1,13 @@ +--- +title: Formatting TypeScript Code Cells +subtitle: https://github.com/quarto-dev/quarto/pull/655 +format: html +--- + +```{ts} +//| label: ts-cell +//| echo: false +const x=1; +const y=2; +const z=x+y; +``` diff --git a/apps/vscode/src/test/formatting.test.ts b/apps/vscode/src/test/formatting.test.ts index 3a596eed..bf244f5d 100644 --- a/apps/vscode/src/test/formatting.test.ts +++ b/apps/vscode/src/test/formatting.test.ts @@ -1,6 +1,8 @@ import * as vscode from "vscode"; import * as assert from "assert"; import { openAndShowExamplesTextDocument, wait } from "./test-utils"; +import { embeddedDocumentFormattingProvider } from "../providers/format"; +import { MarkdownEngine } from "../markdown/engine"; /** * Creates a document formatting provider from a formatting function. @@ -47,18 +49,20 @@ function setCursorPosition(line: number, character: number): void { * @param filename - Name of test file * @param position - Tuple of line and character position * @param format - Formatting function + * @param language - Language identifier to register the formatter for * @returns Formatted document text */ async function testFormatter( filename: string, [line, character]: [number, number], - format: (sourceText: string) => string + format: (sourceText: string) => string, + language: string = "python" ) { const { doc } = await openAndShowExamplesTextDocument(filename); const formattingEditProvider = vscode.languages.registerDocumentFormattingEditProvider( - { scheme: "file", language: "python" }, + { scheme: "file", language }, createFormatterFromStringFunc(format) ); @@ -108,6 +112,39 @@ function spaceBinaryOperators(src: string): string { .replace(/(\w)\+(\w)/g, "$1 + $2"); } +/** + * Aggressive formatter that rewrites any line that looks remotely like a + * Quarto option directive, regardless of whitespace around the pipe. If the + * strip-before-format path is working, the formatter never sees a directive + * line, so `# MANGLED` must not appear in the final result. Inject lines + * like `# type: ignore` do not match and are therefore untouched. + */ +function mangleHashPipeLines(sourceText: string): string { + return spaceBinaryOperators( + sourceText.replace(/^#\s*\|.*$/gm, "# MANGLED") + ); +} + +/** + * Hostile R formatter that rewrites `#|` directives and normalises the + * assignment arrow. + */ +function hostileRFormatter(sourceText: string): string { + return sourceText + .replace(/#\s*\|.*$/gm, "# PIPE MANGLED") + .replace(/(\w)<-(\w)/g, "$1 <- $2"); +} + +/** + * Hostile TypeScript formatter that rewrites `//|` directives and + * normalises `=`. + */ +function hostileTypeScriptFormatter(sourceText: string): string { + return spaceBinaryOperators( + sourceText.replace(/^\/\/\s*\|.*$/gm, "// PIPE MANGLED") + ); +} + suite("Code Block Formatting", function () { test("single option directive is preserved while code is formatted", async function () { const formattedResult = await testFormatter( @@ -272,4 +309,149 @@ suite("Code Block Formatting", function () { "Code should still be reformatted by the hostile formatter" ); }); + + test("option directives with non-canonical whitespace are still protected", async function () { + const formattedResult = await testFormatter( + "format-python-option-variants.qmd", + [10, 0], + mangleHashPipeLines + ); + + // All three whitespace variants must survive byte-identical, since + // Quarto's own option parser accepts `^#\s*\| ?`. + assert.ok( + formattedResult.includes("#|label: no-space"), + "`#|label: no-space` must be preserved verbatim" + ); + assert.ok( + formattedResult.includes("# | space-before-pipe"), + "`# | space-before-pipe` must be preserved verbatim" + ); + assert.ok( + formattedResult.includes("#| extra-space-after"), + "`#| extra-space-after` must be preserved verbatim" + ); + assert.ok( + !formattedResult.includes("# MANGLED"), + "Aggressive formatter must not have touched any option line" + ); + assert.ok( + formattedResult.includes("x = 1"), + "Code should still be reformatted" + ); + assert.ok( + formattedResult.includes("z = x + y"), + "Binary operators in code should still be spaced" + ); + }); + + test("typescript option directives are protected via language.comment", async function () { + const formattedResult = await testFormatter( + "format-typescript.qmd", + [9, 0], + hostileTypeScriptFormatter, + "typescript" + ); + + // TypeScript uses `//` for comments and `//|` for directives. Before + // the fix this path was silently broken because the lookup used + // `language.ids[0] === "ts"`, which wasn't in the old `kLangCommentChars` + // map. Since `language.comment` is populated from editor-core, the + // directive is now recognised. + assert.ok( + formattedResult.includes("//| label: ts-cell"), + "`//| label: ts-cell` must be preserved verbatim" + ); + assert.ok( + formattedResult.includes("//| echo: false"), + "`//| echo: false` must be preserved verbatim" + ); + assert.ok( + !formattedResult.includes("// PIPE MANGLED"), + "Hostile rewrite of the hashpipe must not appear anywhere" + ); + assert.ok( + formattedResult.includes("const x = 1;"), + "TypeScript assignment should be reformatted" + ); + assert.ok( + formattedResult.includes("const z = x + y;"), + "TypeScript binary operators should be spaced" + ); + }); + + test("document formatting protects options across every block", async function () { + const { doc } = await openAndShowExamplesTextDocument( + "format-r-multiple-blocks.qmd" + ); + + const formattingEditProvider = + vscode.languages.registerDocumentFormattingEditProvider( + { scheme: "file", language: "r" }, + createFormatterFromStringFunc(hostileRFormatter) + ); + + // Park the cursor on a markdown line so nothing in particular is + // selected. The document-formatting path should still visit every R + // block. + setCursorPosition(11, 0); + await wait(450); + + // Invoke the document-formatting provider directly. In the real + // extension this is wired as LSP middleware, but tests don't run + // the LSP, so calling the exported callback is the cleanest way to + // exercise the path without reimplementing VS Code's routing. + const engine = new MarkdownEngine(); + const provider = embeddedDocumentFormattingProvider(engine); + const token: vscode.CancellationToken = { + isCancellationRequested: false, + onCancellationRequested: () => ({ dispose: () => { } }), + }; + const edits = await provider( + doc, + { tabSize: 2, insertSpaces: true }, + token, + async () => [] + ); + + assert.ok(edits, "Document-formatting provider should return edits"); + + const editor = vscode.window.activeTextEditor!; + await editor.edit((eb) => { + (edits as vscode.TextEdit[]) + .slice() + .sort((a, b) => b.range.start.compareTo(a.range.start)) + .forEach((edit) => eb.replace(edit.range, edit.newText)); + }); + await wait(450); + + const result = doc.getText(); + formattingEditProvider.dispose(); + await vscode.commands.executeCommand("workbench.action.closeActiveEditor"); + + assert.ok( + result.includes("#| label: first"), + "First block's option directive must be preserved" + ); + assert.ok( + result.includes("#| label: second"), + "Second block's option directive must be preserved" + ); + assert.ok( + result.includes("#| echo: false"), + "`#| echo: false` must be preserved" + ); + assert.ok( + !result.includes("# PIPE MANGLED"), + "Hostile rewrite of the hashpipe must not appear anywhere" + ); + assert.ok( + result.includes("x <- 1"), + "First block's code should be reformatted" + ); + assert.ok( + result.includes("y <- 2"), + "Second block's code should be reformatted" + ); + }); }); From e25622c2f11ce86dc7a05f8b00f57da2c685bbeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Canouil?= <8896044+mcanouil@users.noreply.github.com> Date: Sat, 11 Apr 2026 23:37:01 +0200 Subject: [PATCH 6/8] refactor(format): reuse canonical option pattern and harden doc-format path Address adversarial review findings on the option-protection patch: - Reuse `optionCommentPattern` from `cell/options.ts` instead of re-encoding the regex in `format.ts`. The protection path can no longer drift from Quarto's own cell-option parser, and the local `escapeRegExp` helper is gone. - Make document formatting all-or-nothing: aggregate per-block bails into a single message and abort the whole operation rather than apply a partial format. Per-block toasts are silenced via a new `silentOutOfRange` flag on `formatBlock` so the doc path no longer spams a message per failing block. - Thread `defaultOptions` through `embeddedDocumentRangeFormattingProvider` and `FormatCellCommand`, both of which previously fell back to a hardcoded `tabSize: 4 / insertSpaces: true` regardless of the user's editor settings. - Mark `languageOptionComment` as private in `option.ts` since it has no remaining external consumers. --- apps/vscode/src/providers/cell/options.ts | 2 +- apps/vscode/src/providers/format.ts | 58 ++++++++++++++++------- apps/vscode/src/providers/option.ts | 2 +- 3 files changed, 42 insertions(+), 20 deletions(-) diff --git a/apps/vscode/src/providers/cell/options.ts b/apps/vscode/src/providers/cell/options.ts index 936c09c0..f833d0c4 100644 --- a/apps/vscode/src/providers/cell/options.ts +++ b/apps/vscode/src/providers/cell/options.ts @@ -91,7 +91,7 @@ function langCommentChars(lang: string): string[] { return chars; } } -function optionCommentPattern(comment: string) { +export function optionCommentPattern(comment: string) { return new RegExp("^" + escapeRegExp(comment) + "\\s*\\| ?"); } diff --git a/apps/vscode/src/providers/format.ts b/apps/vscode/src/providers/format.ts index 860c98b1..a5491da2 100644 --- a/apps/vscode/src/providers/format.ts +++ b/apps/vscode/src/providers/format.ts @@ -35,6 +35,7 @@ import { TokenCodeBlock, TokenMath, codeForExecutableLanguageBlock, languageBloc import { Command } from "../core/command"; import { isQuartoDoc } from "../core/doc"; import { MarkdownEngine } from "../markdown/engine"; +import { optionCommentPattern } from "./cell/options"; import { EmbeddedLanguage, languageCanFormatDocument } from "../vdoc/languages"; import { isBlockOfLanguage, @@ -101,12 +102,29 @@ export function embeddedDocumentFormattingProvider(engine: MarkdownEngine) { ? [block] : []; + // Document formatting is all-or-nothing: if any block fails the + // out-of-range guard, abort the whole operation rather than apply a + // partial format. We pass `silentOutOfRange: true` so per-block + // failures don't toast individually; a single aggregated message is + // shown below. const allEdits: TextEdit[] = []; + let outOfRangeBlockFailures = 0; for (const target of targetBlocks) { - const edits = await formatBlock(document, target, options); - if (edits) { - allEdits.push(...edits); + const edits = await formatBlock(document, target, options, true); + if (edits === undefined) { + continue; } + if (edits.length === 0) { + outOfRangeBlockFailures++; + continue; + } + allEdits.push(...edits); + } + if (outOfRangeBlockFailures > 0) { + window.showInformationMessage( + `Formatting edits were out of range in ${outOfRangeBlockFailures} code cell${outOfRangeBlockFailures === 1 ? "" : "s"}; document was not modified.` + ); + return []; } return allEdits; }; @@ -148,7 +166,7 @@ export function embeddedDocumentRangeFormattingProvider( return []; } - const edits = await formatBlock(document, block); + const edits = await formatBlock(document, block, options); if (!edits) { return []; } @@ -183,7 +201,11 @@ class FormatCellCommand implements Command { return; } - const edits = await formatBlock(document, block); + const editorOptions: FormattingOptions = { + tabSize: typeof editor.options.tabSize === "number" ? editor.options.tabSize : 4, + insertSpaces: typeof editor.options.insertSpaces === "boolean" ? editor.options.insertSpaces : true, + }; + const edits = await formatBlock(document, block, editorOptions); if (!edits) { // Nothing to do! Already formatted, or no formatter picked us up, or this language doesn't support formatting. return; @@ -241,7 +263,8 @@ async function executeFormatDocumentProvider( async function formatBlock( doc: TextDocument, block: TokenMath | TokenCodeBlock, - defaultOptions?: FormattingOptions + defaultOptions?: FormattingOptions, + silentOutOfRange: boolean = false ): Promise { // Extract language const language = languageFromBlock(block); @@ -259,15 +282,16 @@ async function formatBlock( // Count leading Quarto option directives (e.g. `#| label: foo`) so we can // hide them from the formatter entirely. Feeding these lines to formatters // like Black or styler risks reflowing or rewriting them, which would - // silently break the cell's behaviour on the next render. The pattern - // mirrors Quarto's own cell-option parser in `cell/options.ts`, so every - // variant the executor recognises (`#| label`, `#|label`, `# | label`, - // `#| label`, ...) is also protected here. `language.comment` is the + // silently break the cell's behaviour on the next render. We reuse + // `optionCommentPattern` from `cell/options.ts` so this code path can + // never drift from Quarto's own cell-option parser: any variant the + // executor recognises (`#| label`, `#|label`, `# | label`, `#| label`, + // ...) is automatically protected here. `language.comment` is the // canonical comment string from `editor-core` and covers every formatter // language (including TypeScript, which was missing from the ad-hoc map // the previous implementation used). const optionPattern = language.comment - ? new RegExp("^" + escapeRegExp(language.comment) + "\\s*\\| ?") + ? optionCommentPattern(language.comment) : undefined; let optionLines = 0; if (optionPattern) { @@ -323,9 +347,11 @@ async function formatBlock( // Bail if any edit is out of range. We used to filter these edits out but // this could bork the cell. Return `[]` to indicate that we tried. if (adjustedEdits.some(edit => !blockRange.contains(edit.range))) { - window.showInformationMessage( - "Formatting edits were out of range and could not be applied to the code cell." - ); + if (!silentOutOfRange) { + window.showInformationMessage( + "Formatting edits were out of range and could not be applied to the code cell." + ); + } return []; } @@ -340,7 +366,3 @@ function unadjustedEdits( return new TextEdit(unadjustedRange(language, edit.range), edit.newText); }); } - -function escapeRegExp(str: string): string { - return str.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); -} diff --git a/apps/vscode/src/providers/option.ts b/apps/vscode/src/providers/option.ts index 885322ec..eff92603 100644 --- a/apps/vscode/src/providers/option.ts +++ b/apps/vscode/src/providers/option.ts @@ -89,7 +89,7 @@ function handleOptionEnter(editor: TextEditor, comment: string) { } } -export function languageOptionComment(language: string) { +function languageOptionComment(language: string) { // some mappings if (language === "ojs") { language = "js"; From b4dc4f4d796d5167fc536debb9f57916aa5251c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Canouil?= <8896044+mcanouil@users.noreply.github.com> Date: Sat, 11 Apr 2026 23:37:13 +0200 Subject: [PATCH 7/8] test(format): exercise real edit-apply path and multi-edit aggregation - Replace the direct provider call in the document-formatting test with a real `vscode.executeFormatDocumentProvider` invocation. The test now registers `embeddedDocumentFormattingProvider` against the `quarto` language and lets VS Code's command machinery validate ordering and overlap. Pairwise non-overlap is asserted explicitly so a future regression in offset arithmetic surfaces here. - Add a test that exercises a formatter returning one `TextEdit` per non-empty line, covering the multi-edit aggregation path that all previous hostile formatters skipped. - Wrap both new tests in try/finally so a failed assertion can never leak a registered formatter into a later suite. - Drop the misleading comment about inject lines from `mangleHashPipeLines` and remove an unnecessary `wait(450)` from the document-formatting test. --- apps/vscode/src/test/formatting.test.ts | 235 ++++++++++++++++++------ 1 file changed, 175 insertions(+), 60 deletions(-) diff --git a/apps/vscode/src/test/formatting.test.ts b/apps/vscode/src/test/formatting.test.ts index bf244f5d..bf5fc5a5 100644 --- a/apps/vscode/src/test/formatting.test.ts +++ b/apps/vscode/src/test/formatting.test.ts @@ -31,6 +31,43 @@ function createFormatterFromStringFunc( }; } +/** + * Creates a document formatting provider that returns one `TextEdit` per + * non-empty line, instead of a single block-wide edit. This stresses the + * per-block loop with multiple discrete edits per cell so that any future + * regression in edit aggregation, sorting, or offset adjustment is caught. + */ +function createPerLineFormatter( + format: (line: string) => string +): vscode.DocumentFormattingEditProvider { + return { + provideDocumentFormattingEdits( + document: vscode.TextDocument + ): vscode.TextEdit[] { + const edits: vscode.TextEdit[] = []; + for (let i = 0; i < document.lineCount; i++) { + const lineText = document.lineAt(i).text; + if (lineText.length === 0) { + continue; + } + const replaced = format(lineText); + if (replaced !== lineText) { + edits.push( + new vscode.TextEdit( + new vscode.Range( + new vscode.Position(i, 0), + new vscode.Position(i, lineText.length) + ), + replaced + ) + ); + } + } + return edits; + }, + }; +} + /** * Sets the cursor position in the active editor. * @param line - Line number @@ -116,8 +153,7 @@ function spaceBinaryOperators(src: string): string { * Aggressive formatter that rewrites any line that looks remotely like a * Quarto option directive, regardless of whitespace around the pipe. If the * strip-before-format path is working, the formatter never sees a directive - * line, so `# MANGLED` must not appear in the final result. Inject lines - * like `# type: ignore` do not match and are therefore untouched. + * line, so `# MANGLED` must not appear in the final result. */ function mangleHashPipeLines(sourceText: string): string { return spaceBinaryOperators( @@ -381,77 +417,156 @@ suite("Code Block Formatting", function () { }); test("document formatting protects options across every block", async function () { - const { doc } = await openAndShowExamplesTextDocument( + const { doc, editor } = await openAndShowExamplesTextDocument( "format-r-multiple-blocks.qmd" ); - const formattingEditProvider = + // Register the embedded document formatter for `quarto` documents so + // VS Code routes `vscode.executeFormatDocumentProvider` through the + // real path the extension uses in production (validation, ordering, + // overlap checks, transactional application). In production this is + // wired as LSP middleware, but tests don't run the LSP — registering + // the provider directly is the closest stand-in. + const engine = new MarkdownEngine(); + const provider = embeddedDocumentFormattingProvider(engine); + const quartoFormatter = + vscode.languages.registerDocumentFormattingEditProvider( + { scheme: "file", language: "quarto" }, + { + provideDocumentFormattingEdits: async (document, options, token) => + (await provider(document, options, token, async () => [])) ?? [], + } + ); + + const innerFormatter = vscode.languages.registerDocumentFormattingEditProvider( { scheme: "file", language: "r" }, createFormatterFromStringFunc(hostileRFormatter) ); - // Park the cursor on a markdown line so nothing in particular is - // selected. The document-formatting path should still visit every R - // block. - setCursorPosition(11, 0); - await wait(450); + try { + // Park the cursor on a markdown line so nothing in particular is + // selected. The document-formatting path should still visit every R + // block. + setCursorPosition(11, 0); - // Invoke the document-formatting provider directly. In the real - // extension this is wired as LSP middleware, but tests don't run - // the LSP, so calling the exported callback is the cleanest way to - // exercise the path without reimplementing VS Code's routing. - const engine = new MarkdownEngine(); - const provider = embeddedDocumentFormattingProvider(engine); - const token: vscode.CancellationToken = { - isCancellationRequested: false, - onCancellationRequested: () => ({ dispose: () => { } }), - }; - const edits = await provider( - doc, - { tabSize: 2, insertSpaces: true }, - token, - async () => [] + const edits = await vscode.commands.executeCommand( + "vscode.executeFormatDocumentProvider", + doc.uri, + { tabSize: 2, insertSpaces: true } + ); + + assert.ok(edits, "Document-formatting provider should return edits"); + assert.ok(edits.length > 0, "Expected at least one edit"); + + // Verify the edits VS Code received are pairwise non-overlapping. This + // catches a class of regression where two block edits could collide + // because of a wrong offset or overlapping block range. + const sorted = edits.slice().sort((a, b) => a.range.start.compareTo(b.range.start)); + for (let i = 1; i < sorted.length; i++) { + assert.ok( + sorted[i - 1].range.end.isBeforeOrEqual(sorted[i].range.start), + `Edits ${i - 1} and ${i} overlap` + ); + } + + await editor.edit((eb) => { + sorted + .slice() + .reverse() + .forEach((edit) => eb.replace(edit.range, edit.newText)); + }); + + const result = doc.getText(); + + assert.ok( + result.includes("#| label: first"), + "First block's option directive must be preserved" + ); + assert.ok( + result.includes("#| label: second"), + "Second block's option directive must be preserved" + ); + assert.ok( + result.includes("#| echo: false"), + "`#| echo: false` must be preserved" + ); + assert.ok( + !result.includes("# PIPE MANGLED"), + "Hostile rewrite of the hashpipe must not appear anywhere" + ); + assert.ok( + result.includes("x <- 1"), + "First block's code should be reformatted" + ); + assert.ok( + result.includes("y <- 2"), + "Second block's code should be reformatted" + ); + } finally { + innerFormatter.dispose(); + quartoFormatter.dispose(); + await vscode.commands.executeCommand("workbench.action.closeActiveEditor"); + } + }); + + test("formatter returning multiple discrete edits is applied correctly", async function () { + const { doc } = await openAndShowExamplesTextDocument( + "format-python-multiple-options.qmd" ); - assert.ok(edits, "Document-formatting provider should return edits"); + // Per-line formatter returns one TextEdit per non-empty line. After + // offset adjustment in `formatBlock` these must be reassembled in the + // correct order; if the offset or sort logic regressed this test + // would surface mis-ordered or duplicated lines. + const innerFormatter = + vscode.languages.registerDocumentFormattingEditProvider( + { scheme: "file", language: "python" }, + createPerLineFormatter((line) => + line.replace(/(\w)=(\w)/g, "$1 = $2").replace(/(\w)\+(\w)/g, "$1 + $2") + ) + ); - const editor = vscode.window.activeTextEditor!; - await editor.edit((eb) => { - (edits as vscode.TextEdit[]) - .slice() - .sort((a, b) => b.range.start.compareTo(a.range.start)) - .forEach((edit) => eb.replace(edit.range, edit.newText)); - }); - await wait(450); + try { + setCursorPosition(10, 0); + await wait(450); + await vscode.commands.executeCommand("quarto.formatCell"); + await wait(450); - const result = doc.getText(); - formattingEditProvider.dispose(); - await vscode.commands.executeCommand("workbench.action.closeActiveEditor"); + const result = doc.getText(); - assert.ok( - result.includes("#| label: first"), - "First block's option directive must be preserved" - ); - assert.ok( - result.includes("#| label: second"), - "Second block's option directive must be preserved" - ); - assert.ok( - result.includes("#| echo: false"), - "`#| echo: false` must be preserved" - ); - assert.ok( - !result.includes("# PIPE MANGLED"), - "Hostile rewrite of the hashpipe must not appear anywhere" - ); - assert.ok( - result.includes("x <- 1"), - "First block's code should be reformatted" - ); - assert.ok( - result.includes("y <- 2"), - "Second block's code should be reformatted" - ); + assert.ok( + result.includes("#| label: multi"), + "Option directive should be preserved" + ); + assert.ok( + result.includes("#| echo: false"), + "Option directive should be preserved" + ); + assert.ok( + result.includes("#| warning: false"), + "Option directive should be preserved" + ); + assert.ok( + result.includes("x = 1"), + "First assignment should be reformatted" + ); + assert.ok( + result.includes("y = 2"), + "Second assignment should be reformatted" + ); + assert.ok( + result.includes("z = x + y"), + "Third assignment should be reformatted" + ); + assert.ok( + !result.includes("x=1"), + "Original unformatted source should be gone" + ); + } finally { + innerFormatter.dispose(); + await vscode.commands.executeCommand("workbench.action.closeActiveEditor"); + } }); + }); From 65717f8d1cfec903cf031fae4f4cd6d76e67f68f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Canouil?= <8896044+mcanouil@users.noreply.github.com> Date: Sun, 12 Apr 2026 10:28:27 +0200 Subject: [PATCH 8/8] fix(format): harden edge cases from adversarial review - Treat empty formatter results as no-op (not out-of-range bail) in both formatBlock and FormatCellCommand, preventing spurious error toasts. - Use trim() in the options-only guard so whitespace-only tails after option directives are correctly treated as empty. - Wrap testFormatter disposal in try/finally to prevent leaked formatter registrations on test failure. - Document why block-comment languages are not affected by the option stripping logic. --- apps/vscode/src/providers/format.ts | 25 ++++++++++++++++--------- apps/vscode/src/test/formatting.test.ts | 22 ++++++++++++---------- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/apps/vscode/src/providers/format.ts b/apps/vscode/src/providers/format.ts index a5491da2..0c48e259 100644 --- a/apps/vscode/src/providers/format.ts +++ b/apps/vscode/src/providers/format.ts @@ -206,8 +206,10 @@ class FormatCellCommand implements Command { insertSpaces: typeof editor.options.insertSpaces === "boolean" ? editor.options.insertSpaces : true, }; const edits = await formatBlock(document, block, editorOptions); - if (!edits) { - // Nothing to do! Already formatted, or no formatter picked us up, or this language doesn't support formatting. + if (!edits || edits.length === 0) { + // Nothing to do! Already formatted, no formatter picked us up, this + // language doesn't support formatting, or the edits were out of range + // (the user already saw a toast from formatBlock in that case). return; } @@ -289,7 +291,10 @@ async function formatBlock( // ...) is automatically protected here. `language.comment` is the // canonical comment string from `editor-core` and covers every formatter // language (including TypeScript, which was missing from the ad-hoc map - // the previous implementation used). + // the previous implementation used). Note: block-comment languages (C, + // CSS, SAS) use a tuple comment-char in `cell/options.ts` with a suffix + // check; those languages do not have `canFormat: true` in + // `vdoc/languages.ts`, so they never reach this code path. const optionPattern = language.comment ? optionCommentPattern(language.comment) : undefined; @@ -304,14 +309,16 @@ async function formatBlock( } } - // Nothing to format if the block is entirely option directives. - if (optionLines === blockLines.length) { - return undefined; - } - // Create virtual document containing only the code portion of the block // so the formatter never sees the option directives. const codeLines = blockLines.slice(optionLines); + + // Nothing to format if the block is entirely option directives (or only + // trailing whitespace after them, which `lines()` may produce from a + // final newline in `token.data`). + if (codeLines.every(l => l.trim() === "")) { + return undefined; + } const vdoc = virtualDocForCode(codeLines, language); const edits = await executeFormatDocumentProvider( @@ -320,7 +327,7 @@ async function formatBlock( formattingOptions(doc.uri, vdoc.language, defaultOptions) ); - if (!edits) { + if (!edits || edits.length === 0) { // Either no formatter picked us up, or there were no edits required. // We can't determine the difference though! return undefined; diff --git a/apps/vscode/src/test/formatting.test.ts b/apps/vscode/src/test/formatting.test.ts index bf5fc5a5..26b4e6b3 100644 --- a/apps/vscode/src/test/formatting.test.ts +++ b/apps/vscode/src/test/formatting.test.ts @@ -103,16 +103,18 @@ async function testFormatter( createFormatterFromStringFunc(format) ); - setCursorPosition(line, character); - await wait(450); - await vscode.commands.executeCommand("quarto.formatCell"); - await wait(450); - - const result = doc.getText(); - formattingEditProvider.dispose(); - await vscode.commands.executeCommand("workbench.action.closeActiveEditor"); - - return result; + try { + setCursorPosition(line, character); + await wait(450); + await vscode.commands.executeCommand("quarto.formatCell"); + await wait(450); + + const result = doc.getText(); + return result; + } finally { + formattingEditProvider.dispose(); + await vscode.commands.executeCommand("workbench.action.closeActiveEditor"); + } } /**