From ef0d5004afc1b112fcc97c0a263d2c41d4b4a156 Mon Sep 17 00:00:00 2001 From: skovhus Date: Tue, 19 May 2020 21:24:53 +0200 Subject: [PATCH 1/9] Replace complex tree sitter helper The helper was originally introduced to make the detection of a word at a given point more accurate (this is used for completion, hovering, etc) and fixes some issues with the tree sitter grammar. It turns out this is really not needed and introduces a bunch of issues. Related to https://github.com/bash-lsp/bash-language-server/pull/192 --- server/src/__tests__/analyzer.test.ts | 16 ++++++-- server/src/__tests__/server.test.ts | 10 ++--- server/src/analyser.ts | 19 ++------- server/src/server.ts | 13 ++++++- server/src/util/tree-sitter.ts | 56 +-------------------------- 5 files changed, 33 insertions(+), 81 deletions(-) diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index f59e3dad1..7520acc2b 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -87,16 +87,26 @@ describe('findSymbolsForFile', () => { describe('wordAtPoint', () => { it('returns current word at a given point', () => { analyzer.analyze(CURRENT_URI, 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) + expect(analyzer.wordAtPoint(CURRENT_URI, 25, 3)).toEqual(null) + expect(analyzer.wordAtPoint(CURRENT_URI, 25, 4)).toEqual('rm') expect(analyzer.wordAtPoint(CURRENT_URI, 25, 5)).toEqual('rm') + expect(analyzer.wordAtPoint(CURRENT_URI, 25, 6)).toEqual(null) + expect(analyzer.wordAtPoint(CURRENT_URI, 25, 7)).toEqual('npm-install-') - // FIXME: grammar issue: else is not found - // expect(analyzer.wordAtPoint(CURRENT_URI, 24, 5)).toEqual('else') + expect(analyzer.wordAtPoint(CURRENT_URI, 24, 2)).toEqual('else') + expect(analyzer.wordAtPoint(CURRENT_URI, 24, 3)).toEqual('else') + expect(analyzer.wordAtPoint(CURRENT_URI, 24, 5)).toEqual('else') + expect(analyzer.wordAtPoint(CURRENT_URI, 24, 7)).toEqual(null) expect(analyzer.wordAtPoint(CURRENT_URI, 30, 1)).toEqual(null) + expect(analyzer.wordAtPoint(CURRENT_URI, 30, 2)).toEqual('ret') expect(analyzer.wordAtPoint(CURRENT_URI, 30, 3)).toEqual('ret') expect(analyzer.wordAtPoint(CURRENT_URI, 30, 4)).toEqual('ret') - expect(analyzer.wordAtPoint(CURRENT_URI, 30, 5)).toEqual('ret') + expect(analyzer.wordAtPoint(CURRENT_URI, 30, 5)).toEqual('=') expect(analyzer.wordAtPoint(CURRENT_URI, 38, 5)).toEqual('configures') }) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index f25d43592..0270a78ba 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -181,7 +181,7 @@ describe('server', () => { {} as any, ) - expect(result2).toMatchInlineSnapshot(`null`) + expect(result2).toMatchInlineSnapshot(`Array []`) }) it('responds to onWorkspaceSymbol', async () => { @@ -279,9 +279,9 @@ describe('server', () => { uri: FIXTURE_URI.INSTALL, }, position: { - // else - line: 24, - character: 5, + // empty space + line: 26, + character: 0, }, }, {} as any, @@ -289,7 +289,7 @@ describe('server', () => { ) // Entire list - expect(result && 'length' in result && result.length > 50).toBe(true) + expect(result && 'length' in result && result.length).toBeGreaterThanOrEqual(50) }) it('responds to onCompletion with empty list when word is a comment', async () => { diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 0ed007aac..5ca34610e 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -371,32 +371,19 @@ export default class Analyzer { */ public wordAtPoint(uri: string, line: number, column: number): string | null { const document = this.uriToTreeSitterTrees[uri] - const contents = this.uriToFileContent[uri] if (!document.rootNode) { // Check for lacking rootNode (due to failed parse?) return null } - const point = { row: line, column } - - const node = TreeSitterUtil.namedLeafDescendantForPosition(point, document.rootNode) + const node = document.rootNode.descendantForPosition({ row: line, column }) - if (!node) { + if (!node || node.childCount > 0 || node.text.trim() === '') { return null } - const start = node.startIndex - const end = node.endIndex - const name = contents.slice(start, end) - - // Hack. Might be a problem with the grammar. - // TODO: Document this with a test case - if (name.endsWith('=')) { - return name.slice(0, name.length - 1) - } - - return name + return node.text.trim() } /** diff --git a/server/src/server.ts b/server/src/server.ts index 735252a7c..372c2a94b 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -273,9 +273,11 @@ export default class BashServer { ): LSP.DocumentHighlight[] | null { const word = this.getWordAtPoint(params) this.logRequest({ request: 'onDocumentHighlight', params, word }) + if (!word) { - return null + return [] } + return this.analyzer .findOccurrences(params.textDocument.uri, word) .map(n => ({ range: n.range })) @@ -291,7 +293,14 @@ export default class BashServer { } private onCompletion(params: LSP.TextDocumentPositionParams): BashCompletionItem[] { - const word = this.getWordAtPoint(params) + const word = this.getWordAtPoint({ + ...params, + position: { + line: params.position.line, + // Go one character back to get completion on the current word + character: Math.max(params.position.character - 1, 0), + }, + }) this.logRequest({ request: 'onCompletion', params, word }) const currentUri = params.textDocument.uri diff --git a/server/src/util/tree-sitter.ts b/server/src/util/tree-sitter.ts index cdf7ee572..3a80b4cfe 100644 --- a/server/src/util/tree-sitter.ts +++ b/server/src/util/tree-sitter.ts @@ -1,5 +1,5 @@ import { Range } from 'vscode-languageserver/lib/main' -import { Point, SyntaxNode } from 'web-tree-sitter' +import { SyntaxNode } from 'web-tree-sitter' export function forEach(node: SyntaxNode, cb: (n: SyntaxNode) => void) { cb(node) @@ -52,57 +52,3 @@ export function findParent( } return null } - -/** - * Given a tree and a point, try to find the named leaf node that the point corresponds to. - * This is a helper for wordAtPoint, useful in cases where the point occurs at the boundary of - * a word so the normal behavior of "namedDescendantForPosition" does not find the desired leaf. - * For example, if you do - * > (new Parser()).setLanguage(bash).parse("echo 42").rootNode.descendantForIndex(4).text - * then you get 'echo 42', not the leaf node for 'echo'. - * - * TODO: the need for this function might reveal a flaw in tree-sitter-bash. - */ -export function namedLeafDescendantForPosition( - point: Point, - rootNode: SyntaxNode, -): SyntaxNode | null { - const node = rootNode.namedDescendantForPosition(point) - - if (node.childCount === 0) { - return node - } else { - // The node wasn't a leaf. Try to figure out what word we should use. - const nodeToUse = searchForLeafNode(point, node) - if (nodeToUse) { - return nodeToUse - } else { - return null - } - } -} - -function searchForLeafNode(point: Point, parent: SyntaxNode): SyntaxNode | null { - let child = parent.firstNamedChild - - while (child) { - if ( - pointsEqual(child.startPosition, point) || - pointsEqual(child.endPosition, point) - ) { - if (child.childCount === 0) { - return child - } else { - return searchForLeafNode(point, child) - } - } - - child = child.nextNamedSibling - } - - return null -} - -function pointsEqual(point1: Point, point2: Point) { - return point1.row === point2.row && point1.column === point2.column -} From c62c574ff80475adf9fc83ebb9e605aa6b07c27d Mon Sep 17 00:00:00 2001 From: skovhus Date: Tue, 19 May 2020 21:33:16 +0200 Subject: [PATCH 2/9] Update triggerCharacters to support parameter expansions --- server/src/__tests__/__snapshots__/server.test.ts.snap | 4 ++++ server/src/server.ts | 1 + 2 files changed, 5 insertions(+) diff --git a/server/src/__tests__/__snapshots__/server.test.ts.snap b/server/src/__tests__/__snapshots__/server.test.ts.snap index ebc6d2339..fdcfa104b 100644 --- a/server/src/__tests__/__snapshots__/server.test.ts.snap +++ b/server/src/__tests__/__snapshots__/server.test.ts.snap @@ -4,6 +4,10 @@ exports[`server initializes and responds to capabilities 1`] = ` Object { "completionProvider": Object { "resolveProvider": true, + "triggerCharacters": Array [ + "$", + "{", + ], }, "definitionProvider": true, "documentHighlightProvider": true, diff --git a/server/src/server.ts b/server/src/server.ts index 372c2a94b..bd9210cb5 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -100,6 +100,7 @@ export default class BashServer { textDocumentSync: LSP.TextDocumentSyncKind.Full, completionProvider: { resolveProvider: true, + triggerCharacters: ['$', '{'], }, hoverProvider: true, documentHighlightProvider: true, From 01a692c985e60d11c268612e92766e650db7789c Mon Sep 17 00:00:00 2001 From: skovhus Date: Tue, 19 May 2020 21:34:12 +0200 Subject: [PATCH 3/9] Return only variables for parameter expansion completions --- server/src/analyser.ts | 4 ++++ server/src/server.ts | 30 +++++++++++++++++++++--------- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 5ca34610e..12986a677 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -434,6 +434,10 @@ export default class Analyzer { return null } + public getAllVariableSymbols(): LSP.SymbolInformation[] { + return this.getAllSymbols().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[] = [] diff --git a/server/src/server.ts b/server/src/server.ts index bd9210cb5..42179c936 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -13,6 +13,8 @@ import { BashCompletionItem, CompletionItemDataType } from './types' import { uniqueBasedOnHash } from './util/array' import { getShellDocumentation } from './util/sh' +const PARAMETER_EXPANSION_PREFIXES = new Set(['$', '${', '${#']) + /** * The BashServer glues together the separate components to implement * the various parts of the Language Server Protocol. @@ -304,19 +306,34 @@ export default class BashServer { }) this.logRequest({ request: 'onCompletion', params, word }) + if (word && word.startsWith('#')) { + // Inside a comment block + return [] + } + const currentUri = params.textDocument.uri + const shouldCompleteOnVariables = word + ? PARAMETER_EXPANSION_PREFIXES.has(word) + : false + const symbolCompletions = word === null ? [] : this.getCompletionItemsForSymbols({ - symbols: this.analyzer.findSymbolsMatchingWord({ - exactMatch: false, - word, - }), + symbols: shouldCompleteOnVariables + ? this.analyzer.getAllVariableSymbols() + : this.analyzer.findSymbolsMatchingWord({ + exactMatch: false, + word, + }), currentUri, }) + if (shouldCompleteOnVariables) { + return symbolCompletions + } + const reservedWordsCompletions = ReservedWords.LIST.map(reservedWord => ({ label: reservedWord, kind: LSP.SymbolKind.Interface, // ?? @@ -357,11 +374,6 @@ export default class BashServer { ] if (word) { - if (word.startsWith('#')) { - // Inside a comment block - return [] - } - // Filter to only return suffixes of the current word return allCompletions.filter(item => item.label.startsWith(word)) } From 08d969abd7f441f7a386c4983f7b41f04185eb79 Mon Sep 17 00:00:00 2001 From: skovhus Date: Tue, 19 May 2020 21:46:05 +0200 Subject: [PATCH 4/9] Document variable expansion support --- server/src/__tests__/server.test.ts | 29 +++++++++++++++++++++++++++++ testing/fixtures/sourcing.sh | 2 ++ 2 files changed, 31 insertions(+) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 0270a78ba..c748d49d8 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -418,4 +418,33 @@ Helper function to add a user", ] `) }) + + it('responds to onCompletion with all variables when starting to expand parameters', async () => { + const { connection, server } = await initializeServer() + server.register(connection) + + const onCompletion = connection.onCompletion.mock.calls[0][0] + + const result: any = await onCompletion( + { + textDocument: { + uri: FIXTURE_URI.SOURCING, + }, + position: { + // $ + line: 14, + character: 7, + }, + }, + {} as any, + {} as any, + ) + + const itemKinds = result.map((item: any) => item.kind) + + // they are all variables + expect(itemKinds.every((kind: any) => kind === lsp.CompletionItemKind.Variable)).toBe( + true, + ) + }) }) diff --git a/testing/fixtures/sourcing.sh b/testing/fixtures/sourcing.sh index 7e285c4d6..4c090617f 100644 --- a/testing/fixtures/sourcing.sh +++ b/testing/fixtures/sourcing.sh @@ -11,3 +11,5 @@ add_a_us BOLD=`tput bold` # redefined echo $BOL + +echo "$" From 1788538096c93c251c66b416c292b1461b159e56 Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 20 May 2020 10:47:32 +0200 Subject: [PATCH 5/9] Stabilize test --- server/src/__tests__/server.test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index c748d49d8..3c15d7302 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -440,11 +440,9 @@ Helper function to add a user", {} as any, ) - const itemKinds = result.map((item: any) => item.kind) - // they are all variables - expect(itemKinds.every((kind: any) => kind === lsp.CompletionItemKind.Variable)).toBe( - true, - ) + expect(Array.from(new Set(result.map((item: any) => item.kind)))).toEqual([ + lsp.CompletionItemKind.Variable, + ]) }) }) From 4241bb6c524b40363cf16d328c73a91daf718ab8 Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 20 May 2020 10:47:50 +0200 Subject: [PATCH 6/9] Update expansion prefix and document handler --- server/src/server.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/server.ts b/server/src/server.ts index 42179c936..b0092dba8 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -13,7 +13,7 @@ import { BashCompletionItem, CompletionItemDataType } from './types' import { uniqueBasedOnHash } from './util/array' import { getShellDocumentation } from './util/sh' -const PARAMETER_EXPANSION_PREFIXES = new Set(['$', '${', '${#']) +const PARAMETER_EXPANSION_PREFIXES = new Set(['$', '${']) /** * The BashServer glues together the separate components to implement @@ -331,6 +331,10 @@ export default class BashServer { }) if (shouldCompleteOnVariables) { + // In case we auto complete on a word that starts a parameter expansion, + // we do not return anything else than variable/parameter suggestions. + // Note: that LSP clients should not call onCompletion in the middle + // of a word, so the following should work for client. return symbolCompletions } From a1da15e749e582c890f23b95c6f001e3650569a2 Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 20 May 2020 11:11:09 +0200 Subject: [PATCH 7/9] Skip documentation for some builtins Previously this was skipped as the parser logic was broken. --- server/src/util/__tests__/sh.test.ts | 5 +++++ server/src/util/sh.ts | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/server/src/util/__tests__/sh.test.ts b/server/src/util/__tests__/sh.test.ts index ad026fda4..b87235d94 100644 --- a/server/src/util/__tests__/sh.test.ts +++ b/server/src/util/__tests__/sh.test.ts @@ -20,6 +20,11 @@ describe('getDocumentation', () => { expect(lines[1]).toContain('list directory contents') }) + it('skips documentation for some builtins', async () => { + const result = await sh.getShellDocumentation({ word: 'else' }) + expect(result).toEqual(null) + }) + it('sanity checks the given word', async () => { await expect(sh.getShellDocumentation({ word: 'ls foo' })).rejects.toThrow() }) diff --git a/server/src/util/sh.ts b/server/src/util/sh.ts index 46c31816c..4194e3aca 100644 --- a/server/src/util/sh.ts +++ b/server/src/util/sh.ts @@ -24,6 +24,18 @@ export function execShellScript(body: string): Promise { }) } +// Currently only reserved words where documentation doesn't make sense. +// At least on OS X these just return the builtin man. On ubuntu there +// are no documentaiton for them. +const WORDS_WITHOUT_DOCUMENTATION = new Set([ + 'else', + 'fi', + 'then', + 'esac', + 'elif', + 'done', +]) + /** * Get documentation for the given word by usingZZ help and man. */ @@ -36,6 +48,10 @@ export async function getShellDocumentationWithoutCache({ throw new Error(`lookupDocumentation should be given a word, received "${word}"`) } + if (WORDS_WITHOUT_DOCUMENTATION.has(word)) { + return null + } + const DOCUMENTATION_COMMANDS = [ { type: 'help', command: `help ${word} | col -bx` }, // We have experimented with setting MANWIDTH to different values for reformatting. From dbf5ba21aa2f4443a81c7c3be653df7501b6f9a8 Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 20 May 2020 11:11:45 +0200 Subject: [PATCH 8/9] Prepare server for release (1.16.0) --- server/CHANGELOG.md | 5 ++++- server/package.json | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/server/CHANGELOG.md b/server/CHANGELOG.md index 154f68300..47203361e 100644 --- a/server/CHANGELOG.md +++ b/server/CHANGELOG.md @@ -1,10 +1,13 @@ # Bash Language Server +## 1.16.0 + +* Improved completion handler for parameter expansions (https://github.com/bash-lsp/bash-language-server/pull/237) + ## 1.15.0 * Use comments above symbols for documentation (https://github.com/bash-lsp/bash-language-server/pull/234, https://github.com/bash-lsp/bash-language-server/pull/235) - ## 1.14.0 * onHover and onCompletion documentation improvements (https://github.com/bash-lsp/bash-language-server/pull/230) diff --git a/server/package.json b/server/package.json index d2275a983..2383c5203 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": "1.15.0", + "version": "1.16.0", "publisher": "mads-hartmann", "main": "./out/server.js", "typings": "./out/server.d.ts", From e6f759b65cafa39122ddf13d0594a9d6a5332145 Mon Sep 17 00:00:00 2001 From: skovhus Date: Wed, 20 May 2020 11:21:51 +0200 Subject: [PATCH 9/9] Document improvement for completion handler --- server/src/server.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/src/server.ts b/server/src/server.ts index b0092dba8..eee594b5e 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -313,6 +313,9 @@ export default class BashServer { const currentUri = params.textDocument.uri + // TODO: an improvement here would be to detect if the current word is + // not only a parameter expansion prefix, but also if the word is actually + // inside a parameter expansion (e.g. auto completing on a word $MY_VARIA). const shouldCompleteOnVariables = word ? PARAMETER_EXPANSION_PREFIXES.has(word) : false