Improve error reporting when SourceKit-LSP fails to start (#982)

* Improve error reporting when SourceKit-LSP fails to start

If SourceKit-LSP fails to initialize, typically because of unrecognized
launch arguments, don't attempt to restart it and limit the number of
erorr dialogs to just one descriptive one. Clicking the "Go to Output"
button on this dialog takes you to the SourceKit-LSP Output panel which
now shows the SourceKit-LSP launch error.

Issue: #976

* Address comments

Author: plemarquand

Diff for: src/sourcekit-lsp/LanguageClientManager.ts

@@ -434,11 +434,14 @@ export class LanguageClientManager {
             this.languageClient = undefined;
             return;
         }
-        const client = this.createLSPClient(folder);
-        return this.startClient(client);
+        const { client, errorHandler } = this.createLSPClient(folder);
+        return this.startClient(client, errorHandler);
     }
 
-    private createLSPClient(folder?: vscode.Uri): langclient.LanguageClient {
+    private createLSPClient(folder?: vscode.Uri): {
+        client: langclient.LanguageClient;
+        errorHandler: SourceKitLSPErrorHandler;
+    } {
         const toolchainSourceKitLSP =
             this.workspaceContext.toolchain.getToolchainExecutable("sourcekit-lsp");
         const lspConfig = configuration.lsp;
@@ -488,6 +491,7 @@ export class LanguageClientManager {
             workspaceFolder = { uri: folder, name: FolderContext.uriName(folder), index: 0 };
         }
 
+        const errorHandler = new SourceKitLSPErrorHandler(5);
         const clientOptions: langclient.LanguageClientOptions = {
             documentSelector: LanguageClientManager.documentSelector,
             revealOutputChannelOn: langclient.RevealOutputChannelOn.Never,
@@ -563,7 +567,10 @@ export class LanguageClientManager {
                     };
                 })(),
             },
-            errorHandler: new SourceKitLSPErrorHandler(5),
+            errorHandler,
+            // Avoid attempting to reinitialize multiple times. If we fail to initialize
+            // we aren't doing anything different the second time and so will fail again.
+            initializationFailedHandler: () => false,
             initializationOptions: {
                 "workspace/peekDocuments": true, // workaround for client capability to handle `PeekDocumentsRequest`
                 "textDocument/codeLens": {
@@ -575,15 +582,21 @@ export class LanguageClientManager {
             },
         };
 
-        return new langclient.LanguageClient(
-            "swift.sourcekit-lsp",
-            "SourceKit Language Server",
-            serverOptions,
-            clientOptions
-        );
+        return {
+            client: new langclient.LanguageClient(
+                "swift.sourcekit-lsp",
+                "SourceKit Language Server",
+                serverOptions,
+                clientOptions
+            ),
+            errorHandler,
+        };
     }
 
-    private async startClient(client: langclient.LanguageClient) {
+    private async startClient(
+        client: langclient.LanguageClient,
+        errorHandler: SourceKitLSPErrorHandler
+    ) {
         client.onDidChangeState(e => {
             // if state is now running add in any sub-folder workspaces that
             // we have cached. If this is the first time we are starting then
@@ -595,7 +608,6 @@ export class LanguageClientManager {
             ) {
                 this.addSubFolderWorkspaces(client);
             }
-            //console.log(e);
         });
         if (client.clientOptions.workspaceFolder) {
             this.workspaceContext.outputChannel.log(
@@ -615,6 +627,10 @@ export class LanguageClientManager {
         this.clientReadyPromise = client
             .start()
             .then(() => {
+                // Now that we've started up correctly, start the error handler to auto-restart
+                // if sourcekit-lsp crashes during normal operation.
+                errorHandler.enable();
+
                 if (this.workspaceContext.swiftVersion.isLessThan(new Version(5, 7, 0))) {
                     this.legacyInlayHints = activateLegacyInlayHints(client);
                 }
@@ -623,7 +639,6 @@ export class LanguageClientManager {
             })
             .catch(reason => {
                 this.workspaceContext.outputChannel.log(`${reason}`);
-                // if language client failed to initialise then shutdown and set to undefined
                 this.languageClient?.stop();
                 this.languageClient = undefined;
                 throw reason;
@@ -671,10 +686,17 @@ export class LanguageClientManager {
  */
 class SourceKitLSPErrorHandler implements langclient.ErrorHandler {
     private restarts: number[];
+    private enabled: boolean = false;
 
     constructor(private maxRestartCount: number) {
         this.restarts = [];
     }
+    /**
+     * Start listening for errors and requesting to restart the LSP server when appropriate.
+     */
+    enable() {
+        this.enabled = true;
+    }
     /**
      * An error has occurred while writing or reading from the connection.
      *
@@ -697,6 +719,13 @@ class SourceKitLSPErrorHandler implements langclient.ErrorHandler {
      * The connection to the server got closed.
      */
     closed(): langclient.CloseHandlerResult | Promise<langclient.CloseHandlerResult> {
+        if (!this.enabled) {
+            return {
+                action: langclient.CloseAction.DoNotRestart,
+                handled: true,
+            };
+        }
+
         this.restarts.push(Date.now());
         if (this.restarts.length <= this.maxRestartCount) {
             return { action: langclient.CloseAction.Restart };

Diff for: src/ui/SwiftOutputChannel.ts

@@ -115,6 +115,7 @@ class RollingLog {
     private startIndex: number = 0;
     private endIndex: number = 0;
     private logCount: number = 0;
+    private appending: boolean = false;
 
     constructor(private maxLogs: number) {
         this._logs = new Array(maxLogs).fill(null);
@@ -133,23 +134,28 @@ class RollingLog {
     }
 
     appendLine(log: string) {
+        // Writing to a new line that isn't the very first, increment the end index
+        if (this.logCount > 0) {
+            this.endIndex = this.incrementIndex(this.endIndex);
+        }
+
+        // We're over the window size, move the start index
         if (this.logCount === this.maxLogs) {
             this.startIndex = this.incrementIndex(this.startIndex);
         } else {
             this.logCount++;
         }
 
         this._logs[this.endIndex] = log;
-        this.endIndex = this.incrementIndex(this.endIndex);
     }
 
     append(log: string) {
-        const endIndex = this.endIndex - 1 < 0 ? Math.max(this.logCount - 1, 0) : this.endIndex;
-        if (this._logs[endIndex] === null) {
+        if (this.logCount === 0) {
             this.logCount = 1;
         }
-        const newLogLine = (this._logs[endIndex] ?? "") + log;
-        this._logs[endIndex] = newLogLine;
+        const newLogLine = (this._logs[this.endIndex] ?? "") + log;
+        this._logs[this.endIndex] = newLogLine;
+        this.appending = true;
     }
 
     replace(log: string) {

Diff for: test/suite/ui/SwiftOutputChannel.test.ts

@@ -56,6 +56,16 @@ suite("SwiftOutputChannel", function () {
         assert.deepEqual(channel.logs, ["b", "cd", "e"]);
     });
 
+    test("Appends after rolling over", () => {
+        channel.appendLine("a");
+        channel.appendLine("b");
+        channel.appendLine("c");
+        channel.appendLine("d");
+        channel.append("e");
+        channel.appendLine("f");
+        assert.deepEqual(channel.logs, ["c", "de", "f"]);
+    });
+
     test("Replaces", () => {
         channel.appendLine("a");
         channel.appendLine("b");
@@ -64,4 +74,11 @@ suite("SwiftOutputChannel", function () {
         channel.replace("e");
         assert.deepEqual(channel.logs, ["e"]);
     });
+
+    test("AppendLine after append terminates appending line", () => {
+        channel.append("a");
+        channel.append("b");
+        channel.appendLine("c");
+        assert.deepEqual(channel.logs, ["ab", "c"]);
+    });
 });