Skip to content

Code completion improvements #237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion server/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
2 changes: 1 addition & 1 deletion server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
4 changes: 4 additions & 0 deletions server/src/__tests__/__snapshots__/server.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ exports[`server initializes and responds to capabilities 1`] = `
Object {
"completionProvider": Object {
"resolveProvider": true,
"triggerCharacters": Array [
"$",
"{",
],
},
"definitionProvider": true,
"documentHighlightProvider": true,
Expand Down
16 changes: 13 additions & 3 deletions server/src/__tests__/analyzer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
})
Expand Down
37 changes: 32 additions & 5 deletions server/src/__tests__/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ describe('server', () => {
{} as any,
)

expect(result2).toMatchInlineSnapshot(`null`)
expect(result2).toMatchInlineSnapshot(`Array []`)
})

it('responds to onWorkspaceSymbol', async () => {
Expand Down Expand Up @@ -279,17 +279,17 @@ describe('server', () => {
uri: FIXTURE_URI.INSTALL,
},
position: {
// else
line: 24,
character: 5,
// empty space
line: 26,
character: 0,
},
},
{} as any,
{} as any,
)

// 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 () => {
Expand Down Expand Up @@ -418,4 +418,31 @@ 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,
)

// they are all variables
expect(Array.from(new Set(result.map((item: any) => item.kind)))).toEqual([
lsp.CompletionItemKind.Variable,
])
})
})
23 changes: 7 additions & 16 deletions server/src/analyser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}

/**
Expand Down Expand Up @@ -447,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[] = []
Expand Down
51 changes: 40 additions & 11 deletions server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(['$', '${'])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this variable is used to test if:

    const shouldCompleteOnVariables = word
      ? PARAMETER_EXPANSION_PREFIXES.has(word)
      : false

This check sets shouldCompleteOnVariables to true only for the first two, and only if the user hasnt typed anything else. I've done some tests, and I can't find a case where ${# gets detected.

/**
* The BashServer glues together the separate components to implement
* the various parts of the Language Server Protocol.
Expand Down Expand Up @@ -100,6 +102,7 @@ export default class BashServer {
textDocumentSync: LSP.TextDocumentSyncKind.Full,
completionProvider: {
resolveProvider: true,
triggerCharacters: ['$', '{'],
},
hoverProvider: true,
documentHighlightProvider: true,
Expand Down Expand Up @@ -273,9 +276,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 }))
Expand All @@ -291,22 +296,51 @@ 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 })

if (word && word.startsWith('#')) {
// Inside a comment block
return []
}

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an optimization to avoid creating the massive allCompletions array below?
If so, this if statement only returns when the user type:

echo $
echo ${
echo "$
echo "${

and if the user types anything after that, the value of shouldCompleteOnVariables will be false.
Is this desired behavior, or do we want shouldCompleteOnVariables to be true when the user has something like:

start="some variable"
# user begins to type 'st':
# and hopes to autocomplete 'start'
echo ${st

# shouldCompleteOnVariables is false though

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reading through this carefully.

is this an optimization to avoid creating the massive allCompletions array below?

This is actually not an optimization. The reason for the early exit is that we only want to suggest parameters/variables here if the word that we complete on is $ or ${.

and if the user types anything after that, the value of shouldCompleteOnVariables will be false.

From my experience, this is not how language server protocol clients work. They get completion for a word (e.g. $) once, meaning they do not continuously call the onCompletion while the user types. This makes our life a bit easier here, as we do not need to adjust while the user types out a word.

// 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
}

const reservedWordsCompletions = ReservedWords.LIST.map(reservedWord => ({
label: reservedWord,
kind: LSP.SymbolKind.Interface, // ??
Expand Down Expand Up @@ -347,11 +381,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))
}
Expand Down
5 changes: 5 additions & 0 deletions server/src/util/__tests__/sh.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})
Expand Down
16 changes: 16 additions & 0 deletions server/src/util/sh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,18 @@ export function execShellScript(body: string): Promise<string> {
})
}

// 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.
*/
Expand All @@ -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.
Expand Down
56 changes: 1 addition & 55 deletions server/src/util/tree-sitter.ts
Original file line number Diff line number Diff line change
@@ -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)
Expand Down Expand Up @@ -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
}
2 changes: 2 additions & 0 deletions testing/fixtures/sourcing.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@ add_a_us
BOLD=`tput bold` # redefined

echo $BOL

echo "$"