Merge pull request #237 from bash-lsp/word-at-point-improvements

Code completion improvements

Author: skovhus

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)

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",

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,

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')
   })

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,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 () => {
@@ -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,
+    ])
+  })
 })

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()
   }
 
   /**
@@ -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[] = []

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.
@@ -100,6 +102,7 @@ export default class BashServer {
       textDocumentSync: LSP.TextDocumentSyncKind.Full,
       completionProvider: {
         resolveProvider: true,
+        triggerCharacters: ['$', '{'],
       },
       hoverProvider: true,
       documentHighlightProvider: true,
@@ -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 }))
@@ -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) {
+      // 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, // ??
@@ -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))
     }

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()
   })

server/src/util/sh.ts

@@ -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.
  */
@@ -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.

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

testing/fixtures/sourcing.sh

@@ -11,3 +11,5 @@ add_a_us
 BOLD=`tput bold` # redefined
 
 echo $BOL
+
+echo "$"