Merge pull request #691 from bash-lsp/fix-parser-issue

skovhus · web-flow · commit 0f7f7c59d2b0 · 2023-01-24T22:44:34.000+01:00
Fix onHover and goto definition in case of syntax issues
diff --git a/server/CHANGELOG.md b/server/CHANGELOG.md
@@ -1,9 +1,13 @@
 # Bash Language Server
 
+## 4.5.3
+
+- Fix issue where some features would work as expected in case of a syntax issue https://github.com/bash-lsp/bash-language-server/pull/691
+
 ## 4.5.2
 
-- fixed `onReferences` to respect the `context.includeDeclaration` flag
-- removed unnecessary dependency `urijs`
+- Fixed `onReferences` to respect the `context.includeDeclaration` flag https://github.com/bash-lsp/bash-language-server/pull/688
+- Removed unnecessary dependency `urijs` https://github.com/bash-lsp/bash-language-server/pull/688
 
 ## 4.5.1
 
diff --git a/server/package.json b/server/package.json
@@ -3,7 +3,7 @@
   "description": "A language server for Bash",
   "author": "Mads Hartmann",
   "license": "MIT",
-  "version": "4.5.2",
+  "version": "4.5.3",
   "main": "./out/server.js",
   "typings": "./out/server.d.ts",
   "bin": {
diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts
@@ -18,7 +18,7 @@ let analyzer: Analyzer
 const CURRENT_URI = 'dummy-uri.sh'
 
 // if you add a .sh file to testing/fixtures, update this value
-const FIXTURE_FILES_MATCHING_GLOB = 15
+const FIXTURE_FILES_MATCHING_GLOB = 16
 
 const defaultConfig = getDefaultConfiguration()
 
@@ -46,12 +46,30 @@ describe('analyze', () => {
     expect(loggerWarn).not.toHaveBeenCalled()
   })
 
-  it('parses files with a missing node', () => {
+  it('parses files with a missing nodes and return relevant diagnostics', () => {
     const diagnostics = analyzer.analyze({
       uri: CURRENT_URI,
       document: FIXTURE_DOCUMENT.MISSING_NODE,
     })
-    expect(diagnostics).toEqual([])
+    expect(diagnostics).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "message": "Syntax error: \\"fi\\" missing",
+          "range": Object {
+            "end": Object {
+              "character": 0,
+              "line": 12,
+            },
+            "start": Object {
+              "character": 0,
+              "line": 12,
+            },
+          },
+          "severity": 2,
+          "source": "bash-language-server",
+        },
+      ]
+    `)
     expect(loggerWarn).toHaveBeenCalledWith(
       'Error while parsing dummy-uri.sh: syntax error',
     )
@@ -783,6 +801,7 @@ describe('initiateBackgroundAnalysis', () => {
     expect(loggerWarn).toHaveBeenCalled()
     expect(loggerWarn.mock.calls).toEqual([
       [expect.stringContaining('missing-node.sh: syntax error')],
+      [expect.stringContaining('missing-node2.sh: syntax error')],
       [expect.stringContaining('not-a-shell-script.sh: syntax error')],
       [expect.stringContaining('parse-problems.sh: syntax error')],
     ])
diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts
@@ -955,27 +955,29 @@ describe('server', () => {
   })
 
   describe('onHover', () => {
-    it('responds with documentation for command', async () => {
+    async function getHoverResult(uri: string, position: LSP.Position) {
       const { connection } = await initializeServer()
 
       const onHover = connection.onHover.mock.calls[0][0]
 
-      const result = await onHover(
+      return onHover(
         {
           textDocument: {
-            uri: FIXTURE_URI.INSTALL,
-          },
-          position: {
-            // rm
-            line: 25,
-            character: 5,
+            uri,
           },
+          position,
         },
         {} as any,
         {} as any,
       )
+    }
+    it('responds with documentation for command', async () => {
+      const result = await getHoverResult(FIXTURE_URI.INSTALL, {
+        // rm
+        line: 25,
+        character: 5,
+      })
 
-      expect(result).toBeDefined()
       expect(result).toEqual({
         contents: {
           kind: 'markdown',
@@ -985,25 +987,11 @@ describe('server', () => {
     })
 
     it('responds with function documentation extracted from comments', async () => {
-      const { connection } = await initializeServer()
-
-      const onHover = connection.onHover.mock.calls[0][0]
-
-      const result = await onHover(
-        {
-          textDocument: {
-            uri: FIXTURE_URI.COMMENT_DOC,
-          },
-          position: {
-            line: 17,
-            character: 0,
-          },
-        },
-        {} as any,
-        {} as any,
-      )
+      const result = await getHoverResult(FIXTURE_URI.COMMENT_DOC, {
+        line: 17,
+        character: 0,
+      })
 
-      expect(result).toBeDefined()
       expect(result).toMatchInlineSnapshot(`
               Object {
                 "contents": Object {
@@ -1022,25 +1010,11 @@ describe('server', () => {
     })
 
     it('displays correct documentation for symbols in file that override path executables', async () => {
-      const { connection } = await initializeServer()
-
-      const onHover = connection.onHover.mock.calls[0][0]
-
-      const result = await onHover(
-        {
-          textDocument: {
-            uri: FIXTURE_URI.OVERRIDE_SYMBOL,
-          },
-          position: {
-            line: 9,
-            character: 1,
-          },
-        },
-        {} as any,
-        {} as any,
-      )
+      const result = await getHoverResult(FIXTURE_URI.OVERRIDE_SYMBOL, {
+        line: 9,
+        character: 1,
+      })
 
-      expect(result).toBeDefined()
       expect(result).toMatchInlineSnapshot(`
               Object {
                 "contents": Object {
@@ -1056,30 +1030,38 @@ describe('server', () => {
     })
 
     it('returns executable documentation if the function is not redefined', async () => {
-      const { connection } = await initializeServer()
-
-      const onHover = connection.onHover.mock.calls[0][0]
+      const result1 = await getHoverResult(FIXTURE_URI.OVERRIDE_SYMBOL, {
+        line: 2,
+        character: 1,
+      })
+      expect(result1).toEqual({
+        contents: {
+          kind: 'markdown',
+          value: expect.stringContaining('list directory contents'),
+        },
+      })
 
-      const getHoverResult = (position: LSP.Position) =>
-        onHover(
-          {
-            textDocument: {
-              uri: FIXTURE_URI.OVERRIDE_SYMBOL,
-            },
-            position,
-          },
-          {} as any,
-          {} as any,
-        )
+      // return null same result if the cursor is on the arguments
+      const result2 = await getHoverResult(FIXTURE_URI.OVERRIDE_SYMBOL, {
+        line: 2,
+        character: 3,
+      })
+      expect(result2).toEqual(null)
+    })
 
-      const result1 = await getHoverResult({ line: 2, character: 1 })
-      expect(result1).toBeDefined()
-      expect((result1 as any)?.contents.value).toContain('list directory contents')
+    it('responds with documentation even if parsing fails', async () => {
+      const result = await getHoverResult(FIXTURE_URI.MISSING_NODE2, {
+        // sleep
+        line: 12,
+        character: 4,
+      })
 
-      // return null same result if the cursor is on the arguments
-      const result2 = await getHoverResult({ line: 2, character: 3 })
-      expect(result2).toBeDefined()
-      expect((result2 as any)?.contents.value).toBeUndefined()
+      expect(result).toEqual({
+        contents: {
+          kind: 'markdown',
+          value: expect.stringContaining('sleep'),
+        },
+      })
     })
 
     it.skip('returns documentation from explainshell', async () => {
diff --git a/server/src/analyser.ts b/server/src/analyser.ts
@@ -116,7 +116,11 @@ export default class Analyzer {
       logger.warn(`Error while parsing ${uri}: syntax error`)
     }
 
-    return diagnostics
+    const missingNodesDiagnostics = TreeSitterUtil.getDiagnosticsForMissingNodes(
+      tree.rootNode,
+    )
+
+    return diagnostics.concat(missingNodesDiagnostics)
   }
 
   /**
diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts
@@ -154,20 +154,20 @@ export function getLocalDeclarations({
       node.type === 'program'
         ? node
         : TreeSitterUtil.findParent(node, (p) => p.type === 'program')
-    if (!rootNode) {
-      throw new Error('did not find root node')
-    }
 
-    Object.entries(
-      getAllGlobalVariableDeclarations({
-        rootNode,
-        uri,
-      }),
-    ).map(([name, symbols]) => {
-      if (!declarations[name]) {
-        declarations[name] = symbols
-      }
-    })
+    if (rootNode) {
+      // In case of parsing errors, the root node might not be found
+      Object.entries(
+        getAllGlobalVariableDeclarations({
+          rootNode,
+          uri,
+        }),
+      ).map(([name, symbols]) => {
+        if (!declarations[name]) {
+          declarations[name] = symbols
+        }
+      })
+    }
   }
 
   return declarations
diff --git a/server/src/util/tree-sitter.ts b/server/src/util/tree-sitter.ts
@@ -1,4 +1,4 @@
-import { Range } from 'vscode-languageserver/node'
+import { Diagnostic, DiagnosticSeverity, Range } from 'vscode-languageserver/node'
 import { SyntaxNode } from 'web-tree-sitter'
 
 /**
@@ -56,3 +56,24 @@ export function findParent(
   }
   return null
 }
+
+export function getDiagnosticsForMissingNodes(node: SyntaxNode) {
+  const diagnostics: Diagnostic[] = []
+
+  forEach(node, (node) => {
+    if (node.isMissing()) {
+      diagnostics.push(
+        Diagnostic.create(
+          range(node),
+          `Syntax error: "${node.type}" missing`,
+          DiagnosticSeverity.Warning,
+          undefined,
+          'bash-language-server',
+        ),
+      )
+    }
+    return node.hasError()
+  })
+
+  return diagnostics
+}
diff --git a/testing/fixtures.ts b/testing/fixtures.ts
@@ -23,6 +23,7 @@ export const FIXTURE_URI = {
   MISSING_EXTENSION: `file://${path.join(FIXTURE_FOLDER, 'extension')}`,
   EXTENSION_INC: `file://${path.join(FIXTURE_FOLDER, 'extension.inc')}`,
   MISSING_NODE: `file://${path.join(FIXTURE_FOLDER, 'missing-node.sh')}`,
+  MISSING_NODE2: `file://${path.join(FIXTURE_FOLDER, 'missing-node2.sh')}`,
   OPTIONS: `file://${path.join(FIXTURE_FOLDER, 'options.sh')}`,
   OVERRIDE_SYMBOL: `file://${path.join(FIXTURE_FOLDER, 'override-executable-symbol.sh')}`,
   PARSE_PROBLEMS: `file://${path.join(FIXTURE_FOLDER, 'parse-problems.sh')}`,
diff --git a/testing/fixtures/missing-node2.sh b/testing/fixtures/missing-node2.sh
@@ -0,0 +1,18 @@
+#!/usr/bin/env bash
+
+a=14
+while :; do
+  # doc for function
+  random_op() {
+    # parsing issue: missing node ")", but is not reported as MISSING
+    a=$(((RANDOM % 1000) + 1))
+  }
+  # issue: hovering does not show documentation for function due to parsing error
+  random_op
+
+  sleep 2
+
+  echo $a
+done
+
+3
diff --git a/vscode-client/src/server.ts b/vscode-client/src/server.ts
@@ -20,7 +20,8 @@ connection.listen()
 
 // Don't die on unhandled Promise rejections
 process.on('unhandledRejection', (reason, p) => {
-  connection.console.error(`Unhandled Rejection at promise: ${p}, reason: ${reason}`)
+  const stack = reason instanceof Error ? reason.stack : reason
+  connection.console.error(`Unhandled Rejection at promise: ${p}, reason: ${stack}`)
 })
 
 process.on('SIGPIPE', () => {

Original file line number	Diff line number	Diff line change
`@@ -116,7 +116,11 @@ export default class Analyzer {`
`116`	`116`	logger.warn(`Error while parsing ${uri}: syntax error`)
`117`	`117`	`}`
`118`	`118`
`119`		`- return diagnostics`
	`119`	`+ const missingNodesDiagnostics = TreeSitterUtil.getDiagnosticsForMissingNodes(`
	`120`	`+ tree.rootNode,`
	`121`	`+ )`
	`122`	`+`
	`123`	`+ return diagnostics.concat(missingNodesDiagnostics)`
`120`	`124`	`}`
`121`	`125`
`122`	`126`	`/**`