Fix cell formatting issues

[lionel-]

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-]

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

[lionel-]

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

[vezwork]

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]

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-]

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]

@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]

@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]

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]

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]

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]

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]

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

1. 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.
2. 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]

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-]

@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!

1. 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.
2. 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-]

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:

1. Bail as I did here
2. 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]

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-]

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]

@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-]

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]

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-]

oh gotcha, yep I'm all for that

[vezwork]

@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-]

@vezwork Sounds good!

[vezwork]

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

[vezwork]

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.