Remove virtualDocUri() as a footgun
#713
Conversation
| await vdocUri.cleanup(); | ||
| return await withVirtualDocUri(vdoc, document.uri, "hover", async (uri: Uri) => { | ||
| try { | ||
| return await getHover(uri, vdoc.language, position); |
There was a problem hiding this comment.
We did not await before but I'm guessing it's probably better to?
There was a problem hiding this comment.
I think you are correct on this; I occasionally would observe slight glitchy-ness with the embedded help in Quarto files and I can't get it to happen now with this PR.
| await vdocUri.cleanup(); | ||
| return await withVirtualDocUri(vdoc, document.uri, "signature", async (uri: Uri) => { | ||
| try { | ||
| return await getSignatureHelpHover(uri, vdoc.language, position, context.triggerCharacter); |
There was a problem hiding this comment.
We did not await before but I'm guessing it's probably better to?
| @@ -280,64 +272,61 @@ function embeddedGoToDefinitionProvider(engine: MarkdownEngine) { | |||
| ): Promise<Definition | LocationLink[] | null | undefined> => { | |||
| const vdoc = await virtualDoc(document, position, engine); | |||
| if (vdoc) { | |||
| const vdocUri = await virtualDocUri(vdoc, document.uri, "definition"); | |||
There was a problem hiding this comment.
This diff looks ugly but I didn't change anything in the try or catch block. It's just switching to withVirtualDocUri() and removing the finally block
| await vdocUri.cleanup(); | ||
| return await withVirtualDocUri(vdoc, document.uri, "hover", async (uri: Uri) => { | ||
| try { | ||
| return await getHover(uri, vdoc.language, position); |
There was a problem hiding this comment.
I think you are correct on this; I occasionally would observe slight glitchy-ness with the embedded help in Quarto files and I can't get it to happen now with this PR.
There were 3 remaining places where we used
virtualDocUri()instead ofwithVirtualDocUri(). The ad hoc usage of this is scary because it requires us to carefully analyze the call to ensure we call thecleanup()hook (#708).withVirtualDocUri()always handles this, so this PR removesvirtualDocUri()as a footgun altogether by routing everything throughwithVirtualDocUri()and then unexportingvirtualDocUri().