Simplify code action fingerprinting

skovhus · skovhus · commit 5990d5ff5d71 · 2023-01-27T10:22:54.000+01:00
This solves an issue with some clients returning diagnostics with
properties sorted differently than on node... Embarassing and silly
bug.
diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts
@@ -177,29 +177,30 @@ describe('server', () => {
       const fixableDiagnostic = diagnostics.filter(({ code }) => code === 'SC2086')[0]
 
       expect(fixableDiagnostic).toMatchInlineSnapshot(`
-              Object {
-                "code": "SC2086",
-                "codeDescription": Object {
-                  "href": "https://www.shellcheck.net/wiki/SC2086",
-                },
-                "message": "Double quote to prevent globbing and word splitting.",
-                "range": Object {
-                  "end": Object {
-                    "character": 13,
-                    "line": 55,
-                  },
-                  "start": Object {
-                    "character": 5,
-                    "line": 55,
-                  },
-                },
-                "severity": 3,
-                "source": "shellcheck",
-                "tags": undefined,
-              }
-          `)
-
-      // TODO: we could find the diagnostics and then use the range to test the code action
+        Object {
+          "code": "SC2086",
+          "codeDescription": Object {
+            "href": "https://www.shellcheck.net/wiki/SC2086",
+          },
+          "data": Object {
+            "id": "shellcheck|2086|55:5-55:13",
+          },
+          "message": "Double quote to prevent globbing and word splitting.",
+          "range": Object {
+            "end": Object {
+              "character": 13,
+              "line": 55,
+            },
+            "start": Object {
+              "character": 5,
+              "line": 55,
+            },
+          },
+          "severity": 3,
+          "source": "shellcheck",
+          "tags": undefined,
+        }
+      `)
 
       const onCodeAction = connection.onCodeAction.mock.calls[0][0]
 
diff --git a/server/src/server.ts b/server/src/server.ts
@@ -13,7 +13,7 @@ import * as config from './config'
 import Executables from './executables'
 import { initializeParser } from './parser'
 import * as ReservedWords from './reserved-words'
-import { Linter } from './shellcheck'
+import { Linter, LintingResult } from './shellcheck'
 import { SNIPPETS } from './snippets'
 import { BashCompletionItem, CompletionItemDataType } from './types'
 import { uniqueBasedOnHash } from './util/array'
@@ -37,7 +37,9 @@ export default class BashServer {
   private executables: Executables
   private linter?: Linter
   private workspaceFolder: string | null
-  private uriToCodeActions: { [uri: string]: CodeAction[] | undefined } = {}
+  private uriToCodeActions: {
+    [uri: string]: LintingResult['codeActions'] | undefined
+  } = {}
 
   private constructor({
     analyzer,
@@ -385,41 +387,15 @@ export default class BashServer {
   // ==============================
 
   private async onCodeAction(params: LSP.CodeActionParams): Promise<LSP.CodeAction[]> {
-    const codeActions = this.uriToCodeActions[params.textDocument.uri]
+    const codeActionsForUri = this.uriToCodeActions[params.textDocument.uri] || {}
 
-    if (!codeActions) {
-      return []
-    }
-
-    const getDiagnosticsFingerPrint = (diagnostics?: LSP.Diagnostic[]): string[] =>
-      diagnostics
-        ?.map(({ code, source, range }) =>
-          code !== undefined && source && range
-            ? JSON.stringify({
-                code,
-                source,
-                range,
-              })
-            : null,
-        )
-        .filter((fingerPrint): fingerPrint is string => fingerPrint != null) || []
+    const codeActions = params.context.diagnostics
+      .map(({ data }) => codeActionsForUri[data?.id])
+      .filter((action): action is LSP.CodeAction => action != null)
 
-    const paramsDiagnosticsKeys = getDiagnosticsFingerPrint(params.context.diagnostics)
-
-    // find actions that match the paramsDiagnosticsKeys
-    const actions = codeActions.filter((action) => {
-      const actionDiagnosticsKeys = getDiagnosticsFingerPrint(action.diagnostics)
-      // actions without diagnostics are always returned
-      if (actionDiagnosticsKeys.length === 0) {
-        return true
-      }
-
-      return actionDiagnosticsKeys.some((actionDiagnosticKey) =>
-        paramsDiagnosticsKeys.includes(actionDiagnosticKey),
-      )
-    })
+    logger.debug(`onCodeAction: found ${codeActions.length} code action(s)`)
 
-    return actions
+    return codeActions
   }
 
   private onCompletion(params: LSP.TextDocumentPositionParams): BashCompletionItem[] {
diff --git a/server/src/shellcheck/__tests__/index.test.ts b/server/src/shellcheck/__tests__/index.test.ts
@@ -54,7 +54,7 @@ describe('linter', () => {
     })
 
     expect(result).toEqual({
-      codeActions: [],
+      codeActions: {},
       diagnostics: [],
     })
 
@@ -76,14 +76,17 @@ describe('linter', () => {
     const [result] = await getLintingResult({ document: textToDoc(shell) })
     expect(result).toMatchInlineSnapshot(`
       Object {
-        "codeActions": Array [
-          Object {
+        "codeActions": Object {
+          "shellcheck|2086|1:5-1:9": Object {
             "diagnostics": Array [
               Object {
                 "code": "SC2086",
                 "codeDescription": Object {
                   "href": "https://www.shellcheck.net/wiki/SC2086",
                 },
+                "data": Object {
+                  "id": "shellcheck|2086|1:5-1:9",
+                },
                 "message": "Double quote to prevent globbing and word splitting.",
                 "range": Object {
                   "end": Object {
@@ -135,13 +138,16 @@ describe('linter', () => {
             "kind": "quickfix",
             "title": "Apply fix for SC2086",
           },
-        ],
+        },
         "diagnostics": Array [
           Object {
             "code": "SC2154",
             "codeDescription": Object {
               "href": "https://www.shellcheck.net/wiki/SC2154",
             },
+            "data": Object {
+              "id": "shellcheck|2154|1:5-1:9",
+            },
             "message": "foo is referenced but not assigned.",
             "range": Object {
               "end": Object {
@@ -162,6 +168,9 @@ describe('linter', () => {
             "codeDescription": Object {
               "href": "https://www.shellcheck.net/wiki/SC2086",
             },
+            "data": Object {
+              "id": "shellcheck|2086|1:5-1:9",
+            },
             "message": "Double quote to prevent globbing and word splitting.",
             "range": Object {
               "end": Object {
@@ -197,7 +206,7 @@ describe('linter', () => {
 
     const result = await promises[promises.length - 1]
     expect(result).toEqual({
-      codeActions: [],
+      codeActions: {},
       diagnostics: [],
     })
   })
@@ -209,7 +218,7 @@ describe('linter', () => {
     })
 
     expect(result).toEqual({
-      codeActions: [],
+      codeActions: {},
       diagnostics: [],
     })
   })
@@ -222,13 +231,16 @@ describe('linter', () => {
 
     expect(result).toMatchInlineSnapshot(`
       Object {
-        "codeActions": Array [],
+        "codeActions": Object {},
         "diagnostics": Array [
           Object {
             "code": "SC1091",
             "codeDescription": Object {
               "href": "https://www.shellcheck.net/wiki/SC1091",
             },
+            "data": Object {
+              "id": "shellcheck|1091|3:7-3:19",
+            },
             "message": "Not following: shellcheck/sourced.sh: openBinaryFile: does not exist (No such file or directory)",
             "range": Object {
               "end": Object {
@@ -249,6 +261,9 @@ describe('linter', () => {
             "codeDescription": Object {
               "href": "https://www.shellcheck.net/wiki/SC2154",
             },
+            "data": Object {
+              "id": "shellcheck|2154|5:6-5:10",
+            },
             "message": "foo is referenced but not assigned.",
             "range": Object {
               "end": Object {
@@ -276,7 +291,7 @@ describe('linter', () => {
       sourcePaths: [path.resolve(FIXTURE_FOLDER)],
     })
     expect(result).toEqual({
-      codeActions: [],
+      codeActions: {},
       diagnostics: [],
     })
   })
diff --git a/server/src/shellcheck/index.ts b/server/src/shellcheck/index.ts
@@ -23,9 +23,9 @@ type LinterOptions = {
   cwd?: string
 }
 
-type LintingResult = {
+export type LintingResult = {
   diagnostics: LSP.Diagnostic[]
-  codeActions: LSP.CodeAction[]
+  codeActions: Record<string, LSP.CodeAction | undefined>
 }
 
 export class Linter {
@@ -53,7 +53,7 @@ export class Linter {
     additionalShellCheckArguments: string[] = [],
   ): Promise<LintingResult> {
     if (!this._canLint) {
-      return { diagnostics: [], codeActions: [] }
+      return { diagnostics: [], codeActions: {} }
     }
 
     const { uri } = document
@@ -80,7 +80,7 @@ export class Linter {
 
     if (shellDialect && !SUPPORTED_BASH_DIALECTS.includes(shellDialect)) {
       // We found a dialect that isn't supported by ShellCheck.
-      return { diagnostics: [], codeActions: [] }
+      return { diagnostics: [], codeActions: {} }
     }
 
     // NOTE: that ShellCheck actually does shebang parsing, but we manually
@@ -99,7 +99,7 @@ export class Linter {
     )
 
     if (!this._canLint) {
-      return { diagnostics: [], codeActions: [] }
+      return { diagnostics: [], codeActions: {} }
     }
 
     // Clean up the debounced function
@@ -180,40 +180,37 @@ export class Linter {
   }
 }
 
-function mapShellCheckResult({
-  uri,
-  result,
-}: {
-  uri: string
-  result: ShellCheckResult
-}): {
-  diagnostics: LSP.Diagnostic[]
-  codeActions: LSP.CodeAction[]
-} {
-  const diagnostics: LSP.Diagnostic[] = []
-  const codeActions: LSP.CodeAction[] = []
+function mapShellCheckResult({ uri, result }: { uri: string; result: ShellCheckResult }) {
+  const diagnostics: LintingResult['diagnostics'] = []
+  const codeActions: LintingResult['codeActions'] = {}
 
   for (const comment of result.comments) {
-    const start: LSP.Position = {
-      line: comment.line - 1,
-      character: comment.column - 1,
-    }
-    const end: LSP.Position = {
-      line: comment.endLine - 1,
-      character: comment.endColumn - 1,
-    }
+    const range = LSP.Range.create(
+      {
+        line: comment.line - 1,
+        character: comment.column - 1,
+      },
+      {
+        line: comment.endLine - 1,
+        character: comment.endColumn - 1,
+      },
+    )
+
+    const id = `shellcheck|${comment.code}|${range.start.line}:${range.start.character}-${range.end.line}:${range.end.character}`
 
     const diagnostic: LSP.Diagnostic = {
       message: comment.message,
       severity: LEVEL_TO_SEVERITY[comment.level] || LSP.DiagnosticSeverity.Error,
       code: `SC${comment.code}`,
       source: 'shellcheck',
-      range: LSP.Range.create(start, end),
+      range,
       codeDescription: {
         href: `https://www.shellcheck.net/wiki/SC${comment.code}`,
       },
       tags: CODE_TO_TAGS[comment.code],
-      // NOTE: we could use the 'data' property this enable easier fingerprinting
+      data: {
+        id,
+      },
     }
 
     diagnostics.push(diagnostic)
@@ -225,7 +222,7 @@ function mapShellCheckResult({
     })
 
     if (codeAction) {
-      codeActions.push(codeAction)
+      codeActions[id] = codeAction
     }
   }
 
