Fix cell formatting issues #754

lionel- · 2025-07-04T10:35:55Z

Addresses #745

But needs to be tested more widely and deal with formatters that don't ensure a trailing newline as discussed in #745 (comment).

lionel- · 2025-07-04T11:05:55Z

apps/vscode/src/providers/format.ts

-          edits.forEach((edit) => {
-            editBuilder.replace(edit.range, edit.newText);
-          });
+          // Sort edits by descending start position to avoid range shifting issues


Unrelated but I thought we should make this robust to providers returning unsorted edits.

lionel- · 2025-07-04T11:07:04Z

apps/vscode/src/providers/format.ts

+
+    // Bail if any edit is out of range. We used to filter these edits out but
+    // this could bork the cell.
+    if (edits.some(edit => !blockRange.contains(edit.range))) {


Unrelated but I'm pretty sure applying edits partially can bork our cells 😬

I pulled down this PR, rebased on main, and followed the reproduction steps in #745 (which this PR should address). I ran into a new error that prevented any formatting from occuring:

I reverted this code change locally (bailing if edits are out of range) and formatting works again and the problem described in #745 is addressed.

cscheid · 2025-07-07T15:06:52Z

Thanks for the PR! Something for us to consider:

Every time I see a PR here I get a feeling of dread: we have no test coverage and so I have very little visibility or intuition of the impact that these changes make. What kind of infrastructure would we have to add for us to have unit test coverage for these kinds of changes?

lionel- · 2025-07-07T16:06:56Z

Part of the reason this is still a draft is that I'd like to add some tests here. They'll be limited in scope though.

Here are the levels of tests we have in our R stack, by ascending level of scope:

Internal LSP tests, these are easiest to write.
Protocol-level tests for LSP with a custom client.
Extension tests. With these you can test effects of vscode commands on e.g. text in an editor. These commands may delegate to the LSP. Example: https://github.com/posit-dev/positron/blob/main/extensions/positron-r/src/test/indentation.test.ts
UI tests in the Positron suite: https://github.com/posit-dev/positron/tree/main/test/e2e/tests/quarto

We spend most of our time and energy on internal LSP tests.

vezwork · 2025-09-02T20:19:05Z

@lionel- we now have extension tests! The tests are focused on roundtripping right now, but they do already involve executing commands. Do you think an extension test (a snapshot test?) that opens our test qmd files, executes the format command, and checks the contents of the file would provide enough coverage for this PR?

I'd like to get this PR in, so I'm happy to write the tests. Please advise!

juliasilge · 2025-09-03T14:35:04Z

@vezwork It turns out that @lionel- is OOO all this week. I think an extension test that formats and checks the contents is a great way to go, and if you are wanting to work on this soon-ish (before conf) I am happy to review if you can contribute a test or two!

vezwork · 2025-09-04T17:08:36Z

I pulled down this PR, rebased on main, and followed the reproduction steps in #745 (which this PR should address). I ran into a new error that prevented any formatting from occuring:

I was able to reproduce #745 in Positron on the main branch of the extension, so this error is confirmed to be caused by this PR.

vezwork · 2025-09-05T18:51:45Z

I have some local changes where I'm able to load the minimal repro qmd specified in #745, set the editor's cursor to inside a code cell, and do vscode.commands.executeCommand("quarto.formatCell"); but no formatting happens and a toast pops up in the bottom-right of the test VSCode that says

Editor selection is not within a code cell that supports formatting.

...but the cursor is definitely within a cell. I suspect that the Quarto extension is toasting this because there is no R formatter available.

The test VSCode instance has 0 extensions installed: I know from looking under the extensions tab while the tests are running. That means there's no R extension. That means there's no R formatter available!

It is confusing to me that there are 0 extensions installed because the vscode testing guide says:

When you debug an extension test in VS Code, VS Code uses the globally installed instance of VS Code and will load all installed extensions.

but that does not seem to be the case for our extension tests. For one other thing, the test instance of VSCode always has an Abyss theme which is not my local VSCode theme, I don't know why. @juliasilge do you know why our test VSCode instance does not have any extensions installed?

juliasilge · 2025-09-05T21:24:29Z

Interesting, no, I don't know why. I just tried it myself and I do in fact also see no extensions installed in the test runner VS Code that gets downloaded when you start such tests:

I wonder if this literally means "debug" rather than run:

When you debug an extension test in VS Code

Like when you do Test: Debug All Tests rather than run from the CLI, for example.

You might try adding the air and/or ruff extensions as extension dependencies just to see if you can get it installed in the test runner that way:

  "extensionDependencies": [
    "posit.air-vscode",
    "charliermarsh.ruff"
  ]

Not that we want to really depend on these, but to explore what our options are.

juliasilge · 2025-09-06T02:34:31Z

Another thought I had is that we might want to mock out a formatter to use in the tests, or maybe two (one that adds a final newline and one that does not).

vezwork · 2025-09-09T20:02:12Z

@juliasilge and I discussed a couple possible ways to test this PR:

create two very simple formatter classes (like in https://github.com/posit-dev/positron/pull/1686/files), one that adds a trailing newline and one that does not, then use registerDocumentFormattingEditProvider when setting up a test.
set up a new test runner that uses @vscode/test-electron, which has a more granular API for installing VSCode and any extensions your tests require. This one is potentially quite a bit more work, so I'm gonna go ahead with the first one. Something to consider for the future though.

vezwork · 2025-09-10T20:25:45Z

I made a copy of this PR: #818, it is

rebased on main,
with filtering of partial edits reverted,
with an extension test that registers a custom formatting provider

Excuse me for copying the branch&PR to a new branch&PR, but I didn't feel comfortable force pushing (because of the rebase on main) to this one.

I don't currently know how to interpret the test results I am getting in #818: The test does what I expect until I ran the test after reverting the main fix by @lionel- (the change in codeForExecutableLanguageBlock). After reverting that fix the formatter does not seem to run at all... I don't understand why.

lionel- · 2025-09-11T07:29:18Z

@lionel- we now have extension tests! The tests are focused on roundtripping right now, but they do already involve executing commands. Do you think an extension test (a snapshot test?) that opens our test qmd files, executes the format command, and checks the contents of the file would provide enough coverage for this PR?

Sounds reasonable!

create two very simple formatter classes (like in https://github.com/posit-dev/positron/pull/1686/files), one that adds a trailing newline and one that does not, then use registerDocumentFormattingEditProvider when setting up a test.

set up a new test runner that uses @vscode/test-electron, which has a more granular API for installing VSCode and any extensions your tests require. This one is potentially quite a bit more work, so I'm gonna go ahead with the first one. Something to consider for the future though.

I like (1) as it reliably targets the behaviour we want to test. The second solution would make the snapshots dependent on external behaviour (formatters like ruff change behaviour over time). This might be reasonable for monitoring a file as a sanity check, but probably not as a basis for comprehensive testing.

lionel- · 2025-09-11T08:29:33Z

I reverted this code change locally (bailing if edits are out of range) and formatting works again and the problem described in #745 is addressed.

Do you mean that we should keep applying partial edits? I don't think we should do that ever because this runs the risk of corrupting the cell contents. To see this, imagine a formatter returning two edits, one that removes lines 4-6 with 6 being out of range, and another edit that changes line 4. If you discard the first edit, the second will change the wrong line.

Relevant bit from the LSP protocol:

All text edits ranges refer to positions in the document they are computed on. They therefore move a document from state S1 to S2 without describing any intermediate state. Text edits ranges must never overlap, that means no part of the original document must be manipulated by more than one edit. However, it is possible that multiple edits have the same start position: multiple inserts, or any number of inserts followed by a single remove or replace edit. If multiple inserts have the same position, the order in the array defines the order in which the inserted strings appear in the resulting text.

Note that this requires the sorting that I added in this PR to be stable, so that the order of edits starting at the same position is preserved, which is guaranteed by ES2019. (We should add a comment about that.)

To be robust, which I think we should because of the potential of corrupting the user's work without them being aware, we should either:

Bail as I did here
Adjust the range of out-of-range edits so their high bounds are now in range

I think the reason edits may be OOR is that virtualDocForCode() appends a newline at the end. That's probably a good idea because most formatters will add an empty line at end of document if not there, which we don't want in our chunks. Since there is an additional line in the vdoc, it's possible for the formatter to extend an edit up to that line, which doesn't exist in the original chunk. (We should add a comment about this in the code.)

So it should be safe to adjust OOR edits by substracting 1 to the high bound. If the edit is still OOR after that, then we probably should error out as the edit is not consistent with our assumptions about the document.

@DavisVaughan Does this reasoning make sense to you?

vezwork · 2025-09-15T22:31:48Z

Thanks for the thorough comment @lionel-! Very clarifying.

I reverted this code change locally (bailing if edits are out of range) and formatting works again and the problem described in #745 is addressed.

Do you mean that we should keep applying partial edits? I don't think we should do that ever because this runs the risk of corrupting the cell contents. To see this, imagine a formatter returning two edits, one that removes lines 4-6 with 6 being out of range, and another edit that changes line 4. If you discard the first edit, the second will change the wrong line.

I mostly did that just because I didn't understand your fix or what is going on in this area in general and was trying things to get it working. I understand now that the right thing to do should be to modify the out of range edits like you described.

lionel- · 2025-09-16T18:12:11Z

I think the reason edits may be OOR is that virtualDocForCode() appends a newline at the end. That's probably a good idea because most formatters will add an empty line at end of document if not there, which we don't want in our chunks. Since there is an additional line in the vdoc, it's possible for the formatter to extend an edit up to that line, which doesn't exist in the original chunk. (We should add a comment about this in the code.)

Maybe a better way to go about this is to:

Take note of how many empty lines a code chunk ends with.
Don't append a newline in virtualDocForCode(), so that we format exactly the code chunk text.
Apply edits as is. Most formatters will add a trailing newline if it doesn't exist.
After applying, trim any trailing newlines, and insert the number of newlines we observed in the first step.

This way we don't need to adjust out of range text edits. This approach feels cleaner to me.

DavisVaughan · 2025-09-22T14:42:26Z

@lionel- for this particular Format Code Cell command, the formatter gets a virtual doc with just the one code cell right? If so, what you suggest sounds fairly reasonable.

Note that for something like this

```r
1 + 1



```

it will leave the trailing newlines due to your last bullet, which I don't love?

Is it simpler to do this?

Don't append a newline in virtualDocForCode(), so that we format exactly the code chunk text.
Apply edits as is. Most formatters will add a trailing newline if it doesn't exist.
Trim any trailing newlines, then insert exactly 1 trailing newline.

That way we don't need any fancy tracking, and we always clean up extraneous blank lines in a code chunk

lionel- · 2025-09-23T08:04:47Z

Hmm then maybe we should go all the way and trim all trailing newlines at the end of a chunk?

Leading and trailing newlines in chunks might be considered part of quarto formatting rules, and I think we don't expect any surrounding whitespace in chunks?

DavisVaughan · 2025-09-23T13:47:27Z

trim all trailing newlines at the end of a chunk?

I think we are in agreement already. By "exactly 1 trailing newline", I just mean that you need a \n to separate the end of the code chunk from the ending ```. I'm expecting that:

```r
1 + 1
2 + 2



```

formats to

```r
1 + 1
2 + 2
```

So yea the code content itself becomes just 1 + 1\n2 + 2 (no trailing newline, which I think is what you mean) and the final chunk becomes ```\n1 + 1\n2 + 2\n``` (trailing newline before the ```, which is what I meant)

lionel- · 2025-09-23T14:17:36Z

oh gotcha, yep I'm all for that

vezwork · 2025-09-23T15:13:36Z

@lionel- If it sounds good to you, I'll fix up my test (which is currently in #818) and commit it to this PR/branch. It should run in CI and cause the PR's checks to fail until the out-of-bounds edits behaviour is corrected.

lionel- · 2025-09-24T07:14:36Z

@vezwork Sounds good!

vezwork · 2025-09-26T19:52:34Z

rebased on main and force pushed. For some reason I'm a co-author on the commits now.

vezwork · 2025-09-26T20:09:20Z

Added a test that currently fails on this branch because of "out of range edits" from the mock formatter. It will also fail if the quarto.formatCell command does not normalize trailing newlines from formatters.

vezwork · 2026-01-16T21:08:19Z

rebased on main

vezwork · 2026-01-20T19:18:41Z

Coming back to this PR. I added a code change so virtualDocForCode(.. does not append newlines. This fixes "Formatting edits were out of range and could not be applied to the code cell." occurring when trying out the reproduction steps from #745. So the PR now fixes the issue!

I'm now trying to normalize the trailing newlines in the way that @DavisVaughan described:

Don't append a newline in virtualDocForCode(), so that we format exactly the code chunk text.

Apply edits as is. Most formatters will add a trailing newline if it doesn't exist.

Trim any trailing newlines, then insert exactly 1 trailing newline.

I am having trouble doing the 2nd and 3rd bullet: In formatBlock we start by getting TextEdit[] from a language formatter, then process those and return them; in order to trim trailing newlines, we would have to both inspect the contents of the block being formatted AND the last TextEdit received from the language formatter, and decide if we need to either 1. modify that edit (in order leave exactly 1 trailing newline), or 2. add our own TextEdit to trim trailing newlines and leaving only one. I could figure out the logic for that, but it feels kinda gross.

In other words, we have to return TextEdit[], and its tricky to logic around.

Still working through, just thought I'd drop my thoughts so far.

vezwork · 2026-01-20T21:44:10Z

I've decided the best thing to do is to try to land this PR without trailing newline normalization.

I attempted to make a more direct test of the functionality of this PR: for the formatting of

```{r}
list(foobar, foobar, foobar, foobar, foobar, foobar, foobar, foobar, foobar, foobar, foobar, foobar)
```

by logging out the edits that come from the R formatter for the virtual doc of this cell, then constructing a provider in the tests that is hard-coded to return those edits... but I realized this doesn't work because this PR fixes the virtual doc that is passed to the R formatter, resulting in it giving correct edits. So I don't think we can reasonably test that without having access to the actual R formatter, which is not reasonable to do.

So, I am going to remove the test from this PR. Those tests would still be useful if we want to tackle normalizing trailing newlines, I put the test here #890 for later.

I have tested locally in VSCode, and this PR now fixes all the issues listed in #745. It also works for Python cells.

vezwork · 2026-01-20T22:01:11Z

Ok, the PR is ready for review.

Summary of what you need to know to review:

This PR doesn't have tests for the reason I described in the previous comment.

This PR does not cause trailing newlines to be normalized when formatting cells (like a lot of the discussion in this PR was talking about). It is considered out of scope for this PR. It is left up to the language formatter. Tests for normalized trailing newlines during cell formatting were moved to #890.

I manually tested that this PR fixes the issues in #754.

The PR was almost completely written by @lionel-, though I caught a problem where we were checking if edits was out of range rather than adjustedEdits.

DavisVaughan · 2026-01-21T16:48:50Z

apps/vscode/src/providers/format.ts

-      .filter(edit => blockRange.contains(edit.range));
+      });
+
+    // Bail if any edit is out of range. We used to filter these edits out but
+    // this could bork the cell.
+    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."
+      );
+      return [];
+    }


This does feel safer to me

juliasilge

I'm supportive of us merging this because it gives us an improvement, but I want to highlight a difference in results between very similar formatters, when you use Format Document.

If you have settings like this:

{
    "[python]": {
        "editor.defaultFormatter": "charliermarsh.ruff"
    },
    "[r]": {
        "editor.defaultFormatter": "Posit.air-vscode"
    },
    "[quarto]": {
        "editor.defaultFormatter": "quarto.quarto"
    }
}

And two .qmd files like this:

---
title: "Untitled"
format: html
---

```{python}
1 + 1
```


Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

```{python}
from typing import Iterable

import os


def sum_even_numbers(numbers: Iterable[int]) -> int:
    """Given an iterable of integers, return the sum of all even numbers in the iterable."""
    return sum(
        num for num in numbers
        if num % 2 == 0
    )
```

Second one:

---
title: "Untitled"
format: html
---

```{r}
1 + 1
```


Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

```{r}
list(foobar, foobar, foobar, foobar, foobar, foobar, foobar, foobar, foobar, foobar, foobar, foobar)
```

Put your cursor in the 1 + 1 code cell
Run Format Document
In the R file, the other code cell will get formatted, which is what I would argue is the correct behavior
In the Python file, the other code cell not get formatted, which I think is not right

If you run Format Document from the code cell that needs formatting, the behavior is the same for both; it does format that code cell. The command Quarto: Format Cell also works well for both files.

We can log this problem to follow up on.

juliasilge · 2026-01-21T16:59:24Z

Can you update the CHANGELOG before merging, @vezwork?

lionel- marked this pull request as draft July 4, 2025 10:36

lionel- force-pushed the bugfix/vscode-format-cell branch from 8c9747e to 0902ef2 Compare July 4, 2025 10:52

lionel- commented Jul 4, 2025

View reviewed changes

juliasilge mentioned this pull request Sep 10, 2025

VScode extension: linting is not working #208

Open

vezwork mentioned this pull request Sep 10, 2025

Fix/vscode format cell continued #818

Closed

vezwork force-pushed the bugfix/vscode-format-cell branch from 0902ef2 to 186ac61 Compare September 26, 2025 19:52

juliasilge mentioned this pull request Oct 5, 2025

fix: protect code cell options from formatting #655

Draft

juliasilge mentioned this pull request Oct 26, 2025

Python code cells will separate # and | after formatting in quarto posit-dev/positron#9432

Open

lionel- and others added 4 commits January 16, 2026 16:06

Sort text edits by descending start position

6e0db05

Don't append lines to block code before formatting

2e1e3c4

Don't apply partial edits

2112b8c

Add cell-format test

87bfcc5

vezwork force-pushed the bugfix/vscode-format-cell branch from 873fcab to 87bfcc5 Compare January 16, 2026 21:08

don't append newline in vdoc, fix edits out-of-range check

4b7cc1f

vezwork mentioned this pull request Jan 20, 2026

Add cell formatting trailing newline normalization test #890

Draft

Remove test

a98d4cc

vezwork force-pushed the bugfix/vscode-format-cell branch from 6738101 to a98d4cc Compare January 20, 2026 21:52

Undo unecessary virtualDocForCode changes

bac96ee

vezwork marked this pull request as ready for review January 20, 2026 21:57

vezwork requested review from DavisVaughan and juliasilge January 20, 2026 22:01

DavisVaughan approved these changes Jan 21, 2026

View reviewed changes

juliasilge reviewed Jan 21, 2026

View reviewed changes

Fix cell formatting issues #754

Are you sure you want to change the base?

Fix cell formatting issues #754

Uh oh!

Conversation

lionel- commented Jul 4, 2025

Uh oh!

lionel- Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

lionel- Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

vezwork Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cscheid commented Jul 7, 2025

Uh oh!

lionel- commented Jul 7, 2025

Uh oh!

vezwork commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juliasilge commented Sep 3, 2025

Uh oh!

vezwork commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vezwork commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juliasilge commented Sep 5, 2025

Uh oh!

juliasilge commented Sep 6, 2025

Uh oh!

vezwork commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vezwork commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lionel- commented Sep 11, 2025

Uh oh!

lionel- commented Sep 11, 2025

Uh oh!

vezwork commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lionel- commented Sep 16, 2025

Uh oh!

DavisVaughan commented Sep 22, 2025

Uh oh!

lionel- commented Sep 23, 2025

Uh oh!

DavisVaughan commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lionel- commented Sep 23, 2025

Uh oh!

vezwork commented Sep 23, 2025

Uh oh!

lionel- commented Sep 24, 2025

Uh oh!

vezwork commented Sep 26, 2025

Uh oh!

vezwork commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vezwork commented Jan 16, 2026

Uh oh!

vezwork commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vezwork commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vezwork commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of what you need to know to review:

vezwork Sep 4, 2025 •

edited

Loading

vezwork commented Sep 2, 2025 •

edited

Loading

vezwork commented Sep 4, 2025 •

edited

Loading

vezwork commented Sep 5, 2025 •

edited

Loading

vezwork commented Sep 9, 2025 •

edited

Loading

vezwork commented Sep 10, 2025 •

edited

Loading

vezwork commented Sep 15, 2025 •

edited

Loading

DavisVaughan commented Sep 23, 2025 •

edited

Loading

vezwork commented Sep 26, 2025 •

edited

Loading

vezwork commented Jan 20, 2026 •

edited

Loading

vezwork commented Jan 20, 2026 •

edited

Loading

vezwork commented Jan 20, 2026 •

edited

Loading