Drop textDocument/didClose if dropping textDocument/didOpen (#1682)

* Drop textDocument/didClose if dropping textDocument/didOpen

When using delayOpenNotifications=true, the open notifications are queued and dropped if a close notification immediately follows. However, the original implementation still sent the close notification. This does not match the spec:

> A close notification requires a previous open notification to be sent

This change also drops the close notification if the open notification was dropped.

Fixes #1681

* Fix comment

Author: DanTup

client-node-tests/src/integration.test.ts

@@ -27,6 +27,17 @@ namespace SetDiagnosticsNotification {
 	export const type = new lsclient.NotificationType<proto.DocumentDiagnosticReport>(method);
 }
 
+/**
+ * A custom request to get a list of all text sync notifications that the server
+ * has been sent.
+ *
+ * Implemented in textSyncServer.ts
+ */
+namespace GetNotificationsRequest {
+	export const method: 'testing/getNotifications' = 'testing/getNotifications';
+	export const type = new lsclient.RequestType0<string[], void>(method);
+}
+
 async function revertAllDirty(): Promise<void> {
 	return vscode.commands.executeCommand('_workbench.revertAllDirty');
 }
@@ -2227,3 +2238,73 @@ suite('Server activation', () => {
 		await client.stop();
 	});
 });
+
+suite('delayOpenNotifications', () => {
+	let client: lsclient.LanguageClient;
+
+	async function startClient(delayOpen: boolean): Promise<void> {
+		const serverModule = path.join(__dirname, './servers/textSyncServer.js');
+		const serverOptions: lsclient.ServerOptions = {
+			run: { module: serverModule, transport: lsclient.TransportKind.ipc },
+			debug: { module: serverModule, transport: lsclient.TransportKind.ipc, options: { execArgv: ['--nolazy', '--inspect=6014'] } }
+		};
+
+		const clientOptions: lsclient.LanguageClientOptions = {
+			documentSelector: [{ language: 'plaintext' }],
+			synchronize: {},
+			initializationOptions: {},
+			middleware: {},
+			textSynchronization: {
+				delayOpenNotifications: delayOpen
+			}
+		};
+		(clientOptions as ({ $testMode?: boolean })).$testMode = true;
+
+		client = new lsclient.LanguageClient('test svr', 'Test Language Server', serverOptions, clientOptions);
+		await client.start();
+	}
+
+	const fakeDocument = {
+		uri: 'untitled:test.txt',
+		languageId: 'plaintext',
+		getText: () => '',
+	} as any as vscode.TextDocument;
+
+	function sendDidOpen(document: vscode.TextDocument) {
+		return client.getFeature(lsclient.DidOpenTextDocumentNotification.method).getProvider(document)!.send(document);
+	}
+
+	function sendDidClose(document: vscode.TextDocument) {
+		return client.getFeature(lsclient.DidCloseTextDocumentNotification.method).getProvider(document)!.send(document);
+	}
+
+	teardown(async () =>  client.stop());
+
+	test('didOpen/didClose are not sent when delayOpenNotifications=true', async () => {
+		await startClient(true);
+
+		await sendDidOpen(fakeDocument);
+		await sendDidClose(fakeDocument);
+
+		// Ensure no notifications.
+		const notifications = await client.sendRequest(GetNotificationsRequest.type);
+		assert.deepStrictEqual(notifications, []);
+	});
+
+	test('didOpen/didClose are always sent when delayOpenNotifications=false', async () => {
+		await startClient(false);
+
+		await sendDidOpen(fakeDocument);
+		await sendDidClose(fakeDocument);
+
+		// Ensure both notifications.
+		const notifications = await client.sendRequest(GetNotificationsRequest.type);
+		assert.deepStrictEqual(
+			notifications,
+			[
+				'textDocument/didOpen',
+				'textDocument/didClose',
+			],
+		);
+	});
+});

client-node-tests/src/servers/textSyncServer.ts

@@ -0,0 +1,51 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import {
+	createConnection, Connection, InitializeParams, InitializeResult,
+	TextDocumentSyncKind, RequestType0
+} from 'vscode-languageserver/node';
+
+const connection: Connection = createConnection();
+
+const receivedNotifications: string[] = [];
+
+/**
+ * A custom request to get a list of all text sync notifications that the server
+ * has been sent.
+ */
+namespace GetNotificationsRequest {
+	export const method: 'testing/getNotifications' = 'testing/getNotifications';
+	export const type = new RequestType0<string[], void>(method);
+}
+
+connection.onInitialize((_params: InitializeParams): InitializeResult => {
+	return {
+		capabilities: {
+			textDocumentSync: {
+				openClose: true,
+				change: TextDocumentSyncKind.Incremental
+			}
+		}
+	};
+});
+
+connection.onDidOpenTextDocument(() => {
+	receivedNotifications.push('textDocument/didOpen');
+});
+
+connection.onDidChangeTextDocument(() => {
+	receivedNotifications.push('textDocument/didChange');
+});
+
+connection.onDidCloseTextDocument(() => {
+	receivedNotifications.push('textDocument/didClose');
+});
+
+connection.onRequest(GetNotificationsRequest.type, () => {
+	return receivedNotifications;
+});
+
+connection.listen();

client/src/common/client.ts

@@ -1004,8 +1004,13 @@ export abstract class BaseLanguageClient implements FeatureClient<Middleware, La
 		// Ensure we have a connection before we force the document sync.
 		const connection = await this.$start();
 
-		// Send ony depending open notifications
-		await this._didOpenTextDocumentFeature!.sendPendingOpenNotifications(documentToClose);
+		// Send any depending open notifications
+		const didDropOpenNotification = await this._didOpenTextDocumentFeature!.sendPendingOpenNotifications(documentToClose);
+		if (didDropOpenNotification) {
+			// Don't forward this close notification if we dropped the
+			// corresponding open notification.
+			return;
+		}
 
 		// If any document is synced in full mode make sure we flush any pending
 		// full document syncs.

client/src/common/textSynchronization.ts

@@ -153,15 +153,26 @@ export class DidOpenTextDocumentFeature extends TextDocumentEventFeature<DidOpen
 		}
 	}
 
-	public async sendPendingOpenNotifications(closingDocument?: string): Promise<void> {
+	/**
+	 * Sends any pending open notifications unless they are for the document
+	 * being closed.
+	 *
+	 * @param closingDocument The document being closed.
+	 * @returns Whether a pending open notification was dropped because it was
+	 *          for the closing document.
+	 */
+	public async sendPendingOpenNotifications(closingDocument?: string): Promise<boolean> {
 		const notifications = Array.from(this._pendingOpenNotifications.values());
 		this._pendingOpenNotifications.clear();
+		let didDropOpenNotification = false;
 		for (const notification of notifications) {
 			if (closingDocument !== undefined && notification.uri.toString() === closingDocument) {
+				didDropOpenNotification = true;
 				continue;
 			}
 			await super.callback(notification);
 		}
+		return didDropOpenNotification;
 	}
 
 	protected getTextDocument(data: TextDocument): TextDocument {