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": { diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 41f32669d..82894ca7f 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() @@ -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 index 79241917b..c9159e4b2 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,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 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/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 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 +} 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 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', () => {