Skip to content

Commit

Permalink
Replace complex tree sitter helper
Browse files Browse the repository at this point in the history
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 bash-lsp/bash-language-server#192
  • Loading branch information
domnewkirk committed May 19, 2020
1 parent 4eb6ffc commit a6b9735
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 81 deletions.
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
10 changes: 5 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
19 changes: 3 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
13 changes: 11 additions & 2 deletions server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }))
Expand All @@ -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
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
}

0 comments on commit a6b9735

Please sign in to comment.