From 0b765f7ed7dcd911dd4b45fe0b8187888d36b7ee Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 18 Jan 2023 09:50:29 +0100 Subject: [PATCH 1/5] Update fixtures with additional cases --- server/src/__tests__/analyzer.test.ts | 28 +++++++++++++-------------- testing/fixtures.ts | 1 + testing/fixtures/extension.inc | 15 ++++++++++---- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index b0348af86..196999313 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -454,11 +454,11 @@ describe('findDeclarationsMatchingWord', () => { "location": Object { "range": Object { "end": Object { - "character": 19, + "character": 25, "line": 6, }, "start": Object { - "character": 0, + "character": 6, "line": 6, }, }, @@ -483,11 +483,11 @@ describe('findDeclarationsMatchingWord', () => { "location": Object { "range": Object { "end": Object { - "character": 19, + "character": 25, "line": 6, }, "start": Object { - "character": 0, + "character": 6, "line": 6, }, }, @@ -535,11 +535,11 @@ describe('findDeclarationsMatchingWord', () => { "location": Object { "range": Object { "end": Object { - "character": 19, + "character": 25, "line": 6, }, "start": Object { - "character": 0, + "character": 6, "line": 6, }, }, @@ -901,11 +901,11 @@ describe('getAllVariables', () => { "location": Object { "range": Object { "end": Object { - "character": 19, + "character": 25, "line": 6, }, "start": Object { - "character": 0, + "character": 6, "line": 6, }, }, @@ -918,11 +918,11 @@ describe('getAllVariables', () => { "location": Object { "range": Object { "end": Object { - "character": 16, + "character": 23, "line": 7, }, "start": Object { - "character": 0, + "character": 7, "line": 7, }, }, @@ -935,12 +935,12 @@ describe('getAllVariables', () => { "location": Object { "range": Object { "end": Object { - "character": 17, - "line": 8, + "character": 19, + "line": 10, }, "start": Object { - "character": 0, - "line": 8, + "character": 2, + "line": 10, }, }, "uri": "file://${FIXTURE_FOLDER}extension.inc", diff --git a/testing/fixtures.ts b/testing/fixtures.ts index 250645fb6..5b9a0e84b 100644 --- a/testing/fixtures.ts +++ b/testing/fixtures.ts @@ -21,6 +21,7 @@ export const FIXTURE_URI = { ISSUE101: `file://${path.join(FIXTURE_FOLDER, 'issue101.sh')}`, ISSUE206: `file://${path.join(FIXTURE_FOLDER, 'issue206.sh')}`, 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')}`, OPTIONS: `file://${path.join(FIXTURE_FOLDER, 'options.sh')}`, OVERRIDE_SYMBOL: `file://${path.join(FIXTURE_FOLDER, 'override-executable-symbol.sh')}`, diff --git a/testing/fixtures/extension.inc b/testing/fixtures/extension.inc index b7869e5a9..8b519df4f 100644 --- a/testing/fixtures/extension.inc +++ b/testing/fixtures/extension.inc @@ -4,9 +4,12 @@ source ./issue101.sh RED=`tput setaf 1` GREEN=`tput setaf 2` -BLUE=`tput setaf 4` -BOLD=`tput bold` -RESET=`tput sgr0` +local BLUE=`tput setaf 4` # local at the root is still exposed to the global scope +export BOLD=`tput bold` + +{ # non colors in a group + RESET=`tput sgr0` +} extensionFunc() { LOCAL_VARIABLE='local' @@ -14,4 +17,8 @@ extensionFunc() { innerExtensionFunc() { echo $LOCAL_VARIABLE } -} \ No newline at end of file +} + +if [ "${ENV}" = "prod" ]; then + RED="" # Ignored as we do not flowtrace. +fi \ No newline at end of file From 1732deb401263d9640d22cb0b6706106ab791834 Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 18 Jan 2023 09:51:46 +0100 Subject: [PATCH 2/5] Traverse additional nodes for global definitions --- server/src/util/declarations.ts | 15 ++++++++++++--- server/src/util/tree-sitter.ts | 14 ++++++++++---- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts index fef7dd17c..c1ca9121a 100644 --- a/server/src/util/declarations.ts +++ b/server/src/util/declarations.ts @@ -13,11 +13,16 @@ const TREE_SITTER_TYPE_TO_LSP_KIND: { [type: string]: LSP.SymbolKind | undefined export type GlobalDeclarations = { [word: string]: LSP.SymbolInformation } export type Declarations = { [word: string]: LSP.SymbolInformation[] } +const GLOBAL_DECLARATION_LEAF_NODE_TYPES = new Set([ + 'if_statement', + 'function_definition', +]) + /** * Returns declarations (functions or variables) from a given root node * that would be available after sourcing the file. This currently does - * not include global variables defined inside function as we do not do - * any flow tracing. + * not include global variables defined inside if statements or functions + * as we do not do any flow tracing. * * Will only return one declaration per symbol name – the latest definition. * This behavior is consistent with how Bash behaves, but differs between @@ -35,7 +40,9 @@ export function getGlobalDeclarations({ }): GlobalDeclarations { const globalDeclarations: GlobalDeclarations = {} - tree.rootNode.children.forEach((node) => { + TreeSitterUtil.forEach(tree.rootNode, (node) => { + const followChildren = !GLOBAL_DECLARATION_LEAF_NODE_TYPES.has(node.type) + if (TreeSitterUtil.isDefinition(node)) { const symbol = nodeToSymbolInformation({ node, uri }) if (symbol) { @@ -43,6 +50,8 @@ export function getGlobalDeclarations({ globalDeclarations[word] = symbol } } + + return followChildren }) return globalDeclarations diff --git a/server/src/util/tree-sitter.ts b/server/src/util/tree-sitter.ts index 99b5b7466..fbd130156 100644 --- a/server/src/util/tree-sitter.ts +++ b/server/src/util/tree-sitter.ts @@ -1,10 +1,16 @@ import { Range } from 'vscode-languageserver/node' import { SyntaxNode } from 'web-tree-sitter' -export function forEach(node: SyntaxNode, cb: (n: SyntaxNode) => void) { - cb(node) - if (node.children.length) { - node.children.forEach((n) => forEach(n, cb)) +/** + * Recursively iterate over all nodes in a tree. + * + * @param node The node to start iterating from + * @param callback The callback to call for each node. Return false to stop following children. + */ +export function forEach(node: SyntaxNode, callback: (n: SyntaxNode) => void | boolean) { + const followChildren = callback(node) !== false + if (followChildren && node.children.length) { + node.children.forEach((n) => forEach(n, callback)) } } From 681e17851516582aab4d2ce05b93887d0875f475 Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 18 Jan 2023 09:57:26 +0100 Subject: [PATCH 3/5] Get rid of manual steps when updating snapshots --- server/src/__tests__/analyzer.test.ts | 129 ++++++++++++++------------ testing/fixtures.ts | 15 +++ 2 files changed, 87 insertions(+), 57 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 196999313..41f32669d 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -5,6 +5,7 @@ import { FIXTURE_FOLDER, FIXTURE_URI, REPO_ROOT_FOLDER, + updateSnapshotUris, } from '../../../testing/fixtures' import Analyzer from '../analyser' import { getDefaultConfiguration } from '../config' @@ -122,7 +123,7 @@ describe('findDeclarationLocations', () => { word: './extension.inc', position: { character: 10, line: 2 }, }) - expect(result).toMatchInlineSnapshot(` + expect(updateSnapshotUris(result)).toMatchInlineSnapshot(` Array [ Object { "range": Object { @@ -135,7 +136,7 @@ describe('findDeclarationLocations', () => { "line": 0, }, }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/extension.inc", }, ] `) @@ -157,7 +158,7 @@ describe('findDeclarationLocations', () => { word: './scripts/tag-release.inc', position: { character: 10, line: 16 }, }) - expect(result).toMatchInlineSnapshot(` + expect(updateSnapshotUris(result)).toMatchInlineSnapshot(` Array [ Object { "range": Object { @@ -170,7 +171,7 @@ describe('findDeclarationLocations', () => { "line": 0, }, }, - "uri": "file://${REPO_ROOT_FOLDER}/scripts/tag-release.inc", + "uri": "file://__REPO_ROOT_FOLDER__/scripts/tag-release.inc", }, ] `) @@ -183,7 +184,7 @@ describe('findDeclarationLocations', () => { uri: CURRENT_URI, word: 'node_version', }) - expect(result).toMatchInlineSnapshot(` + expect(updateSnapshotUris(result)).toMatchInlineSnapshot(` Array [ Object { "range": Object { @@ -208,7 +209,7 @@ describe('findDeclarationLocations', () => { uri: FIXTURE_URI.SCOPE, word: 'X', }) - expect(result).toMatchInlineSnapshot(` + expect(updateSnapshotUris(result)).toMatchInlineSnapshot(` Array [ Object { "range": Object { @@ -221,7 +222,7 @@ describe('findDeclarationLocations', () => { "line": 12, }, }, - "uri": "file://${FIXTURE_FOLDER}scope.sh", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/scope.sh", }, ] `) @@ -233,7 +234,7 @@ describe('findDeclarationLocations', () => { uri: FIXTURE_URI.SCOPE, word: 'i', }) - expect(result).toMatchInlineSnapshot(` + expect(updateSnapshotUris(result)).toMatchInlineSnapshot(` Array [ Object { "range": Object { @@ -246,7 +247,7 @@ describe('findDeclarationLocations', () => { "line": 37, }, }, - "uri": "file://${FIXTURE_FOLDER}scope.sh", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/scope.sh", }, ] `) @@ -403,12 +404,14 @@ describe('findDeclarationsMatchingWord', () => { }) expect( - analyzer.findDeclarationsMatchingWord({ - word: 'npm_config_logl', - uri: FIXTURE_URI.INSTALL, - exactMatch: false, - position: { line: 1000, character: 0 }, - }), + updateSnapshotUris( + analyzer.findDeclarationsMatchingWord({ + word: 'npm_config_logl', + uri: FIXTURE_URI.INSTALL, + exactMatch: false, + position: { line: 1000, character: 0 }, + }), + ), ).toMatchInlineSnapshot(` Array [ Object { @@ -424,7 +427,7 @@ describe('findDeclarationsMatchingWord', () => { "line": 40, }, }, - "uri": "file://${FIXTURE_FOLDER}install.sh", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/install.sh", }, "name": "npm_config_loglevel", }, @@ -441,12 +444,14 @@ describe('findDeclarationsMatchingWord', () => { ).toMatchInlineSnapshot(`Array []`) expect( - analyzer.findDeclarationsMatchingWord({ - word: 'BLU', - uri: FIXTURE_URI.INSTALL, - exactMatch: false, - position: { line: 6, character: 9 }, - }), + updateSnapshotUris( + analyzer.findDeclarationsMatchingWord({ + word: 'BLU', + uri: FIXTURE_URI.INSTALL, + exactMatch: false, + position: { line: 6, character: 9 }, + }), + ), ).toMatchInlineSnapshot(` Array [ Object { @@ -462,7 +467,7 @@ describe('findDeclarationsMatchingWord', () => { "line": 6, }, }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/extension.inc", }, "name": "BLUE", }, @@ -470,12 +475,14 @@ describe('findDeclarationsMatchingWord', () => { `) expect( - analyzer.findDeclarationsMatchingWord({ - word: 'BLU', - uri: FIXTURE_URI.SOURCING, - exactMatch: false, - position: { line: 6, character: 9 }, - }), + updateSnapshotUris( + analyzer.findDeclarationsMatchingWord({ + word: 'BLU', + uri: FIXTURE_URI.SOURCING, + exactMatch: false, + position: { line: 6, character: 9 }, + }), + ), ).toMatchInlineSnapshot(` Array [ Object { @@ -491,7 +498,7 @@ describe('findDeclarationsMatchingWord', () => { "line": 6, }, }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/extension.inc", }, "name": "BLUE", }, @@ -522,12 +529,14 @@ describe('findDeclarationsMatchingWord', () => { ).toMatchInlineSnapshot(`Array []`) expect( - analyzer.findDeclarationsMatchingWord({ - word: 'BLU', - uri: FIXTURE_URI.SOURCING, - exactMatch: false, - position: { line: 6, character: 9 }, - }), + updateSnapshotUris( + analyzer.findDeclarationsMatchingWord({ + word: 'BLU', + uri: FIXTURE_URI.SOURCING, + exactMatch: false, + position: { line: 6, character: 9 }, + }), + ), ).toMatchInlineSnapshot(` Array [ Object { @@ -543,7 +552,7 @@ describe('findDeclarationsMatchingWord', () => { "line": 6, }, }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/extension.inc", }, "name": "BLUE", }, @@ -573,7 +582,7 @@ describe('findDeclarationsMatchingWord', () => { expect(findWordFromLine('f', 0)).toEqual([]) // First definition - expect(findWordFromLine('X', 3)).toMatchInlineSnapshot(` + expect(updateSnapshotUris(findWordFromLine('X', 3))).toMatchInlineSnapshot(` Array [ Object { "kind": 13, @@ -588,7 +597,7 @@ describe('findDeclarationsMatchingWord', () => { "line": 2, }, }, - "uri": "file://${FIXTURE_FOLDER}scope.sh", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/scope.sh", }, "name": "X", }, @@ -596,7 +605,7 @@ describe('findDeclarationsMatchingWord', () => { `) // Local variable definition - expect(findWordFromLine('X', 13)).toMatchInlineSnapshot(` + expect(updateSnapshotUris(findWordFromLine('X', 13))).toMatchInlineSnapshot(` Array [ Object { "containerName": "g", @@ -612,7 +621,7 @@ describe('findDeclarationsMatchingWord', () => { "line": 12, }, }, - "uri": "file://${FIXTURE_FOLDER}scope.sh", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/scope.sh", }, "name": "X", }, @@ -620,7 +629,7 @@ describe('findDeclarationsMatchingWord', () => { `) // Local function definition - expect(findWordFromLine('f', 23)).toMatchInlineSnapshot(` + expect(updateSnapshotUris(findWordFromLine('f', 23))).toMatchInlineSnapshot(` Array [ Object { "containerName": "g", @@ -636,7 +645,7 @@ describe('findDeclarationsMatchingWord', () => { "line": 18, }, }, - "uri": "file://${FIXTURE_FOLDER}scope.sh", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/scope.sh", }, "name": "f", }, @@ -644,7 +653,7 @@ describe('findDeclarationsMatchingWord', () => { `) // Last definition - expect(findWordFromLine('X', 1000)).toMatchInlineSnapshot(` + expect(updateSnapshotUris(findWordFromLine('X', 1000))).toMatchInlineSnapshot(` Array [ Object { "kind": 13, @@ -659,14 +668,14 @@ describe('findDeclarationsMatchingWord', () => { "line": 4, }, }, - "uri": "file://${FIXTURE_FOLDER}scope.sh", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/scope.sh", }, "name": "X", }, ] `) - expect(findWordFromLine('f', 1000)).toMatchInlineSnapshot(` + expect(updateSnapshotUris(findWordFromLine('f', 1000))).toMatchInlineSnapshot(` Array [ Object { "kind": 12, @@ -681,7 +690,7 @@ describe('findDeclarationsMatchingWord', () => { "line": 7, }, }, - "uri": "file://${FIXTURE_FOLDER}scope.sh", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/scope.sh", }, "name": "f", }, @@ -689,7 +698,7 @@ describe('findDeclarationsMatchingWord', () => { `) // Global variable defined inside a function - expect(findWordFromLine('GLOBAL_1', 1000)).toMatchInlineSnapshot(` + expect(updateSnapshotUris(findWordFromLine('GLOBAL_1', 1000))).toMatchInlineSnapshot(` Array [ Object { "containerName": "g", @@ -705,7 +714,7 @@ describe('findDeclarationsMatchingWord', () => { "line": 13, }, }, - "uri": "file://${FIXTURE_FOLDER}scope.sh", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/scope.sh", }, "name": "GLOBAL_1", }, @@ -842,8 +851,14 @@ describe('getAllVariables', () => { newAnalyzer.analyze({ uri, document }) - expect(newAnalyzer.getAllVariables({ uri, position: { line: 20, character: 0 } })) - .toMatchInlineSnapshot(` + expect( + updateSnapshotUris( + newAnalyzer.getAllVariables({ + uri, + position: { line: 20, character: 0 }, + }), + ), + ).toMatchInlineSnapshot(` Array [ Object { "kind": 13, @@ -858,7 +873,7 @@ describe('getAllVariables', () => { "line": 10, }, }, - "uri": "file://${FIXTURE_FOLDER}sourcing.sh", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/sourcing.sh", }, "name": "BOLD", }, @@ -875,7 +890,7 @@ describe('getAllVariables', () => { "line": 4, }, }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/extension.inc", }, "name": "RED", }, @@ -892,7 +907,7 @@ describe('getAllVariables', () => { "line": 5, }, }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/extension.inc", }, "name": "GREEN", }, @@ -909,7 +924,7 @@ describe('getAllVariables', () => { "line": 6, }, }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/extension.inc", }, "name": "BLUE", }, @@ -926,7 +941,7 @@ describe('getAllVariables', () => { "line": 7, }, }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/extension.inc", }, "name": "BOLD", }, @@ -943,7 +958,7 @@ describe('getAllVariables', () => { "line": 10, }, }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", + "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/extension.inc", }, "name": "RESET", }, diff --git a/testing/fixtures.ts b/testing/fixtures.ts index 5b9a0e84b..64261d307 100644 --- a/testing/fixtures.ts +++ b/testing/fixtures.ts @@ -39,3 +39,18 @@ export const FIXTURE_DOCUMENT: Record = ( }, {} as any) export const REPO_ROOT_FOLDER = path.resolve(path.join(FIXTURE_FOLDER, '../..')) + +export function updateSnapshotUris(obj: any) { + if (obj.uri) { + obj.uri = obj.uri.replace(REPO_ROOT_FOLDER, '__REPO_ROOT_FOLDER__') + } + Object.values(obj).forEach((child) => { + if (Array.isArray(child)) { + child.forEach((el) => updateSnapshotUris(el)) + } else if (typeof child === 'object' && child != null) { + updateSnapshotUris(child) + } + }) + + return obj +} From 608dd8772868d2c704ba22ff6b9f1ef867925771 Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 18 Jan 2023 09:58:07 +0100 Subject: [PATCH 4/5] Remove outdated comment --- server/src/util/tree-sitter.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/src/util/tree-sitter.ts b/server/src/util/tree-sitter.ts index fbd130156..f6413573f 100644 --- a/server/src/util/tree-sitter.ts +++ b/server/src/util/tree-sitter.ts @@ -25,8 +25,6 @@ export function range(n: SyntaxNode): Range { export function isDefinition(n: SyntaxNode): boolean { switch (n.type) { - // For now. Later we'll have a command_declaration take precedence over - // variable_assignment case 'variable_assignment': case 'function_definition': return true From 43be0487126c60abcaedbbca52743e680da0813b Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 18 Jan 2023 10:02:38 +0100 Subject: [PATCH 5/5] Release server 4.5.1 --- server/CHANGELOG.md | 6 ++++++ server/package.json | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/server/CHANGELOG.md b/server/CHANGELOG.md index 9506a65f3..405fa84cd 100644 --- a/server/CHANGELOG.md +++ b/server/CHANGELOG.md @@ -1,5 +1,11 @@ # Bash Language Server +## 4.5.1 + +- Include grouped variables and functions when finding global declarations https://github.com/bash-lsp/bash-language-server/pull/685 +- Skip completions in the middle of a non word when the following characters is not an empty list or whitespace. https://github.com/bash-lsp/bash-language-server/pull/684 +- Remove infrequent and rather useless "Failed to parse" diagnostics (and thereby the `HIGHLIGHT_PARSING_ERRORS` and `highlightParsingErrors` configuration option) – the tree sitter parser is actually rather good at error recovery. Note that these messages will now be shown in the log. https://github.com/bash-lsp/bash-language-server/pull/686 + ## 4.5.0 - Include 30 snippets for language constructs (e.g. `if`), builtins (e.g. `test`), expansions (e.g. `[##]`), and external programs (e.g. `sed`) https://github.com/bash-lsp/bash-language-server/pull/683 diff --git a/server/package.json b/server/package.json index 69841ae5d..9bf2a9838 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.0", + "version": "4.5.1", "main": "./out/server.js", "typings": "./out/server.d.ts", "bin": {