fix: protect code cell options from formatting#655
fix: protect code cell options from formatting#655mcanouil wants to merge 8 commits intoquarto-dev:mainfrom
Conversation
|
Thanks for the PR. I assume this was done to prevent |
|
Another thought here is that this kind of PR really makes me wish we had a test suite in this repo. |
I was thinking about making it specific at first but then I realised that the expected comments are all hardcoded, so I reconsidered, and made a general protection for the comments symbols (hardcoded in two places).
I'm not yet very familiar on this part, but I can't agree more with this. I did "manual tests" as far as this approach can go. |
|
Let's hold on on this PR as the behaviour is not new at all so it can wait a bit for me to implement a test soon as I get some spare time. |
|
@mcanouil we now have extension tests! If you're interested in merging this PR, want to add some tests? |
|
We are continuing to get more user reports of this such as in posit-dev/positron#9432, so I'd love to see a test written here so we can merge this fix in! |
|
I'll try to make some time to make this happen. |
|
I've setup the test, but I can't get to make the formatting command to work. npx @vscode/test-cli --grep "Code Block Formatting" --install-extensions "ms-python.black-formatter"I don't know how to trigger all the logic of the formatCellCommand The easiest would be to export I've also tried to trigger the whole document formatting, but it does not work for an unknown reason. I have to drop this for now, but if someone has a better understand of the whole formatting logic of the extension (not that trivial) and knows how to trigger the activation, feel free to take over. |
|
Ah, I don't think we can use any other extensions in the tests, because of the way that they use a special build of VS Code that is specially built for running extension tests. Instead, what we'll want to do is mock out a formatter to run in the tests. This is the same thing that #754 needs. We may want to put the formatting mock somewhere that all tests can use it. I also think that exporting a function to use in a test is a very good/reasonable way to go. |
|
Well, I don't think the issue is the presence/absence of the formatter. I can only make the test install the extension to be sure |
|
Ok, now I have some information. The vscode-test instance shows a notification that the selection marker is not within a cell that supports formatting which is clearly wrong, but no clue why from yarn run test --grep "Code Block Formatting" |
|
For reference: In another PR I added a mocked cell formatter in a test here: 873fcab |
|
@vezwork Thanks, I'll try to take a look next week-end and see to finalise this PR. |
|
Even changing to your testFormatter function does not work. See when running extension debug mode: Screen.Recording.2025-10-19.at.19.10.58.movI have to drop this as It costs me way too much time for no good reasons. Feel free to take over as I won't spend more time on this for the next few months at least. |
…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.
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.
mcanouil
force-pushed
the
feat/protect-options-formatting
branch
from
446a935 to
dfcc6a7
Compare
…ock 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("<comment>| ")`, which
only matched the canonical form. Quarto's own cell-option parser
(`cell/options.ts`) accepts `^<comment>\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.
…matting 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.
…t 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.
- 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.
- 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.
|
I took another stab at this with some help this time (roborev + claude code for review). |
4 participants
Implement a mechanism to prevent code cell options from being formatted, ensuring that only relevant code is affected during formatting operations.
Fixes #368, fixes #135