Skip to content
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.

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

Don't open closed documents #7826

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions src/lsptoolshost/roslynLanguageServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ import {
import { registerSourceGeneratedFilesContentProvider } from './sourceGeneratedFilesContentProvider';
import { registerMiscellaneousFileNotifier } from './miscellaneousFileNotifier';
import { TelemetryEventNames } from '../shared/telemetryEventNames';
import { RazorDynamicFileChangedParams } from '../razor/src/dynamicFile/dynamicFileUpdatedParams';

let _channel: vscode.LogOutputChannel;
let _traceChannel: vscode.OutputChannel;
Expand Down Expand Up @@ -789,6 +790,11 @@ export class RoslynLanguageServer {
async (notification) =>
vscode.commands.executeCommand(DynamicFileInfoHandler.removeDynamicFileInfoCommand, notification)
);
vscode.commands.registerCommand(
DynamicFileInfoHandler.dynamicFileUpdatedCommand,
async (notification: RazorDynamicFileChangedParams) =>
this.sendNotification<RazorDynamicFileChangedParams>('razor/dynamicFileInfoChanged', notification)
);
}

// eslint-disable-next-line @typescript-eslint/promise-function-async
Expand Down
61 changes: 52 additions & 9 deletions src/razor/src/csharp/csharpProjectedDocument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export class CSharpProjectedDocument implements IProjectedDocument {
private resolveProvisionalEditAt: number | undefined;
private ProvisionalDotPosition: Position | undefined;
private hostDocumentVersion: number | null = null;
private updates: Update[] | null = null;

public constructor(public readonly uri: vscode.Uri) {
this.path = getUriPath(uri);
Expand All @@ -39,19 +40,43 @@ export class CSharpProjectedDocument implements IProjectedDocument {
public update(edits: ServerTextChange[], hostDocumentVersion: number) {
this.removeProvisionalDot();

// Apply any stored edits if needed
if (this.updates) {
for (const update of this.updates) {
this.updateContent(update.changes);
}

this.updates = null;
}

this.hostDocumentVersion = hostDocumentVersion;
this.updateContent(edits);
}

if (edits.length === 0) {
return;
public storeEdits(edits: ServerTextChange[], hostDocumentVersion: number) {
this.hostDocumentVersion = hostDocumentVersion;
if (this.updates) {
this.updates = this.updates.concat(new Update(edits));
} else {
this.updates = [new Update(edits)];
}
}

let content = this.content;
for (const edit of edits.reverse()) {
// TODO: Use a better data structure to represent the content, string concatenation is slow.
content = this.getEditedContent(edit.newText, edit.span.start, edit.span.start + edit.span.length, content);
public getAndApplyEdits() {
const updates = this.updates;
this.updates = null;

if (updates) {
let changes: ServerTextChange[] = [];
for (const update of updates) {
this.updateContent(update.changes);
changes = changes.concat(update.changes);
}

return changes;
}

this.setContent(content);
return null;
}

public getContent() {
Expand Down Expand Up @@ -140,8 +165,8 @@ export class CSharpProjectedDocument implements IProjectedDocument {
}

private getEditedContent(newText: string, start: number, end: number, content: string) {
const before = content.substr(0, start);
const after = content.substr(end);
const before = content.substring(0, start);
const after = content.substring(end);
content = `${before}${newText}${after}`;

return content;
Expand All @@ -150,4 +175,22 @@ export class CSharpProjectedDocument implements IProjectedDocument {
private setContent(content: string) {
this.content = content;
}

private updateContent(edits: ServerTextChange[]) {
if (edits.length === 0) {
return;
}

let content = this.content;
for (const edit of edits.reverse()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised this works, given elsewhere the edits are concated together. Wouldn't we need to insert subsequent edits at position 0?

Though that could be at odds with the Roslyn algorithm that processes the same edits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good point. I don't think I've actually hit a scenario where they got concat together in testing so probably missed this. Idk why reverse was used initially. Let me look...

Copy link
Contributor

Choose a reason for hiding this comment

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

Reverse makes sense for a single batch of ordered changes, it means you don't need to track forward when applying edits earlier in the file, and adjust positions later in the file. But the batches would need to be applied in order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize they were guaranteed to be ordered. Or that it was guaranteed to not overlap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can just update to applying stored edits first instead of doing a concat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be better now. I updated to keep track of Updates as individual units sent from Razor. How they get applied remains consistent and the edits being returned on ordered to be sent back to Roslyn

// TODO: Use a better data structure to represent the content, string concatenation is slow.
content = this.getEditedContent(edit.newText, edit.span.start, edit.span.start + edit.span.length, content);
}

this.setContent(content);
}
}

class Update {
constructor(public readonly changes: ServerTextChange[]) {}
}
1 change: 1 addition & 0 deletions src/razor/src/document/IRazorDocument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ export interface IRazorDocument {
readonly uri: vscode.Uri;
readonly csharpDocument: IProjectedDocument;
readonly htmlDocument: IProjectedDocument;
readonly isOpen: boolean;
}
32 changes: 32 additions & 0 deletions src/razor/src/document/razorDocument.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import * as vscode from 'vscode';
import { CSharpProjectedDocument } from '../csharp/csharpProjectedDocument';
import { HtmlProjectedDocument } from '../html/htmlProjectedDocument';
import { getUriPath } from '../uriPaths';
import { IRazorDocument } from './IRazorDocument';

export class RazorDocument implements IRazorDocument {
public readonly path: string;

constructor(
readonly uri: vscode.Uri,
readonly csharpDocument: CSharpProjectedDocument,
readonly htmlDocument: HtmlProjectedDocument
) {
this.path = getUriPath(uri);
}

public get isOpen(): boolean {
for (const textDocument of vscode.workspace.textDocuments) {
if (textDocument.uri.fsPath == this.uri.fsPath) {
return true;
}
}

return false;
}
}
12 changes: 3 additions & 9 deletions src/razor/src/document/razorDocumentFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,12 @@ import { HtmlProjectedDocumentContentProvider } from '../html/htmlProjectedDocum
import { virtualCSharpSuffix, virtualHtmlSuffix } from '../razorConventions';
import { getUriPath } from '../uriPaths';
import { IRazorDocument } from './IRazorDocument';
import { RazorDocument } from './razorDocument';

export function createDocument(uri: vscode.Uri) {
export function createDocument(uri: vscode.Uri): IRazorDocument {
const csharpDocument = createProjectedCSharpDocument(uri);
const htmlDocument = createProjectedHtmlDocument(uri);
const path = getUriPath(uri);

const document: IRazorDocument = {
uri,
path,
csharpDocument,
htmlDocument,
};
const document = new RazorDocument(uri, csharpDocument, htmlDocument);

return document;
}
Expand Down
57 changes: 16 additions & 41 deletions src/razor/src/document/razorDocumentManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,21 +50,12 @@ export class RazorDocumentManager implements IRazorDocumentManager {
return Object.values(this.razorDocuments);
}

public async getDocument(uri: vscode.Uri) {
public async getDocument(uri: vscode.Uri): Promise<IRazorDocument> {
const document = this._getDocument(uri);

// VS Code closes virtual documents after some timeout if they are not open in the IDE. Since our generated C# and Html
// documents are never open in the IDE, we need to ensure that VS Code considers them open so that requests against them
// succeed. Without this, even a simple diagnostics request will fail in Roslyn if the user just opens a .razor document
// and leaves it open past the timeout.
if (this.razorDocumentGenerationInitialized) {
await this.ensureDocumentAndProjectedDocumentsOpen(document);
}

return document;
}

public async getActiveDocument() {
public async getActiveDocument(): Promise<IRazorDocument | null> {
if (!vscode.window.activeTextEditor) {
return null;
}
Expand Down Expand Up @@ -147,7 +138,7 @@ export class RazorDocumentManager implements IRazorDocumentManager {
return vscode.Disposable.from(watcher, didCreateRegistration, didOpenRegistration, didCloseRegistration);
}

private _getDocument(uri: vscode.Uri) {
private _getDocument(uri: vscode.Uri): IRazorDocument {
const path = getUriPath(uri);
let document = this.findDocument(path);

Expand All @@ -159,7 +150,7 @@ export class RazorDocumentManager implements IRazorDocumentManager {
document = this.addDocument(uri);
}

return document;
return document!;
}

private async openDocument(uri: vscode.Uri) {
Expand All @@ -182,10 +173,6 @@ export class RazorDocumentManager implements IRazorDocumentManager {
await vscode.commands.executeCommand(razorInitializeCommand, pipeName);
await this.serverClient.connectNamedPipe(pipeName);

for (const document of this.documents) {
await this.ensureDocumentAndProjectedDocumentsOpen(document);
}

this.onRazorInitializedEmitter.fire();
}
}
Expand All @@ -205,7 +192,7 @@ export class RazorDocumentManager implements IRazorDocumentManager {
this.notifyDocumentChange(document, RazorDocumentChangeKind.closed);
}

private addDocument(uri: vscode.Uri) {
private addDocument(uri: vscode.Uri): IRazorDocument {
const path = getUriPath(uri);
let document = this.findDocument(path);
if (document) {
Expand Down Expand Up @@ -261,10 +248,6 @@ export class RazorDocumentManager implements IRazorDocumentManager {
) {
// We allow re-setting of the updated content from the same doc sync version in the case
// of project or file import changes.

// Make sure the document is open, because updating will cause a didChange event to fire.
await vscode.workspace.openTextDocument(document.csharpDocument.uri);

const csharpProjectedDocument = projectedDocument as CSharpProjectedDocument;

// If the language server is telling us that the previous document was empty, then we should clear
Expand All @@ -275,7 +258,17 @@ export class RazorDocumentManager implements IRazorDocumentManager {
csharpProjectedDocument.clear();
}

csharpProjectedDocument.update(updateBufferRequest.changes, updateBufferRequest.hostDocumentVersion);
if (document.isOpen) {
// Make sure the document is open, because updating will cause a didChange event to fire.
await vscode.workspace.openTextDocument(document.csharpDocument.uri);

csharpProjectedDocument.update(updateBufferRequest.changes, updateBufferRequest.hostDocumentVersion);
} else {
csharpProjectedDocument.storeEdits(
updateBufferRequest.changes,
updateBufferRequest.hostDocumentVersion
);
}

this.notifyDocumentChange(document, RazorDocumentChangeKind.csharpChanged);
} else {
Expand Down Expand Up @@ -342,22 +335,4 @@ export class RazorDocumentManager implements IRazorDocumentManager {

this.onChangeEmitter.fire(args);
}

private async ensureDocumentAndProjectedDocumentsOpen(document: IRazorDocument) {
// vscode.workspace.openTextDocument may send a textDocument/didOpen
// request to the C# language server. We need to keep track of
// this to make sure we don't send a duplicate request later on.
const razorUri = vscode.Uri.file(document.path);
if (!this.isRazorDocumentOpenInCSharpWorkspace(razorUri)) {
this.didOpenRazorCSharpDocument(razorUri);

// Need to tell the Razor server that the document is open, or it won't generate C# code
// for it, and our projected document will always be empty, until the user manually
// opens the razor file.
await vscode.workspace.openTextDocument(razorUri);
}

await vscode.workspace.openTextDocument(document.csharpDocument.uri);
await vscode.workspace.openTextDocument(document.htmlDocument.uri);
}
}
54 changes: 38 additions & 16 deletions src/razor/src/dynamicFile/dynamicFileInfoHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,23 @@
*--------------------------------------------------------------------------------------------*/

import * as vscode from 'vscode';
import { DocumentUri } from 'vscode-languageclient/node';
import { UriConverter } from '../../../lsptoolshost/uriConverter';
import { RazorDocumentManager } from '../document/razorDocumentManager';
import { RazorLogger } from '../razorLogger';
import { ProvideDynamicFileParams } from './provideDynamicFileParams';
import { ProvideDynamicFileResponse } from './provideDynamicFileResponse';
import { RemoveDynamicFileParams } from './removeDynamicFileParams';
import { CSharpProjectedDocument } from '../csharp/csharpProjectedDocument';
import { RazorDocumentChangeKind } from '../document/razorDocumentChangeKind';
import { RazorDynamicFileChangedParams } from './dynamicFileUpdatedParams';
import { TextDocumentIdentifier } from 'vscode-languageserver-protocol';

// Handles Razor generated doc communication between the Roslyn workspace and Razor.
// didChange behavior for Razor generated docs is handled in the RazorDocumentManager.
export class DynamicFileInfoHandler {
public static readonly provideDynamicFileInfoCommand = 'razor.provideDynamicFileInfo';
public static readonly removeDynamicFileInfoCommand = 'razor.removeDynamicFileInfo';
public static readonly dynamicFileUpdatedCommand = 'razor.dynamicFileUpdated';

constructor(private readonly documentManager: RazorDocumentManager, private readonly logger: RazorLogger) {}

Expand All @@ -33,39 +37,57 @@ export class DynamicFileInfoHandler {
await this.removeDynamicFileInfo(request);
}
);
this.documentManager.onChange(async (e) => {
if (e.kind == RazorDocumentChangeKind.csharpChanged && !e.document.isOpen) {
const uriString = UriConverter.serialize(e.document.uri);
const identifier = TextDocumentIdentifier.create(uriString);
await vscode.commands.executeCommand(
DynamicFileInfoHandler.dynamicFileUpdatedCommand,
new RazorDynamicFileChangedParams(identifier)
);
}
});
}

// Given Razor document URIs, returns associated generated doc URIs
private async provideDynamicFileInfo(
request: ProvideDynamicFileParams
): Promise<ProvideDynamicFileResponse | null> {
let virtualUri: DocumentUri | null = null;
this.documentManager.roslynActivated = true;
const vscodeUri = vscode.Uri.parse(request.razorDocument.uri, true);

// Normally we start receiving dynamic info after Razor is initialized, but if the user had a .razor file open
// when they started VS Code, the order is the other way around. This no-ops if Razor is already initialized.
await this.documentManager.ensureRazorInitialized();

const razorDocument = await this.documentManager.getDocument(vscodeUri);
try {
const vscodeUri = vscode.Uri.parse(request.razorDocument.uri, true);
const razorDocument = await this.documentManager.getDocument(vscodeUri);
if (razorDocument === undefined) {
this.logger.logWarning(
`Could not find Razor document ${vscodeUri.fsPath}; adding null as a placeholder in URI array.`
);
} else {
// Retrieve generated doc URIs for each Razor URI we are given
const virtualCsharpUri = UriConverter.serialize(razorDocument.csharpDocument.uri);
virtualUri = virtualCsharpUri;

return null;
}

this.documentManager.roslynActivated = true;
const virtualCsharpUri = UriConverter.serialize(razorDocument.csharpDocument.uri);

// Normally we start receiving dynamic info after Razor is initialized, but if the user had a .razor file open
// when they started VS Code, the order is the other way around. This no-ops if Razor is already initialized.
await this.documentManager.ensureRazorInitialized();
if (this.documentManager.isRazorDocumentOpenInCSharpWorkspace(vscodeUri)) {
// Open documents have didOpen/didChange to update the csharp buffer. Razor
// does not send edits and instead lets vscode handle them.
return new ProvideDynamicFileResponse({ uri: virtualCsharpUri }, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this reset any stored edits, so that when a document is closed, things have a clean slate?

ie, when a document is opened, LSP will send a didOpen with the full contents, then any future edits, and then when its closed, are we at risk of sending some edits that were made before the open?

} else {
// Closed documents provide edits since the last time they were requested since
// there is no open buffer in vscode corresponding to the csharp content.
const csharpDocument = razorDocument.csharpDocument as CSharpProjectedDocument;
const edits = csharpDocument.getAndApplyEdits();

return new ProvideDynamicFileResponse({ uri: virtualCsharpUri }, edits ?? null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this can send null, and open docs always do, and thinking about the above comment, I wonder if these two branches can be combined. ie, on open, clear the edits, then provide the dynamic file response with edits ?? null.

}
} catch (error) {
this.logger.logWarning(`${DynamicFileInfoHandler.provideDynamicFileInfoCommand} failed with ${error}`);
}

if (virtualUri) {
return new ProvideDynamicFileResponse({ uri: virtualUri });
}

return null;
}

Expand Down
10 changes: 10 additions & 0 deletions src/razor/src/dynamicFile/dynamicFileUpdatedParams.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { TextDocumentIdentifier } from 'vscode-languageserver-protocol';

export class RazorDynamicFileChangedParams {
constructor(public readonly razorDocument: TextDocumentIdentifier) {}
}
Loading
Loading