Merge pull request #700 from bash-lsp/fix-codeactions

Fix ShellCheck code actions not working on some clients

Author: skovhus

Diff for: docs/development-guide.md

@@ -16,9 +16,9 @@ convenience - it proxies to the `package.json` files in the `vscode-client` and
 This guide presumes you have the following dependencies installed:
 
 - [`yarn`][yarn].
-- [`node`][node] (v6 or newer)
+- [`node`][node] (v14 or newer)
 - `g++`
-- `make`
+- `make` (optional)
 
 ## Initial setup
 
@@ -85,6 +85,20 @@ reload your vscode window to re-launch the server.
 yarn run reinstall-server
 ```
 
+If you for some reason cannot get access to logs through the client,
+then you can hack the `server/util/logger` with:
+
+```typescript
+const fs = require('fs')
+const util = require('util')
+const log_file = fs.createWriteStream(`/tmp/bash-language-server-debug.log`, {
+  flags: 'w',
+})
+
+// inside log function
+log_file.write(`${severity} ${util.format(message)}\n`)
+```
+
 ## Performance
 
 To analyze the performance of the extension or server using the Chrome inspector:

Diff for: server/CHANGELOG.md

@@ -1,5 +1,9 @@
 # Bash Language Server
 
+## 4.6.1
+
+- Fix the ShellCheck code action feature that for some clients did not return any code actions. https://github.com/bash-lsp/bash-language-server/pull/700
+
 ## 4.6.0
 
 - Support parsing `: "${VARIABLE:="default"}"` as a variable definition https://github.com/bash-lsp/bash-language-server/pull/693

Diff for: server/package.json

@@ -3,7 +3,7 @@
   "description": "A language server for Bash",
   "author": "Mads Hartmann",
   "license": "MIT",
-  "version": "4.6.0",
+  "version": "4.6.1",
   "main": "./out/server.js",
   "typings": "./out/server.d.ts",
   "bin": {

Diff for: 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 for: server/src/server.ts

@@ -4,7 +4,6 @@ import { isDeepStrictEqual } from 'node:util'
 
 import * as TurndownService from 'turndown'
 import * as LSP from 'vscode-languageserver/node'
-import { CodeAction } from 'vscode-languageserver/node'
 import { TextDocument } from 'vscode-languageserver-textdocument'
 
 import Analyzer from './analyser'
@@ -13,7 +12,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 +36,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 +386,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 for: 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: [],
     })
   })