From aae3bf1e3df32f20cb3de1f518e063b2b86c2d30 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 25 May 2020 16:28:51 +0200 Subject: [PATCH 01/13] Add utilities for supporting sourcing --- server/src/util/__tests__/sourcing.test.ts | 50 ++++++++++ server/src/util/sourcing.ts | 108 +++++++++++++++++++++ 2 files changed, 158 insertions(+) create mode 100644 server/src/util/__tests__/sourcing.test.ts create mode 100644 server/src/util/sourcing.ts diff --git a/server/src/util/__tests__/sourcing.test.ts b/server/src/util/__tests__/sourcing.test.ts new file mode 100644 index 000000000..40ca39ae3 --- /dev/null +++ b/server/src/util/__tests__/sourcing.test.ts @@ -0,0 +1,50 @@ +import { homedir } from 'os' + +import { getSourcedUris } from '../sourcing' + +const fileDirectory = '/Users/bash' +const fileUri = `${fileDirectory}/file.sh` + +describe('getSourcedUris', () => { + it('returns an empty set if no files were sourced', () => { + const result = getSourcedUris({ fileContent: '', fileUri }) + expect(result).toEqual(new Set([])) + }) + + it('returns a set of sourced files', () => { + const result = getSourcedUris({ + fileContent: ` + + source file-in-path.sh # does not contain a slash (i.e. is maybe somewhere on the path) + + source /bin/f.inc + + source ./x a b c # some arguments + + . ./relative/to-this.sh + + source ~/myscript + + # source ... + + source "./my_quoted_file.sh" + + source "$LIBPATH" # dynamic imports not supported + + # conditional is currently not supported + if [[ -z $__COMPLETION_LIB_LOADED ]]; then source "$LIBPATH" ; fi + `, + fileUri, + }) + expect(result).toEqual( + new Set([ + `${fileDirectory}/file-in-path.sh`, // as we don't resolve it, we hope it is here + `${fileDirectory}/bin/f.inc`, + `${fileDirectory}/x`, + `${fileDirectory}/relative/to-this.sh`, + `${homedir()}/myscript`, + `${fileDirectory}/my_quoted_file.sh`, + ]), + ) + }) +}) diff --git a/server/src/util/sourcing.ts b/server/src/util/sourcing.ts new file mode 100644 index 000000000..515b170ce --- /dev/null +++ b/server/src/util/sourcing.ts @@ -0,0 +1,108 @@ +import * as path from 'path' +import * as LSP from 'vscode-languageserver' +import * as Parser from 'web-tree-sitter' + +import { untildify } from './fs' + +// Until the grammar supports sourcing, we use this little regular expression +const SOURCED_FILES_REG_EXP = /^(?:\t|[ ])*(?:source|[.])\s*(\S*)/gm + +export function getSourcedUris({ + fileContent, + fileUri, +}: { + fileContent: string + fileUri: string +}): Set { + const uris: Set = new Set([]) + let match: RegExpExecArray | null + + while ((match = SOURCED_FILES_REG_EXP.exec(fileContent)) !== null) { + const relativePath = match[1] + const sourcedUri = getSourcedUri({ relativePath, uri: fileUri }) + if (sourcedUri) { + uris.add(sourcedUri) + } + } + + return uris +} + +/** + * Investigates if the given position is a path to a sourced file and maps it + * to a location. Useful for jump to definition. + * @returns an optional location + */ +export function getSourcedLocation({ + tree, + position, + uri, + word, +}: { + tree: Parser.Tree + position: { line: number; character: number } + uri: string + word: string +}): LSP.Location | null { + // NOTE: when a word is a file path to a sourced file, we return a location to + // that file. + if (tree.rootNode) { + const node = tree.rootNode.descendantForPosition({ + row: position.line, + column: position.character, + }) + + if (!node || node.text.trim() !== word) { + throw new Error('Implementation error: word was not found at the given position') + } + + const isSourced = node.previousNamedSibling + ? ['.', 'source'].includes(node.previousNamedSibling.text.trim()) + : false + + const sourcedUri = isSourced ? getSourcedUri({ relativePath: word, uri }) : null + + if (sourcedUri) { + return LSP.Location.create(sourcedUri, LSP.Range.create(0, 0, 0, 0)) + } + } + + return null +} + +const mapPathToUri = (path: string): string => path.replace('file:', 'file://') + +const stripQuotes = (path: string): string => { + const first = path[0] + const last = path[path.length - 1] + + if (first === last && [`"`, `'`].includes(first)) { + return path.slice(1, -1) + } + + return path +} + +const getSourcedUri = ({ + relativePath, + uri, +}: { + relativePath: string + uri: string +}): string | null => { + // NOTE: improvements: + // - we could try to resolve the path + // - "If filename does not contain a slash, file names in PATH are used to find + // the directory containing filename." (see https://ss64.com/osx/source.html) + const unquotedRelativePath = stripQuotes(relativePath) + + if (unquotedRelativePath.includes('$')) { + return null + } + + const resultPath = unquotedRelativePath.startsWith('~') + ? untildify(unquotedRelativePath) + : path.join(path.dirname(uri), unquotedRelativePath) + + return mapPathToUri(resultPath) +} From 58964ced450d312d07e70d0d691978b07bf537b2 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 25 May 2020 16:29:17 +0200 Subject: [PATCH 02/13] Update fixtures --- testing/fixtures.ts | 1 + testing/fixtures/extension | 2 ++ testing/fixtures/extension.inc | 2 ++ 3 files changed, 5 insertions(+) diff --git a/testing/fixtures.ts b/testing/fixtures.ts index f1ba38c35..4c73d64e7 100644 --- a/testing/fixtures.ts +++ b/testing/fixtures.ts @@ -19,6 +19,7 @@ export const FIXTURE_URI = { 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')}`, diff --git a/testing/fixtures/extension b/testing/fixtures/extension index 81cf153fe..03312803f 100644 --- a/testing/fixtures/extension +++ b/testing/fixtures/extension @@ -1,3 +1,5 @@ #!/bin/sh +. sourcing.sh + echo "It works, but is not parsed initially" diff --git a/testing/fixtures/extension.inc b/testing/fixtures/extension.inc index 40c13ab44..73b151c1c 100644 --- a/testing/fixtures/extension.inc +++ b/testing/fixtures/extension.inc @@ -1,5 +1,7 @@ #!/bin/sh +source ./issue101.sh + RED=`tput setaf 1` GREEN=`tput setaf 2` BLUE=`tput setaf 4` From d4420c99d6fe584c5a7485965e7e62bd73325e98 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 25 May 2020 16:30:30 +0200 Subject: [PATCH 03/13] Support sourcing files - Only variables found in the sourced files are completed on - Jump to definition works on the source file path (e.g. clicking "source my-file.sh") --- .../__snapshots__/analyzer.test.ts.snap | 18 -- server/src/__tests__/analyzer.test.ts | 200 +++++++++++------- server/src/__tests__/server.test.ts | 80 +++++++ server/src/analyser.ts | 107 ++++++++-- server/src/server.ts | 10 +- 5 files changed, 294 insertions(+), 121 deletions(-) diff --git a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap index 5d2e25cb9..3b3671146 100644 --- a/server/src/__tests__/__snapshots__/analyzer.test.ts.snap +++ b/server/src/__tests__/__snapshots__/analyzer.test.ts.snap @@ -38,24 +38,6 @@ Array [ ] `; -exports[`findDefinition returns a list of locations if parameter is found 1`] = ` -Array [ - Object { - "range": Object { - "end": Object { - "character": 37, - "line": 148, - }, - "start": Object { - "character": 0, - "line": 148, - }, - }, - "uri": "dummy-uri.sh", - }, -] -`; - exports[`findReferences returns a list of locations if parameter is found 1`] = ` Array [ Object { diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 19d45e4c5..9542d9019 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -1,4 +1,4 @@ -import FIXTURES, { FIXTURE_FOLDER } from '../../../testing/fixtures' +import FIXTURES, { FIXTURE_FOLDER, FIXTURE_URI } from '../../../testing/fixtures' import { getMockConnection } from '../../../testing/mocks' import Analyzer from '../analyser' import { getDefaultConfiguration } from '../config' @@ -42,15 +42,60 @@ describe('analyze', () => { describe('findDefinition', () => { it('returns an empty list if word is not found', () => { analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) - const result = analyzer.findDefinition({ word: 'foobar' }) + 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', () => { + analyzer.analyze(CURRENT_URI, FIXTURES.SOURCING) + const result = analyzer.findDefinition({ + uri: CURRENT_URI, + word: './extension.inc', + position: { character: 10, line: 2 }, + }) + expect(result).toMatchInlineSnapshot(` + Array [ + Object { + "range": Object { + "end": Object { + "character": 0, + "line": 0, + }, + "start": Object { + "character": 0, + "line": 0, + }, + }, + "uri": "extension.inc", + }, + ] + `) + }) + it('returns a list of locations if parameter is found', () => { analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) - const result = analyzer.findDefinition({ word: 'node_version' }) + const result = analyzer.findDefinition({ + uri: CURRENT_URI, + word: 'node_version', + }) expect(result).not.toEqual([]) - expect(result).toMatchSnapshot() + expect(result).toMatchInlineSnapshot(` + Array [ + Object { + "range": Object { + "end": Object { + "character": 37, + "line": 148, + }, + "start": Object { + "character": 0, + "line": 148, + }, + }, + "uri": "dummy-uri.sh", + }, + ] + `) }) }) @@ -91,6 +136,52 @@ describe('findSymbolsForFile', () => { }) }) +describe('findAllSourcedUris', () => { + it('returns references to sourced files', async () => { + const parser = await initializeParser() + const connection = getMockConnection() + + const newAnalyzer = new Analyzer({ console: connection.console, parser }) + await newAnalyzer.initiateBackgroundAnalysis({ + backgroundAnalysisMaxFiles: defaultConfig.backgroundAnalysisMaxFiles, + globPattern: defaultConfig.globPattern, + rootPath: FIXTURE_FOLDER, + }) + + const result = newAnalyzer.findAllSourcedUris({ uri: FIXTURE_URI.SOURCING }) + expect(result).toEqual( + new Set([ + `file://${FIXTURE_FOLDER}issue101.sh`, + `file://${FIXTURE_FOLDER}extension.inc`, + ]), + ) + }) + + it('returns references to sourced files without file extension', async () => { + const parser = await initializeParser() + const connection = getMockConnection() + + const newAnalyzer = new Analyzer({ console: connection.console, parser }) + await newAnalyzer.initiateBackgroundAnalysis({ + backgroundAnalysisMaxFiles: defaultConfig.backgroundAnalysisMaxFiles, + globPattern: defaultConfig.globPattern, + rootPath: FIXTURE_FOLDER, + }) + + // Parse the file without extension + newAnalyzer.analyze(FIXTURE_URI.MISSING_EXTENSION, FIXTURES.MISSING_EXTENSION) + + const result = newAnalyzer.findAllSourcedUris({ uri: FIXTURE_URI.MISSING_EXTENSION }) + expect(result).toEqual( + new Set([ + `file://${FIXTURE_FOLDER}extension.inc`, + `file://${FIXTURE_FOLDER}issue101.sh`, + `file://${FIXTURE_FOLDER}sourcing.sh`, + ]), + ) + }) +}) + describe('wordAtPoint', () => { it('returns current word at a given point', () => { analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) @@ -137,92 +228,41 @@ describe('commandNameAtPoint', () => { }) }) -describe('findSymbolCompletions', () => { +describe('findSymbolsMatchingWord', () => { it('return a list of symbols across the workspace', () => { analyzer.analyze('install.sh', FIXTURES.INSTALL) analyzer.analyze('sourcing-sh', FIXTURES.SOURCING) expect( - analyzer.findSymbolsMatchingWord({ word: 'npm_config_logl', exactMatch: false }), - ).toMatchInlineSnapshot(` - Array [ - 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": 27, - "line": 40, - }, - "start": Object { - "character": 0, - "line": 40, - }, - }, - "uri": "install.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": "install.sh", - }, - "name": "npm_config_loglevel", - }, - ] - `) + analyzer.findSymbolsMatchingWord({ + word: 'npm_config_logl', + uri: FIXTURE_URI.INSTALL, + exactMatch: false, + }), + ).toMatchInlineSnapshot(`Array []`) + + expect( + analyzer.findSymbolsMatchingWord({ + word: 'xxxxxxxx', + uri: FIXTURE_URI.INSTALL, + exactMatch: false, + }), + ).toMatchInlineSnapshot(`Array []`) expect( - analyzer.findSymbolsMatchingWord({ word: 'xxxxxxxx', exactMatch: false }), + analyzer.findSymbolsMatchingWord({ + word: 'BLU', + uri: FIXTURE_URI.INSTALL, + exactMatch: false, + }), ).toMatchInlineSnapshot(`Array []`) expect( - analyzer.findSymbolsMatchingWord({ word: 'BLU', exactMatch: false }), + analyzer.findSymbolsMatchingWord({ + word: 'BLU', + uri: FIXTURE_URI.SOURCING, + exactMatch: false, + }), ).toMatchInlineSnapshot(`Array []`) }) }) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 33aef8c4d..2ef4bd9f8 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -511,6 +511,86 @@ describe('server', () => { expect(Array.from(new Set(result.map((item: any) => item.kind)))).toEqual([ LSP.CompletionItemKind.Variable, ]) + expect(result).toMatchInlineSnapshot(` + Array [ + Object { + "data": Object { + "name": "BOLD", + "type": 3, + }, + "documentation": undefined, + "kind": 6, + "label": "BOLD", + }, + Object { + "data": Object { + "name": "RED", + "type": 3, + }, + "documentation": "### Variable: **RED** - *defined in ../extension.inc*", + "kind": 6, + "label": "RED", + }, + Object { + "data": Object { + "name": "GREEN", + "type": 3, + }, + "documentation": "### Variable: **GREEN** - *defined in ../extension.inc*", + "kind": 6, + "label": "GREEN", + }, + Object { + "data": Object { + "name": "BLUE", + "type": 3, + }, + "documentation": "### Variable: **BLUE** - *defined in ../extension.inc*", + "kind": 6, + "label": "BLUE", + }, + Object { + "data": Object { + "name": "RESET", + "type": 3, + }, + "documentation": "### Variable: **RESET** - *defined in ../extension.inc*", + "kind": 6, + "label": "RESET", + }, + Object { + "data": Object { + "name": "USER", + "type": 3, + }, + "documentation": "### Variable: **USER** - *defined in ../issue101.sh*", + "kind": 6, + "label": "USER", + }, + Object { + "data": Object { + "name": "PASSWORD", + "type": 3, + }, + "documentation": "### Variable: **PASSWORD** - *defined in ../issue101.sh*", + "kind": 6, + "label": "PASSWORD", + }, + Object { + "data": Object { + "name": "COMMENTS", + "type": 3, + }, + "documentation": "### Variable: **COMMENTS** - *defined in ../issue101.sh* + + \`\`\`txt + Having shifted twice, the rest is now comments ... + \`\`\`", + "kind": 6, + "label": "COMMENTS", + }, + ] + `) }) it('responds to onCodeAction', async () => { diff --git a/server/src/analyser.ts b/server/src/analyser.ts index f2e9f2098..38d946348 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -11,6 +11,7 @@ import * as Parser from 'web-tree-sitter' import { flattenArray, flattenObjectValues } from './util/flatten' import { getFilePaths } from './util/fs' import { analyzeShebang } from './util/shebang' +import * as sourcing from './util/sourcing' import * as TreeSitterUtil from './util/tree-sitter' const readFileAsync = promisify(fs.readFile) @@ -36,6 +37,9 @@ export default class Analyzer { // We need this to find the word at a given point etc. private uriToFileContent: Texts = {} private uriToDeclarations: FileDeclarations = {} + + private uriToSourcedUris: { [uri: string]: Set } = {} + private treeSitterTypeToLSPKind: Kinds = { // These keys are using underscores as that's the naming convention in tree-sitter. environment_variable_assignment: LSP.SymbolKind.Variable, @@ -133,13 +137,34 @@ export default class Analyzer { /** * Find all the locations where something has been defined. */ - public findDefinition({ word }: { word: string }): LSP.Location[] { - const symbols: LSP.SymbolInformation[] = [] - Object.keys(this.uriToDeclarations).forEach((uri) => { - const declarationNames = this.uriToDeclarations[uri][word] || [] - declarationNames.forEach((d) => symbols.push(d)) - }) - return symbols.map((s) => s.location) + public findDefinition({ + position, + uri, + word, + }: { + position?: { line: number; character: number } + uri: string + word: string + }): LSP.Location[] { + const fileDeclarations = this.getAllFileDeclarations({ uri }) + + const tree = this.uriToTreeSitterTrees[uri] + if (position && tree) { + // NOTE: when a word is a file path to a sourced file, we return a location to + // that file. + const sourcedLocation = sourcing.getSourcedLocation({ position, tree, uri, word }) + if (sourcedLocation) { + return [sourcedLocation] + } + } + + return Object.keys(fileDeclarations) + .reduce((symbols, uri) => { + const declarationNames = fileDeclarations[uri][word] || [] + declarationNames.forEach((d) => symbols.push(d)) + return symbols + }, [] as LSP.SymbolInformation[]) + .map((symbol) => symbol.location) } /** @@ -266,24 +291,26 @@ export default class Analyzer { */ public findSymbolsMatchingWord({ exactMatch, + uri, word, }: { exactMatch: boolean + uri: string word: string }): LSP.SymbolInformation[] { - const symbols: LSP.SymbolInformation[] = [] + const fileDeclarations = this.getAllFileDeclarations({ uri }) - Object.keys(this.uriToDeclarations).forEach((uri) => { - const declarationsInFile = this.uriToDeclarations[uri] || {} + return Object.keys(fileDeclarations).reduce((symbols, uri) => { + const declarationsInFile = fileDeclarations[uri] Object.keys(declarationsInFile).map((name) => { const match = exactMatch ? name === word : name.startsWith(word) if (match) { declarationsInFile[name].forEach((symbol) => symbols.push(symbol)) } }) - }) - return symbols + return symbols + }, [] as LSP.SymbolInformation[]) } /** @@ -302,6 +329,10 @@ export default class Analyzer { this.uriToTreeSitterTrees[uri] = tree this.uriToDeclarations[uri] = {} this.uriToFileContent[uri] = contents + this.uriToSourcedUris[uri] = sourcing.getSourcedUris({ + fileContent: contents, + fileUri: uri, + }) const problems: LSP.Diagnostic[] = [] @@ -369,6 +400,29 @@ export default class Analyzer { return problems } + public findAllSourcedUris({ uri }: { uri: string }): Set { + const allSourcedUris = new Set([]) + + const addSourcedFilesFromUri = (fromUri: string) => { + const sourcedUris = this.uriToSourcedUris[fromUri] + + 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. */ @@ -474,23 +528,34 @@ export default class Analyzer { return null } - public getAllVariableSymbols(): LSP.SymbolInformation[] { - return this.getAllSymbols().filter( + public getAllVariableSymbols({ uri }: { uri: string }): LSP.SymbolInformation[] { + return this.getAllSymbols({ uri }).filter( (symbol) => symbol.kind === LSP.SymbolKind.Variable, ) } - private getAllSymbols(): LSP.SymbolInformation[] { - // NOTE: this could be cached, it takes < 1 ms to generate for a project with 250 bash files... - const symbols: LSP.SymbolInformation[] = [] + private getAllFileDeclarations({ uri }: { uri?: string } = {}): FileDeclarations { + const uris = uri + ? [uri, ...Array.from(this.findAllSourcedUris({ uri }))] + : Object.keys(this.uriToDeclarations) - Object.keys(this.uriToDeclarations).forEach((uri) => { - Object.keys(this.uriToDeclarations[uri]).forEach((name) => { + return uris.reduce((fileDeclarations, uri) => { + fileDeclarations[uri] = this.uriToDeclarations[uri] || {} + return fileDeclarations + }, {} as FileDeclarations) + } + + private getAllSymbols({ uri }: { uri?: string } = {}): LSP.SymbolInformation[] { + const fileDeclarations = this.getAllFileDeclarations({ uri }) + + return Object.keys(fileDeclarations).reduce((symbols, uri) => { + const declarationsInFile = fileDeclarations[uri] + Object.keys(declarationsInFile).forEach((name) => { const declarationNames = this.uriToDeclarations[uri][name] || [] declarationNames.forEach((d) => symbols.push(d)) }) - }) - return symbols + return symbols + }, [] as LSP.SymbolInformation[]) } } diff --git a/server/src/server.ts b/server/src/server.ts index 45f0d113c..79b24a19e 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -428,6 +428,7 @@ export default class BashServer { const symbolDocumentation = deduplicateSymbols({ symbols: this.analyzer.findSymbolsMatchingWord({ exactMatch: true, + uri: currentUri, word, }), currentUri, @@ -452,7 +453,11 @@ export default class BashServer { if (!word) { return null } - return this.analyzer.findDefinition({ word }) + return this.analyzer.findDefinition({ + position: params.position, + uri: params.textDocument.uri, + word, + }) } private onDocumentSymbol(params: LSP.DocumentSymbolParams): LSP.SymbolInformation[] { @@ -541,9 +546,10 @@ export default class BashServer { ? [] : this.getCompletionItemsForSymbols({ symbols: shouldCompleteOnVariables - ? this.analyzer.getAllVariableSymbols() + ? this.analyzer.getAllVariableSymbols({ uri: currentUri }) : this.analyzer.findSymbolsMatchingWord({ exactMatch: false, + uri: currentUri, word, }), currentUri, From 4d49d05b7a42aa5130c841c29af427b87a6bbb52 Mon Sep 17 00:00:00 2001 From: skovhus Date: Mon, 25 May 2020 16:32:56 +0200 Subject: [PATCH 04/13] Add maintenance comment --- server/src/analyser.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 38d946348..6b1829119 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -325,6 +325,8 @@ export default class Analyzer { const tree = this.parser.parse(contents) + // TODO: would be nicer to save one map from uri to object containing all + // these fields. this.uriToTextDocument[uri] = document this.uriToTreeSitterTrees[uri] = tree this.uriToDeclarations[uri] = {} @@ -336,6 +338,7 @@ export default class Analyzer { const problems: LSP.Diagnostic[] = [] + // TODO: move this somewhere TreeSitterUtil.forEach(tree.rootNode, (n: Parser.SyntaxNode) => { if (n.type === 'ERROR') { problems.push( From a9b8c71c4f6b60fa6b865ac5d2f0a48290a03b94 Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 28 May 2020 09:53:35 +0200 Subject: [PATCH 05/13] Make completion based on imports configurable --- server/src/__tests__/analyzer.test.ts | 150 +++++++++++++++++++++++++- server/src/__tests__/config.test.ts | 2 + server/src/analyser.ts | 18 +++- server/src/config.ts | 35 +++--- server/src/server.ts | 2 + vscode-client/package.json | 5 + 6 files changed, 186 insertions(+), 26 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 9542d9019..bd8cfc330 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -229,9 +229,20 @@ describe('commandNameAtPoint', () => { }) describe('findSymbolsMatchingWord', () => { - it('return a list of symbols across the workspace', () => { - analyzer.analyze('install.sh', FIXTURES.INSTALL) - analyzer.analyze('sourcing-sh', FIXTURES.SOURCING) + it('return a list of symbols across the workspace when isSourcingAware is false', async () => { + const parser = await initializeParser() + const connection = getMockConnection() + + const analyzer = new Analyzer({ + console: connection.console, + parser, + isSourcingAware: false, + }) + await analyzer.initiateBackgroundAnalysis({ + backgroundAnalysisMaxFiles: defaultConfig.backgroundAnalysisMaxFiles, + globPattern: defaultConfig.globPattern, + rootPath: FIXTURE_FOLDER, + }) expect( analyzer.findSymbolsMatchingWord({ @@ -239,7 +250,44 @@ describe('findSymbolsMatchingWord', () => { uri: FIXTURE_URI.INSTALL, exactMatch: false, }), - ).toMatchInlineSnapshot(`Array []`) + ).toMatchInlineSnapshot(` + Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 27, + "line": 40, + }, + "start": Object { + "character": 0, + "line": 40, + }, + }, + "uri": "file://${FIXTURE_FOLDER}install.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": "file://${FIXTURE_FOLDER}install.sh", + }, + "name": "npm_config_loglevel", + }, + ] + `) expect( analyzer.findSymbolsMatchingWord({ @@ -255,7 +303,27 @@ describe('findSymbolsMatchingWord', () => { uri: FIXTURE_URI.INSTALL, exactMatch: false, }), - ).toMatchInlineSnapshot(`Array []`) + ).toMatchInlineSnapshot(` +Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 19, + "line": 6, + }, + "start": Object { + "character": 0, + "line": 6, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "name": "BLUE", + }, +] +`) expect( analyzer.findSymbolsMatchingWord({ @@ -263,7 +331,79 @@ describe('findSymbolsMatchingWord', () => { uri: FIXTURE_URI.SOURCING, exactMatch: false, }), + ).toMatchInlineSnapshot(` +Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 19, + "line": 6, + }, + "start": Object { + "character": 0, + "line": 6, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "name": "BLUE", + }, +] +`) + }) + + it('return a list of symbols accessible to the uri when isSourcingAware is true', async () => { + const parser = await initializeParser() + const connection = getMockConnection() + + const analyzer = new Analyzer({ + console: connection.console, + parser, + isSourcingAware: true, + }) + await analyzer.initiateBackgroundAnalysis({ + backgroundAnalysisMaxFiles: defaultConfig.backgroundAnalysisMaxFiles, + globPattern: defaultConfig.globPattern, + rootPath: FIXTURE_FOLDER, + }) + + expect( + analyzer.findSymbolsMatchingWord({ + word: 'BLU', + uri: FIXTURE_URI.INSTALL, + exactMatch: false, + }), ).toMatchInlineSnapshot(`Array []`) + + expect( + analyzer.findSymbolsMatchingWord({ + word: 'BLU', + uri: FIXTURE_URI.SOURCING, + exactMatch: false, + }), + ).toMatchInlineSnapshot(` +Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 19, + "line": 6, + }, + "start": Object { + "character": 0, + "line": 6, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "name": "BLUE", + }, +] +`) }) }) diff --git a/server/src/__tests__/config.test.ts b/server/src/__tests__/config.test.ts index 938a853a8..fffc86617 100644 --- a/server/src/__tests__/config.test.ts +++ b/server/src/__tests__/config.test.ts @@ -5,6 +5,7 @@ describe('ConfigSchema', () => { expect(ConfigSchema.parse({})).toMatchInlineSnapshot(` Object { "backgroundAnalysisMaxFiles": 500, + "completionBasedOnImports": true, "explainshellEndpoint": "", "globPattern": "**/*@(.sh|.inc|.bash|.command)", "highlightParsingErrors": false, @@ -24,6 +25,7 @@ describe('ConfigSchema', () => { ).toMatchInlineSnapshot(` Object { "backgroundAnalysisMaxFiles": 1, + "completionBasedOnImports": true, "explainshellEndpoint": "localhost:8080", "globPattern": "**/*@(.sh)", "highlightParsingErrors": true, diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 6b1829119..7d7d83d7b 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -47,15 +47,20 @@ export default class Analyzer { variable_assignment: LSP.SymbolKind.Variable, } + private isSourcingAware: boolean + public constructor({ console, + isSourcingAware = true, parser, }: { console: LSP.RemoteConsole + isSourcingAware?: boolean parser: Parser }) { - this.parser = parser this.console = console + this.isSourcingAware = isSourcingAware + this.parser = parser } /** @@ -537,10 +542,15 @@ export default class Analyzer { ) } + public setIsSourcingAware(isSourcingAware: boolean): void { + this.isSourcingAware = isSourcingAware + } + private getAllFileDeclarations({ uri }: { uri?: string } = {}): FileDeclarations { - const uris = uri - ? [uri, ...Array.from(this.findAllSourcedUris({ uri }))] - : Object.keys(this.uriToDeclarations) + const uris = + uri && this.isSourcingAware + ? [uri, ...Array.from(this.findAllSourcedUris({ uri }))] + : Object.keys(this.uriToDeclarations) return uris.reduce((fileDeclarations, uri) => { fileDeclarations[uri] = this.uriToDeclarations[uri] || {} diff --git a/server/src/config.ts b/server/src/config.ts index 9e9beadee..94bd9dc36 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -2,18 +2,22 @@ import { z } from 'zod' export const ConfigSchema = z .object({ + // Controls if completions are based only on analyzing the import/sourcing of files. + // If false, completion will be based on all files in the workspace. + completionBasedOnImports: z.boolean().default(true), + + // Maximum number of files to analyze in the background. Set to 0 to disable background analysis. + backgroundAnalysisMaxFiles: z.number().int().min(0).default(500), + // Glob pattern for finding and parsing shell script files in the workspace. Used by the background analysis features across files. globPattern: z.string().trim().default('**/*@(.sh|.inc|.bash|.command)'), - // Controls if Treesitter parsing errors will be highlighted as problems. - highlightParsingErrors: z.boolean().default(false), - // Configure explainshell server endpoint in order to get hover documentation on flags and options. // And empty string will disable the feature. explainshellEndpoint: z.string().trim().default(''), - // Controls the executable used for ShellCheck linting information. An empty string will disable linting. - shellcheckPath: z.string().trim().default('shellcheck'), + // Controls if Treesitter parsing errors will be highlighted as problems. + highlightParsingErrors: z.boolean().default(false), // Additional ShellCheck arguments. Note that we already add the following arguments: --shell, --format, --external-sources." shellcheckArguments: z @@ -31,8 +35,8 @@ export const ConfigSchema = z }, z.array(z.string())) .default([]), - // Maximum number of files to analyze in the background. Set to 0 to disable background analysis. - backgroundAnalysisMaxFiles: z.number().int().min(0).default(500), + // Controls the executable used for ShellCheck linting information. An empty string will disable linting. + shellcheckPath: z.string().trim().default('shellcheck'), }) .strict() @@ -42,18 +46,14 @@ export function getConfigFromEnvironmentVariables(): { config: z.infer environmentVariablesUsed: string[] } { - const { HIGHLIGHT_PARSING_ERRORS } = process.env - const rawConfig = { - globPattern: process.env.GLOB_PATTERN, - highlightParsingErrors: - typeof HIGHLIGHT_PARSING_ERRORS !== 'undefined' - ? toBoolean(HIGHLIGHT_PARSING_ERRORS) - : undefined, + backgroundAnalysisMaxFiles: process.env.BACKGROUND_ANALYSIS_MAX_FILES, + completionBasedOnImports: toBoolean(process.env.COMPLETION_BASED_ON_IMPORTS), explainshellEndpoint: process.env.EXPLAINSHELL_ENDPOINT, - shellcheckPath: process.env.SHELLCHECK_PATH, + globPattern: process.env.GLOB_PATTERN, + highlightParsingErrors: toBoolean(process.env.HIGHLIGHT_PARSING_ERRORS), shellcheckArguments: process.env.SHELLCHECK_ARGUMENTS, - backgroundAnalysisMaxFiles: process.env.BACKGROUND_ANALYSIS_MAX_FILES, + shellcheckPath: process.env.SHELLCHECK_PATH, } const environmentVariablesUsed = Object.entries(rawConfig) @@ -69,4 +69,5 @@ export function getDefaultConfiguration(): z.infer { return ConfigSchema.parse({}) } -const toBoolean = (s: string): boolean => s === 'true' || s === '1' +const toBoolean = (s?: string): boolean | undefined => + typeof s !== 'undefined' ? s === 'true' || s === '1' : undefined diff --git a/server/src/server.ts b/server/src/server.ts index 79b24a19e..0435951a3 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -224,6 +224,8 @@ export default class BashServer { }) } + this.analyzer.setIsSourcingAware(this.config.completionBasedOnImports) + return true } } catch (err) { diff --git a/vscode-client/package.json b/vscode-client/package.json index c2b3bd7f3..88c967f8a 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -37,6 +37,11 @@ "description": "Maximum number of files to analyze in the background. Set to 0 to disable background analysis.", "minimum": 0 }, + "bashIde.completionBasedOnImports": { + "type": "boolean", + "default": true, + "description": "Controls if symbols completion across files is based on parsing sourcing statements (i.e. 'source file.sh' or '. file.sh'). If false then all symbols found in the workspace will be used." + }, "bashIde.explainshellEndpoint": { "type": "string", "default": "", From 96678565e6682a766ad56b8946d093ab53683fd9 Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 7 Dec 2022 15:25:18 +0100 Subject: [PATCH 06/13] Resolve sourced paths --- server/src/__tests__/analyzer.test.ts | 52 ++++++++++------- server/src/analyser.ts | 19 ++++-- server/src/server.ts | 2 +- server/src/util/__tests__/sourcing.test.ts | 37 +++++++----- server/src/util/sourcing.ts | 68 ++++++++++++++++------ 5 files changed, 120 insertions(+), 58 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index bd8cfc330..e77ec8764 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -22,18 +22,21 @@ beforeAll(async () => { describe('analyze', () => { it('returns an empty list of errors for a file with no parsing errors', () => { - const result = analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) + const result = analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) expect(result).toEqual([]) }) it('returns a list of errors for a file with a missing node', () => { - const result = analyzer.analyze(CURRENT_URI, FIXTURES.MISSING_NODE) + const result = analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.MISSING_NODE }) expect(result).not.toEqual([]) expect(result).toMatchSnapshot() }) it('returns a list of errors for a file with parsing errors', () => { - const result = analyzer.analyze(CURRENT_URI, FIXTURES.PARSE_PROBLEMS) + const result = analyzer.analyze({ + uri: CURRENT_URI, + document: FIXTURES.PARSE_PROBLEMS, + }) expect(result).not.toEqual([]) expect(result).toMatchSnapshot() }) @@ -41,15 +44,17 @@ describe('analyze', () => { describe('findDefinition', () => { it('returns an empty list if word is not found', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.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', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.SOURCING) + const document = FIXTURES.SOURCING + const { uri } = document + analyzer.analyze({ uri, document }) const result = analyzer.findDefinition({ - uri: CURRENT_URI, + uri, word: './extension.inc', position: { character: 10, line: 2 }, }) @@ -66,14 +71,14 @@ describe('findDefinition', () => { "line": 0, }, }, - "uri": "extension.inc", + "uri": "file://${FIXTURE_FOLDER}extension.inc", }, ] `) }) it('returns a list of locations if parameter is found', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) const result = analyzer.findDefinition({ uri: CURRENT_URI, word: 'node_version', @@ -101,13 +106,13 @@ describe('findDefinition', () => { describe('findReferences', () => { it('returns empty list if parameter is not found', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) const result = analyzer.findReferences('foobar') expect(result).toEqual([]) }) it('returns a list of locations if parameter is found', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) const result = analyzer.findReferences('node_version') expect(result).not.toEqual([]) expect(result).toMatchSnapshot() @@ -116,20 +121,20 @@ describe('findReferences', () => { describe('findSymbolsForFile', () => { it('returns empty list if uri is not found', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) const result = analyzer.findSymbolsForFile({ uri: 'foobar.sh' }) expect(result).toEqual([]) }) it('returns a list of SymbolInformation if uri is found', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) const result = analyzer.findSymbolsForFile({ uri: CURRENT_URI }) expect(result).not.toEqual([]) expect(result).toMatchSnapshot() }) it('issue 101', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.ISSUE101) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.ISSUE101 }) const result = analyzer.findSymbolsForFile({ uri: CURRENT_URI }) expect(result).not.toEqual([]) expect(result).toMatchSnapshot() @@ -169,7 +174,10 @@ describe('findAllSourcedUris', () => { }) // Parse the file without extension - newAnalyzer.analyze(FIXTURE_URI.MISSING_EXTENSION, FIXTURES.MISSING_EXTENSION) + newAnalyzer.analyze({ + uri: FIXTURE_URI.MISSING_EXTENSION, + document: FIXTURES.MISSING_EXTENSION, + }) const result = newAnalyzer.findAllSourcedUris({ uri: FIXTURE_URI.MISSING_EXTENSION }) expect(result).toEqual( @@ -184,7 +192,7 @@ describe('findAllSourcedUris', () => { describe('wordAtPoint', () => { it('returns current word at a given point', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) expect(analyzer.wordAtPoint(CURRENT_URI, 25, 0)).toEqual(null) expect(analyzer.wordAtPoint(CURRENT_URI, 25, 1)).toEqual(null) expect(analyzer.wordAtPoint(CURRENT_URI, 25, 2)).toEqual(null) @@ -212,7 +220,7 @@ describe('wordAtPoint', () => { describe('commandNameAtPoint', () => { it('returns current command name at a given point', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.INSTALL) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) expect(analyzer.commandNameAtPoint(CURRENT_URI, 15, 0)).toEqual(null) expect(analyzer.commandNameAtPoint(CURRENT_URI, 20, 2)).toEqual('curl') @@ -409,40 +417,40 @@ Array [ describe('commentsAbove', () => { it('returns a string of a comment block above a line', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.COMMENT_DOC) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 22)).toEqual( '```txt\ndoc for func_one\n```', ) }) it('handles line breaks in comments', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.COMMENT_DOC) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 28)).toEqual( '```txt\ndoc for func_two\nhas two lines\n```', ) }) it('only returns connected comments', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.COMMENT_DOC) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 36)).toEqual( '```txt\ndoc for func_three\n```', ) }) it('returns null if no comment found', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.COMMENT_DOC) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 45)).toEqual(null) }) it('works for variables', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.COMMENT_DOC) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 42)).toEqual( '```txt\nworks for variables\n```', ) }) it('returns connected comments with empty comment line', () => { - analyzer.analyze(CURRENT_URI, FIXTURES.COMMENT_DOC) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 51)).toEqual( '```txt\nthis is also included\n\ndoc for func_four\n```', ) diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 7d7d83d7b..5dcd2e247 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -37,9 +37,7 @@ export default class Analyzer { // We need this to find the word at a given point etc. private uriToFileContent: Texts = {} private uriToDeclarations: FileDeclarations = {} - private uriToSourcedUris: { [uri: string]: Set } = {} - private treeSitterTypeToLSPKind: Kinds = { // These keys are using underscores as that's the naming convention in tree-sitter. environment_variable_assignment: LSP.SymbolKind.Variable, @@ -124,7 +122,11 @@ export default class Analyzer { continue } - this.analyze(uri, TextDocument.create(uri, 'shell', 1, fileContent)) + this.analyze({ + document: TextDocument.create(uri, 'shell', 1, fileContent), + rootPath, + uri, + }) } catch (error) { const errorMessage = error instanceof Error ? error.message : error this.console.warn( @@ -325,7 +327,15 @@ export default class Analyzer { * Returns all, if any, syntax errors that occurred while parsing the file. * */ - public analyze(uri: string, document: TextDocument): LSP.Diagnostic[] { + public analyze({ + document, + rootPath, + uri, // NOTE: we don't use document.uri to make testing easier + }: { + document: TextDocument + rootPath?: string + uri: string + }): LSP.Diagnostic[] { const contents = document.getText() const tree = this.parser.parse(contents) @@ -339,6 +349,7 @@ export default class Analyzer { this.uriToSourcedUris[uri] = sourcing.getSourcedUris({ fileContent: contents, fileUri: uri, + rootPath, }) const problems: LSP.Diagnostic[] = [] diff --git a/server/src/server.ts b/server/src/server.ts index 0435951a3..6bb2e5343 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -245,7 +245,7 @@ export default class BashServer { let diagnostics: LSP.Diagnostic[] = [] // Load the tree for the modified contents into the analyzer: - const analyzeDiagnostics = this.analyzer.analyze(uri, document) + const analyzeDiagnostics = this.analyzer.analyze({ uri, document }) // Treesitter's diagnostics can be a bit inaccurate, so we only merge the // analyzer's diagnostics if the setting is enabled: if (this.config.highlightParsingErrors) { diff --git a/server/src/util/__tests__/sourcing.test.ts b/server/src/util/__tests__/sourcing.test.ts index 40ca39ae3..58d753e9e 100644 --- a/server/src/util/__tests__/sourcing.test.ts +++ b/server/src/util/__tests__/sourcing.test.ts @@ -1,10 +1,18 @@ -import { homedir } from 'os' +import * as os from 'os' import { getSourcedUris } from '../sourcing' const fileDirectory = '/Users/bash' const fileUri = `${fileDirectory}/file.sh` +// mock fs.existsSync to always return true +jest.mock('fs', () => ({ + existsSync: () => true, +})) + +// mock os.homedir() to return a fixed path +jest.spyOn(os, 'homedir').mockImplementation(() => '/Users/bash-user') + describe('getSourcedUris', () => { it('returns an empty set if no files were sourced', () => { const result = getSourcedUris({ fileContent: '', fileUri }) @@ -17,17 +25,17 @@ describe('getSourcedUris', () => { source file-in-path.sh # does not contain a slash (i.e. is maybe somewhere on the path) - source /bin/f.inc + source /bin/extension.inc # absolute path source ./x a b c # some arguments - . ./relative/to-this.sh + . ../scripts/release-client.sh source ~/myscript # source ... - source "./my_quoted_file.sh" + source "./issue206.sh" # quoted file in fixtures folder source "$LIBPATH" # dynamic imports not supported @@ -36,15 +44,16 @@ describe('getSourcedUris', () => { `, fileUri, }) - expect(result).toEqual( - new Set([ - `${fileDirectory}/file-in-path.sh`, // as we don't resolve it, we hope it is here - `${fileDirectory}/bin/f.inc`, - `${fileDirectory}/x`, - `${fileDirectory}/relative/to-this.sh`, - `${homedir()}/myscript`, - `${fileDirectory}/my_quoted_file.sh`, - ]), - ) + + expect(result).toMatchInlineSnapshot(` + Set { + "file:///Users/bash/file-in-path.sh", + "file:///bin/extension.inc", + "file:///Users/bash/x", + "file:///Users/scripts/release-client.sh", + "file:///Users/bash-user/myscript", + "file:///Users/bash/issue206.sh", + } + `) }) }) diff --git a/server/src/util/sourcing.ts b/server/src/util/sourcing.ts index 515b170ce..94c7d63db 100644 --- a/server/src/util/sourcing.ts +++ b/server/src/util/sourcing.ts @@ -1,3 +1,4 @@ +import * as fs from 'fs' import * as path from 'path' import * as LSP from 'vscode-languageserver' import * as Parser from 'web-tree-sitter' @@ -7,19 +8,28 @@ import { untildify } from './fs' // Until the grammar supports sourcing, we use this little regular expression const SOURCED_FILES_REG_EXP = /^(?:\t|[ ])*(?:source|[.])\s*(\S*)/gm +/** + * Analysis the given file content and returns a set of URIs that are + * sourced. Note that the URIs are not resolved. + * + * FIXME: should this be URIs or paths? + */ export function getSourcedUris({ fileContent, fileUri, + rootPath, }: { fileContent: string fileUri: string + rootPath?: string }): Set { const uris: Set = new Set([]) let match: RegExpExecArray | null + const rootPaths = [path.dirname(fileUri), rootPath].filter(Boolean) as string[] while ((match = SOURCED_FILES_REG_EXP.exec(fileContent)) !== null) { - const relativePath = match[1] - const sourcedUri = getSourcedUri({ relativePath, uri: fileUri }) + const word = match[1] + const sourcedUri = getSourcedUri({ rootPaths, word }) if (sourcedUri) { uris.add(sourcedUri) } @@ -43,6 +53,7 @@ export function getSourcedLocation({ position: { line: number; character: number } uri: string word: string + // FIXME: take in a rootPath }): LSP.Location | null { // NOTE: when a word is a file path to a sourced file, we return a location to // that file. @@ -60,7 +71,9 @@ export function getSourcedLocation({ ? ['.', 'source'].includes(node.previousNamedSibling.text.trim()) : false - const sourcedUri = isSourced ? getSourcedUri({ relativePath: word, uri }) : null + const sourcedUri = isSourced + ? getSourcedUri({ word, rootPaths: [path.dirname(uri)] }) + : null if (sourcedUri) { return LSP.Location.create(sourcedUri, LSP.Range.create(0, 0, 0, 0)) @@ -70,8 +83,6 @@ export function getSourcedLocation({ return null } -const mapPathToUri = (path: string): string => path.replace('file:', 'file://') - const stripQuotes = (path: string): string => { const first = path[0] const last = path[path.length - 1] @@ -83,26 +94,49 @@ const stripQuotes = (path: string): string => { return path } -const getSourcedUri = ({ - relativePath, - uri, +/** + * Tries to parse the given path and returns a URI if possible. + * - Filters out dynamic sources + * - Converts a relative paths to absolute paths + */ +function getSourcedUri({ + rootPaths, + word, }: { - relativePath: string - uri: string -}): string | null => { + rootPaths: string[] + word: string +}): string | null { // NOTE: improvements: // - we could try to resolve the path // - "If filename does not contain a slash, file names in PATH are used to find // the directory containing filename." (see https://ss64.com/osx/source.html) - const unquotedRelativePath = stripQuotes(relativePath) + let unquotedPath = stripQuotes(word) - if (unquotedRelativePath.includes('$')) { + if (unquotedPath.includes('$')) { + // NOTE: we don't support dynamic sourcing return null } - const resultPath = unquotedRelativePath.startsWith('~') - ? untildify(unquotedRelativePath) - : path.join(path.dirname(uri), unquotedRelativePath) + if (unquotedPath.startsWith('~')) { + unquotedPath = untildify(unquotedPath) + } - return mapPathToUri(resultPath) + if (unquotedPath.startsWith('/')) { + if (fs.existsSync(unquotedPath)) { + return `file://${unquotedPath}` + } + return null + } + + // resolve relative path + for (const rootPath of rootPaths) { + const potentialPath = path.join(rootPath.replace('file://', ''), unquotedPath) + + // check if path is a file + if (fs.existsSync(potentialPath)) { + return `file://${potentialPath}` + } + } + + return null } From e87ef232e9f7f2fb7707a85761af8a51eb9aa05f Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 8 Dec 2022 15:32:03 +0100 Subject: [PATCH 07/13] Handle resolving based on root paths --- server/src/__tests__/analyzer.test.ts | 396 ++++++++++++++++----- server/src/__tests__/server.test.ts | 73 ++++ server/src/analyser.ts | 70 ++-- server/src/server.ts | 13 +- server/src/util/__tests__/sourcing.test.ts | 3 +- server/src/util/sourcing.ts | 17 +- testing/fixtures/sourcing.sh | 4 + 7 files changed, 447 insertions(+), 129 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index e77ec8764..e539a8bbf 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -1,4 +1,6 @@ -import FIXTURES, { FIXTURE_FOLDER, FIXTURE_URI } from '../../../testing/fixtures' +import * as path from 'path' + +import { FIXTURE_DOCUMENT, FIXTURE_FOLDER, FIXTURE_URI } from '../../../testing/fixtures' import { getMockConnection } from '../../../testing/mocks' import Analyzer from '../analyser' import { getDefaultConfiguration } from '../config' @@ -8,6 +10,7 @@ import * as fsUtil from '../util/fs' let analyzer: Analyzer const CURRENT_URI = 'dummy-uri.sh' +const REPO_ROOT_FOLDER = path.resolve(path.join(FIXTURE_FOLDER, '../..')) const mockConsole = getMockConnection().console // if you add a .sh file to testing/fixtures, update this value @@ -17,17 +20,27 @@ const defaultConfig = getDefaultConfiguration() beforeAll(async () => { const parser = await initializeParser() - analyzer = new Analyzer({ console: mockConsole, parser }) + analyzer = new Analyzer({ + console: mockConsole, + parser, + workspaceFolder: FIXTURE_FOLDER, + }) }) describe('analyze', () => { it('returns an empty list of errors for a file with no parsing errors', () => { - const result = analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) + const result = analyzer.analyze({ + uri: CURRENT_URI, + document: FIXTURE_DOCUMENT.INSTALL, + }) expect(result).toEqual([]) }) it('returns a list of errors for a file with a missing node', () => { - const result = analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.MISSING_NODE }) + const result = analyzer.analyze({ + uri: CURRENT_URI, + document: FIXTURE_DOCUMENT.MISSING_NODE, + }) expect(result).not.toEqual([]) expect(result).toMatchSnapshot() }) @@ -35,7 +48,7 @@ describe('analyze', () => { it('returns a list of errors for a file with parsing errors', () => { const result = analyzer.analyze({ uri: CURRENT_URI, - document: FIXTURES.PARSE_PROBLEMS, + document: FIXTURE_DOCUMENT.PARSE_PROBLEMS, }) expect(result).not.toEqual([]) expect(result).toMatchSnapshot() @@ -44,13 +57,13 @@ describe('analyze', () => { describe('findDefinition', () => { it('returns an empty list if word is not found', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) + 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 = FIXTURES.SOURCING + const document = FIXTURE_DOCUMENT.SOURCING const { uri } = document analyzer.analyze({ uri, document }) const result = analyzer.findDefinition({ @@ -77,8 +90,45 @@ describe('findDefinition', () => { `) }) + it('returns a location to a file if word is the path in a sourcing statement (resolved by using the workspacePath)', async () => { + const document = FIXTURE_DOCUMENT.SOURCING + const { uri } = document + + const parser = await initializeParser() + const connection = getMockConnection() + const newAnalyzer = new Analyzer({ + console: connection.console, + parser, + workspaceFolder: REPO_ROOT_FOLDER, + }) + + newAnalyzer.analyze({ uri, document }) + const result = newAnalyzer.findDefinition({ + uri, + word: './scripts/tag-release.inc', + position: { character: 10, line: 16 }, + }) + expect(result).toMatchInlineSnapshot(` + Array [ + Object { + "range": Object { + "end": Object { + "character": 0, + "line": 0, + }, + "start": Object { + "character": 0, + "line": 0, + }, + }, + "uri": "file://${REPO_ROOT_FOLDER}/scripts/tag-release.inc", + }, + ] + `) + }) + it('returns a list of locations if parameter is found', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) const result = analyzer.findDefinition({ uri: CURRENT_URI, word: 'node_version', @@ -106,13 +156,13 @@ describe('findDefinition', () => { describe('findReferences', () => { it('returns empty list if parameter is not found', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) const result = analyzer.findReferences('foobar') expect(result).toEqual([]) }) it('returns a list of locations if parameter is found', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) const result = analyzer.findReferences('node_version') expect(result).not.toEqual([]) expect(result).toMatchSnapshot() @@ -121,20 +171,20 @@ describe('findReferences', () => { describe('findSymbolsForFile', () => { it('returns empty list if uri is not found', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) const result = analyzer.findSymbolsForFile({ uri: 'foobar.sh' }) expect(result).toEqual([]) }) it('returns a list of SymbolInformation if uri is found', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) const result = analyzer.findSymbolsForFile({ uri: CURRENT_URI }) expect(result).not.toEqual([]) expect(result).toMatchSnapshot() }) it('issue 101', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.ISSUE101 }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.ISSUE101 }) const result = analyzer.findSymbolsForFile({ uri: CURRENT_URI }) expect(result).not.toEqual([]) expect(result).toMatchSnapshot() @@ -146,18 +196,22 @@ describe('findAllSourcedUris', () => { const parser = await initializeParser() const connection = getMockConnection() - const newAnalyzer = new Analyzer({ console: connection.console, parser }) + const newAnalyzer = new Analyzer({ + console: connection.console, + parser, + workspaceFolder: REPO_ROOT_FOLDER, + }) await newAnalyzer.initiateBackgroundAnalysis({ backgroundAnalysisMaxFiles: defaultConfig.backgroundAnalysisMaxFiles, globPattern: defaultConfig.globPattern, - rootPath: FIXTURE_FOLDER, }) const result = newAnalyzer.findAllSourcedUris({ uri: FIXTURE_URI.SOURCING }) expect(result).toEqual( new Set([ - `file://${FIXTURE_FOLDER}issue101.sh`, - `file://${FIXTURE_FOLDER}extension.inc`, + `file://${REPO_ROOT_FOLDER}/scripts/tag-release.inc`, // resolved based on repoRootFolder + `file://${FIXTURE_FOLDER}issue101.sh`, // resolved based on current file + `file://${FIXTURE_FOLDER}extension.inc`, // resolved based on current file ]), ) }) @@ -166,17 +220,20 @@ describe('findAllSourcedUris', () => { const parser = await initializeParser() const connection = getMockConnection() - const newAnalyzer = new Analyzer({ console: connection.console, parser }) + const newAnalyzer = new Analyzer({ + console: connection.console, + parser, + workspaceFolder: FIXTURE_FOLDER, + }) await newAnalyzer.initiateBackgroundAnalysis({ backgroundAnalysisMaxFiles: defaultConfig.backgroundAnalysisMaxFiles, globPattern: defaultConfig.globPattern, - rootPath: FIXTURE_FOLDER, }) // Parse the file without extension newAnalyzer.analyze({ uri: FIXTURE_URI.MISSING_EXTENSION, - document: FIXTURES.MISSING_EXTENSION, + document: FIXTURE_DOCUMENT.MISSING_EXTENSION, }) const result = newAnalyzer.findAllSourcedUris({ uri: FIXTURE_URI.MISSING_EXTENSION }) @@ -192,7 +249,7 @@ describe('findAllSourcedUris', () => { describe('wordAtPoint', () => { it('returns current word at a given point', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) expect(analyzer.wordAtPoint(CURRENT_URI, 25, 0)).toEqual(null) expect(analyzer.wordAtPoint(CURRENT_URI, 25, 1)).toEqual(null) expect(analyzer.wordAtPoint(CURRENT_URI, 25, 2)).toEqual(null) @@ -220,7 +277,7 @@ describe('wordAtPoint', () => { describe('commandNameAtPoint', () => { it('returns current command name at a given point', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.INSTALL }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.INSTALL }) expect(analyzer.commandNameAtPoint(CURRENT_URI, 15, 0)).toEqual(null) expect(analyzer.commandNameAtPoint(CURRENT_URI, 20, 2)).toEqual('curl') @@ -245,11 +302,11 @@ describe('findSymbolsMatchingWord', () => { console: connection.console, parser, isSourcingAware: false, + workspaceFolder: FIXTURE_FOLDER, }) await analyzer.initiateBackgroundAnalysis({ backgroundAnalysisMaxFiles: defaultConfig.backgroundAnalysisMaxFiles, globPattern: defaultConfig.globPattern, - rootPath: FIXTURE_FOLDER, }) expect( @@ -312,26 +369,26 @@ describe('findSymbolsMatchingWord', () => { exactMatch: false, }), ).toMatchInlineSnapshot(` -Array [ - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 19, - "line": 6, - }, - "start": Object { - "character": 0, - "line": 6, + Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 19, + "line": 6, + }, + "start": Object { + "character": 0, + "line": 6, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "name": "BLUE", }, - }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", - }, - "name": "BLUE", - }, -] -`) + ] + `) expect( analyzer.findSymbolsMatchingWord({ @@ -340,26 +397,26 @@ Array [ exactMatch: false, }), ).toMatchInlineSnapshot(` -Array [ - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 19, - "line": 6, - }, - "start": Object { - "character": 0, - "line": 6, + Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 19, + "line": 6, + }, + "start": Object { + "character": 0, + "line": 6, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "name": "BLUE", }, - }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", - }, - "name": "BLUE", - }, -] -`) + ] + `) }) it('return a list of symbols accessible to the uri when isSourcingAware is true', async () => { @@ -370,11 +427,11 @@ Array [ console: connection.console, parser, isSourcingAware: true, + workspaceFolder: FIXTURE_FOLDER, }) await analyzer.initiateBackgroundAnalysis({ backgroundAnalysisMaxFiles: defaultConfig.backgroundAnalysisMaxFiles, globPattern: defaultConfig.globPattern, - rootPath: FIXTURE_FOLDER, }) expect( @@ -392,65 +449,65 @@ Array [ exactMatch: false, }), ).toMatchInlineSnapshot(` -Array [ - Object { - "kind": 13, - "location": Object { - "range": Object { - "end": Object { - "character": 19, - "line": 6, - }, - "start": Object { - "character": 0, - "line": 6, + Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 19, + "line": 6, + }, + "start": Object { + "character": 0, + "line": 6, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "name": "BLUE", }, - }, - "uri": "file://${FIXTURE_FOLDER}extension.inc", - }, - "name": "BLUE", - }, -] -`) + ] + `) }) }) describe('commentsAbove', () => { it('returns a string of a comment block above a line', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 22)).toEqual( '```txt\ndoc for func_one\n```', ) }) it('handles line breaks in comments', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 28)).toEqual( '```txt\ndoc for func_two\nhas two lines\n```', ) }) it('only returns connected comments', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 36)).toEqual( '```txt\ndoc for func_three\n```', ) }) it('returns null if no comment found', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 45)).toEqual(null) }) it('works for variables', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 42)).toEqual( '```txt\nworks for variables\n```', ) }) it('returns connected comments with empty comment line', () => { - analyzer.analyze({ uri: CURRENT_URI, document: FIXTURES.COMMENT_DOC }) + analyzer.analyze({ uri: CURRENT_URI, document: FIXTURE_DOCUMENT.COMMENT_DOC }) expect(analyzer.commentsAbove(CURRENT_URI, 51)).toEqual( '```txt\nthis is also included\n\ndoc for func_four\n```', ) @@ -465,11 +522,14 @@ describe('initiateBackgroundAnalysis', () => { const connection = getMockConnection() - const newAnalyzer = new Analyzer({ console: connection.console, parser }) + const newAnalyzer = new Analyzer({ + console: connection.console, + parser, + workspaceFolder: FIXTURE_FOLDER, + }) const { filesParsed } = await newAnalyzer.initiateBackgroundAnalysis({ backgroundAnalysisMaxFiles: defaultConfig.backgroundAnalysisMaxFiles, globPattern: defaultConfig.globPattern, - rootPath: FIXTURE_FOLDER, }) expect(connection.window.showWarningMessage).not.toHaveBeenCalled() @@ -493,11 +553,14 @@ describe('initiateBackgroundAnalysis', () => { const connection = getMockConnection() - const newAnalyzer = new Analyzer({ console: connection.console, parser }) + const newAnalyzer = new Analyzer({ + console: connection.console, + parser, + workspaceFolder: FIXTURE_FOLDER, + }) const { filesParsed } = await newAnalyzer.initiateBackgroundAnalysis({ backgroundAnalysisMaxFiles: defaultConfig.backgroundAnalysisMaxFiles, globPattern: defaultConfig.globPattern, - rootPath: FIXTURE_FOLDER, }) expect(connection.window.showWarningMessage).not.toHaveBeenCalled() @@ -512,11 +575,14 @@ describe('initiateBackgroundAnalysis', () => { const connection = getMockConnection() - const newAnalyzer = new Analyzer({ console: connection.console, parser }) + const newAnalyzer = new Analyzer({ + console: connection.console, + parser, + workspaceFolder: FIXTURE_FOLDER, + }) const { filesParsed } = await newAnalyzer.initiateBackgroundAnalysis({ backgroundAnalysisMaxFiles: 0, globPattern: defaultConfig.globPattern, - rootPath: FIXTURE_FOLDER, }) expect(connection.window.showWarningMessage).not.toHaveBeenCalled() @@ -525,3 +591,147 @@ describe('initiateBackgroundAnalysis', () => { expect(filesParsed).toEqual(0) }) }) + +describe('getAllVariableSymbols', () => { + it('returns all variable symbols', async () => { + const document = FIXTURE_DOCUMENT.SOURCING + const { uri } = document + + const parser = await initializeParser() + const connection = getMockConnection() + + const newAnalyzer = new Analyzer({ + console: connection.console, + parser, + workspaceFolder: REPO_ROOT_FOLDER, + }) + // NOTE: no background analysis + + newAnalyzer.analyze({ uri, document }) + + expect(newAnalyzer.getAllVariableSymbols({ uri })).toMatchInlineSnapshot(` + Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 16, + "line": 10, + }, + "start": Object { + "character": 0, + "line": 10, + }, + }, + "uri": "file://${FIXTURE_FOLDER}sourcing.sh", + }, + "name": "BOLD", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 18, + "line": 4, + }, + "start": Object { + "character": 0, + "line": 4, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "name": "RED", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 20, + "line": 5, + }, + "start": Object { + "character": 0, + "line": 5, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "name": "GREEN", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 19, + "line": 6, + }, + "start": Object { + "character": 0, + "line": 6, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "name": "BLUE", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 16, + "line": 7, + }, + "start": Object { + "character": 0, + "line": 7, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "name": "BOLD", + }, + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 17, + "line": 8, + }, + "start": Object { + "character": 0, + "line": 8, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + "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 2ef4bd9f8..2ea9aed90 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -108,6 +108,79 @@ describe('server', () => { }) }) + it('responds to onDefinition', async () => { + const { connection } = await initializeServer() + + const onDefinition = connection.onDefinition.mock.calls[0][0] + + const result = await onDefinition( + { + textDocument: { + uri: FIXTURE_URI.SOURCING, + }, + position: { character: 10, line: 2 }, + }, + {} as any, + {} as any, + ) + + expect(result).toMatchInlineSnapshot(` + Array [ + Object { + "range": Object { + "end": Object { + "character": 0, + "line": 0, + }, + "start": Object { + "character": 0, + "line": 0, + }, + }, + "uri": "file://${FIXTURE_FOLDER}extension.inc", + }, + ] + `) + }) + + it('responds to onDocumentSymbol', async () => { + const { connection } = await initializeServer() + + const onDocumentSymbol = connection.onDocumentSymbol.mock.calls[0][0] + + const result = await onDocumentSymbol( + { + textDocument: { + uri: FIXTURE_URI.SOURCING, + }, + }, + {} as any, + {} as any, + ) + + expect(result).toMatchInlineSnapshot(` + Array [ + Object { + "kind": 13, + "location": Object { + "range": Object { + "end": Object { + "character": 16, + "line": 10, + }, + "start": Object { + "character": 0, + "line": 10, + }, + }, + "uri": "file://${FIXTURE_FOLDER}sourcing.sh", + }, + "name": "BOLD", + }, + ] + `) + }) + it('responds to onDocumentHighlight', async () => { const { connection } = await initializeServer() diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 5dcd2e247..ccfc13374 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -46,34 +46,44 @@ export default class Analyzer { } private isSourcingAware: boolean + private workspaceFolder: string | null public constructor({ console, isSourcingAware = true, parser, + workspaceFolder, }: { console: LSP.RemoteConsole isSourcingAware?: boolean parser: Parser + workspaceFolder: string | null }) { this.console = console this.isSourcingAware = isSourcingAware this.parser = parser + this.workspaceFolder = workspaceFolder } /** - * Initiates a background analysis of the files in the given rootPath to + * Initiates a background analysis of the files in the workspaceFolder to * enable features across files. + * + * NOTE: if the source aware feature works well, we can likely remove this and + * rely on parsing files as they are sourced. */ public async initiateBackgroundAnalysis({ backgroundAnalysisMaxFiles, globPattern, - rootPath, }: { backgroundAnalysisMaxFiles: number globPattern: string - rootPath: string }): Promise<{ filesParsed: number }> { + const rootPath = this.workspaceFolder + if (!rootPath) { + return { filesParsed: 0 } + } + if (backgroundAnalysisMaxFiles <= 0) { this.console.log( `BackgroundAnalysis: skipping as backgroundAnalysisMaxFiles was 0...`, @@ -124,7 +134,6 @@ export default class Analyzer { this.analyze({ document: TextDocument.create(uri, 'shell', 1, fileContent), - rootPath, uri, }) } catch (error) { @@ -153,18 +162,23 @@ export default class Analyzer { uri: string word: string }): LSP.Location[] { - const fileDeclarations = this.getAllFileDeclarations({ uri }) - const tree = this.uriToTreeSitterTrees[uri] if (position && tree) { - // NOTE: when a word is a file path to a sourced file, we return a location to - // that file. - const sourcedLocation = sourcing.getSourcedLocation({ position, tree, uri, word }) + // NOTE: when a word is a file path to a sourced file, we return a location it. + const sourcedLocation = sourcing.getSourcedLocation({ + position, + rootPath: this.workspaceFolder, + tree, + uri, + word, + }) if (sourcedLocation) { return [sourcedLocation] } } + const fileDeclarations = this.getReachableUriToDeclarations({ uri }) + return Object.keys(fileDeclarations) .reduce((symbols, uri) => { const declarationNames = fileDeclarations[uri][word] || [] @@ -293,8 +307,6 @@ export default class Analyzer { /** * Find symbol completions for the given word. - * - * TODO: if a file is not included we probably shouldn't include it declarations from it. */ public findSymbolsMatchingWord({ exactMatch, @@ -305,7 +317,7 @@ export default class Analyzer { uri: string word: string }): LSP.SymbolInformation[] { - const fileDeclarations = this.getAllFileDeclarations({ uri }) + const fileDeclarations = this.getReachableUriToDeclarations({ uri }) return Object.keys(fileDeclarations).reduce((symbols, uri) => { const declarationsInFile = fileDeclarations[uri] @@ -329,11 +341,9 @@ export default class Analyzer { */ public analyze({ document, - rootPath, uri, // NOTE: we don't use document.uri to make testing easier }: { document: TextDocument - rootPath?: string uri: string }): LSP.Diagnostic[] { const contents = document.getText() @@ -349,7 +359,7 @@ export default class Analyzer { this.uriToSourcedUris[uri] = sourcing.getSourcedUris({ fileContent: contents, fileUri: uri, - rootPath, + rootPath: this.workspaceFolder, }) const problems: LSP.Diagnostic[] = [] @@ -557,20 +567,38 @@ export default class Analyzer { this.isSourcingAware = isSourcingAware } - private getAllFileDeclarations({ uri }: { uri?: string } = {}): FileDeclarations { - const uris = - uri && this.isSourcingAware - ? [uri, ...Array.from(this.findAllSourcedUris({ uri }))] - : Object.keys(this.uriToDeclarations) + private getReachableUriToDeclarations({ + uri: fromUri, + }: { uri?: string } = {}): FileDeclarations { + if (!fromUri || !this.isSourcingAware) { + return this.uriToDeclarations + } + + const uris = [fromUri, ...Array.from(this.findAllSourcedUris({ uri: fromUri }))] + // for each uri (sourced and base file) we find the declarations return uris.reduce((fileDeclarations, uri) => { + if (!this.uriToDeclarations[uri]) { + // Either the background analysis didn't run or the file is outside + // the workspace. Let us try to analyze the file. + try { + const fileContent = fs.readFileSync(new URL(uri), 'utf8') + this.analyze({ + document: TextDocument.create(uri, 'shell', 1, fileContent), + uri, + }) + } catch (err) { + this.console.log(`Error while analyzing sourced file ${uri}: ${err}`) + } + } + fileDeclarations[uri] = this.uriToDeclarations[uri] || {} return fileDeclarations }, {} as FileDeclarations) } private getAllSymbols({ uri }: { uri?: string } = {}): LSP.SymbolInformation[] { - const fileDeclarations = this.getAllFileDeclarations({ uri }) + const fileDeclarations = this.getReachableUriToDeclarations({ uri }) return Object.keys(fileDeclarations).reduce((symbols, uri) => { const declarationsInFile = fileDeclarations[uri] diff --git a/server/src/server.ts b/server/src/server.ts index 6bb2e5343..779b63420 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -32,7 +32,7 @@ export default class BashServer { private documents: LSP.TextDocuments = new LSP.TextDocuments(TextDocument) private executables: Executables private linter?: Linter - private workspaceFolder: string | null | undefined + private workspaceFolder: string | null private uriToCodeActions: { [uri: string]: CodeAction[] | undefined } = {} private constructor({ @@ -48,7 +48,7 @@ export default class BashServer { connection: LSP.Connection executables: Executables linter?: Linter - workspaceFolder: string | null | undefined + workspaceFolder: string | null }) { this.analyzer = analyzer this.clientCapabilities = capabilities @@ -69,14 +69,18 @@ export default class BashServer { ): // TODO: use workspaceFolders instead of rootPath Promise { const { PATH } = process.env - const workspaceFolder = rootUri || rootPath + const workspaceFolder = rootUri || rootPath || null if (!PATH) { throw new Error('Expected PATH environment variable to be set') } const parser = await initializeParser() - const analyzer = new Analyzer({ console: connection.console, parser }) + const analyzer = new Analyzer({ + console: connection.console, + parser, + workspaceFolder, + }) const executables = await Executables.fromPath(PATH) @@ -192,7 +196,6 @@ export default class BashServer { return this.analyzer.initiateBackgroundAnalysis({ globPattern: this.config.globPattern, backgroundAnalysisMaxFiles: this.config.backgroundAnalysisMaxFiles, - rootPath: workspaceFolder, }) } diff --git a/server/src/util/__tests__/sourcing.test.ts b/server/src/util/__tests__/sourcing.test.ts index 58d753e9e..74e7bcffb 100644 --- a/server/src/util/__tests__/sourcing.test.ts +++ b/server/src/util/__tests__/sourcing.test.ts @@ -15,7 +15,7 @@ jest.spyOn(os, 'homedir').mockImplementation(() => '/Users/bash-user') describe('getSourcedUris', () => { it('returns an empty set if no files were sourced', () => { - const result = getSourcedUris({ fileContent: '', fileUri }) + const result = getSourcedUris({ fileContent: '', fileUri, rootPath: null }) expect(result).toEqual(new Set([])) }) @@ -43,6 +43,7 @@ describe('getSourcedUris', () => { if [[ -z $__COMPLETION_LIB_LOADED ]]; then source "$LIBPATH" ; fi `, fileUri, + rootPath: null, }) expect(result).toMatchInlineSnapshot(` diff --git a/server/src/util/sourcing.ts b/server/src/util/sourcing.ts index 94c7d63db..bc5b2bccd 100644 --- a/server/src/util/sourcing.ts +++ b/server/src/util/sourcing.ts @@ -11,8 +11,6 @@ const SOURCED_FILES_REG_EXP = /^(?:\t|[ ])*(?:source|[.])\s*(\S*)/gm /** * Analysis the given file content and returns a set of URIs that are * sourced. Note that the URIs are not resolved. - * - * FIXME: should this be URIs or paths? */ export function getSourcedUris({ fileContent, @@ -21,7 +19,7 @@ export function getSourcedUris({ }: { fileContent: string fileUri: string - rootPath?: string + rootPath: string | null }): Set { const uris: Set = new Set([]) let match: RegExpExecArray | null @@ -44,16 +42,17 @@ export function getSourcedUris({ * @returns an optional location */ export function getSourcedLocation({ - tree, position, + rootPath, + tree, uri, word, }: { - tree: Parser.Tree position: { line: number; character: number } + rootPath: string | null + tree: Parser.Tree uri: string word: string - // FIXME: take in a rootPath }): LSP.Location | null { // NOTE: when a word is a file path to a sourced file, we return a location to // that file. @@ -71,9 +70,9 @@ export function getSourcedLocation({ ? ['.', 'source'].includes(node.previousNamedSibling.text.trim()) : false - const sourcedUri = isSourced - ? getSourcedUri({ word, rootPaths: [path.dirname(uri)] }) - : null + const rootPaths = [path.dirname(uri), rootPath].filter(Boolean) as string[] + + const sourcedUri = isSourced ? getSourcedUri({ word, rootPaths }) : null if (sourcedUri) { return LSP.Location.create(sourcedUri, LSP.Range.create(0, 0, 0, 0)) diff --git a/testing/fixtures/sourcing.sh b/testing/fixtures/sourcing.sh index 4c090617f..981c0ff48 100644 --- a/testing/fixtures/sourcing.sh +++ b/testing/fixtures/sourcing.sh @@ -13,3 +13,7 @@ BOLD=`tput bold` # redefined echo $BOL echo "$" + +source ./scripts/tag-release.inc + +tagRelease '1.0.0' # FIXME: the "defined on" looks wierd From 6434dc742f551e3455bcfada1ae7429e0083bb1a Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 8 Dec 2022 20:57:49 +0100 Subject: [PATCH 08/13] Correct documentation path --- server/src/__tests__/analyzer.test.ts | 10 ++++--- server/src/__tests__/server.test.ts | 42 ++++++++++++++++++--------- server/src/server.ts | 11 +++---- testing/fixtures.ts | 2 +- 4 files changed, 40 insertions(+), 25 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index e539a8bbf..d75cae4aa 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -1,6 +1,9 @@ -import * as path from 'path' - -import { FIXTURE_DOCUMENT, FIXTURE_FOLDER, FIXTURE_URI } from '../../../testing/fixtures' +import { + FIXTURE_DOCUMENT, + FIXTURE_FOLDER, + FIXTURE_URI, + REPO_ROOT_FOLDER, +} from '../../../testing/fixtures' import { getMockConnection } from '../../../testing/mocks' import Analyzer from '../analyser' import { getDefaultConfiguration } from '../config' @@ -10,7 +13,6 @@ import * as fsUtil from '../util/fs' let analyzer: Analyzer const CURRENT_URI = 'dummy-uri.sh' -const REPO_ROOT_FOLDER = path.resolve(path.join(FIXTURE_FOLDER, '../..')) const mockConsole = getMockConnection().console // if you add a .sh file to testing/fixtures, update this value diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 2ea9aed90..01455b6f9 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -3,18 +3,25 @@ import * as Path from 'path' import * as LSP from 'vscode-languageserver/node' import { CodeAction } from 'vscode-languageserver/node' -import FIXTURE_DOCUMENT, { FIXTURE_FOLDER, FIXTURE_URI } from '../../../testing/fixtures' +import { + FIXTURE_DOCUMENT, + FIXTURE_FOLDER, + FIXTURE_URI, + REPO_ROOT_FOLDER, +} from '../../../testing/fixtures' import { getMockConnection } from '../../../testing/mocks' import LspServer from '../server' import { CompletionItemDataType } from '../types' -async function initializeServer() { +async function initializeServer( + { rootPath }: { rootPath?: string } = { rootPath: FIXTURE_FOLDER }, +) { const diagnostics: Array = [] const connection = getMockConnection() const server = await LspServer.initialize(connection, { - rootPath: FIXTURE_FOLDER, + rootPath, rootUri: null, processId: 42, capabilities: {} as any, @@ -484,7 +491,7 @@ describe('server', () => { "name": "BLUE", "type": 3, }, - "documentation": "### Variable: **BLUE** - *defined in ../extension.inc*", + "documentation": "### Variable: **BLUE** - *defined in extension.inc*", "kind": 6, "label": "BLUE", }, @@ -513,7 +520,7 @@ describe('server', () => { "name": "add_a_user", "type": 3, }, - "documentation": "### Function: **add_a_user** - *defined in ../issue101.sh* + "documentation": "### Function: **add_a_user** - *defined in issue101.sh* \`\`\`txt Helper function to add a user @@ -561,7 +568,7 @@ describe('server', () => { }) it('responds to onCompletion with all variables when starting to expand parameters', async () => { - const { connection } = await initializeServer() + const { connection } = await initializeServer({ rootPath: REPO_ROOT_FOLDER }) const onCompletion = connection.onCompletion.mock.calls[0][0] @@ -600,7 +607,7 @@ describe('server', () => { "name": "RED", "type": 3, }, - "documentation": "### Variable: **RED** - *defined in ../extension.inc*", + "documentation": "### Variable: **RED** - *defined in extension.inc*", "kind": 6, "label": "RED", }, @@ -609,7 +616,7 @@ describe('server', () => { "name": "GREEN", "type": 3, }, - "documentation": "### Variable: **GREEN** - *defined in ../extension.inc*", + "documentation": "### Variable: **GREEN** - *defined in extension.inc*", "kind": 6, "label": "GREEN", }, @@ -618,7 +625,7 @@ describe('server', () => { "name": "BLUE", "type": 3, }, - "documentation": "### Variable: **BLUE** - *defined in ../extension.inc*", + "documentation": "### Variable: **BLUE** - *defined in extension.inc*", "kind": 6, "label": "BLUE", }, @@ -627,7 +634,7 @@ describe('server', () => { "name": "RESET", "type": 3, }, - "documentation": "### Variable: **RESET** - *defined in ../extension.inc*", + "documentation": "### Variable: **RESET** - *defined in extension.inc*", "kind": 6, "label": "RESET", }, @@ -636,7 +643,7 @@ describe('server', () => { "name": "USER", "type": 3, }, - "documentation": "### Variable: **USER** - *defined in ../issue101.sh*", + "documentation": "### Variable: **USER** - *defined in issue101.sh*", "kind": 6, "label": "USER", }, @@ -645,7 +652,7 @@ describe('server', () => { "name": "PASSWORD", "type": 3, }, - "documentation": "### Variable: **PASSWORD** - *defined in ../issue101.sh*", + "documentation": "### Variable: **PASSWORD** - *defined in issue101.sh*", "kind": 6, "label": "PASSWORD", }, @@ -654,7 +661,7 @@ describe('server', () => { "name": "COMMENTS", "type": 3, }, - "documentation": "### Variable: **COMMENTS** - *defined in ../issue101.sh* + "documentation": "### Variable: **COMMENTS** - *defined in issue101.sh* \`\`\`txt Having shifted twice, the rest is now comments ... @@ -662,6 +669,15 @@ describe('server', () => { "kind": 6, "label": "COMMENTS", }, + Object { + "data": Object { + "name": "tag", + "type": 3, + }, + "documentation": "### Variable: **tag** - *defined in ../../scripts/tag-release.inc*", + "kind": 6, + "label": "tag", + }, ] `) }) diff --git a/server/src/server.ts b/server/src/server.ts index 779b63420..6801a638e 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -1,5 +1,5 @@ -import * as Process from 'child_process' -import * as Path from 'path' +import { spawnSync } from 'child_process' +import * as path from 'path' import * as TurndownService from 'turndown' import { isDeepStrictEqual } from 'util' import * as LSP from 'vscode-languageserver/node' @@ -352,7 +352,7 @@ export default class BashServer { const hoverHeader = `### ${symbolKindToDescription(symbol.kind)}: **${symbol.name}**` const symbolLocation = symbolUri !== currentUri - ? `in ${Path.relative(currentUri, symbolUri)}` + ? `in ${path.relative(path.dirname(currentUri), symbolUri)}` : `on line ${symbolStarLine + 1}` return `${hoverHeader} - *defined ${symbolLocation}*${symbolDocumentation}` @@ -799,10 +799,7 @@ const getMarkdownContent = (documentation: string): LSP.MarkupContent => ({ function getCommandOptions(name: string, word: string): string[] { // TODO: The options could be cached. - const options = Process.spawnSync(Path.join(__dirname, '../src/get-options.sh'), [ - name, - word, - ]) + const options = spawnSync(path.join(__dirname, '../src/get-options.sh'), [name, word]) if (options.status !== 0) { return [] diff --git a/testing/fixtures.ts b/testing/fixtures.ts index 4c73d64e7..628070087 100644 --- a/testing/fixtures.ts +++ b/testing/fixtures.ts @@ -35,4 +35,4 @@ export const FIXTURE_DOCUMENT: Record = ( return acc }, {} as any) -export default FIXTURE_DOCUMENT +export const REPO_ROOT_FOLDER = path.resolve(path.join(FIXTURE_FOLDER, '../..')) From d6a3fd5d47174857c6efec73a7be63b66358da7a Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 8 Dec 2022 21:52:40 +0100 Subject: [PATCH 09/13] Always return markdown content This fixes a small bug in vscode where documentation that pops out to the right was not formatted using markdown. --- server/src/__tests__/server.test.ts | 69 +++++++++++++++++++++++------ server/src/server.ts | 35 +++++++++------ 2 files changed, 77 insertions(+), 27 deletions(-) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 01455b6f9..d55a98b8d 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -109,10 +109,21 @@ describe('server', () => { ) expect(result).toBeDefined() - expect(result).toEqual({ - contents: - '### Function: **hello_world** - *defined on line 8*\n\n```txt\nthis is a comment\ndescribing the function\nhello_world\nthis function takes two arguments\n```', - }) + expect(result).toMatchInlineSnapshot(` + Object { + "contents": Object { + "kind": "markdown", + "value": "Function: **hello_world** - *defined on line 8* + + \`\`\`txt + this is a comment + describing the function + hello_world + this function takes two arguments + \`\`\`", + }, + } + `) }) it('responds to onDefinition', async () => { @@ -491,7 +502,10 @@ describe('server', () => { "name": "BLUE", "type": 3, }, - "documentation": "### Variable: **BLUE** - *defined in extension.inc*", + "documentation": Object { + "kind": "markdown", + "value": "Variable: **BLUE** - *defined in extension.inc*", + }, "kind": 6, "label": "BLUE", }, @@ -520,11 +534,14 @@ describe('server', () => { "name": "add_a_user", "type": 3, }, - "documentation": "### Function: **add_a_user** - *defined in issue101.sh* + "documentation": Object { + "kind": "markdown", + "value": "Function: **add_a_user** - *defined in issue101.sh* \`\`\`txt Helper function to add a user \`\`\`", + }, "kind": 3, "label": "add_a_user", }, @@ -607,7 +624,10 @@ describe('server', () => { "name": "RED", "type": 3, }, - "documentation": "### Variable: **RED** - *defined in extension.inc*", + "documentation": Object { + "kind": "markdown", + "value": "Variable: **RED** - *defined in extension.inc*", + }, "kind": 6, "label": "RED", }, @@ -616,7 +636,10 @@ describe('server', () => { "name": "GREEN", "type": 3, }, - "documentation": "### Variable: **GREEN** - *defined in extension.inc*", + "documentation": Object { + "kind": "markdown", + "value": "Variable: **GREEN** - *defined in extension.inc*", + }, "kind": 6, "label": "GREEN", }, @@ -625,7 +648,10 @@ describe('server', () => { "name": "BLUE", "type": 3, }, - "documentation": "### Variable: **BLUE** - *defined in extension.inc*", + "documentation": Object { + "kind": "markdown", + "value": "Variable: **BLUE** - *defined in extension.inc*", + }, "kind": 6, "label": "BLUE", }, @@ -634,7 +660,10 @@ describe('server', () => { "name": "RESET", "type": 3, }, - "documentation": "### Variable: **RESET** - *defined in extension.inc*", + "documentation": Object { + "kind": "markdown", + "value": "Variable: **RESET** - *defined in extension.inc*", + }, "kind": 6, "label": "RESET", }, @@ -643,7 +672,10 @@ describe('server', () => { "name": "USER", "type": 3, }, - "documentation": "### Variable: **USER** - *defined in issue101.sh*", + "documentation": Object { + "kind": "markdown", + "value": "Variable: **USER** - *defined in issue101.sh*", + }, "kind": 6, "label": "USER", }, @@ -652,7 +684,10 @@ describe('server', () => { "name": "PASSWORD", "type": 3, }, - "documentation": "### Variable: **PASSWORD** - *defined in issue101.sh*", + "documentation": Object { + "kind": "markdown", + "value": "Variable: **PASSWORD** - *defined in issue101.sh*", + }, "kind": 6, "label": "PASSWORD", }, @@ -661,11 +696,14 @@ describe('server', () => { "name": "COMMENTS", "type": 3, }, - "documentation": "### Variable: **COMMENTS** - *defined in issue101.sh* + "documentation": Object { + "kind": "markdown", + "value": "Variable: **COMMENTS** - *defined in issue101.sh* \`\`\`txt Having shifted twice, the rest is now comments ... \`\`\`", + }, "kind": 6, "label": "COMMENTS", }, @@ -674,7 +712,10 @@ describe('server', () => { "name": "tag", "type": 3, }, - "documentation": "### Variable: **tag** - *defined in ../../scripts/tag-release.inc*", + "documentation": Object { + "kind": "markdown", + "value": "Variable: **tag** - *defined in ../../scripts/tag-release.inc*", + }, "kind": 6, "label": "tag", }, diff --git a/server/src/server.ts b/server/src/server.ts index 6801a638e..61ad2c1e6 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -343,19 +343,24 @@ export default class BashServer { }: { symbol: LSP.SymbolInformation currentUri: string - }): string { + }): LSP.MarkupContent { const symbolUri = symbol.location.uri - const symbolStarLine = symbol.location.range.start.line + const symbolStartLine = symbol.location.range.start.line - const commentAboveSymbol = this.analyzer.commentsAbove(symbolUri, symbolStarLine) + const commentAboveSymbol = this.analyzer.commentsAbove(symbolUri, symbolStartLine) const symbolDocumentation = commentAboveSymbol ? `\n\n${commentAboveSymbol}` : '' - const hoverHeader = `### ${symbolKindToDescription(symbol.kind)}: **${symbol.name}**` + const hoverHeader = `${symbolKindToDescription(symbol.kind)}: **${symbol.name}**` const symbolLocation = symbolUri !== currentUri ? `in ${path.relative(path.dirname(currentUri), symbolUri)}` - : `on line ${symbolStarLine + 1}` + : `on line ${symbolStartLine + 1}` - return `${hoverHeader} - *defined ${symbolLocation}*${symbolDocumentation}` + // TODO: An improvement could be to add show the symbol definition in the hover instead + // of the defined location – similar to how VSCode works for languages like TypeScript. + + return getMarkdownContent( + `${hoverHeader} - *defined ${symbolLocation}*${symbolDocumentation}`, + ) } private getCompletionItemsForSymbols({ @@ -427,7 +432,7 @@ export default class BashServer { ) { const shellDocumentation = await getShellDocumentation({ word }) if (shellDocumentation) { - return { contents: getMarkdownContent(shellDocumentation) } + return { contents: getMarkdownContent(shellDocumentation, 'man') } } } else { const symbolDocumentation = deduplicateSymbols({ @@ -688,7 +693,7 @@ export default class BashServer { return documentation ? { ...item, - documentation: getMarkdownContent(documentation), + documentation: getMarkdownContent(documentation, 'man'), } : item } catch (error) { @@ -791,11 +796,15 @@ function symbolKindToDescription(s: LSP.SymbolKind): string { } } -const getMarkdownContent = (documentation: string): LSP.MarkupContent => ({ - value: ['``` man', documentation, '```'].join('\n'), - // Passed as markdown for syntax highlighting - kind: 'markdown' as const, -}) +function getMarkdownContent(documentation: string, language?: string): LSP.MarkupContent { + return { + value: language + ? // eslint-disable-next-line prefer-template + ['``` ' + language, documentation, '```'].join('\n') + : documentation, + kind: LSP.MarkupKind.Markdown, + } +} function getCommandOptions(name: string, word: string): string[] { // TODO: The options could be cached. From ea219006abe021707b4aba8ee4ab7ba7d5cc3c13 Mon Sep 17 00:00:00 2001 From: skovhus Date: Thu, 8 Dec 2022 22:05:51 +0100 Subject: [PATCH 10/13] Update documentation --- server/src/util/sourcing.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/server/src/util/sourcing.ts b/server/src/util/sourcing.ts index bc5b2bccd..df07831de 100644 --- a/server/src/util/sourcing.ts +++ b/server/src/util/sourcing.ts @@ -10,7 +10,7 @@ const SOURCED_FILES_REG_EXP = /^(?:\t|[ ])*(?:source|[.])\s*(\S*)/gm /** * Analysis the given file content and returns a set of URIs that are - * sourced. Note that the URIs are not resolved. + * sourced. Note that the URIs are resolved. */ export function getSourcedUris({ fileContent, @@ -97,6 +97,12 @@ const stripQuotes = (path: string): string => { * Tries to parse the given path and returns a URI if possible. * - Filters out dynamic sources * - Converts a relative paths to absolute paths + * - Converts a tilde path to an absolute path + * - Resolves the path + * + * NOTE: for future improvements: + * "If filename does not contain a slash, file names in PATH are used to find + * the directory containing filename." (see https://ss64.com/osx/source.html) */ function getSourcedUri({ rootPaths, @@ -105,10 +111,6 @@ function getSourcedUri({ rootPaths: string[] word: string }): string | null { - // NOTE: improvements: - // - we could try to resolve the path - // - "If filename does not contain a slash, file names in PATH are used to find - // the directory containing filename." (see https://ss64.com/osx/source.html) let unquotedPath = stripQuotes(word) if (unquotedPath.includes('$')) { From dff893ff8a782967c0ee506ab11910a350a92ee3 Mon Sep 17 00:00:00 2001 From: skovhus Date: Fri, 9 Dec 2022 09:34:14 +0100 Subject: [PATCH 11/13] Ignore sourcing like statements in heredoc and raw strings --- server/src/analyser.ts | 1 + server/src/util/__tests__/sourcing.test.ts | 52 ++++++++++++++++++---- server/src/util/sourcing.ts | 34 ++++++++++---- 3 files changed, 69 insertions(+), 18 deletions(-) diff --git a/server/src/analyser.ts b/server/src/analyser.ts index ccfc13374..eb9ae6656 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -360,6 +360,7 @@ export default class Analyzer { fileContent: contents, fileUri: uri, rootPath: this.workspaceFolder, + tree, }) const problems: LSP.Diagnostic[] = [] diff --git a/server/src/util/__tests__/sourcing.test.ts b/server/src/util/__tests__/sourcing.test.ts index 74e7bcffb..4d2e62fa8 100644 --- a/server/src/util/__tests__/sourcing.test.ts +++ b/server/src/util/__tests__/sourcing.test.ts @@ -1,28 +1,37 @@ +import * as fs from 'fs' import * as os from 'os' +import * as Parser from 'web-tree-sitter' +import { initializeParser } from '../../parser' import { getSourcedUris } from '../sourcing' const fileDirectory = '/Users/bash' const fileUri = `${fileDirectory}/file.sh` -// mock fs.existsSync to always return true -jest.mock('fs', () => ({ - existsSync: () => true, -})) +let parser: Parser +beforeAll(async () => { + parser = await initializeParser() +}) + +jest.spyOn(fs, 'existsSync').mockImplementation(() => true) // mock os.homedir() to return a fixed path jest.spyOn(os, 'homedir').mockImplementation(() => '/Users/bash-user') describe('getSourcedUris', () => { it('returns an empty set if no files were sourced', () => { - const result = getSourcedUris({ fileContent: '', fileUri, rootPath: null }) + const fileContent = '' + const result = getSourcedUris({ + fileContent, + fileUri, + rootPath: null, + tree: parser.parse(fileContent), + }) expect(result).toEqual(new Set([])) }) it('returns a set of sourced files', () => { - const result = getSourcedUris({ - fileContent: ` - + const fileContent = ` source file-in-path.sh # does not contain a slash (i.e. is maybe somewhere on the path) source /bin/extension.inc # absolute path @@ -41,9 +50,34 @@ describe('getSourcedUris', () => { # conditional is currently not supported if [[ -z $__COMPLETION_LIB_LOADED ]]; then source "$LIBPATH" ; fi - `, + + show () + { + about 'Shows SVN proxy settings' + group 'proxy' + + echo "SVN Proxy Settings" + echo "==================" + python2 - < $counts_file + fi + done + + ` + + const result = getSourcedUris({ + fileContent, fileUri, rootPath: null, + tree: parser.parse(fileContent), }) expect(result).toMatchInlineSnapshot(` diff --git a/server/src/util/sourcing.ts b/server/src/util/sourcing.ts index df07831de..ff1266d10 100644 --- a/server/src/util/sourcing.ts +++ b/server/src/util/sourcing.ts @@ -6,7 +6,8 @@ import * as Parser from 'web-tree-sitter' import { untildify } from './fs' // Until the grammar supports sourcing, we use this little regular expression -const SOURCED_FILES_REG_EXP = /^(?:\t|[ ])*(?:source|[.])\s*(\S*)/gm +const SOURCING_STATEMENTS = /^(?:\t|[ ])*(?:source|[.])\s*(\S*)/ +const SOURCING_COMMANDS = ['source', '.'] /** * Analysis the given file content and returns a set of URIs that are @@ -16,22 +17,37 @@ export function getSourcedUris({ fileContent, fileUri, rootPath, + tree, }: { fileContent: string fileUri: string rootPath: string | null + tree: Parser.Tree }): Set { const uris: Set = new Set([]) - let match: RegExpExecArray | null const rootPaths = [path.dirname(fileUri), rootPath].filter(Boolean) as string[] - while ((match = SOURCED_FILES_REG_EXP.exec(fileContent)) !== null) { - const word = match[1] - const sourcedUri = getSourcedUri({ rootPaths, word }) - if (sourcedUri) { - uris.add(sourcedUri) + fileContent.split(/\r?\n/).forEach((line, lineIndex) => { + const match = line.match(SOURCING_STATEMENTS) + if (match) { + const [statement, word] = match + + if (tree.rootNode) { + const node = tree.rootNode.descendantForPosition({ + row: lineIndex, + column: statement.length - 2, + }) + if (['heredoc_body', 'raw_string'].includes(node?.type)) { + return + } + } + + const sourcedUri = getSourcedUri({ rootPaths, word }) + if (sourcedUri) { + uris.add(sourcedUri) + } } - } + }) return uris } @@ -67,7 +83,7 @@ export function getSourcedLocation({ } const isSourced = node.previousNamedSibling - ? ['.', 'source'].includes(node.previousNamedSibling.text.trim()) + ? SOURCING_COMMANDS.includes(node.previousNamedSibling.text.trim()) : false const rootPaths = [path.dirname(uri), rootPath].filter(Boolean) as string[] From 285a279f4b22f24fe240791422cd540f870e55f1 Mon Sep 17 00:00:00 2001 From: skovhus Date: Fri, 9 Dec 2022 13:06:19 +0100 Subject: [PATCH 12/13] Rename configuration option --- server/src/__tests__/analyzer.test.ts | 8 ++++---- server/src/__tests__/config.test.ts | 9 ++++++--- server/src/analyser.ts | 14 +++++++------- server/src/config.ts | 11 ++++++----- server/src/server.ts | 4 +++- vscode-client/package.json | 10 +++++----- 6 files changed, 31 insertions(+), 25 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index d75cae4aa..230f41a0d 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -296,14 +296,14 @@ describe('commandNameAtPoint', () => { }) describe('findSymbolsMatchingWord', () => { - it('return a list of symbols across the workspace when isSourcingAware is false', async () => { + it('return a list of symbols across the workspace when includeAllWorkspaceSymbols is true', async () => { const parser = await initializeParser() const connection = getMockConnection() const analyzer = new Analyzer({ console: connection.console, parser, - isSourcingAware: false, + includeAllWorkspaceSymbols: true, workspaceFolder: FIXTURE_FOLDER, }) await analyzer.initiateBackgroundAnalysis({ @@ -421,14 +421,14 @@ describe('findSymbolsMatchingWord', () => { `) }) - it('return a list of symbols accessible to the uri when isSourcingAware is true', async () => { + it('return a list of symbols accessible to the uri when includeAllWorkspaceSymbols is false', async () => { const parser = await initializeParser() const connection = getMockConnection() const analyzer = new Analyzer({ console: connection.console, parser, - isSourcingAware: true, + includeAllWorkspaceSymbols: false, workspaceFolder: FIXTURE_FOLDER, }) await analyzer.initiateBackgroundAnalysis({ diff --git a/server/src/__tests__/config.test.ts b/server/src/__tests__/config.test.ts index fffc86617..82be2125c 100644 --- a/server/src/__tests__/config.test.ts +++ b/server/src/__tests__/config.test.ts @@ -1,34 +1,37 @@ import { ConfigSchema, getConfigFromEnvironmentVariables } from '../config' describe('ConfigSchema', () => { - it('parses an object', () => { + it('returns a default', () => { expect(ConfigSchema.parse({})).toMatchInlineSnapshot(` Object { "backgroundAnalysisMaxFiles": 500, - "completionBasedOnImports": true, "explainshellEndpoint": "", "globPattern": "**/*@(.sh|.inc|.bash|.command)", "highlightParsingErrors": false, + "includeAllWorkspaceSymbols": false, "shellcheckArguments": Array [], "shellcheckPath": "shellcheck", } `) + }) + it('parses an object', () => { expect( ConfigSchema.parse({ backgroundAnalysisMaxFiles: 1, explainshellEndpoint: 'localhost:8080', globPattern: '**/*@(.sh)', highlightParsingErrors: true, + includeAllWorkspaceSymbols: true, shellcheckArguments: ' -e SC2001 -e SC2002 ', shellcheckPath: '', }), ).toMatchInlineSnapshot(` Object { "backgroundAnalysisMaxFiles": 1, - "completionBasedOnImports": true, "explainshellEndpoint": "localhost:8080", "globPattern": "**/*@(.sh)", "highlightParsingErrors": true, + "includeAllWorkspaceSymbols": true, "shellcheckArguments": Array [ "-e", "SC2001", diff --git a/server/src/analyser.ts b/server/src/analyser.ts index eb9ae6656..150448dc3 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -45,22 +45,22 @@ export default class Analyzer { variable_assignment: LSP.SymbolKind.Variable, } - private isSourcingAware: boolean + private includeAllWorkspaceSymbols: boolean private workspaceFolder: string | null public constructor({ console, - isSourcingAware = true, + includeAllWorkspaceSymbols = false, parser, workspaceFolder, }: { console: LSP.RemoteConsole - isSourcingAware?: boolean + includeAllWorkspaceSymbols?: boolean parser: Parser workspaceFolder: string | null }) { this.console = console - this.isSourcingAware = isSourcingAware + this.includeAllWorkspaceSymbols = includeAllWorkspaceSymbols this.parser = parser this.workspaceFolder = workspaceFolder } @@ -564,14 +564,14 @@ export default class Analyzer { ) } - public setIsSourcingAware(isSourcingAware: boolean): void { - this.isSourcingAware = isSourcingAware + public setIncludeAllWorkspaceSymbols(includeAllWorkspaceSymbols: boolean): void { + this.includeAllWorkspaceSymbols = includeAllWorkspaceSymbols } private getReachableUriToDeclarations({ uri: fromUri, }: { uri?: string } = {}): FileDeclarations { - if (!fromUri || !this.isSourcingAware) { + if (!fromUri || this.includeAllWorkspaceSymbols) { return this.uriToDeclarations } diff --git a/server/src/config.ts b/server/src/config.ts index 94bd9dc36..9023e79ac 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -2,10 +2,6 @@ import { z } from 'zod' export const ConfigSchema = z .object({ - // Controls if completions are based only on analyzing the import/sourcing of files. - // If false, completion will be based on all files in the workspace. - completionBasedOnImports: z.boolean().default(true), - // Maximum number of files to analyze in the background. Set to 0 to disable background analysis. backgroundAnalysisMaxFiles: z.number().int().min(0).default(500), @@ -19,6 +15,11 @@ export const ConfigSchema = z // Controls if Treesitter parsing errors will be highlighted as problems. highlightParsingErrors: z.boolean().default(false), + // Controls how symbols (e.g. variables and functions) are included and used for completion and documentation. + // If false, then we only include symbols from sourced files (i.e. using non dynamic statements like 'source file.sh' or '. file.sh'). + // If true, then all symbols from the workspace are included. + includeAllWorkspaceSymbols: z.boolean().default(false), + // Additional ShellCheck arguments. Note that we already add the following arguments: --shell, --format, --external-sources." shellcheckArguments: z .preprocess((arg) => { @@ -48,10 +49,10 @@ export function getConfigFromEnvironmentVariables(): { } { const rawConfig = { backgroundAnalysisMaxFiles: process.env.BACKGROUND_ANALYSIS_MAX_FILES, - completionBasedOnImports: toBoolean(process.env.COMPLETION_BASED_ON_IMPORTS), explainshellEndpoint: process.env.EXPLAINSHELL_ENDPOINT, globPattern: process.env.GLOB_PATTERN, highlightParsingErrors: toBoolean(process.env.HIGHLIGHT_PARSING_ERRORS), + includeAllWorkspaceSymbols: toBoolean(process.env.INCLUDE_ALL_WORKSPACE_SYMBOLS), shellcheckArguments: process.env.SHELLCHECK_ARGUMENTS, shellcheckPath: process.env.SHELLCHECK_PATH, } diff --git a/server/src/server.ts b/server/src/server.ts index 61ad2c1e6..944cee957 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -227,7 +227,9 @@ export default class BashServer { }) } - this.analyzer.setIsSourcingAware(this.config.completionBasedOnImports) + this.analyzer.setIncludeAllWorkspaceSymbols( + this.config.includeAllWorkspaceSymbols, + ) return true } diff --git a/vscode-client/package.json b/vscode-client/package.json index 88c967f8a..6ff8d4664 100644 --- a/vscode-client/package.json +++ b/vscode-client/package.json @@ -37,11 +37,6 @@ "description": "Maximum number of files to analyze in the background. Set to 0 to disable background analysis.", "minimum": 0 }, - "bashIde.completionBasedOnImports": { - "type": "boolean", - "default": true, - "description": "Controls if symbols completion across files is based on parsing sourcing statements (i.e. 'source file.sh' or '. file.sh'). If false then all symbols found in the workspace will be used." - }, "bashIde.explainshellEndpoint": { "type": "string", "default": "", @@ -57,6 +52,11 @@ "default": false, "description": "Controls if Treesitter parsing errors will be highlighted as problems." }, + "bashIde.includeAllWorkspaceSymbols": { + "type": "boolean", + "default": false, + "description": "Controls how symbols (e.g. variables and functions) are included and used for completion and documentation. If false (default and recommended), then we only include symbols from sourced files (i.e. using non dynamic statements like 'source file.sh' or '. file.sh'). If true, then all symbols from the workspace are included." + }, "bashIde.shellcheckPath": { "type": "string", "default": "shellcheck", From 458aeb00155a41de6412e8bcebe91e3bcf76e29b Mon Sep 17 00:00:00 2001 From: skovhus Date: Fri, 9 Dec 2022 13:16:26 +0100 Subject: [PATCH 13/13] Release new 4.1.0 server --- server/CHANGELOG.md | 4 ++++ server/package.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/server/CHANGELOG.md b/server/CHANGELOG.md index 71d019d9b..97dd5248c 100644 --- a/server/CHANGELOG.md +++ b/server/CHANGELOG.md @@ -1,5 +1,9 @@ # Bash Language Server +## 4.1.0 + +- Symbols across files are now only included based on sourced files (using non dynamic statements like `source file.sh` or `. ~/file.inc`) instead of including symbols from all files in the workspace. We now also support jump-to-definition on the file path used in a source command. The new behavior can be disabled by turning on the `includeAllWorkspaceSymbols` configuration option. https://github.com/bash-lsp/bash-language-server/pull/244 + ## 4.0.1 - **Breaking**: Drop support for Node 12, which reached its official end of life on April 30th 2022. Doing so enables new features. https://github.com/bash-lsp/bash-language-server/pull/584 diff --git a/server/package.json b/server/package.json index 8f5fb21c4..21911c8b7 100644 --- a/server/package.json +++ b/server/package.json @@ -3,7 +3,7 @@ "description": "A language server for Bash", "author": "Mads Hartmann", "license": "MIT", - "version": "4.0.1", + "version": "4.1.0", "publisher": "mads-hartmann", "main": "./out/server.js", "typings": "./out/server.d.ts",