From 807e9b66e109753bbfb727baf29fbf20058ae701 Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 22 Dec 2022 16:18:36 +0100 Subject: [PATCH 01/16] Skip symbols defined after the given location --- server/src/__tests__/server.test.ts | 41 +++++++++++++---------------- server/src/analyser.ts | 19 ++++++++++--- server/src/server.ts | 1 + 3 files changed, 36 insertions(+), 25 deletions(-) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 4a0e93ff5..d2e5598c1 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -849,30 +849,27 @@ describe('server', () => { `) }) - it.failing( - 'returns executable documentation if the function is not redefined', - async () => { - const { connection, server } = await initializeServer() - server.register(connection) + it('returns executable documentation if the function is not redefined', async () => { + const { connection, server } = await initializeServer() + server.register(connection) - const onHover = connection.onHover.mock.calls[0][0] + const onHover = connection.onHover.mock.calls[0][0] - const result = await onHover( - { - textDocument: { - uri: FIXTURE_URI.OVERRIDE_SYMBOL, - }, - position: { - line: 2, - character: 1, - }, + const result = await onHover( + { + textDocument: { + uri: FIXTURE_URI.OVERRIDE_SYMBOL, }, - {} as any, - {} as any, - ) + position: { + line: 2, + character: 1, + }, + }, + {} as any, + {} as any, + ) - expect(result).toBeDefined() - expect((result as any)?.contents.value).toContain('ls – list directory contents') - }, - ) + expect(result).toBeDefined() + expect((result as any)?.contents.value).toContain('ls – list directory contents') + }) }) diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 303cf60c0..c49d6ced3 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -314,22 +314,35 @@ export default class Analyzer { */ public findSymbolsMatchingWord({ exactMatch, - uri, + uri: fromUri, word, + position, }: { exactMatch: boolean uri: string word: string + position?: LSP.Position // FIXME: rename to location }): LSP.SymbolInformation[] { - return this.getReachableUris({ uri }).reduce((symbols, uri) => { + return this.getReachableUris({ uri: fromUri }).reduce((symbols, uri) => { const analyzedDocument = this.uriToAnalyzedDocument[uri] if (analyzedDocument) { const { declarations } = analyzedDocument Object.keys(declarations).map((name) => { + const namedDeclaration = declarations[name] const match = exactMatch ? name === word : name.startsWith(word) if (match) { - declarations[name].forEach((symbol) => symbols.push(symbol)) + namedDeclaration.forEach((symbol) => { + // Skip if the symbol is defined in the current file after the requested position + // Ideally we want to be a lot smarter here... + if (uri === fromUri) { + if (position && symbol.location.range.start.line > position.line) { + return + } + } + + symbols.push(symbol) + }) } }) } diff --git a/server/src/server.ts b/server/src/server.ts index 21e39dd5c..bf78f4472 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -430,6 +430,7 @@ export default class BashServer { exactMatch: true, uri: currentUri, word, + position: params.position, }) if ( ReservedWords.isReservedWord(word) || From e8ccecc85b24988330df0b94cd9207036d22841a Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 22 Dec 2022 16:25:41 +0100 Subject: [PATCH 02/16] Make position required --- server/src/__tests__/analyzer.test.ts | 6 ++++++ server/src/analyser.ts | 4 ++-- server/src/server.ts | 1 + 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 60b396887..8ee71e206 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -318,6 +318,7 @@ describe('findSymbolsMatchingWord', () => { word: 'npm_config_logl', uri: FIXTURE_URI.INSTALL, exactMatch: false, + position: { line: 1000, character: 0 }, }), ).toMatchInlineSnapshot(` Array [ @@ -363,6 +364,7 @@ describe('findSymbolsMatchingWord', () => { word: 'xxxxxxxx', uri: FIXTURE_URI.INSTALL, exactMatch: false, + position: { line: 1000, character: 0 }, }), ).toMatchInlineSnapshot(`Array []`) @@ -371,6 +373,7 @@ describe('findSymbolsMatchingWord', () => { word: 'BLU', uri: FIXTURE_URI.INSTALL, exactMatch: false, + position: { line: 6, character: 9 }, }), ).toMatchInlineSnapshot(` Array [ @@ -399,6 +402,7 @@ describe('findSymbolsMatchingWord', () => { word: 'BLU', uri: FIXTURE_URI.SOURCING, exactMatch: false, + position: { line: 6, character: 9 }, }), ).toMatchInlineSnapshot(` Array [ @@ -443,6 +447,7 @@ describe('findSymbolsMatchingWord', () => { word: 'BLU', uri: FIXTURE_URI.INSTALL, exactMatch: false, + position: { line: 1000, character: 0 }, }), ).toMatchInlineSnapshot(`Array []`) @@ -451,6 +456,7 @@ describe('findSymbolsMatchingWord', () => { word: 'BLU', uri: FIXTURE_URI.SOURCING, exactMatch: false, + position: { line: 6, character: 9 }, }), ).toMatchInlineSnapshot(` Array [ diff --git a/server/src/analyser.ts b/server/src/analyser.ts index c49d6ced3..05c501427 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -321,7 +321,7 @@ export default class Analyzer { exactMatch: boolean uri: string word: string - position?: LSP.Position // FIXME: rename to location + position: LSP.Position // FIXME: rename to location }): LSP.SymbolInformation[] { return this.getReachableUris({ uri: fromUri }).reduce((symbols, uri) => { const analyzedDocument = this.uriToAnalyzedDocument[uri] @@ -336,7 +336,7 @@ export default class Analyzer { // Skip if the symbol is defined in the current file after the requested position // Ideally we want to be a lot smarter here... if (uri === fromUri) { - if (position && symbol.location.range.start.line > position.line) { + if (symbol.location.range.start.line > position.line) { return } } diff --git a/server/src/server.ts b/server/src/server.ts index bf78f4472..eda50613e 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -564,6 +564,7 @@ export default class BashServer { exactMatch: false, uri: currentUri, word, + position: params.position, }), currentUri, }) From 2c651fbaa344b76edc5aad0bb5a34d2d885c089b Mon Sep 17 00:00:00 2001 From: skovhus Date: Fri, 23 Dec 2022 11:23:22 +0100 Subject: [PATCH 03/16] Add fixture --- testing/fixtures.ts | 9 +++++---- testing/fixtures/extension.inc | 8 ++++++++ testing/fixtures/scope.sh | 30 ++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 testing/fixtures/scope.sh diff --git a/testing/fixtures.ts b/testing/fixtures.ts index 7d5b19c1f..250645fb6 100644 --- a/testing/fixtures.ts +++ b/testing/fixtures.ts @@ -16,17 +16,18 @@ function getDocument(uri: string) { type FIXTURE_KEY = keyof typeof FIXTURE_URI export const FIXTURE_URI = { + COMMENT_DOC: `file://${path.join(FIXTURE_FOLDER, 'comment-doc-on-hover.sh')}`, INSTALL: `file://${path.join(FIXTURE_FOLDER, 'install.sh')}`, ISSUE101: `file://${path.join(FIXTURE_FOLDER, 'issue101.sh')}`, ISSUE206: `file://${path.join(FIXTURE_FOLDER, 'issue206.sh')}`, MISSING_EXTENSION: `file://${path.join(FIXTURE_FOLDER, 'extension')}`, MISSING_NODE: `file://${path.join(FIXTURE_FOLDER, 'missing-node.sh')}`, - PARSE_PROBLEMS: `file://${path.join(FIXTURE_FOLDER, 'parse-problems.sh')}`, - SOURCING: `file://${path.join(FIXTURE_FOLDER, 'sourcing.sh')}`, - COMMENT_DOC: `file://${path.join(FIXTURE_FOLDER, 'comment-doc-on-hover.sh')}`, OPTIONS: `file://${path.join(FIXTURE_FOLDER, 'options.sh')}`, - SHELLCHECK_SOURCE: `file://${path.join(FIXTURE_FOLDER, 'shellcheck', 'source.sh')}`, OVERRIDE_SYMBOL: `file://${path.join(FIXTURE_FOLDER, 'override-executable-symbol.sh')}`, + PARSE_PROBLEMS: `file://${path.join(FIXTURE_FOLDER, 'parse-problems.sh')}`, + SCOPE: `file://${path.join(FIXTURE_FOLDER, 'scope.sh')}`, + SHELLCHECK_SOURCE: `file://${path.join(FIXTURE_FOLDER, 'shellcheck', 'source.sh')}`, + SOURCING: `file://${path.join(FIXTURE_FOLDER, 'sourcing.sh')}`, } export const FIXTURE_DOCUMENT: Record = ( diff --git a/testing/fixtures/extension.inc b/testing/fixtures/extension.inc index 73b151c1c..b7869e5a9 100644 --- a/testing/fixtures/extension.inc +++ b/testing/fixtures/extension.inc @@ -7,3 +7,11 @@ GREEN=`tput setaf 2` BLUE=`tput setaf 4` BOLD=`tput bold` RESET=`tput sgr0` + +extensionFunc() { + LOCAL_VARIABLE='local' + + innerExtensionFunc() { + echo $LOCAL_VARIABLE + } +} \ No newline at end of file diff --git a/testing/fixtures/scope.sh b/testing/fixtures/scope.sh new file mode 100644 index 000000000..d9a2b979a --- /dev/null +++ b/testing/fixtures/scope.sh @@ -0,0 +1,30 @@ +#!/usr/bin/env bash + +X="Horse" + +X="Mouse" + +# some function +f() ( + local X="Dog" + + g() { + local X="Cat" + echo "${X}" + + # another function function + f() { + local X="Bird" + echo "${X}" + } + + f + } + + g + + echo "${X}" +) + +echo "${X}" +f From 7b5db2cdbe7c3417483959143e65b5fe6fa8f7e5 Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 29 Dec 2022 17:45:27 +0100 Subject: [PATCH 04/16] Only preprocess global declarations --- .../__snapshots__/analyzer.test.ts.snap | 623 +----------------- server/src/__tests__/analyzer.test.ts | 39 +- server/src/__tests__/server.test.ts | 52 -- server/src/analyser.ts | 93 +-- 4 files changed, 73 insertions(+), 734 deletions(-) diff --git a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap index 3b3671146..499687b74 100644 --- a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap +++ b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap @@ -22,7 +22,7 @@ Array [ exports[`analyze returns a list of errors for a file with parsing errors 1`] = ` Array [ Object { - "message": "Failed to parse expression", + "message": "Failed to parse", "range": Object { "end": Object { "character": 1, @@ -127,98 +127,61 @@ Array [ }, "name": "add_a_user", }, +] +`; + +exports[`findSymbolsForFile returns a list of SymbolInformation if uri is found 1`] = ` +Array [ Object { - "containerName": "add_a_user", - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 9, - "line": 5, - }, - "start": Object { - "character": 2, - "line": 5, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "USER", - }, - Object { - "containerName": "add_a_user", - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 13, - "line": 6, - }, - "start": Object { - "character": 2, - "line": 6, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "PASSWORD", - }, - Object { - "containerName": "add_a_user", "kind": 13, "location": Object { "range": Object { "end": Object { - "character": 13, - "line": 9, + "character": 68, + "line": 38, }, "start": Object { - "character": 2, - "line": 9, + "character": 0, + "line": 38, }, }, "uri": "dummy-uri.sh", }, - "name": "COMMENTS", + "name": "configures", }, -] -`; - -exports[`findSymbolsForFile returns a list of SymbolInformation if uri is found 1`] = ` -Array [ Object { "kind": 13, "location": Object { "range": Object { "end": Object { - "character": 8, - "line": 21, + "character": 27, + "line": 40, }, "start": Object { - "character": 2, - "line": 21, + "character": 0, + "line": 40, }, }, "uri": "dummy-uri.sh", }, - "name": "ret", + "name": "npm_config_loglevel", }, Object { "kind": 13, "location": Object { "range": Object { "end": Object { - "character": 8, - "line": 30, + "character": 22, + "line": 53, }, "start": Object { - "character": 2, - "line": 30, + "character": 0, + "line": 53, }, }, "uri": "dummy-uri.sh", }, - "name": "ret", + "name": "node", }, Object { "kind": 13, @@ -254,40 +217,6 @@ Array [ }, "name": "ret", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 8, - "line": 90, - }, - "start": Object { - "character": 2, - "line": 90, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 8, - "line": 97, - }, - "start": Object { - "character": 2, - "line": 97, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, Object { "kind": 13, "location": Object { @@ -322,125 +251,6 @@ Array [ }, "name": "ret", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 9, - "line": 183, - }, - "start": Object { - "character": 2, - "line": 183, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 8, - "line": 188, - }, - "start": Object { - "character": 2, - "line": 188, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 11, - "line": 190, - }, - "start": Object { - "character": 4, - "line": 190, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 11, - "line": 220, - }, - "start": Object { - "character": 6, - "line": 220, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 15, - "line": 230, - }, - "start": Object { - "character": 10, - "line": 230, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 16, - "line": 233, - }, - "start": Object { - "character": 10, - "line": 233, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 16, - "line": 236, - }, - "start": Object { - "character": 10, - "line": 236, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, Object { "kind": 13, "location": Object { @@ -458,74 +268,6 @@ Array [ }, "name": "ret", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 68, - "line": 38, - }, - "start": Object { - "character": 0, - "line": 38, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "configures", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 27, - "line": 40, - }, - "start": Object { - "character": 0, - "line": 40, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "npm_config_loglevel", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 31, - "line": 48, - }, - "start": Object { - "character": 2, - "line": 48, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "npm_config_loglevel", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 22, - "line": 53, - }, - "start": Object { - "character": 0, - "line": 53, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "node", - }, Object { "kind": 13, "location": Object { @@ -543,23 +285,6 @@ Array [ }, "name": "TMP", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 12, - "line": 71, - }, - "start": Object { - "character": 2, - "line": 71, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "TMP", - }, Object { "kind": 13, "location": Object { @@ -611,40 +336,6 @@ Array [ }, "name": "tar", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 25, - "line": 86, - }, - "start": Object { - "character": 2, - "line": 86, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "tar", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 22, - "line": 89, - }, - "start": Object { - "character": 2, - "line": 89, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "tar", - }, Object { "kind": 13, "location": Object { @@ -662,142 +353,6 @@ Array [ }, "name": "MAKE", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 25, - "line": 119, - }, - "start": Object { - "character": 2, - "line": 119, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "make", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 26, - "line": 123, - }, - "start": Object { - "character": 4, - "line": 123, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "make", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 17, - "line": 127, - }, - "start": Object { - "character": 6, - "line": 127, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "make", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 14, - "line": 131, - }, - "start": Object { - "character": 2, - "line": 131, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "make", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 13, - "line": 138, - }, - "start": Object { - "character": 2, - "line": 138, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "make", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 21, - "line": 255, - }, - "start": Object { - "character": 8, - "line": 255, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "make", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 12, - "line": 145, - }, - "start": Object { - "character": 2, - "line": 145, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "clean", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 18, - "line": 225, - }, - "start": Object { - "character": 10, - "line": 225, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "clean", - }, Object { "kind": 13, "location": Object { @@ -832,23 +387,6 @@ Array [ }, "name": "t", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 16, - "line": 170, - }, - "start": Object { - "character": 6, - "line": 170, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "t", - }, Object { "kind": 13, "location": Object { @@ -866,124 +404,5 @@ Array [ }, "name": "url", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 24, - "line": 187, - }, - "start": Object { - "character": 2, - "line": 185, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "url", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 65, - "line": 205, - }, - "start": Object { - "character": 6, - "line": 205, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ver", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 15, - "line": 206, - }, - "start": Object { - "character": 6, - "line": 206, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "isnpm10", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 21, - "line": 211, - }, - "start": Object { - "character": 12, - "line": 211, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "isnpm10", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 21, - "line": 215, - }, - "start": Object { - "character": 12, - "line": 215, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "isnpm10", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 22, - "line": 232, - }, - "start": Object { - "character": 10, - "line": 232, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "NODE", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 22, - "line": 235, - }, - "start": Object { - "character": 10, - "line": 235, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "NODE", - }, ] `; diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 8ee71e206..7116f220d 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -18,7 +18,7 @@ const CURRENT_URI = 'dummy-uri.sh' const mockConsole = getMockConnection().console // if you add a .sh file to testing/fixtures, update this value -const FIXTURE_FILES_MATCHING_GLOB = 14 +const FIXTURE_FILES_MATCHING_GLOB = 15 const defaultConfig = getDefaultConfiguration() @@ -298,7 +298,7 @@ describe('commandNameAtPoint', () => { }) describe('findSymbolsMatchingWord', () => { - it('return a list of symbols across the workspace when includeAllWorkspaceSymbols is true', async () => { + it('returns a list of symbols across the workspace when includeAllWorkspaceSymbols is true', async () => { const parser = await initializeParser() const connection = getMockConnection() @@ -339,23 +339,6 @@ describe('findSymbolsMatchingWord', () => { }, "name": "npm_config_loglevel", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 31, - "line": 48, - }, - "start": Object { - "character": 2, - "line": 48, - }, - }, - "uri": "file://${FIXTURE_FOLDER}install.sh", - }, - "name": "npm_config_loglevel", - }, ] `) @@ -723,24 +706,6 @@ describe('getAllVariableSymbols', () => { }, "name": "RESET", }, - Object { - "containerName": "tagRelease", - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 8, - "line": 5, - }, - "start": Object { - "character": 2, - "line": 5, - }, - }, - "uri": "file://${REPO_ROOT_FOLDER}/scripts/tag-release.inc", - }, - "name": "tag", - }, ] `) }) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index d2e5598c1..9c8313814 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -669,58 +669,6 @@ describe('server', () => { "kind": 6, "label": "RESET", }, - Object { - "data": Object { - "name": "USER", - "type": 3, - }, - "documentation": Object { - "kind": "markdown", - "value": "Variable: **USER** - *defined in issue101.sh*", - }, - "kind": 6, - "label": "USER", - }, - Object { - "data": Object { - "name": "PASSWORD", - "type": 3, - }, - "documentation": Object { - "kind": "markdown", - "value": "Variable: **PASSWORD** - *defined in issue101.sh*", - }, - "kind": 6, - "label": "PASSWORD", - }, - Object { - "data": Object { - "name": "COMMENTS", - "type": 3, - }, - "documentation": Object { - "kind": "markdown", - "value": "Variable: **COMMENTS** - *defined in issue101.sh* - - \`\`\`txt - Having shifted twice, the rest is now comments ... - \`\`\`", - }, - "kind": 6, - "label": "COMMENTS", - }, - Object { - "data": Object { - "name": "tag", - "type": 3, - }, - "documentation": Object { - "kind": "markdown", - "value": "Variable: **tag** - *defined in ../../scripts/tag-release.inc*", - }, - "kind": 6, - "label": "tag", - }, ] `) }) diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 05c501427..e252650dc 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -28,7 +28,7 @@ type Declarations = { [word: string]: LSP.SymbolInformation[] } type AnalyzedDocument = { document: TextDocument tree: Parser.Tree - declarations: Declarations + declarations: Declarations // FIXME: rename to globalDeclarations + should be a record instead. sourcedUris: Set } @@ -364,66 +364,45 @@ export default class Analyzer { document: TextDocument uri: string }): LSP.Diagnostic[] { - const contents = document.getText() + const fileContent = document.getText() - const tree = this.parser.parse(contents) + const tree = this.parser.parse(fileContent) const problems: LSP.Diagnostic[] = [] const declarations: Declarations = {} - // TODO: move this somewhere - TreeSitterUtil.forEach(tree.rootNode, (n: Parser.SyntaxNode) => { - if (n.type === 'ERROR') { + // FIXME: move to a declaration utility file + TreeSitterUtil.forEach(tree.rootNode, (node: Parser.SyntaxNode) => { + if (node.type === 'ERROR') { problems.push( LSP.Diagnostic.create( - TreeSitterUtil.range(n), - 'Failed to parse expression', + TreeSitterUtil.range(node), + 'Failed to parse', LSP.DiagnosticSeverity.Error, ), ) return - } else if (TreeSitterUtil.isDefinition(n)) { - const named = n.firstNamedChild + } - if (named === null) { + if (TreeSitterUtil.isDefinition(node)) { + if (node.parent?.type !== 'program') { + // FIXME: maybe we can avoid traversing the whole tree return } - const word = contents.slice(named.startIndex, named.endIndex) - const namedDeclarations = declarations[word] || [] + const symbol = nodeToSymbolInformation({ node, uri }) - const parent = TreeSitterUtil.findParent( - n, - (p) => p.type === 'function_definition', - ) - const parentName = - parent && parent.firstNamedChild - ? contents.slice( - parent.firstNamedChild.startIndex, - parent.firstNamedChild.endIndex, - ) - : '' // TODO: unsure what we should do here? - - const kind = TREE_SITTER_TYPE_TO_LSP_KIND[n.type] - - if (!kind) { - this.console.warn( - `Unmapped tree sitter type: ${n.type}, defaulting to variable`, - ) - } + if (symbol) { + const word = symbol.name - namedDeclarations.push( - LSP.SymbolInformation.create( - word, - kind || LSP.SymbolKind.Variable, - TreeSitterUtil.range(n), - uri, - parentName, - ), - ) - declarations[word] = namedDeclarations + const namedDeclarations = declarations[word] || [] + namedDeclarations.push(symbol) + declarations[word] = namedDeclarations + } } + + return }) this.uriToAnalyzedDocument[uri] = { @@ -431,7 +410,7 @@ export default class Analyzer { document, declarations, sourcedUris: sourcing.getSourcedUris({ - fileContent: contents, + fileContent, fileUri: uri, rootPath: this.workspaceFolder, tree, @@ -643,3 +622,31 @@ export default class Analyzer { return symbols } } + +function nodeToSymbolInformation({ + node, + uri, +}: { + node: Parser.SyntaxNode + uri: string +}): LSP.SymbolInformation | null { + const named = node.firstNamedChild + + if (named === null) { + return null + } + + const containerName = + TreeSitterUtil.findParent(node, (p) => p.type === 'function_definition') + ?.firstNamedChild?.text || '' + + const kind = TREE_SITTER_TYPE_TO_LSP_KIND[node.type] + + return LSP.SymbolInformation.create( + named.text, + kind || LSP.SymbolKind.Variable, + TreeSitterUtil.range(node), + uri, + containerName, + ) +} From 4fb342593f20b8ac8a403b22f9b7621e2fe53e11 Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 29 Dec 2022 17:53:09 +0100 Subject: [PATCH 05/16] Resolve symbols based on basic scoping --- server/src/__tests__/analyzer.test.ts | 142 +++++++++++++++++++++++++- server/src/__tests__/server.test.ts | 11 -- server/src/analyser.ts | 125 ++++++++++++++++++----- 3 files changed, 243 insertions(+), 35 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 7116f220d..ffe9813eb 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -410,7 +410,7 @@ describe('findSymbolsMatchingWord', () => { `) }) - it('return a list of symbols accessible to the uri when includeAllWorkspaceSymbols is false', async () => { + it('returns a list of symbols accessible to the uri when includeAllWorkspaceSymbols is false', async () => { const parser = await initializeParser() const connection = getMockConnection() @@ -463,6 +463,146 @@ describe('findSymbolsMatchingWord', () => { ] `) }) + + it('returns symbols depending on the scope', async () => { + const parser = await initializeParser() + const connection = getMockConnection() + + const analyzer = new Analyzer({ + console: connection.console, + parser, + includeAllWorkspaceSymbols: false, + workspaceFolder: FIXTURE_FOLDER, + }) + + const findWordFromLine = (word: string, line: number) => + analyzer.findSymbolsMatchingWord({ + word, + uri: FIXTURE_URI.SCOPE, + exactMatch: true, + position: { line, character: 0 }, + }) + + // Variable or function defined yet + expect(findWordFromLine('X', 0)).toEqual([]) + expect(findWordFromLine('f', 0)).toEqual([]) + + // First definition + expect(findWordFromLine('X', 3)).toMatchInlineSnapshot(` + Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 9, + "line": 2, + }, + "start": Object { + "character": 0, + "line": 2, + }, + }, + "uri": "file://${FIXTURE_FOLDER}scope.sh", + }, + "name": "X", + }, + ] + `) + + // Local variable definition + expect(findWordFromLine('X', 13)).toMatchInlineSnapshot(` + Array [ + Object { + "containerName": "g", + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 17, + "line": 11, + }, + "start": Object { + "character": 10, + "line": 11, + }, + }, + "uri": "file://${FIXTURE_FOLDER}scope.sh", + }, + "name": "X", + }, + ] + `) + + // Local function definition + expect(findWordFromLine('f', 20)).toMatchInlineSnapshot(` + Array [ + Object { + "containerName": "g", + "kind": 12, + "location": Object { + "range": Object { + "end": Object { + "character": 5, + "line": 18, + }, + "start": Object { + "character": 4, + "line": 15, + }, + }, + "uri": "file://${FIXTURE_FOLDER}scope.sh", + }, + "name": "f", + }, + ] + `) + + // Last definition + expect(findWordFromLine('X', 1000)).toMatchInlineSnapshot(` + Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 9, + "line": 4, + }, + "start": Object { + "character": 0, + "line": 4, + }, + }, + "uri": "file://${FIXTURE_FOLDER}scope.sh", + }, + "name": "X", + }, + ] + `) + + expect(findWordFromLine('f', 1000)).toMatchInlineSnapshot(` + Array [ + Object { + "kind": 12, + "location": Object { + "range": Object { + "end": Object { + "character": 1, + "line": 26, + }, + "start": Object { + "character": 0, + "line": 7, + }, + }, + "uri": "file://${FIXTURE_FOLDER}scope.sh", + }, + "name": "f", + }, + ] + `) + }) }) describe('commentsAbove', () => { diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 9c8313814..7c44c1c14 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -307,17 +307,6 @@ describe('server', () => { }, name: 'npm_config_loglevel', }, - { - kind: expect.any(Number), - location: { - range: { - end: { character: 31, line: 48 }, - start: { character: 2, line: 48 }, - }, - uri: expect.stringContaining('/testing/fixtures/install.sh'), - }, - name: 'npm_config_loglevel', - }, ]) } diff --git a/server/src/analyser.ts b/server/src/analyser.ts index e252650dc..c0a37e5c4 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -256,6 +256,9 @@ export default class Analyzer { /** * Find all the locations where something named name has been defined. + * + * FIXME: take position into account + * FIXME: take file into account */ public findReferences(name: string): LSP.Location[] { const uris = Object.keys(this.uriToAnalyzedDocument) @@ -265,6 +268,8 @@ export default class Analyzer { /** * Find all occurrences of name in the given file. * It's currently not scope-aware. + * + * FIXME: should this take the scope into account? */ public findOccurrences(uri: string, query: string): LSP.Location[] { const analyzedDocument = this.uriToAnalyzedDocument[uri] @@ -273,27 +278,20 @@ export default class Analyzer { } const { tree } = analyzedDocument - const contents = analyzedDocument.document.getText() const locations: LSP.Location[] = [] TreeSitterUtil.forEach(tree.rootNode, (n) => { - let name: null | string = null - let range: null | LSP.Range = null + let namedNode: Parser.SyntaxNode | null = null if (TreeSitterUtil.isReference(n)) { - const node = n.firstNamedChild || n - name = contents.slice(node.startIndex, node.endIndex) - range = TreeSitterUtil.range(node) + namedNode = n.firstNamedChild || n } else if (TreeSitterUtil.isDefinition(n)) { - const namedNode = n.firstNamedChild - if (namedNode) { - name = contents.slice(namedNode.startIndex, namedNode.endIndex) - range = TreeSitterUtil.range(namedNode) - } + namedNode = n.firstNamedChild } - if (name === query && range !== null) { + if (namedNode && namedNode.text === query) { + const range = TreeSitterUtil.range(namedNode) locations.push(LSP.Location.create(uri, range)) } }) @@ -303,6 +301,8 @@ export default class Analyzer { /** * Find all symbol definitions in the given file. + * + * FIXME: should this return all symbols? */ public findSymbolsForFile({ uri }: { uri: string }): LSP.SymbolInformation[] { const declarationsInFile = this.uriToAnalyzedDocument[uri]?.declarations || {} @@ -311,41 +311,68 @@ export default class Analyzer { /** * Find symbol completions for the given word. + * + * FIXME: could this use findAllSymbols? */ public findSymbolsMatchingWord({ exactMatch, + position, uri: fromUri, word, - position, }: { exactMatch: boolean + position: LSP.Position uri: string word: string - position: LSP.Position // FIXME: rename to location }): LSP.SymbolInformation[] { return this.getReachableUris({ uri: fromUri }).reduce((symbols, uri) => { const analyzedDocument = this.uriToAnalyzedDocument[uri] if (analyzedDocument) { - const { declarations } = analyzedDocument + // We use the global declarations from external files + let { declarations } = analyzedDocument + + // For the current file we find declarations based on the current scope + if (uri === fromUri) { + const node = analyzedDocument.tree.rootNode?.descendantForPosition({ + row: position.line, + column: position.character, + }) + + declarations = getLocalDeclarations({ + node, + uri, + }) + } + Object.keys(declarations).map((name) => { - const namedDeclaration = declarations[name] + const symbolsMatchingWord = declarations[name] const match = exactMatch ? name === word : name.startsWith(word) + if (match) { - namedDeclaration.forEach((symbol) => { + // Find the latest definition + let closestSymbol: LSP.SymbolInformation | null = null + symbolsMatchingWord.forEach((symbol) => { // Skip if the symbol is defined in the current file after the requested position - // Ideally we want to be a lot smarter here... - if (uri === fromUri) { - if (symbol.location.range.start.line > position.line) { - return - } + if (uri === fromUri && symbol.location.range.start.line > position.line) { + return } - symbols.push(symbol) + if ( + closestSymbol === null || + symbol.location.range.start.line > closestSymbol.location.range.start.line + ) { + closestSymbol = symbol + } }) + + if (closestSymbol) { + symbols.push(closestSymbol) + } } }) } + return symbols }, [] as LSP.SymbolInformation[]) } @@ -605,6 +632,7 @@ export default class Analyzer { } private getAllSymbols({ uri }: { uri?: string } = {}): LSP.SymbolInformation[] { + // FIXME: this should use getLocalDeclarations! const reachableUris = this.getReachableUris({ uri }) const symbols: LSP.SymbolInformation[] = [] @@ -650,3 +678,54 @@ function nodeToSymbolInformation({ containerName, ) } + +function getLocalDeclarations({ + node, + uri, +}: { + node: Parser.SyntaxNode | null + uri: string +}): Declarations { + const declarations: Declarations = {} + + // bottom up traversal of the tree to capture all local declarations + + const walk = (node: Parser.SyntaxNode | null) => { + // NOTE: there is also node.walk + if (node) { + for (const childNode of node.children) { + let symbol: LSP.SymbolInformation | null = null + + // local variables + if (childNode.type === 'declaration_command') { + const variableAssignmentNode = childNode.children.filter( + (child) => child.type === 'variable_assignment', + )[0] + + if (variableAssignmentNode) { + symbol = nodeToSymbolInformation({ + node: variableAssignmentNode, + uri, + }) + } + } else if (TreeSitterUtil.isDefinition(childNode)) { + // FIXME: does this also capture local variables? + symbol = nodeToSymbolInformation({ node: childNode, uri }) + } + + if (symbol) { + if (!declarations[symbol.name]) { + declarations[symbol.name] = [] + } + declarations[symbol.name].push(symbol) + } + } + + walk(node.parent) + } + } + + walk(node) + + return declarations +} From 7d9a80e3948d5b09c35c0e4a76f6764c10778175 Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 29 Dec 2022 22:08:32 +0100 Subject: [PATCH 06/16] Use latest symbol for global declarations --- .../__snapshots__/analyzer.test.ts.snap | 85 ---------- server/src/analyser.ts | 142 ++--------------- server/src/util/declarations.ts | 150 ++++++++++++++++++ 3 files changed, 167 insertions(+), 210 deletions(-) create mode 100644 server/src/util/declarations.ts diff --git a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap index 499687b74..9dbeafe08 100644 --- a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap +++ b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap @@ -183,74 +183,6 @@ Array [ }, "name": "node", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 6, - "line": 54, - }, - "start": Object { - "character": 0, - "line": 54, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 5, - "line": 83, - }, - "start": Object { - "character": 0, - "line": 83, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 6, - "line": 149, - }, - "start": Object { - "character": 0, - "line": 149, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 6, - "line": 181, - }, - "start": Object { - "character": 0, - "line": 181, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "ret", - }, Object { "kind": 13, "location": Object { @@ -268,23 +200,6 @@ Array [ }, "name": "ret", }, - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 15, - "line": 69, - }, - "start": Object { - "character": 0, - "line": 69, - }, - }, - "uri": "dummy-uri.sh", - }, - "name": "TMP", - }, Object { "kind": 13, "location": Object { diff --git a/server/src/analyser.ts b/server/src/analyser.ts index c0a37e5c4..db7c94df7 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -8,6 +8,11 @@ import * as LSP from 'vscode-languageserver/node' import { TextDocument } from 'vscode-languageserver-textdocument' import * as Parser from 'web-tree-sitter' +import { + Declarations, + getGlobalDeclarations, + getLocalDeclarations, +} from './util/declarations' import { flattenArray, flattenObjectValues } from './util/flatten' import { getFilePaths } from './util/fs' import { analyzeShebang } from './util/shebang' @@ -16,15 +21,6 @@ import * as TreeSitterUtil from './util/tree-sitter' const readFileAsync = promisify(fs.readFile) -const TREE_SITTER_TYPE_TO_LSP_KIND: { [type: string]: LSP.SymbolKind | undefined } = { - // These keys are using underscores as that's the naming convention in tree-sitter. - environment_variable_assignment: LSP.SymbolKind.Variable, - function_definition: LSP.SymbolKind.Function, - variable_assignment: LSP.SymbolKind.Variable, -} - -type Declarations = { [word: string]: LSP.SymbolInformation[] } - type AnalyzedDocument = { document: TextDocument tree: Parser.Tree @@ -395,42 +391,7 @@ export default class Analyzer { const tree = this.parser.parse(fileContent) - const problems: LSP.Diagnostic[] = [] - - const declarations: Declarations = {} - - // FIXME: move to a declaration utility file - TreeSitterUtil.forEach(tree.rootNode, (node: Parser.SyntaxNode) => { - if (node.type === 'ERROR') { - problems.push( - LSP.Diagnostic.create( - TreeSitterUtil.range(node), - 'Failed to parse', - LSP.DiagnosticSeverity.Error, - ), - ) - return - } - - if (TreeSitterUtil.isDefinition(node)) { - if (node.parent?.type !== 'program') { - // FIXME: maybe we can avoid traversing the whole tree - return - } - - const symbol = nodeToSymbolInformation({ node, uri }) - - if (symbol) { - const word = symbol.name - - const namedDeclarations = declarations[word] || [] - namedDeclarations.push(symbol) - declarations[word] = namedDeclarations - } - } - - return - }) + const { declarations, diagnostics } = getGlobalDeclarations({ tree, uri }) this.uriToAnalyzedDocument[uri] = { tree, @@ -446,7 +407,7 @@ export default class Analyzer { function findMissingNodes(node: Parser.SyntaxNode) { if (node.isMissing()) { - problems.push( + diagnostics.push( LSP.Diagnostic.create( TreeSitterUtil.range(node), `Syntax error: expected "${node.type}" somewhere in the file`, @@ -460,7 +421,7 @@ export default class Analyzer { findMissingNodes(tree.rootNode) - return problems + return diagnostics } public findAllSourcedUris({ uri }: { uri: string }): Set { @@ -651,81 +612,12 @@ export default class Analyzer { } } -function nodeToSymbolInformation({ - node, - uri, -}: { - node: Parser.SyntaxNode - uri: string -}): LSP.SymbolInformation | null { - const named = node.firstNamedChild - - if (named === null) { - return null - } - - const containerName = - TreeSitterUtil.findParent(node, (p) => p.type === 'function_definition') - ?.firstNamedChild?.text || '' - - const kind = TREE_SITTER_TYPE_TO_LSP_KIND[node.type] - - return LSP.SymbolInformation.create( - named.text, - kind || LSP.SymbolKind.Variable, - TreeSitterUtil.range(node), - uri, - containerName, - ) -} - -function getLocalDeclarations({ - node, - uri, -}: { - node: Parser.SyntaxNode | null - uri: string -}): Declarations { - const declarations: Declarations = {} - - // bottom up traversal of the tree to capture all local declarations - - const walk = (node: Parser.SyntaxNode | null) => { - // NOTE: there is also node.walk - if (node) { - for (const childNode of node.children) { - let symbol: LSP.SymbolInformation | null = null - - // local variables - if (childNode.type === 'declaration_command') { - const variableAssignmentNode = childNode.children.filter( - (child) => child.type === 'variable_assignment', - )[0] - - if (variableAssignmentNode) { - symbol = nodeToSymbolInformation({ - node: variableAssignmentNode, - uri, - }) - } - } else if (TreeSitterUtil.isDefinition(childNode)) { - // FIXME: does this also capture local variables? - symbol = nodeToSymbolInformation({ node: childNode, uri }) - } - - if (symbol) { - if (!declarations[symbol.name]) { - declarations[symbol.name] = [] - } - declarations[symbol.name].push(symbol) - } - } - - walk(node.parent) - } - } - - walk(node) - - return declarations -} +// Other languages where there are multiple re-declaration of variables +// TS +// - go to definition: takes you to the "let x = 1" line even though it is redefined +// - clicking on first definition: shows references also in the other files +// - docs: "function x(): void" or "let x: number" +// Python +// - go to definition: returns multiple definitions +// - clicking on first definition: shows definitions +// - docs: prefixed with "(function) x() -> None" or "(variable) xxx: Literal[1]" diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts new file mode 100644 index 000000000..f19ecedbf --- /dev/null +++ b/server/src/util/declarations.ts @@ -0,0 +1,150 @@ +import * as LSP from 'vscode-languageserver/node' +import * as Parser from 'web-tree-sitter' + +import * as TreeSitterUtil from './tree-sitter' + +const TREE_SITTER_TYPE_TO_LSP_KIND: { [type: string]: LSP.SymbolKind | undefined } = { + // These keys are using underscores as that's the naming convention in tree-sitter. + environment_variable_assignment: LSP.SymbolKind.Variable, + function_definition: LSP.SymbolKind.Function, + variable_assignment: LSP.SymbolKind.Variable, +} + +export type Declarations = { [word: string]: LSP.SymbolInformation[] } + +/** + * Returns declarations (functions or variables) from a given root node + * that would be available after sourcing the file. + * + * Will only return one declaration per symbol name – the latest definition. + * This behavior is consistent with how Bash behaves, but differs between + * LSP servers. + * + * Used when finding declarations for sourced files and to get declarations + * for the entire workspace. + */ +export function getGlobalDeclarations({ + tree, + uri, +}: { + tree: Parser.Tree + uri: string +}): { diagnostics: LSP.Diagnostic[]; declarations: Declarations } { + const diagnostics: LSP.Diagnostic[] = [] + const declarations: Declarations = {} + + TreeSitterUtil.forEach(tree.rootNode, (node: Parser.SyntaxNode) => { + if (node.parent?.type !== 'program') { + return + } + + if (node.type === 'ERROR') { + diagnostics.push( + LSP.Diagnostic.create( + TreeSitterUtil.range(node), + 'Failed to parse', + LSP.DiagnosticSeverity.Error, + ), + ) + return + } + + if (TreeSitterUtil.isDefinition(node)) { + const symbol = nodeToSymbolInformation({ node, uri }) + + if (symbol) { + const word = symbol.name + declarations[word] = [symbol] // TODO: ensure this is the latest definition + } + } + + return + }) + + return { diagnostics, declarations } +} + +function nodeToSymbolInformation({ + node, + uri, +}: { + node: Parser.SyntaxNode + uri: string +}): LSP.SymbolInformation | null { + const named = node.firstNamedChild + + if (named === null) { + return null + } + + const containerName = + TreeSitterUtil.findParent(node, (p) => p.type === 'function_definition') + ?.firstNamedChild?.text || '' + + const kind = TREE_SITTER_TYPE_TO_LSP_KIND[node.type] + + return LSP.SymbolInformation.create( + named.text, + kind || LSP.SymbolKind.Variable, + TreeSitterUtil.range(node), + uri, + containerName, + ) +} + +/** + * Returns declarations available for the given file and location + * Done by traversing the tree upwards (which is a simplification for + * actual bash behaviour but deemed good enough, compared to the complexity of flow tracing). + * Filters out duplicate definitions. Used when getting declarations for the current scope. + */ +export function getLocalDeclarations({ + node, + uri, +}: { + node: Parser.SyntaxNode | null + uri: string +}): Declarations { + const declarations: Declarations = {} + + // bottom up traversal of the tree to capture all local declarations + + const walk = (node: Parser.SyntaxNode | null) => { + // NOTE: there is also node.walk + if (node) { + for (const childNode of node.children) { + let symbol: LSP.SymbolInformation | null = null + + // local variables + if (childNode.type === 'declaration_command') { + const variableAssignmentNode = childNode.children.filter( + (child) => child.type === 'variable_assignment', + )[0] + + if (variableAssignmentNode) { + symbol = nodeToSymbolInformation({ + node: variableAssignmentNode, + uri, + }) + } + } else if (TreeSitterUtil.isDefinition(childNode)) { + // FIXME: does this also capture local variables? + symbol = nodeToSymbolInformation({ node: childNode, uri }) + } + + if (symbol) { + if (!declarations[symbol.name]) { + declarations[symbol.name] = [] + } + declarations[symbol.name].push(symbol) + } + } + + walk(node.parent) + } + } + + walk(node) + + return declarations +} From 43f2a4b7c441c6f9f52ad33d75851a9a05b6bfe3 Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 29 Dec 2022 22:23:57 +0100 Subject: [PATCH 07/16] Make global declarations a record --- server/src/analyser.ts | 100 +++++++++++++--------- server/src/util/__tests__/flatten.test.ts | 13 +-- server/src/util/declarations.ts | 12 ++- server/src/util/flatten.ts | 4 - 4 files changed, 68 insertions(+), 61 deletions(-) diff --git a/server/src/analyser.ts b/server/src/analyser.ts index db7c94df7..85fcbc35c 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -9,11 +9,11 @@ import { TextDocument } from 'vscode-languageserver-textdocument' import * as Parser from 'web-tree-sitter' import { - Declarations, getGlobalDeclarations, getLocalDeclarations, + GlobalDeclarations, } from './util/declarations' -import { flattenArray, flattenObjectValues } from './util/flatten' +import { flattenArray } from './util/flatten' import { getFilePaths } from './util/fs' import { analyzeShebang } from './util/shebang' import * as sourcing from './util/sourcing' @@ -23,9 +23,9 @@ const readFileAsync = promisify(fs.readFile) type AnalyzedDocument = { document: TextDocument - tree: Parser.Tree - declarations: Declarations // FIXME: rename to globalDeclarations + should be a record instead. + globalDeclarations: GlobalDeclarations sourcedUris: Set + tree: Parser.Tree } /** @@ -173,8 +173,10 @@ export default class Analyzer { .reduce((symbols, uri) => { const analyzedDocument = this.uriToAnalyzedDocument[uri] if (analyzedDocument) { - const declarationNames = analyzedDocument.declarations[word] || [] - declarationNames.forEach((d) => symbols.push(d)) + const globalDeclaration = analyzedDocument.globalDeclarations[word] + if (globalDeclaration) { + symbols.push(globalDeclaration) + } } return symbols }, [] as LSP.SymbolInformation[]) @@ -301,8 +303,8 @@ export default class Analyzer { * FIXME: should this return all symbols? */ public findSymbolsForFile({ uri }: { uri: string }): LSP.SymbolInformation[] { - const declarationsInFile = this.uriToAnalyzedDocument[uri]?.declarations || {} - return flattenObjectValues(declarationsInFile) + const declarationsInFile = this.uriToAnalyzedDocument[uri]?.globalDeclarations || {} + return Object.values(declarationsInFile) } /** @@ -325,8 +327,23 @@ export default class Analyzer { const analyzedDocument = this.uriToAnalyzedDocument[uri] if (analyzedDocument) { - // We use the global declarations from external files - let { declarations } = analyzedDocument + if (uri !== fromUri) { + // We use the global declarations from external files + const { globalDeclarations } = analyzedDocument + + if (exactMatch) { + const symbol = globalDeclarations[word] + if (symbol) { + symbols.push(symbol) + } + } else { + Object.entries(globalDeclarations).forEach(([name, symbol]) => { + if (name.startsWith(word)) { + symbols.push(symbol) + } + }) + } + } // For the current file we find declarations based on the current scope if (uri === fromUri) { @@ -335,38 +352,39 @@ export default class Analyzer { column: position.character, }) - declarations = getLocalDeclarations({ + const localDeclarations = getLocalDeclarations({ node, uri, }) - } - Object.keys(declarations).map((name) => { - const symbolsMatchingWord = declarations[name] - const match = exactMatch ? name === word : name.startsWith(word) - - if (match) { - // Find the latest definition - let closestSymbol: LSP.SymbolInformation | null = null - symbolsMatchingWord.forEach((symbol) => { - // Skip if the symbol is defined in the current file after the requested position - if (uri === fromUri && symbol.location.range.start.line > position.line) { - return + Object.keys(localDeclarations).map((name) => { + const symbolsMatchingWord = localDeclarations[name] + const match = exactMatch ? name === word : name.startsWith(word) + + if (match) { + // Find the latest definition + let closestSymbol: LSP.SymbolInformation | null = null + symbolsMatchingWord.forEach((symbol) => { + // Skip if the symbol is defined in the current file after the requested position + if (uri === fromUri && symbol.location.range.start.line > position.line) { + return + } + + if ( + closestSymbol === null || + symbol.location.range.start.line > + closestSymbol.location.range.start.line + ) { + closestSymbol = symbol + } + }) + + if (closestSymbol) { + symbols.push(closestSymbol) } - - if ( - closestSymbol === null || - symbol.location.range.start.line > closestSymbol.location.range.start.line - ) { - closestSymbol = symbol - } - }) - - if (closestSymbol) { - symbols.push(closestSymbol) } - } - }) + }) + } } return symbols @@ -391,18 +409,18 @@ export default class Analyzer { const tree = this.parser.parse(fileContent) - const { declarations, diagnostics } = getGlobalDeclarations({ tree, uri }) + const { diagnostics, globalDeclarations } = getGlobalDeclarations({ tree, uri }) this.uriToAnalyzedDocument[uri] = { - tree, document, - declarations, + globalDeclarations, sourcedUris: sourcing.getSourcedUris({ fileContent, fileUri: uri, rootPath: this.workspaceFolder, tree, }), + tree, } function findMissingNodes(node: Parser.SyntaxNode) { @@ -603,8 +621,8 @@ export default class Analyzer { return } - Object.values(analyzedDocument.declarations).forEach((declarationNames) => { - declarationNames.forEach((d) => symbols.push(d)) + Object.values(analyzedDocument.globalDeclarations).forEach((symbol) => { + symbols.push(symbol) }) }) diff --git a/server/src/util/__tests__/flatten.test.ts b/server/src/util/__tests__/flatten.test.ts index f77577710..b73de257b 100644 --- a/server/src/util/__tests__/flatten.test.ts +++ b/server/src/util/__tests__/flatten.test.ts @@ -1,4 +1,4 @@ -import { flattenArray, flattenObjectValues } from '../flatten' +import { flattenArray } from '../flatten' describe('flattenArray', () => { it('works on array with one element', () => { @@ -9,14 +9,3 @@ describe('flattenArray', () => { expect(flattenArray([[1], [2, 3], [4]])).toEqual([1, 2, 3, 4]) }) }) - -describe('flattenObjectValues', () => { - it('flatten object values', () => { - expect( - flattenObjectValues({ - 'foo.sh': [1], - 'baz.sh': [2], - }), - ).toEqual([1, 2]) - }) -}) diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts index f19ecedbf..bfa2e3c64 100644 --- a/server/src/util/declarations.ts +++ b/server/src/util/declarations.ts @@ -10,6 +10,7 @@ const TREE_SITTER_TYPE_TO_LSP_KIND: { [type: string]: LSP.SymbolKind | undefined variable_assignment: LSP.SymbolKind.Variable, } +export type GlobalDeclarations = { [word: string]: LSP.SymbolInformation } export type Declarations = { [word: string]: LSP.SymbolInformation[] } /** @@ -29,9 +30,12 @@ export function getGlobalDeclarations({ }: { tree: Parser.Tree uri: string -}): { diagnostics: LSP.Diagnostic[]; declarations: Declarations } { +}): { + diagnostics: LSP.Diagnostic[] + globalDeclarations: GlobalDeclarations +} { const diagnostics: LSP.Diagnostic[] = [] - const declarations: Declarations = {} + const globalDeclarations: GlobalDeclarations = {} TreeSitterUtil.forEach(tree.rootNode, (node: Parser.SyntaxNode) => { if (node.parent?.type !== 'program') { @@ -54,14 +58,14 @@ export function getGlobalDeclarations({ if (symbol) { const word = symbol.name - declarations[word] = [symbol] // TODO: ensure this is the latest definition + globalDeclarations[word] = symbol } } return }) - return { diagnostics, declarations } + return { diagnostics, globalDeclarations } } function nodeToSymbolInformation({ diff --git a/server/src/util/flatten.ts b/server/src/util/flatten.ts index 7c4193af3..28fde991c 100644 --- a/server/src/util/flatten.ts +++ b/server/src/util/flatten.ts @@ -1,7 +1,3 @@ export function flattenArray(nestedArray: T[][]): T[] { return nestedArray.reduce((acc, array) => [...acc, ...array], []) } - -export function flattenObjectValues(object: { [key: string]: T[] }): T[] { - return flattenArray(Object.keys(object).map((objectKey) => object[objectKey])) -} From 59282871fa536df555cdccf09b1e2ab9a4982fbe Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 29 Dec 2022 23:00:03 +0100 Subject: [PATCH 08/16] Fix onDocumentHighlight returning duplicated locations --- .../__snapshots__/analyzer.test.ts.snap | 13 -- server/src/__tests__/server.test.ts | 141 ++++++++++++++++-- server/src/analyser.ts | 14 +- 3 files changed, 139 insertions(+), 29 deletions(-) diff --git a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap index 9dbeafe08..07e1b30f5 100644 --- a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap +++ b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap @@ -53,19 +53,6 @@ Array [ }, "uri": "dummy-uri.sh", }, - Object { - "range": Object { - "end": Object { - "character": 12, - "line": 148, - }, - "start": Object { - "character": 0, - "line": 148, - }, - }, - "uri": "dummy-uri.sh", - }, Object { "range": Object { "end": Object { diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 7c44c1c14..5670cd972 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -221,7 +221,6 @@ describe('server', () => { {} as any, ) - // TODO: there is a superfluous range here on line 0: expect(result1).toMatchInlineSnapshot(` Array [ Object { @@ -236,18 +235,6 @@ describe('server', () => { }, }, }, - Object { - "range": Object { - "end": Object { - "character": 12, - "line": 0, - }, - "start": Object { - "character": 9, - "line": 0, - }, - }, - }, Object { "range": Object { "end": Object { @@ -279,6 +266,134 @@ describe('server', () => { ) expect(result2).toMatchInlineSnapshot(`Array []`) + + const result3 = await onDocumentHighlight( + { + textDocument: { + uri: FIXTURE_URI.SCOPE, + }, + position: { + // X + line: 28, + character: 8, + }, + }, + {} as any, + {} as any, + ) + + expect(result3).toMatchInlineSnapshot(` + Array [ + Object { + "range": Object { + "end": Object { + "character": 1, + "line": 2, + }, + "start": Object { + "character": 0, + "line": 2, + }, + }, + }, + Object { + "range": Object { + "end": Object { + "character": 1, + "line": 4, + }, + "start": Object { + "character": 0, + "line": 4, + }, + }, + }, + Object { + "range": Object { + "end": Object { + "character": 9, + "line": 8, + }, + "start": Object { + "character": 8, + "line": 8, + }, + }, + }, + Object { + "range": Object { + "end": Object { + "character": 11, + "line": 11, + }, + "start": Object { + "character": 10, + "line": 11, + }, + }, + }, + Object { + "range": Object { + "end": Object { + "character": 13, + "line": 12, + }, + "start": Object { + "character": 12, + "line": 12, + }, + }, + }, + Object { + "range": Object { + "end": Object { + "character": 13, + "line": 16, + }, + "start": Object { + "character": 12, + "line": 16, + }, + }, + }, + Object { + "range": Object { + "end": Object { + "character": 15, + "line": 17, + }, + "start": Object { + "character": 14, + "line": 17, + }, + }, + }, + Object { + "range": Object { + "end": Object { + "character": 11, + "line": 25, + }, + "start": Object { + "character": 10, + "line": 25, + }, + }, + }, + Object { + "range": Object { + "end": Object { + "character": 9, + "line": 28, + }, + "start": Object { + "character": 8, + "line": 28, + }, + }, + }, + ] + `) }) it('responds to onWorkspaceSymbol', async () => { diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 85fcbc35c..da2be63a3 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -3,7 +3,7 @@ import * as FuzzySearch from 'fuzzy-search' import fetch from 'node-fetch' import * as URI from 'urijs' import * as url from 'url' -import { promisify } from 'util' +import { isDeepStrictEqual, promisify } from 'util' import * as LSP from 'vscode-languageserver/node' import { TextDocument } from 'vscode-languageserver-textdocument' import * as Parser from 'web-tree-sitter' @@ -267,7 +267,8 @@ export default class Analyzer { * Find all occurrences of name in the given file. * It's currently not scope-aware. * - * FIXME: should this take the scope into account? + * FIXME: should this take the scope into account? I guess it should + * as this is used for highlighting. */ public findOccurrences(uri: string, query: string): LSP.Location[] { const analyzedDocument = this.uriToAnalyzedDocument[uri] @@ -290,7 +291,14 @@ export default class Analyzer { if (namedNode && namedNode.text === query) { const range = TreeSitterUtil.range(namedNode) - locations.push(LSP.Location.create(uri, range)) + + const alreadyInLocations = locations.some((loc) => { + return isDeepStrictEqual(loc.range, range) + }) + + if (!alreadyInLocations) { + locations.push(LSP.Location.create(uri, range)) + } } }) From 92187680b128bca164671d8a1a557a6e41b85d3e Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 29 Dec 2022 23:14:02 +0100 Subject: [PATCH 09/16] Make findDefinition scope aware --- server/src/__tests__/analyzer.test.ts | 36 +++++++++++++++++++++------ server/src/analyser.ts | 25 ++++++++----------- 2 files changed, 38 insertions(+), 23 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index ffe9813eb..829847a26 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -60,12 +60,6 @@ describe('analyze', () => { }) describe('findDefinition', () => { - it('returns an empty list if word is not found', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) - const result = analyzer.findDefinition({ uri: CURRENT_URI, word: 'foobar' }) - expect(result).toEqual([]) - }) - it('returns a location to a file if word is the path in a sourcing statement', () => { const document = FIXTURE_DOCUMENT.SOURCING const { uri } = document @@ -131,13 +125,13 @@ describe('findDefinition', () => { `) }) - it('returns a list of locations if parameter is found', () => { + it('returns a local reference if definition is found', () => { analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) const result = analyzer.findDefinition({ + position: { character: 1, line: 148 }, uri: CURRENT_URI, word: 'node_version', }) - expect(result).not.toEqual([]) expect(result).toMatchInlineSnapshot(` Array [ Object { @@ -156,6 +150,32 @@ describe('findDefinition', () => { ] `) }) + + it('returns local declarations', () => { + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) + const result = analyzer.findDefinition({ + position: { character: 12, line: 12 }, + uri: FIXTURE_URI.SCOPE, + word: 'X', + }) + expect(result).toMatchInlineSnapshot(` + Array [ + Object { + "range": Object { + "end": Object { + "character": 17, + "line": 11, + }, + "start": Object { + "character": 10, + "line": 11, + }, + }, + "uri": "file://${FIXTURE_FOLDER}scope.sh", + }, + ] + `) + }) }) describe('findReferences', () => { diff --git a/server/src/analyser.ts b/server/src/analyser.ts index da2be63a3..4f96bd021 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -149,7 +149,7 @@ export default class Analyzer { uri, word, }: { - position?: { line: number; character: number } + position: LSP.Position uri: string word: string }): LSP.Location[] { @@ -169,18 +169,12 @@ export default class Analyzer { } } - return this.getReachableUris({ uri }) - .reduce((symbols, uri) => { - const analyzedDocument = this.uriToAnalyzedDocument[uri] - if (analyzedDocument) { - const globalDeclaration = analyzedDocument.globalDeclarations[word] - if (globalDeclaration) { - symbols.push(globalDeclaration) - } - } - return symbols - }, [] as LSP.SymbolInformation[]) - .map((symbol) => symbol.location) + return this.findSymbolsMatchingWord({ + exactMatch: true, + position, + uri, + word, + }).map((symbol) => symbol.location) } /** @@ -253,7 +247,7 @@ export default class Analyzer { } /** - * Find all the locations where something named name has been defined. + * Find all the locations where something named name has been defined or referenced. * * FIXME: take position into account * FIXME: take file into account @@ -316,9 +310,10 @@ export default class Analyzer { } /** - * Find symbol completions for the given word. + * Find declarations/definitions for the given word. * * FIXME: could this use findAllSymbols? + * FIXME: rename to findDeclarationsMatchingWord */ public findSymbolsMatchingWord({ exactMatch, From 64f41a662d65471001728ed0b2cddac67b9844d8 Mon Sep 17 00:00:00 2001 From: skovhus Date: Fri, 30 Dec 2022 18:44:33 +0100 Subject: [PATCH 10/16] Reuse declaration logic for completions --- server/src/__tests__/analyzer.test.ts | 4 +- server/src/analyser.ts | 170 ++++++++++++-------------- server/src/server.ts | 8 +- 3 files changed, 86 insertions(+), 96 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 829847a26..314504149 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -762,7 +762,9 @@ describe('getAllVariableSymbols', () => { newAnalyzer.analyze({ uri, document }) - expect(newAnalyzer.getAllVariableSymbols({ uri })).toMatchInlineSnapshot(` + expect( + newAnalyzer.getAllVariableSymbols({ uri, position: { line: 20, character: 0 } }), + ).toMatchInlineSnapshot(` Array [ Object { "kind": 13, diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 4f96bd021..6c0b7771d 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -143,6 +143,7 @@ export default class Analyzer { /** * Find all the locations where something has been defined. + * FIXME: rename to findDeclarationLocation or findDefinitionLocation */ public findDefinition({ position, @@ -179,6 +180,7 @@ export default class Analyzer { /** * Find all the symbols matching the query using fuzzy search. + * FIXME: rename to searchAllSymbols */ public search(query: string): LSP.SymbolInformation[] { const searcher = new FuzzySearch(this.getAllSymbols(), ['name'], { @@ -302,7 +304,8 @@ export default class Analyzer { /** * Find all symbol definitions in the given file. * - * FIXME: should this return all symbols? + * FIXME: this should return all symbols, not just global declarations. + * FIXME: convert to DocumentSymbol[] which is a hierarchy of symbols found in a given text document. */ public findSymbolsForFile({ uri }: { uri: string }): LSP.SymbolInformation[] { const declarationsInFile = this.uriToAnalyzedDocument[uri]?.globalDeclarations || {} @@ -312,13 +315,12 @@ export default class Analyzer { /** * Find declarations/definitions for the given word. * - * FIXME: could this use findAllSymbols? * FIXME: rename to findDeclarationsMatchingWord */ public findSymbolsMatchingWord({ exactMatch, position, - uri: fromUri, + uri, word, }: { exactMatch: boolean @@ -326,72 +328,13 @@ export default class Analyzer { uri: string word: string }): LSP.SymbolInformation[] { - return this.getReachableUris({ uri: fromUri }).reduce((symbols, uri) => { - const analyzedDocument = this.uriToAnalyzedDocument[uri] - - if (analyzedDocument) { - if (uri !== fromUri) { - // We use the global declarations from external files - const { globalDeclarations } = analyzedDocument - - if (exactMatch) { - const symbol = globalDeclarations[word] - if (symbol) { - symbols.push(symbol) - } - } else { - Object.entries(globalDeclarations).forEach(([name, symbol]) => { - if (name.startsWith(word)) { - symbols.push(symbol) - } - }) - } - } - - // For the current file we find declarations based on the current scope - if (uri === fromUri) { - const node = analyzedDocument.tree.rootNode?.descendantForPosition({ - row: position.line, - column: position.character, - }) - - const localDeclarations = getLocalDeclarations({ - node, - uri, - }) - - Object.keys(localDeclarations).map((name) => { - const symbolsMatchingWord = localDeclarations[name] - const match = exactMatch ? name === word : name.startsWith(word) - - if (match) { - // Find the latest definition - let closestSymbol: LSP.SymbolInformation | null = null - symbolsMatchingWord.forEach((symbol) => { - // Skip if the symbol is defined in the current file after the requested position - if (uri === fromUri && symbol.location.range.start.line > position.line) { - return - } - - if ( - closestSymbol === null || - symbol.location.range.start.line > - closestSymbol.location.range.start.line - ) { - closestSymbol = symbol - } - }) - - if (closestSymbol) { - symbols.push(closestSymbol) - } - } - }) - } + return this.getAllSymbols({ uri, position }).filter((symbol) => { + if (exactMatch) { + return symbol.name === word + } else { + return symbol.name.startsWith(word) } - - return symbols - }, [] as LSP.SymbolInformation[]) + }) } /** @@ -576,8 +519,14 @@ export default class Analyzer { return null } - public getAllVariableSymbols({ uri }: { uri: string }): LSP.SymbolInformation[] { - return this.getAllSymbols({ uri }).filter( + public getAllVariableSymbols({ + position, + uri, + }: { + position: LSP.Position + uri: string + }): LSP.SymbolInformation[] { + return this.getAllSymbols({ uri, position }).filter( (symbol) => symbol.kind === LSP.SymbolKind.Variable, ) } @@ -613,32 +562,65 @@ export default class Analyzer { }) } - private getAllSymbols({ uri }: { uri?: string } = {}): LSP.SymbolInformation[] { - // FIXME: this should use getLocalDeclarations! - const reachableUris = this.getReachableUris({ uri }) - - const symbols: LSP.SymbolInformation[] = [] - reachableUris.forEach((uri) => { + /** + * Get all declaration symbols (function or variables) from the given file/position + * or from all files in the workspace. + * + * FIXME: rename to getAllDeclarationSymbols + */ + private getAllSymbols({ + uri: fromUri, + position, + }: { uri?: string; position?: LSP.Position } = {}): LSP.SymbolInformation[] { + return this.getReachableUris({ uri: fromUri }).reduce((symbols, uri) => { const analyzedDocument = this.uriToAnalyzedDocument[uri] - if (!analyzedDocument) { - return - } - Object.values(analyzedDocument.globalDeclarations).forEach((symbol) => { - symbols.push(symbol) - }) - }) + if (analyzedDocument) { + if (uri !== fromUri || !position) { + // We use the global declarations for external files or if we do not have a position + const { globalDeclarations } = analyzedDocument + Object.values(globalDeclarations).forEach((symbol) => symbols.push(symbol)) + } + + // For the current file we find declarations based on the current scope + if (uri === fromUri && position) { + const node = analyzedDocument.tree.rootNode?.descendantForPosition({ + row: position.line, + column: position.character, + }) + + const localDeclarations = getLocalDeclarations({ + node, + uri, + }) + + Object.keys(localDeclarations).map((name) => { + const symbolsMatchingWord = localDeclarations[name] + + // Find the latest definition + let closestSymbol: LSP.SymbolInformation | null = null + symbolsMatchingWord.forEach((symbol) => { + // Skip if the symbol is defined in the current file after the requested position + if (uri === fromUri && symbol.location.range.start.line > position.line) { + return + } + + if ( + closestSymbol === null || + symbol.location.range.start.line > closestSymbol.location.range.start.line + ) { + closestSymbol = symbol + } + }) + + if (closestSymbol) { + symbols.push(closestSymbol) + } + }) + } + } - return symbols + return symbols + }, [] as LSP.SymbolInformation[]) } } - -// Other languages where there are multiple re-declaration of variables -// TS -// - go to definition: takes you to the "let x = 1" line even though it is redefined -// - clicking on first definition: shows references also in the other files -// - docs: "function x(): void" or "let x: number" -// Python -// - go to definition: returns multiple definitions -// - clicking on first definition: shows definitions -// - docs: prefixed with "(function) x() -> None" or "(variable) xxx: Literal[1]" diff --git a/server/src/server.ts b/server/src/server.ts index eda50613e..b4a6fcb15 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -474,6 +474,9 @@ export default class BashServer { } private onDocumentSymbol(params: LSP.DocumentSymbolParams): LSP.SymbolInformation[] { + // FIXME: ideally this should return LSP.DocumentSymbol[] instead of LSP.SymbolInformation[] + // which is a hirearchy of symbols. + // https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_documentSymbol this.connection.console.log(`onDocumentSymbol`) return this.analyzer.findSymbolsForFile({ uri: params.textDocument.uri }) } @@ -559,7 +562,10 @@ export default class BashServer { ? [] : this.getCompletionItemsForSymbols({ symbols: shouldCompleteOnVariables - ? this.analyzer.getAllVariableSymbols({ uri: currentUri }) + ? this.analyzer.getAllVariableSymbols({ + uri: currentUri, + position: params.position, + }) : this.analyzer.findSymbolsMatchingWord({ exactMatch: false, uri: currentUri, From 7864d27b5938d22ef0392ac9aa9c2972162ee2de Mon Sep 17 00:00:00 2001 From: skovhus Date: Fri, 30 Dec 2022 19:45:56 +0100 Subject: [PATCH 11/16] Renaming analyzer methods --- .../__snapshots__/analyzer.test.ts.snap | 4 +- server/src/__tests__/analyzer.test.ts | 41 +- server/src/analyser.ts | 392 +++++++++--------- server/src/server.ts | 15 +- server/src/util/declarations.ts | 1 - 5 files changed, 225 insertions(+), 228 deletions(-) diff --git a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap index 07e1b30f5..5c1ecdf74 100644 --- a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap +++ b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap @@ -95,7 +95,7 @@ Array [ ] `; -exports[`findSymbolsForFile issue 101 1`] = ` +exports[`getDeclarationsForUri issue 101 1`] = ` Array [ Object { "kind": 12, @@ -117,7 +117,7 @@ Array [ ] `; -exports[`findSymbolsForFile returns a list of SymbolInformation if uri is found 1`] = ` +exports[`getDeclarationsForUri returns a list of SymbolInformation if uri is found 1`] = ` Array [ Object { "kind": 13, diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 314504149..b083bda9e 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -59,12 +59,12 @@ describe('analyze', () => { }) }) -describe('findDefinition', () => { +describe('findDeclarationLocations', () => { it('returns a location to a file if word is the path in a sourcing statement', () => { const document = FIXTURE_DOCUMENT.SOURCING const { uri } = document analyzer.analyze({ uri, document }) - const result = analyzer.findDefinition({ + const result = analyzer.findDeclarationLocations({ uri, word: './extension.inc', position: { character: 10, line: 2 }, @@ -101,7 +101,7 @@ describe('findDefinition', () => { }) newAnalyzer.analyze({ uri, document }) - const result = newAnalyzer.findDefinition({ + const result = newAnalyzer.findDeclarationLocations({ uri, word: './scripts/tag-release.inc', position: { character: 10, line: 16 }, @@ -127,7 +127,7 @@ describe('findDefinition', () => { it('returns a local reference if definition is found', () => { analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) - const result = analyzer.findDefinition({ + const result = analyzer.findDeclarationLocations({ position: { character: 1, line: 148 }, uri: CURRENT_URI, word: 'node_version', @@ -153,7 +153,7 @@ describe('findDefinition', () => { it('returns local declarations', () => { analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) - const result = analyzer.findDefinition({ + const result = analyzer.findDeclarationLocations({ position: { character: 12, line: 12 }, uri: FIXTURE_URI.SCOPE, word: 'X', @@ -193,23 +193,23 @@ describe('findReferences', () => { }) }) -describe('findSymbolsForFile', () => { +describe('getDeclarationsForUri', () => { it('returns empty list if uri is not found', () => { analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) - const result = analyzer.findSymbolsForFile({ uri: 'foobar.sh' }) + const result = analyzer.getDeclarationsForUri({ uri: 'foobar.sh' }) expect(result).toEqual([]) }) it('returns a list of SymbolInformation if uri is found', () => { analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) - const result = analyzer.findSymbolsForFile({ uri: CURRENT_URI }) + const result = analyzer.getDeclarationsForUri({ uri: CURRENT_URI }) expect(result).not.toEqual([]) expect(result).toMatchSnapshot() }) it('issue 101', () => { analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.ISSUE101 }) - const result = analyzer.findSymbolsForFile({ uri: CURRENT_URI }) + const result = analyzer.getDeclarationsForUri({ uri: CURRENT_URI }) expect(result).not.toEqual([]) expect(result).toMatchSnapshot() }) @@ -317,7 +317,7 @@ describe('commandNameAtPoint', () => { }) }) -describe('findSymbolsMatchingWord', () => { +describe('findDeclarationsMatchingWord', () => { it('returns a list of symbols across the workspace when includeAllWorkspaceSymbols is true', async () => { const parser = await initializeParser() const connection = getMockConnection() @@ -334,7 +334,7 @@ describe('findSymbolsMatchingWord', () => { }) expect( - analyzer.findSymbolsMatchingWord({ + analyzer.findDeclarationsMatchingWord({ word: 'npm_config_logl', uri: FIXTURE_URI.INSTALL, exactMatch: false, @@ -363,7 +363,7 @@ describe('findSymbolsMatchingWord', () => { `) expect( - analyzer.findSymbolsMatchingWord({ + analyzer.findDeclarationsMatchingWord({ word: 'xxxxxxxx', uri: FIXTURE_URI.INSTALL, exactMatch: false, @@ -372,7 +372,7 @@ describe('findSymbolsMatchingWord', () => { ).toMatchInlineSnapshot(`Array []`) expect( - analyzer.findSymbolsMatchingWord({ + analyzer.findDeclarationsMatchingWord({ word: 'BLU', uri: FIXTURE_URI.INSTALL, exactMatch: false, @@ -401,7 +401,7 @@ describe('findSymbolsMatchingWord', () => { `) expect( - analyzer.findSymbolsMatchingWord({ + analyzer.findDeclarationsMatchingWord({ word: 'BLU', uri: FIXTURE_URI.SOURCING, exactMatch: false, @@ -446,7 +446,7 @@ describe('findSymbolsMatchingWord', () => { }) expect( - analyzer.findSymbolsMatchingWord({ + analyzer.findDeclarationsMatchingWord({ word: 'BLU', uri: FIXTURE_URI.INSTALL, exactMatch: false, @@ -455,7 +455,7 @@ describe('findSymbolsMatchingWord', () => { ).toMatchInlineSnapshot(`Array []`) expect( - analyzer.findSymbolsMatchingWord({ + analyzer.findDeclarationsMatchingWord({ word: 'BLU', uri: FIXTURE_URI.SOURCING, exactMatch: false, @@ -496,7 +496,7 @@ describe('findSymbolsMatchingWord', () => { }) const findWordFromLine = (word: string, line: number) => - analyzer.findSymbolsMatchingWord({ + analyzer.findDeclarationsMatchingWord({ word, uri: FIXTURE_URI.SCOPE, exactMatch: true, @@ -745,7 +745,7 @@ describe('initiateBackgroundAnalysis', () => { }) }) -describe('getAllVariableSymbols', () => { +describe('getAllVariables', () => { it('returns all variable symbols', async () => { const document = FIXTURE_DOCUMENT.SOURCING const { uri } = document @@ -762,9 +762,8 @@ describe('getAllVariableSymbols', () => { newAnalyzer.analyze({ uri, document }) - expect( - newAnalyzer.getAllVariableSymbols({ uri, position: { line: 20, character: 0 } }), - ).toMatchInlineSnapshot(` + expect(newAnalyzer.getAllVariables({ uri, position: { line: 20, character: 0 } })) + .toMatchInlineSnapshot(` Array [ Object { "kind": 13, diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 6c0b7771d..f0812d807 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -56,6 +56,57 @@ export default class Analyzer { this.workspaceFolder = workspaceFolder } + /** + * Analyze the given document, cache the tree-sitter AST, and iterate over the + * tree to find declarations. + * + * Returns all, if any, syntax errors that occurred while parsing the file. + * + */ + public analyze({ + document, + uri, // NOTE: we don't use document.uri to make testing easier + }: { + document: TextDocument + uri: string + }): LSP.Diagnostic[] { + const fileContent = document.getText() + + const tree = this.parser.parse(fileContent) + + const { diagnostics, globalDeclarations } = getGlobalDeclarations({ tree, uri }) + + this.uriToAnalyzedDocument[uri] = { + document, + globalDeclarations, + sourcedUris: sourcing.getSourcedUris({ + fileContent, + fileUri: uri, + rootPath: this.workspaceFolder, + tree, + }), + tree, + } + + function findMissingNodes(node: Parser.SyntaxNode) { + if (node.isMissing()) { + diagnostics.push( + LSP.Diagnostic.create( + TreeSitterUtil.range(node), + `Syntax error: expected "${node.type}" somewhere in the file`, + LSP.DiagnosticSeverity.Warning, + ), + ) + } else if (node.hasError()) { + node.children.forEach(findMissingNodes) + } + } + + findMissingNodes(tree.rootNode) + + return diagnostics + } + /** * Initiates a background analysis of the files in the workspaceFolder to * enable features across files. @@ -142,10 +193,9 @@ export default class Analyzer { } /** - * Find all the locations where something has been defined. - * FIXME: rename to findDeclarationLocation or findDefinitionLocation + * Find all the locations where the word was declared. */ - public findDefinition({ + public findDeclarationLocations({ position, uri, word, @@ -170,7 +220,7 @@ export default class Analyzer { } } - return this.findSymbolsMatchingWord({ + return this.findDeclarationsMatchingWord({ exactMatch: true, position, uri, @@ -179,94 +229,57 @@ export default class Analyzer { } /** - * Find all the symbols matching the query using fuzzy search. - * FIXME: rename to searchAllSymbols + * Find all the declaration symbols in the workspace matching the query using fuzzy search. */ - public search(query: string): LSP.SymbolInformation[] { - const searcher = new FuzzySearch(this.getAllSymbols(), ['name'], { + public findDeclarationsWithFuzzySearch(query: string): LSP.SymbolInformation[] { + const searcher = new FuzzySearch(this.getAllDeclarations(), ['name'], { caseSensitive: true, }) return searcher.search(query) } - public async getExplainshellDocumentation({ - params, - endpoint, + /** + * Find declarations for the given word and position. + */ + public findDeclarationsMatchingWord({ + exactMatch, + position, + uri, + word, }: { - params: LSP.TextDocumentPositionParams - endpoint: string - }): Promise<{ helpHTML?: string }> { - const analyzedDocument = this.uriToAnalyzedDocument[params.textDocument.uri] - - const leafNode = analyzedDocument?.tree.rootNode.descendantForPosition({ - row: params.position.line, - column: params.position.character, + exactMatch: boolean + position: LSP.Position + uri: string + word: string + }): LSP.SymbolInformation[] { + return this.getAllDeclarations({ uri, position }).filter((symbol) => { + if (exactMatch) { + return symbol.name === word + } else { + return symbol.name.startsWith(word) + } }) - - if (!leafNode || !analyzedDocument) { - return {} - } - - // explainshell needs the whole command, not just the "word" (tree-sitter - // parlance) that the user hovered over. A relatively successful heuristic - // is to simply go up one level in the AST. If you go up too far, you'll - // start to include newlines, and explainshell completely balks when it - // encounters newlines. - const interestingNode = leafNode.type === 'word' ? leafNode.parent : leafNode - - if (!interestingNode) { - return {} - } - - const cmd = analyzedDocument.document - .getText() - .slice(interestingNode.startIndex, interestingNode.endIndex) - type ExplainshellResponse = { - matches?: Array<{ helpHTML: string; start: number; end: number }> - } - - const url = URI(endpoint).path('/api/explain').addQuery('cmd', cmd).toString() - const explainshellRawResponse = await fetch(url) - const explainshellResponse = - (await explainshellRawResponse.json()) as ExplainshellResponse - - if (!explainshellRawResponse.ok) { - throw new Error(`HTTP request failed: ${url}`) - } else if (!explainshellResponse.matches) { - return {} - } else { - const offsetOfMousePointerInCommand = - analyzedDocument.document.offsetAt(params.position) - interestingNode.startIndex - - const match = explainshellResponse.matches.find( - (helpItem) => - helpItem.start <= offsetOfMousePointerInCommand && - offsetOfMousePointerInCommand < helpItem.end, - ) - - return { helpHTML: match && match.helpHTML } - } } /** - * Find all the locations where something named name has been defined or referenced. + * Find all the locations where the given word was defined or referenced. * * FIXME: take position into account * FIXME: take file into account */ - public findReferences(name: string): LSP.Location[] { + public findReferences(word: string): LSP.Location[] { const uris = Object.keys(this.uriToAnalyzedDocument) - return flattenArray(uris.map((uri) => this.findOccurrences(uri, name))) + return flattenArray(uris.map((uri) => this.findOccurrences(uri, word))) } /** - * Find all occurrences of name in the given file. + * Find all occurrences (references or definitions) of a word in the given file. * It's currently not scope-aware. * * FIXME: should this take the scope into account? I guess it should * as this is used for highlighting. */ - public findOccurrences(uri: string, query: string): LSP.Location[] { + public findOccurrences(uri: string, word: string): LSP.Location[] { const analyzedDocument = this.uriToAnalyzedDocument[uri] if (!analyzedDocument) { return [] @@ -285,7 +298,7 @@ export default class Analyzer { namedNode = n.firstNamedChild } - if (namedNode && namedNode.text === query) { + if (namedNode && namedNode.text === word) { const range = TreeSitterUtil.range(namedNode) const alreadyInLocations = locations.some((loc) => { @@ -301,145 +314,87 @@ export default class Analyzer { return locations } - /** - * Find all symbol definitions in the given file. - * - * FIXME: this should return all symbols, not just global declarations. - * FIXME: convert to DocumentSymbol[] which is a hierarchy of symbols found in a given text document. - */ - public findSymbolsForFile({ uri }: { uri: string }): LSP.SymbolInformation[] { - const declarationsInFile = this.uriToAnalyzedDocument[uri]?.globalDeclarations || {} - return Object.values(declarationsInFile) - } - - /** - * Find declarations/definitions for the given word. - * - * FIXME: rename to findDeclarationsMatchingWord - */ - public findSymbolsMatchingWord({ - exactMatch, + public getAllVariables({ position, uri, - word, }: { - exactMatch: boolean position: LSP.Position uri: string - word: string }): LSP.SymbolInformation[] { - return this.getAllSymbols({ uri, position }).filter((symbol) => { - if (exactMatch) { - return symbol.name === word - } else { - return symbol.name.startsWith(word) - } - }) + return this.getAllDeclarations({ uri, position }).filter( + (symbol) => symbol.kind === LSP.SymbolKind.Variable, + ) } /** - * Analyze the given document, cache the tree-sitter AST, and iterate over the - * tree to find declarations. - * - * Returns all, if any, syntax errors that occurred while parsing the file. + * Get all symbol declarations in the given file. * + * FIXME: this should return all symbols, not just global declarations. + * FIXME: convert to DocumentSymbol[] which is a hierarchy of symbols found in a given text document. */ - public analyze({ - document, - uri, // NOTE: we don't use document.uri to make testing easier - }: { - document: TextDocument - uri: string - }): LSP.Diagnostic[] { - const fileContent = document.getText() - - const tree = this.parser.parse(fileContent) - - const { diagnostics, globalDeclarations } = getGlobalDeclarations({ tree, uri }) - - this.uriToAnalyzedDocument[uri] = { - document, - globalDeclarations, - sourcedUris: sourcing.getSourcedUris({ - fileContent, - fileUri: uri, - rootPath: this.workspaceFolder, - tree, - }), - tree, - } - - function findMissingNodes(node: Parser.SyntaxNode) { - if (node.isMissing()) { - diagnostics.push( - LSP.Diagnostic.create( - TreeSitterUtil.range(node), - `Syntax error: expected "${node.type}" somewhere in the file`, - LSP.DiagnosticSeverity.Warning, - ), - ) - } else if (node.hasError()) { - node.children.forEach(findMissingNodes) - } - } - - findMissingNodes(tree.rootNode) - - return diagnostics + public getDeclarationsForUri({ uri }: { uri: string }): LSP.SymbolInformation[] { + const declarationsInFile = this.uriToAnalyzedDocument[uri]?.globalDeclarations || {} + return Object.values(declarationsInFile) } - public findAllSourcedUris({ uri }: { uri: string }): Set { - const allSourcedUris = new Set([]) - - const addSourcedFilesFromUri = (fromUri: string) => { - const sourcedUris = this.uriToAnalyzedDocument[fromUri]?.sourcedUris + // TODO: move somewhere else than the analyzer... + public async getExplainshellDocumentation({ + params, + endpoint, + }: { + params: LSP.TextDocumentPositionParams + endpoint: string + }): Promise<{ helpHTML?: string }> { + const analyzedDocument = this.uriToAnalyzedDocument[params.textDocument.uri] - if (!sourcedUris) { - return - } + const leafNode = analyzedDocument?.tree.rootNode.descendantForPosition({ + row: params.position.line, + column: params.position.character, + }) - sourcedUris.forEach((sourcedUri) => { - if (!allSourcedUris.has(sourcedUri)) { - allSourcedUris.add(sourcedUri) - addSourcedFilesFromUri(sourcedUri) - } - }) + if (!leafNode || !analyzedDocument) { + return {} } - addSourcedFilesFromUri(uri) - - return allSourcedUris - } + // explainshell needs the whole command, not just the "word" (tree-sitter + // parlance) that the user hovered over. A relatively successful heuristic + // is to simply go up one level in the AST. If you go up too far, you'll + // start to include newlines, and explainshell completely balks when it + // encounters newlines. + const interestingNode = leafNode.type === 'word' ? leafNode.parent : leafNode - /** - * Find the node at the given point. - */ - private nodeAtPoint( - uri: string, - line: number, - column: number, - ): Parser.SyntaxNode | null { - const tree = this.uriToAnalyzedDocument[uri]?.tree + if (!interestingNode) { + return {} + } - if (!tree?.rootNode) { - // Check for lacking rootNode (due to failed parse?) - return null + const cmd = analyzedDocument.document + .getText() + .slice(interestingNode.startIndex, interestingNode.endIndex) + type ExplainshellResponse = { + matches?: Array<{ helpHTML: string; start: number; end: number }> } - return tree.rootNode.descendantForPosition({ row: line, column }) - } + const url = URI(endpoint).path('/api/explain').addQuery('cmd', cmd).toString() + const explainshellRawResponse = await fetch(url) + const explainshellResponse = + (await explainshellRawResponse.json()) as ExplainshellResponse - /** - * Find the full word at the given point. - */ - public wordAtPoint(uri: string, line: number, column: number): string | null { - const node = this.nodeAtPoint(uri, line, column) + if (!explainshellRawResponse.ok) { + throw new Error(`HTTP request failed: ${url}`) + } else if (!explainshellResponse.matches) { + return {} + } else { + const offsetOfMousePointerInCommand = + analyzedDocument.document.offsetAt(params.position) - interestingNode.startIndex - if (!node || node.childCount > 0 || node.text.trim() === '') { - return null - } + const match = explainshellResponse.matches.find( + (helpItem) => + helpItem.start <= offsetOfMousePointerInCommand && + offsetOfMousePointerInCommand < helpItem.end, + ) - return node.text.trim() + return { helpHTML: match && match.helpHTML } + } } /** @@ -519,22 +474,24 @@ export default class Analyzer { return null } - public getAllVariableSymbols({ - position, - uri, - }: { - position: LSP.Position - uri: string - }): LSP.SymbolInformation[] { - return this.getAllSymbols({ uri, position }).filter( - (symbol) => symbol.kind === LSP.SymbolKind.Variable, - ) + /** + * Find the full word at the given point. + */ + public wordAtPoint(uri: string, line: number, column: number): string | null { + const node = this.nodeAtPoint(uri, line, column) + + if (!node || node.childCount > 0 || node.text.trim() === '') { + return null + } + + return node.text.trim() } public setIncludeAllWorkspaceSymbols(includeAllWorkspaceSymbols: boolean): void { this.includeAllWorkspaceSymbols = includeAllWorkspaceSymbols } + // Private methods private getReachableUris({ uri: fromUri }: { uri?: string } = {}): string[] { if (!fromUri || this.includeAllWorkspaceSymbols) { return Object.keys(this.uriToAnalyzedDocument) @@ -566,9 +523,9 @@ export default class Analyzer { * Get all declaration symbols (function or variables) from the given file/position * or from all files in the workspace. * - * FIXME: rename to getAllDeclarationSymbols + * Note that this can return duplicates across the workspace. */ - private getAllSymbols({ + private getAllDeclarations({ uri: fromUri, position, }: { uri?: string; position?: LSP.Position } = {}): LSP.SymbolInformation[] { @@ -601,7 +558,7 @@ export default class Analyzer { let closestSymbol: LSP.SymbolInformation | null = null symbolsMatchingWord.forEach((symbol) => { // Skip if the symbol is defined in the current file after the requested position - if (uri === fromUri && symbol.location.range.start.line > position.line) { + if (symbol.location.range.start.line > position.line) { return } @@ -623,4 +580,45 @@ export default class Analyzer { return symbols }, [] as LSP.SymbolInformation[]) } + + public findAllSourcedUris({ uri }: { uri: string }): Set { + const allSourcedUris = new Set([]) + + const addSourcedFilesFromUri = (fromUri: string) => { + const sourcedUris = this.uriToAnalyzedDocument[fromUri]?.sourcedUris + + if (!sourcedUris) { + return + } + + sourcedUris.forEach((sourcedUri) => { + if (!allSourcedUris.has(sourcedUri)) { + allSourcedUris.add(sourcedUri) + addSourcedFilesFromUri(sourcedUri) + } + }) + } + + addSourcedFilesFromUri(uri) + + return allSourcedUris + } + + /** + * Find the node at the given point. + */ + private nodeAtPoint( + uri: string, + line: number, + column: number, + ): Parser.SyntaxNode | null { + const tree = this.uriToAnalyzedDocument[uri]?.tree + + if (!tree?.rootNode) { + // Check for lacking rootNode (due to failed parse?) + return null + } + + return tree.rootNode.descendantForPosition({ row: line, column }) + } } diff --git a/server/src/server.ts b/server/src/server.ts index b4a6fcb15..5e8654295 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -426,7 +426,7 @@ export default class BashServer { } } - const symbolsMatchingWord = this.analyzer.findSymbolsMatchingWord({ + const symbolsMatchingWord = this.analyzer.findDeclarationsMatchingWord({ exactMatch: true, uri: currentUri, word, @@ -466,7 +466,7 @@ export default class BashServer { if (!word) { return null } - return this.analyzer.findDefinition({ + return this.analyzer.findDeclarationLocations({ position: params.position, uri: params.textDocument.uri, word, @@ -475,15 +475,15 @@ export default class BashServer { private onDocumentSymbol(params: LSP.DocumentSymbolParams): LSP.SymbolInformation[] { // FIXME: ideally this should return LSP.DocumentSymbol[] instead of LSP.SymbolInformation[] - // which is a hirearchy of symbols. + // which is a hierarchy of symbols. // https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_documentSymbol this.connection.console.log(`onDocumentSymbol`) - return this.analyzer.findSymbolsForFile({ uri: params.textDocument.uri }) + return this.analyzer.getDeclarationsForUri({ uri: params.textDocument.uri }) } private onWorkspaceSymbol(params: LSP.WorkspaceSymbolParams): LSP.SymbolInformation[] { this.connection.console.log('onWorkspaceSymbol') - return this.analyzer.search(params.query) + return this.analyzer.findDeclarationsWithFuzzySearch(params.query) } private onDocumentHighlight( @@ -562,11 +562,11 @@ export default class BashServer { ? [] : this.getCompletionItemsForSymbols({ symbols: shouldCompleteOnVariables - ? this.analyzer.getAllVariableSymbols({ + ? this.analyzer.getAllVariables({ uri: currentUri, position: params.position, }) - : this.analyzer.findSymbolsMatchingWord({ + : this.analyzer.findDeclarationsMatchingWord({ exactMatch: false, uri: currentUri, word, @@ -740,6 +740,7 @@ function deduplicateSymbols({ ), ) + // NOTE: it might be that uniqueBasedOnHash is not needed anymore return uniqueBasedOnHash([...symbolsCurrentFile, ...symbolsOtherFiles], getSymbolId) } diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts index bfa2e3c64..cefb0ee99 100644 --- a/server/src/util/declarations.ts +++ b/server/src/util/declarations.ts @@ -132,7 +132,6 @@ export function getLocalDeclarations({ }) } } else if (TreeSitterUtil.isDefinition(childNode)) { - // FIXME: does this also capture local variables? symbol = nodeToSymbolInformation({ node: childNode, uri }) } From 5bb71d4bf53fa31dc60bc65728836536c90a3eb4 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 2 Jan 2023 11:33:12 +0100 Subject: [PATCH 12/16] Correct getDeclarationsForUri --- .../__snapshots__/analyzer.test.ts.snap | 696 +++++++++++++++++- server/src/analyser.ts | 18 +- server/src/server.ts | 2 +- server/src/util/declarations.ts | 67 +- 4 files changed, 741 insertions(+), 42 deletions(-) diff --git a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap index 5c1ecdf74..c838b1df1 100644 --- a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap +++ b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap @@ -114,11 +114,99 @@ Array [ }, "name": "add_a_user", }, + Object { + "containerName": "add_a_user", + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 9, + "line": 5, + }, + "start": Object { + "character": 2, + "line": 5, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "USER", + }, + Object { + "containerName": "add_a_user", + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 13, + "line": 6, + }, + "start": Object { + "character": 2, + "line": 6, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "PASSWORD", + }, + Object { + "containerName": "add_a_user", + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 13, + "line": 9, + }, + "start": Object { + "character": 2, + "line": 9, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "COMMENTS", + }, ] `; exports[`getDeclarationsForUri returns a list of SymbolInformation if uri is found 1`] = ` Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 8, + "line": 21, + }, + "start": Object { + "character": 2, + "line": 21, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 8, + "line": 30, + }, + "start": Object { + "character": 2, + "line": 30, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, Object { "kind": 13, "location": Object { @@ -153,6 +241,23 @@ Array [ }, "name": "npm_config_loglevel", }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 31, + "line": 48, + }, + "start": Object { + "character": 2, + "line": 48, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "npm_config_loglevel", + }, Object { "kind": 13, "location": Object { @@ -176,17 +281,51 @@ Array [ "range": Object { "end": Object { "character": 6, - "line": 265, + "line": 54, }, "start": Object { "character": 0, - "line": 265, + "line": 54, }, }, "uri": "dummy-uri.sh", }, "name": "ret", }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 15, + "line": 69, + }, + "start": Object { + "character": 0, + "line": 69, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "TMP", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 12, + "line": 71, + }, + "start": Object { + "character": 2, + "line": 71, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "TMP", + }, Object { "kind": 13, "location": Object { @@ -221,6 +360,23 @@ Array [ }, "name": "BACK", }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 5, + "line": 83, + }, + "start": Object { + "character": 0, + "line": 83, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, Object { "kind": 13, "location": Object { @@ -238,6 +394,74 @@ Array [ }, "name": "tar", }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 25, + "line": 86, + }, + "start": Object { + "character": 2, + "line": 86, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "tar", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 22, + "line": 89, + }, + "start": Object { + "character": 2, + "line": 89, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "tar", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 8, + "line": 90, + }, + "start": Object { + "character": 2, + "line": 90, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 8, + "line": 97, + }, + "start": Object { + "character": 2, + "line": 97, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, Object { "kind": 13, "location": Object { @@ -260,51 +484,493 @@ Array [ "location": Object { "range": Object { "end": Object { - "character": 37, - "line": 148, + "character": 25, + "line": 119, }, "start": Object { - "character": 0, - "line": 148, + "character": 2, + "line": 119, }, }, "uri": "dummy-uri.sh", }, - "name": "node_version", + "name": "make", }, Object { "kind": 13, "location": Object { "range": Object { "end": Object { - "character": 18, - "line": 158, + "character": 26, + "line": 123, + }, + "start": Object { + "character": 4, + "line": 123, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "make", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 17, + "line": 127, + }, + "start": Object { + "character": 6, + "line": 127, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "make", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 14, + "line": 131, + }, + "start": Object { + "character": 2, + "line": 131, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "make", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 13, + "line": 138, + }, + "start": Object { + "character": 2, + "line": 138, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "make", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 12, + "line": 145, + }, + "start": Object { + "character": 2, + "line": 145, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "clean", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 37, + "line": 148, }, "start": Object { "character": 0, - "line": 158, + "line": 148, }, }, "uri": "dummy-uri.sh", }, - "name": "t", + "name": "node_version", }, Object { "kind": 13, "location": Object { "range": Object { "end": Object { - "character": 25, - "line": 179, + "character": 6, + "line": 149, }, "start": Object { "character": 0, - "line": 177, + "line": 149, }, }, "uri": "dummy-uri.sh", }, - "name": "url", + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 18, + "line": 158, + }, + "start": Object { + "character": 0, + "line": 158, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "t", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 16, + "line": 170, + }, + "start": Object { + "character": 6, + "line": 170, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "t", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 25, + "line": 179, + }, + "start": Object { + "character": 0, + "line": 177, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "url", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 6, + "line": 181, + }, + "start": Object { + "character": 0, + "line": 181, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 9, + "line": 183, + }, + "start": Object { + "character": 2, + "line": 183, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 24, + "line": 187, + }, + "start": Object { + "character": 2, + "line": 185, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "url", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 8, + "line": 188, + }, + "start": Object { + "character": 2, + "line": 188, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 11, + "line": 190, + }, + "start": Object { + "character": 4, + "line": 190, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 65, + "line": 205, + }, + "start": Object { + "character": 6, + "line": 205, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ver", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 15, + "line": 206, + }, + "start": Object { + "character": 6, + "line": 206, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "isnpm10", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 21, + "line": 211, + }, + "start": Object { + "character": 12, + "line": 211, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "isnpm10", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 21, + "line": 215, + }, + "start": Object { + "character": 12, + "line": 215, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "isnpm10", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 11, + "line": 220, + }, + "start": Object { + "character": 6, + "line": 220, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 18, + "line": 225, + }, + "start": Object { + "character": 10, + "line": 225, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "clean", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 15, + "line": 230, + }, + "start": Object { + "character": 10, + "line": 230, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 22, + "line": 232, + }, + "start": Object { + "character": 10, + "line": 232, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "NODE", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 16, + "line": 233, + }, + "start": Object { + "character": 10, + "line": 233, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 22, + "line": 235, + }, + "start": Object { + "character": 10, + "line": 235, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "NODE", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 16, + "line": 236, + }, + "start": Object { + "character": 10, + "line": 236, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 21, + "line": 255, + }, + "start": Object { + "character": 8, + "line": 255, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "make", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 6, + "line": 265, + }, + "start": Object { + "character": 0, + "line": 265, + }, + }, + "uri": "dummy-uri.sh", + }, + "name": "ret", }, ] `; diff --git a/server/src/analyser.ts b/server/src/analyser.ts index f0812d807..7a2c35e86 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -9,6 +9,7 @@ import { TextDocument } from 'vscode-languageserver-textdocument' import * as Parser from 'web-tree-sitter' import { + getAllDeclarationsInTree, getGlobalDeclarations, getLocalDeclarations, GlobalDeclarations, @@ -327,14 +328,18 @@ export default class Analyzer { } /** - * Get all symbol declarations in the given file. + * Get all symbol declarations in the given file. This is used for generating an outline. * - * FIXME: this should return all symbols, not just global declarations. - * FIXME: convert to DocumentSymbol[] which is a hierarchy of symbols found in a given text document. + * TODO: convert to DocumentSymbol[] which is a hierarchy of symbols found in a given text document. */ public getDeclarationsForUri({ uri }: { uri: string }): LSP.SymbolInformation[] { - const declarationsInFile = this.uriToAnalyzedDocument[uri]?.globalDeclarations || {} - return Object.values(declarationsInFile) + const tree = this.uriToAnalyzedDocument[uri]?.tree + + if (!tree?.rootNode) { + return [] + } + + return getAllDeclarationsInTree({ uri, tree }) } // TODO: move somewhere else than the analyzer... @@ -521,7 +526,8 @@ export default class Analyzer { /** * Get all declaration symbols (function or variables) from the given file/position - * or from all files in the workspace. + * or from all files in the workspace. It will take into account the given position + * to filter out irrelevant symbols. * * Note that this can return duplicates across the workspace. */ diff --git a/server/src/server.ts b/server/src/server.ts index 5e8654295..5f7d78c60 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -474,7 +474,7 @@ export default class BashServer { } private onDocumentSymbol(params: LSP.DocumentSymbolParams): LSP.SymbolInformation[] { - // FIXME: ideally this should return LSP.DocumentSymbol[] instead of LSP.SymbolInformation[] + // TODO: ideally this should return LSP.DocumentSymbol[] instead of LSP.SymbolInformation[] // which is a hierarchy of symbols. // https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_documentSymbol this.connection.console.log(`onDocumentSymbol`) diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts index cefb0ee99..e32bc1b5c 100644 --- a/server/src/util/declarations.ts +++ b/server/src/util/declarations.ts @@ -68,36 +68,35 @@ export function getGlobalDeclarations({ return { diagnostics, globalDeclarations } } -function nodeToSymbolInformation({ - node, +/** + * Returns all declarations (functions or variables) from a given tree. + */ +export function getAllDeclarationsInTree({ + tree, uri, }: { - node: Parser.SyntaxNode + tree: Parser.Tree uri: string -}): LSP.SymbolInformation | null { - const named = node.firstNamedChild +}): LSP.SymbolInformation[] { + const symbols: LSP.SymbolInformation[] = [] - if (named === null) { - return null - } + TreeSitterUtil.forEach(tree.rootNode, (node: Parser.SyntaxNode) => { + if (TreeSitterUtil.isDefinition(node)) { + const symbol = nodeToSymbolInformation({ node, uri }) - const containerName = - TreeSitterUtil.findParent(node, (p) => p.type === 'function_definition') - ?.firstNamedChild?.text || '' + if (symbol) { + symbols.push(symbol) + } + } - const kind = TREE_SITTER_TYPE_TO_LSP_KIND[node.type] + return + }) - return LSP.SymbolInformation.create( - named.text, - kind || LSP.SymbolKind.Variable, - TreeSitterUtil.range(node), - uri, - containerName, - ) + return symbols } /** - * Returns declarations available for the given file and location + * Returns declarations available for the given file and location. * Done by traversing the tree upwards (which is a simplification for * actual bash behaviour but deemed good enough, compared to the complexity of flow tracing). * Filters out duplicate definitions. Used when getting declarations for the current scope. @@ -151,3 +150,31 @@ export function getLocalDeclarations({ return declarations } + +function nodeToSymbolInformation({ + node, + uri, +}: { + node: Parser.SyntaxNode + uri: string +}): LSP.SymbolInformation | null { + const named = node.firstNamedChild + + if (named === null) { + return null + } + + const containerName = + TreeSitterUtil.findParent(node, (p) => p.type === 'function_definition') + ?.firstNamedChild?.text || '' + + const kind = TREE_SITTER_TYPE_TO_LSP_KIND[node.type] + + return LSP.SymbolInformation.create( + named.text, + kind || LSP.SymbolKind.Variable, + TreeSitterUtil.range(node), + uri, + containerName, + ) +} From 2effa522254a3cc7eb5d0584e3e6f1e84a232f23 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 2 Jan 2023 11:34:54 +0100 Subject: [PATCH 13/16] Simplify getGlobalDeclarations --- server/src/util/declarations.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts index e32bc1b5c..98c934199 100644 --- a/server/src/util/declarations.ts +++ b/server/src/util/declarations.ts @@ -37,11 +37,7 @@ export function getGlobalDeclarations({ const diagnostics: LSP.Diagnostic[] = [] const globalDeclarations: GlobalDeclarations = {} - TreeSitterUtil.forEach(tree.rootNode, (node: Parser.SyntaxNode) => { - if (node.parent?.type !== 'program') { - return - } - + tree.rootNode.children.forEach((node) => { if (node.type === 'ERROR') { diagnostics.push( LSP.Diagnostic.create( @@ -61,8 +57,6 @@ export function getGlobalDeclarations({ globalDeclarations[word] = symbol } } - - return }) return { diagnostics, globalDeclarations } From 8b457856ea9aece7f42c703343a360383f05cd37 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 2 Jan 2023 12:16:57 +0100 Subject: [PATCH 14/16] Add failing test case https://github.com/bash-lsp/bash-language-server/issues/515 --- server/src/__tests__/analyzer.test.ts | 19 +++++++++++-------- server/src/__tests__/server.test.ts | 26 +++++++++++++------------- server/src/util/declarations.ts | 3 +++ testing/fixtures/scope.sh | 6 ++++++ 4 files changed, 33 insertions(+), 21 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index b083bda9e..9a9fbde43 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -164,11 +164,11 @@ describe('findDeclarationLocations', () => { "range": Object { "end": Object { "character": 17, - "line": 11, + "line": 12, }, "start": Object { "character": 10, - "line": 11, + "line": 12, }, }, "uri": "file://${FIXTURE_FOLDER}scope.sh", @@ -540,11 +540,11 @@ describe('findDeclarationsMatchingWord', () => { "range": Object { "end": Object { "character": 17, - "line": 11, + "line": 12, }, "start": Object { "character": 10, - "line": 11, + "line": 12, }, }, "uri": "file://${FIXTURE_FOLDER}scope.sh", @@ -555,7 +555,7 @@ describe('findDeclarationsMatchingWord', () => { `) // Local function definition - expect(findWordFromLine('f', 20)).toMatchInlineSnapshot(` + expect(findWordFromLine('f', 23)).toMatchInlineSnapshot(` Array [ Object { "containerName": "g", @@ -564,11 +564,11 @@ describe('findDeclarationsMatchingWord', () => { "range": Object { "end": Object { "character": 5, - "line": 18, + "line": 21, }, "start": Object { "character": 4, - "line": 15, + "line": 18, }, }, "uri": "file://${FIXTURE_FOLDER}scope.sh", @@ -609,7 +609,7 @@ describe('findDeclarationsMatchingWord', () => { "range": Object { "end": Object { "character": 1, - "line": 26, + "line": 30, }, "start": Object { "character": 0, @@ -622,6 +622,9 @@ describe('findDeclarationsMatchingWord', () => { }, ] `) + + // FIXME: this should return the a reference to the function + expect(findWordFromLine('GLOBAL_1', 1000)).toHaveLength(1) }) }) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 5670cd972..894491b49 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -274,7 +274,7 @@ describe('server', () => { }, position: { // X - line: 28, + line: 32, character: 8, }, }, @@ -324,11 +324,11 @@ describe('server', () => { "range": Object { "end": Object { "character": 11, - "line": 11, + "line": 12, }, "start": Object { "character": 10, - "line": 11, + "line": 12, }, }, }, @@ -336,11 +336,11 @@ describe('server', () => { "range": Object { "end": Object { "character": 13, - "line": 12, + "line": 15, }, "start": Object { "character": 12, - "line": 12, + "line": 15, }, }, }, @@ -348,11 +348,11 @@ describe('server', () => { "range": Object { "end": Object { "character": 13, - "line": 16, + "line": 19, }, "start": Object { "character": 12, - "line": 16, + "line": 19, }, }, }, @@ -360,11 +360,11 @@ describe('server', () => { "range": Object { "end": Object { "character": 15, - "line": 17, + "line": 20, }, "start": Object { "character": 14, - "line": 17, + "line": 20, }, }, }, @@ -372,11 +372,11 @@ describe('server', () => { "range": Object { "end": Object { "character": 11, - "line": 25, + "line": 29, }, "start": Object { "character": 10, - "line": 25, + "line": 29, }, }, }, @@ -384,11 +384,11 @@ describe('server', () => { "range": Object { "end": Object { "character": 9, - "line": 28, + "line": 32, }, "start": Object { "character": 8, - "line": 28, + "line": 32, }, }, }, diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts index 98c934199..c610fca72 100644 --- a/server/src/util/declarations.ts +++ b/server/src/util/declarations.ts @@ -94,6 +94,9 @@ export function getAllDeclarationsInTree({ * Done by traversing the tree upwards (which is a simplification for * actual bash behaviour but deemed good enough, compared to the complexity of flow tracing). * Filters out duplicate definitions. Used when getting declarations for the current scope. + * + * FIXME: unfortunately this doesn't capture all global variables defined inside functions. + * Wondering if getGlobalDeclarations should return this or we should make that a custom feature of this function. */ export function getLocalDeclarations({ node, diff --git a/testing/fixtures/scope.sh b/testing/fixtures/scope.sh index d9a2b979a..a13da0c0c 100644 --- a/testing/fixtures/scope.sh +++ b/testing/fixtures/scope.sh @@ -7,9 +7,12 @@ X="Mouse" # some function f() ( local X="Dog" + GLOBAL_1="Global 1" g() { local X="Cat" + GLOBAL_1="Global 1" + GLOBAL_2="Global 1" echo "${X}" # another function function @@ -23,8 +26,11 @@ f() ( g + echo "${GLOBAL_1}" echo "${X}" ) echo "${X}" f + +echo "${GLOBAL_2}" From 2d4123979c2892ef4e832c177430a834cdb8c358 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 2 Jan 2023 19:42:30 +0100 Subject: [PATCH 15/16] Add missing global variables --- server/src/__tests__/analyzer.test.ts | 25 +++++++++- server/src/util/declarations.ts | 68 +++++++++++++++++++++++---- 2 files changed, 83 insertions(+), 10 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 9a9fbde43..c804c7302 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -623,8 +623,29 @@ describe('findDeclarationsMatchingWord', () => { ] `) - // FIXME: this should return the a reference to the function - expect(findWordFromLine('GLOBAL_1', 1000)).toHaveLength(1) + // Global variable defined inside a function + expect(findWordFromLine('GLOBAL_1', 1000)).toMatchInlineSnapshot(` + Array [ + Object { + "containerName": "g", + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 23, + "line": 13, + }, + "start": Object { + "character": 4, + "line": 13, + }, + }, + "uri": "file://${FIXTURE_FOLDER}scope.sh", + }, + "name": "GLOBAL_1", + }, + ] + `) }) }) diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts index c610fca72..4ada75fea 100644 --- a/server/src/util/declarations.ts +++ b/server/src/util/declarations.ts @@ -15,7 +15,9 @@ export type Declarations = { [word: string]: LSP.SymbolInformation[] } /** * Returns declarations (functions or variables) from a given root node - * that would be available after sourcing the file. + * 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. * * Will only return one declaration per symbol name – the latest definition. * This behavior is consistent with how Bash behaves, but differs between @@ -64,6 +66,7 @@ export function getGlobalDeclarations({ /** * Returns all declarations (functions or variables) from a given tree. + * This includes local variables. */ export function getAllDeclarationsInTree({ tree, @@ -91,12 +94,10 @@ export function getAllDeclarationsInTree({ /** * Returns declarations available for the given file and location. - * Done by traversing the tree upwards (which is a simplification for - * actual bash behaviour but deemed good enough, compared to the complexity of flow tracing). - * Filters out duplicate definitions. Used when getting declarations for the current scope. + * The heuristics used is a simplification compared to bash behaviour, + * but deemed good enough, compared to the complexity of flow tracing. * - * FIXME: unfortunately this doesn't capture all global variables defined inside functions. - * Wondering if getGlobalDeclarations should return this or we should make that a custom feature of this function. + * Used when getting declarations for the current scope. */ export function getLocalDeclarations({ node, @@ -107,8 +108,7 @@ export function getLocalDeclarations({ }): Declarations { const declarations: Declarations = {} - // bottom up traversal of the tree to capture all local declarations - + // Bottom up traversal to capture all local and scoped declarations const walk = (node: Parser.SyntaxNode | null) => { // NOTE: there is also node.walk if (node) { @@ -145,6 +145,58 @@ export function getLocalDeclarations({ walk(node) + // Top down traversal to add missing global variables from within functions + if (node) { + const rootNode = + 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 + } + }) + } + + return declarations +} + +function getAllGlobalVariableDeclarations({ + uri, + rootNode, +}: { + uri: string + rootNode: Parser.SyntaxNode +}) { + const declarations: Declarations = {} + + TreeSitterUtil.forEach(rootNode, (node: Parser.SyntaxNode) => { + if ( + node.type === 'variable_assignment' && + // exclude local variables + node.parent?.type !== 'declaration_command' + ) { + const symbol = nodeToSymbolInformation({ node, uri }) + if (symbol) { + if (!declarations[symbol.name]) { + declarations[symbol.name] = [] + } + declarations[symbol.name].push(symbol) + } + } + + return + }) + return declarations } From 4b4b2ce02f555e477a361c2f337cf9a9d336fbe0 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 2 Jan 2023 19:47:01 +0100 Subject: [PATCH 16/16] Update test case --- server/src/__tests__/server.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 894491b49..7a33503c1 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -922,6 +922,6 @@ describe('server', () => { ) expect(result).toBeDefined() - expect((result as any)?.contents.value).toContain('ls – list directory contents') + expect((result as any)?.contents.value).toContain('list directory contents') }) })