From b784b3e966f128d875c2a919d49ae8a8e92821c1 Mon Sep 17 00:00:00 2001 From: skovhus Date: Tue, 24 Jan 2023 21:16:50 +0100 Subject: [PATCH 1/6] Add failing fixture --- server/src/__tests__/analyzer.test.ts | 3 ++- testing/fixtures.ts | 1 + testing/fixtures/missing-node2.sh | 18 ++++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 testing/fixtures/missing-node2.sh diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 41f32669d..bc238b436 100644 --- 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() @@ -783,6 +783,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/testing/fixtures.ts b/testing/fixtures.ts index 468505be3..e022d2274 100644 --- 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 new file mode 100644 index 000000000..09d4eb671 --- /dev/null +++ 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 From 5e2cd3cda2a5820c1e4d1c316733053056ae10ac Mon Sep 17 00:00:00 2001 From: skovhus Date: Tue, 24 Jan 2023 21:17:12 +0100 Subject: [PATCH 2/6] Add stack trace to unhandled rejection --- vscode-client/src/server.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vscode-client/src/server.ts b/vscode-client/src/server.ts index 087d6909a..e0be210a4 100644 --- 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', () => { From f874aeeebdb4d51b31231086bba22670e3f77836 Mon Sep 17 00:00:00 2001 From: skovhus Date: Tue, 24 Jan 2023 21:22:24 +0100 Subject: [PATCH 3/6] Dry hover test cases --- server/src/__tests__/server.test.ts | 98 ++++++++++------------------- 1 file changed, 33 insertions(+), 65 deletions(-) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 79241917b..56c55e194 100644 --- 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,28 +1030,22 @@ 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 getHoverResult = (position: LSP.Position) => - onHover( - { - textDocument: { - uri: FIXTURE_URI.OVERRIDE_SYMBOL, - }, - position, - }, - {} as any, - {} as any, - ) - - const result1 = await getHoverResult({ line: 2, character: 1 }) - expect(result1).toBeDefined() - expect((result1 as any)?.contents.value).toContain('list directory contents') + const result1 = await getHoverResult(FIXTURE_URI.OVERRIDE_SYMBOL, { + line: 2, + character: 1, + }) + expect(result1).toEqual({ + contents: { + kind: 'markdown', + value: expect.stringContaining('list directory contents'), + }, + }) // return null same result if the cursor is on the arguments - const result2 = await getHoverResult({ line: 2, character: 3 }) + const result2 = await getHoverResult(FIXTURE_URI.OVERRIDE_SYMBOL, { + line: 2, + character: 3, + }) expect(result2).toBeDefined() expect((result2 as any)?.contents.value).toBeUndefined() }) From 02849c8e23178831a5f261dc656aded43af30f54 Mon Sep 17 00:00:00 2001 From: skovhus Date: Tue, 24 Jan 2023 22:26:54 +0100 Subject: [PATCH 4/6] Make hover work even if parsing fails --- server/src/__tests__/server.test.ts | 18 ++++++++++++++++-- server/src/util/declarations.ts | 26 +++++++++++++------------- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 56c55e194..1521c6d11 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -1046,8 +1046,22 @@ describe('server', () => { line: 2, character: 3, }) - expect(result2).toBeDefined() - expect((result2 as any)?.contents.value).toBeUndefined() + expect(result2).toEqual(null) + }) + + it('responds with documentation even if parsing fails', async () => { + const result = await getHoverResult(FIXTURE_URI.MISSING_NODE2, { + // sleep + line: 12, + character: 4, + }) + + expect(result).toEqual({ + contents: { + kind: 'markdown', + value: expect.stringContaining('suspends execution'), + }, + }) }) it.skip('returns documentation from explainshell', async () => { diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts index c1ca9121a..1554c7cee 100644 --- 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 From bb765fbe3f19e45caabf8efd6bc7b33f78fbe445 Mon Sep 17 00:00:00 2001 From: skovhus Date: Tue, 24 Jan 2023 22:28:04 +0100 Subject: [PATCH 5/6] Resurrect diagnostics for missing nodes This seems useful, even though tree sitter usually do not report these in more complicated docs. --- server/src/__tests__/analyzer.test.ts | 22 ++++++++++++++++++++-- server/src/__tests__/server.test.ts | 2 +- server/src/analyser.ts | 6 +++++- server/src/util/tree-sitter.ts | 23 ++++++++++++++++++++++- 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index bc238b436..82894ca7f 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -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', ) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 1521c6d11..c9159e4b2 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -1059,7 +1059,7 @@ describe('server', () => { expect(result).toEqual({ contents: { kind: 'markdown', - value: expect.stringContaining('suspends execution'), + value: expect.stringContaining('sleep'), }, }) }) diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 90c524f96..de74029c0 100644 --- 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/tree-sitter.ts b/server/src/util/tree-sitter.ts index f6413573f..d4b44aa75 100644 --- 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 +} From 083de281034dd195a1600fc296a6e83c7372c2b3 Mon Sep 17 00:00:00 2001 From: skovhus Date: Tue, 24 Jan 2023 22:38:30 +0100 Subject: [PATCH 6/6] Release server version 4.5.3 --- server/CHANGELOG.md | 8 ++++++-- server/package.json | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/server/CHANGELOG.md b/server/CHANGELOG.md index 55a89195b..e4e665d74 100644 --- 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 index 5b03bf963..6892dbae5 100644 --- 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": {