Skip to content

Always use a unique virtual document file path #688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 4, 2025
Merged

Always use a unique virtual document file path #688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06e5c0d
Add an `outputChannel` we can use for logging
DavisVaughan Apr 2, 2025
11a9c10
Rework `virtualDocUriFromTempFile()` to ensure we always use unique v…
DavisVaughan Apr 2, 2025
9f58463
CHANGELOG
DavisVaughan Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/vscode/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## 1.120.0 (unreleased)

- Fix issue where format on save could overwrite the contents of a document with incorrect results (<https://github.com/quarto-dev/quarto/pull/688>).

## 1.119.0 (Release on 2025-03-21)

- Use `QUARTO_VISUAL_EDITOR_CONFIRMED` > `PW_TEST` > `CI` to bypass (`true`) or force (`false`) the Visual Editor confirmation dialogue (<https://github.com/quarto-dev/quarto/pull/654>).
Expand Down
5 changes: 4 additions & 1 deletion apps/vscode/src/lsp/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
Location,
LocationLink,
Definition,
LogOutputChannel,
} from "vscode";
import {
LanguageClient,
Expand Down Expand Up @@ -70,7 +71,8 @@ let client: LanguageClient;
export async function activateLsp(
context: ExtensionContext,
quartoContext: QuartoContext,
engine: MarkdownEngine
engine: MarkdownEngine,
outputChannel: LogOutputChannel
) {

// The server is implemented in node
Expand Down Expand Up @@ -132,6 +134,7 @@ export async function activateLsp(
},
],
middleware,
outputChannel
};

// Create the language client and start the client.
Expand Down
8 changes: 7 additions & 1 deletion apps/vscode/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ import { configuredQuartoPath } from "./core/quarto";
import { activateDenoConfig } from "./providers/deno-config";

export async function activate(context: vscode.ExtensionContext) {
// create output channel for extension logs and lsp client logs
const outputChannel = vscode.window.createOutputChannel("Quarto", { log: true });

outputChannel.info("Activating Quarto extension.");
Comment on lines +40 to +43
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found it extremely useful to have an outputChannel to log to while debugging. I plumbed this through to points of interest so I could log.

I have since removed the log calls, but I still think this is quite useful for future us so I've left the actual output channel in to make this easier later on.

It is shared with the LSP client, essentially we provide our own output channel now rather than having the LSP client generate one automatically for us (we do the same thing in positron)


// create extension host
const host = extensionHost();
Expand Down Expand Up @@ -84,7 +88,7 @@ export async function activate(context: vscode.ExtensionContext) {
activateDenoConfig(context, engine);

// lsp
const lspClient = await activateLsp(context, quartoContext, engine);
const lspClient = await activateLsp(context, quartoContext, engine, outputChannel);

// provide visual editor
const editorCommands = activateEditor(context, host, quartoContext, lspClient, engine);
Expand Down Expand Up @@ -125,6 +129,8 @@ export async function activate(context: vscode.ExtensionContext) {

// activate providers common to browser/node
activateCommon(context, host, engine, commands);

outputChannel.info("Activated Quarto extension.");
}

export async function deactivate() {
Expand Down
127 changes: 85 additions & 42 deletions apps/vscode/src/vdoc/vdoc-tempfile.ts
View file
Open in desktop
Copy link
Collaborator Author

@DavisVaughan DavisVaughan Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have streamlined this function quite a bit. I found that pretty much everything was shared code, and we don't actually need an "early exit" path for the local case - it's just the normal path tweaked to create the file in a known directory rather than a temp directory.

The diffs are a little large but I hope it is a big improvement overall

(PS, it is much easier to review this file in Split View in the github ui)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really the main actual change is that the local branch that used to exist used a static .vdoc.{ext} as the file path, which was being overwritten in the middle of an lsp request. It now uses .vdoc.{uuid}.{ext} to avoid this.

Original file line number Diff line number Diff line change
Expand Up @@ -29,45 +29,50 @@ import {
} from "vscode";
import { VirtualDoc, VirtualDocUri } from "./vdoc";

/**
* Create an on disk temporary file containing the contents of the virtual document
*
* @param virtualDoc The document to use when populating the temporary file
* @param docPath The path to the original document the virtual document is
* based on. When `local` is `true`, this is used to determine the directory
* to create the temporary file in.
* @param local Whether or not the temporary file should be created "locally" in
* the workspace next to `docPath` or in a temporary directory outside the
* workspace.
* @returns A `VirtualDocUri`
*/
export async function virtualDocUriFromTempFile(
virtualDoc: VirtualDoc,
docPath: string,
local: boolean
): Promise<VirtualDocUri> {
const newVirtualDocUri = (doc: TextDocument) =>
<VirtualDocUri>{
uri: doc.uri,
cleanup: async () => await deleteDocument(doc),
};

// if this is local then create it alongside the docPath and return a cleanup
// function to remove it when the action is completed.
if (local || virtualDoc.language.localTempFile) {
const ext = virtualDoc.language.extension;
const vdocPath = path.join(path.dirname(docPath), `.vdoc.${ext}`);
fs.writeFileSync(vdocPath, virtualDoc.content);
const vdocUri = Uri.file(vdocPath);
const doc = await workspace.openTextDocument(vdocUri);
return newVirtualDocUri(doc);
}
const useLocal = local || virtualDoc.language.localTempFile;

// write the virtual doc as a temp file
const vdocTempFile = createVirtualDocTempFile(virtualDoc);
// If `useLocal`, then create the temporary document alongside the `docPath`
// so tools like formatters have access to workspace configuration. Otherwise,
// create it in a temp directory.
const virtualDocFilepath = useLocal
? createVirtualDocLocalFile(virtualDoc, path.dirname(docPath))
: createVirtualDocTempfile(virtualDoc);

// open the document and save a reference to it
const vdocUri = Uri.file(vdocTempFile);
const doc = await workspace.openTextDocument(vdocUri);
const virtualDocUri = Uri.file(virtualDocFilepath);
const virtualDocTextDocument = await workspace.openTextDocument(virtualDocUri);

// TODO: Reevaluate whether this is necessary. Old comment:
// > if this is the first time getting a virtual doc for this
// > language then execute a dummy request to cause it to load
await commands.executeCommand<Hover[]>(
"vscode.executeHoverProvider",
vdocUri,
new Position(0, 0)
);
if (!useLocal) {
// TODO: Reevaluate whether this is necessary. Old comment:
// > if this is the first time getting a virtual doc for this
// > language then execute a dummy request to cause it to load
await commands.executeCommand<Hover[]>(
"vscode.executeHoverProvider",
virtualDocUri,
new Position(0, 0)
);
}

return newVirtualDocUri(doc);
return <VirtualDocUri>{
uri: virtualDocTextDocument.uri,
cleanup: async () => await deleteDocument(virtualDocTextDocument),
};
}

// delete a document
Expand All @@ -82,19 +87,57 @@ async function deleteDocument(doc: TextDocument) {
}
}

// create temp files for vdocs. use a base directory that has a subdirectory
// for each extension used within the document. this is a no-op if the
// file already exists
tmp.setGracefulCleanup();
const vdocTempDir = tmp.dirSync().name;
function createVirtualDocTempFile(virtualDoc: VirtualDoc) {
const ext = virtualDoc.language.extension;
const dir = path.join(vdocTempDir, ext);
if (!fs.existsSync(dir)) {
fs.mkdirSync(dir);
const VIRTUAL_DOC_TEMP_DIRECTORY = tmp.dirSync().name;

/**
* Creates a virtual document in a temporary directory
*
* The temporary directory is automatically cleaned up on process exit.
*
* @param virtualDoc The document to use when populating the temporary file
* @returns The path to the temporary file
*/
function createVirtualDocTempfile(virtualDoc: VirtualDoc): string {
const filepath = generateVirtualDocFilepath(VIRTUAL_DOC_TEMP_DIRECTORY, virtualDoc.language.extension);
createVirtualDoc(filepath, virtualDoc.content);
return filepath;
}

/**
* Creates a virtual document in the provided directory
*
* @param virtualDoc The document to use when populating the temporary file
* @param directory The directory to create the temporary file in
* @returns The path to the temporary file
*/
function createVirtualDocLocalFile(virtualDoc: VirtualDoc, directory: string): string {
const filepath = generateVirtualDocFilepath(directory, virtualDoc.language.extension);
createVirtualDoc(filepath, virtualDoc.content);
return filepath;
}

/**
* Creates a file filled with the provided content
*/
function createVirtualDoc(filepath: string, content: string): void {
const directory = path.dirname(filepath);

if (!fs.existsSync(directory)) {
fs.mkdirSync(directory);
}
const tmpPath = path.join(vdocTempDir, ext, ".intellisense." + uuid.v4() + "." + ext);
fs.writeFileSync(tmpPath, virtualDoc.content);

return tmpPath;
fs.writeFileSync(filepath, content);
}

/**
* Generates a unique virtual document file path
*
* It is important for virtual documents to have unique file paths. If a static
* name like `.vdoc.{ext}` is used, it is possible for one language server
* request to overwrite the contents of the virtual document while another
* language server request is running (#683).
*/
function generateVirtualDocFilepath(directory: string, extension: string): string {
return path.join(directory, ".vdoc." + uuid.v4() + "." + extension);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that for non-local LSP requests this was called .intellisense.{uuid}.{ext} but I do not think that name mattered. We now always use .vdoc.{uuid}.{extension}.

}