Make provideStatementRange() create a single chunk virtual doc#914
Merged
DavisVaughan merged 4 commits intoquarto-dev:mainfrom Feb 12, 2026
Merged
Conversation
Collaborator
|
Addresses posit-dev/positron#3233 partially |
juliasilge
reviewed
Feb 11, 2026
Collaborator
juliasilge
left a comment
juliasilge
left a comment
There was a problem hiding this comment.
This is working great for me! I reviewed with the current Positron daily so I don't have posit-dev/ark#1030 in it, but the behavior is as expected and looks good.
Before we merge this, can you:
- add a new
vdoc.test.tsto the extension tests here with some basic tests of the virtual documents we make with the new options? One option is to make a utility similar toroundtripSnapshotTest()but for virtual docs (one way only,.qmd➡️.Ror.py) and another option would be to inline the structure of the vdoc in the test file. Take a look at what we have inapps/vscode/src/test/examplesand the tests inquartoDoc.test.tsfor example on snapshots. Be aware you will not have access to any language pack extension in the tests, only the Quarto extension itself, but I think that will be fine in this case. - update the copyright year to say "Copyright (C) 2022-2026 by Posit Software, PBC" on the files you edited?
DavisVaughan
added a commit
to posit-dev/positron
that referenced
this pull request
Feb 24, 2026
Addresses #8350 - Requires posit-dev/ark#1040 - Blocks quarto-dev/quarto#919 Builds on: - quarto-dev/quarto#914 - posit-dev/ark#1030 # Summary This PR adds a new throwable `StatementRangeSyntaxError` to `positron.d.ts`. ```ts /** * An error thrown by a {@link StatementRangeProvider} to indicate that a statement range * cannot be provided due to a syntax error in the document. */ export class StatementRangeSyntaxError extends Error { /** * Zero indexed line number where the syntax error occurred. */ readonly line?: number; /** * Creates a new statement range syntax error. * * @param line Zero indexed line number where the syntax error occurred. */ constructor(line?: number); } ``` This is the only public facing API change from this PR. The idea is that from extension land (`positron-r`, `positron-python`, and `quarto`) you should be able to throw a `StatementRangeSyntaxError` from your `provideStatementRange()` provider, with an optional `line` number of where the syntax error _roughly_ starts. This gets propagated up through the main thread and into our `Cmd+Enter` handling in `positronConsoleActions.ts`, where we now detect this special case and: - Emit an info level notification telling the user that we can't execute code due to a syntax error. If the `line` number is present, we give them a button to jump to that line. - Bail from `Cmd+Enter` handling altogether. We don't even do one-line-at-a-time execution. #8350 has proved that this is just too confusing when you are trying to execute code after a syntax error. Here is what the end result looks like in an R script: https://github.com/user-attachments/assets/23dca240-06af-40d5-8b1b-a8b24af99259 Note how you can execute R code _above_ the first syntax error, thanks to posit-dev/ark#1030. It is only _below_ the first syntax error that we emit this notification and refuse to execute. It even works in roxygen2 comments where we have a little "subdocument": https://github.com/user-attachments/assets/ec920753-400d-4493-b534-188dcf4e2a65 And lastly, it works in Quarto, where we have a "virtual document" for R/Python and map the `line` number from that virtual document back into the original Qmd, thanks to quarto-dev/quarto#919 (note that if you try to run that Quarto PR, you need to remove the version guard on Positron 2026.03.0 manually). Additionally, quarto-dev/quarto#914 ensures that you can use StatementRange in any syntax-error-free cell, even if some other cell in the document has a syntax error. https://github.com/user-attachments/assets/a01b9ed1-22fe-4169-b088-a8393800d9db # Design I considered many different designs along the way, but landed on a model of: - Extension host side _models errors as throwable errors_, i.e. with `StatementRange` and `throw StatementRangeSyntaxError` ```ts export interface StatementRange { readonly range: vscode.Range; readonly code?: string; } export class StatementRangeSyntaxError extends Error { readonly line?: number; constructor(line?: number); } ``` - Main thread side _models errors as data_, i.e. a single `IStatementRange` type made up as: ```ts type IStatementRange = IStatementRangeSuccess | IStatementRangeRejection type IStatementRangeRejection = IStatementRangeSyntaxRejection export interface IStatementRangeSuccess { readonly kind: StatementRangeKind.Success; readonly range: IRange; readonly code?: string; } export interface IStatementRangeSyntaxRejection { readonly kind: StatementRangeKind.Rejection; readonly rejectionKind: StatementRangeRejectionKind.Syntax; readonly line?: number; } ``` Some rationale: - Backwards compatible and public facing `StatementRange` doesn't change - Can add new error/rejection variants as needed - Can't easily model main thread side errors as, well, `Error`s, because the `StatementRangeSyntaxError` doesn't serialize across the extension -> main thread boundary well. You basically have to turn it into some "data" type anyways to do this, and then it doesn't feel worth it to convert back on the main thread side. - Ark's custom `StatementRange` LSP Request also now uses the "success" vs "rejection" model. From Ark's LSP-ish perspective, a "rejection" is an allowed value that can be returned as an LSP Response. We reserve the JSONRPC error path for true "holy crap something horrible happened and I can't complete this request" cases. The end result looks like this: <img width="5127" height="4470" alt="test" src="https://github.com/user-attachments/assets/b341c1dd-edac-45a2-9aed-03a4b05b3c01" /> # Prior art There isn't much prior art for exactly what I'm trying to do here, but there is a little bit: - `FileSystemError` with its various flavors. Not quite the same as us because none of the flavors have any metadata to pass through, like `line`. That metadata is what steered me away from trying to have one overarching `StatementRangeError` class that could contain something like `static SyntaxError(line?: number): StatementRangeError`. I did try this, but the boilerplate didn't feel worth it. https://github.com/posit-dev/positron/blob/31c0111d42eaff76f06e1800836663e5765e1df1/src/vscode-dts/vscode.d.ts#L9474-L9486 - `Rejection`, which is used by the `RenameProvider`. Catches an error from the LSP side of things and converts it into a `Rejection` "data" type, kind of like we do. Strangely does `RenameProvider & Rejection` as the return value type. - Defined here https://github.com/posit-dev/positron/blob/9754ca9e56524315fa6b2d1981fb6cb82ef5cd7b/src/vs/editor/common/languages.ts#L2127-L2138 - Catching the error here https://github.com/posit-dev/positron/blob/9754ca9e56524315fa6b2d1981fb6cb82ef5cd7b/src/vs/workbench/api/common/extHostLanguageFeatures.ts#L882-L903 # QA ### Release Notes #### New Features - Stepping through code with `Cmd + Enter` now works more reliably when there are syntax errors in the document: - For R code: - [Code _above_ the first syntax error](posit-dev/ark#1030) can now be executed reliably with `Cmd + Enter` - [Code _below_ the first syntax error](#11907) causes a notification to pop up that informs you that reliable execution is impossible and provides you with a button to jump to the syntax error - For Quarto documents, a syntax error in one chunk no longer affects the ability to [run code in other chunks](quarto-dev/quarto#914) ### QA Notes QA: It would be nice to have two integration tests ```r 1 + 1 # <put cursor here and execute this, it should work now and send the whole statement> 2 + \ 2 ``` ```r 1 + \ 1 2 + 2 # <put cursor here and execute this, it should not send anything, and a notification should pop up> ``` `@:ark` `@:quarto` `@:console` --------- Signed-off-by: Davis Vaughan <davis@posit.co> Co-authored-by: Lionel Henry <lionel.hry@proton.me>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 2 of the statement range refinement plan
Surgical tweak to
provideStatementRange()andvirtualDoc()to only make the virtual doc for that chunk, so parse errors in one chunk can't render other chunks useless from a statement range perspectiveYou also need posit-dev/ark#1030 if you want to test that you can execute code above a parse error within the same chunk, but that isn't critical to the ability to execute code in an arbitrary chunk if one of them has a parse error.
Before, if you make a typo in one chunk:
quarto-before.mov
After, if you make a typo in one chunk:
quarto-after.mov