Skip to content
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

Add TreeSitterLanguageMode::getSyntaxNodeContainingRange and TreeSitterLanguageMode::getSyntaxNodeAtPosition #17721

Merged
merged 10 commits into from Jul 24, 2018

Conversation

Projects
None yet
3 participants
@queerviolet
Copy link
Contributor

queerviolet commented Jul 20, 2018

Adds:

  • TreeSitterLanguageMode::getSyntaxNodeContainingRange(range: Range, where?: SyntaxNode -> Boolean)
  • TreeSitterLanguageMode::getSyntaxNodeAtPosition(range: Range, where?: SyntaxNode -> Boolean)

These methods let you find a SyntaxNode at a location, optionally specifying a predicate function.

Changes:

  • bufferRangeForScopeAtPosition(position) to bufferRangeForScopeAtPosition(selector, position).
  • getRageForSyntaxNodeAtPosition(position) to getRangeForSyntaxNodeAtPosition(selector, where).

This brings it in line with the TextMateLanguageMode interface. The selector may be a string selector, or a predicate function of type SyntaxNode -> Boolean.

Needed for atom/toggle-quotes#57.

Change TreeSitterLanguageMode::bufferRangeForScopeAtPosition(position…
…) to

bufferRangeForScopeAtPosition(selector, position), to match the TextMateLanguageMode
signature, extracting some selector matching logic to do so.

Needed for atom/toggle-quotes#57

@queerviolet queerviolet requested a review from maxbrunsfeld Jul 20, 2018

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld left a comment

Thanks for jumping on this bug!

It looks like we need some tests for getRangeForSyntaxNodeContainingRange in tree-sitter-language-mode-spec.js. Maybe in those tests, you can show the ways (if any) that we deal with selectors that were written with a TextMate parser in mind.

*
* @param {String|Array<String>} selector
* @returns {Array<String>} selector parts
*/

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Jul 20, 2018

Contributor

There's a slightly different (and more unusual 😬) style of doc comments called AtomDoc that we usually use in this codebase. There's good examples of it in this file. I wouldn't necessarily be opposed to switching to JSDoc at some point. In the meantime, could adjust these to use atomdoc?

This comment has been minimized.

@queerviolet

queerviolet Jul 20, 2018

Author Contributor

👍

@@ -348,25 +349,28 @@ class TreeSitterLanguageMode {
Section - Syntax Tree APIs
*/

getRangeForSyntaxNodeContainingRange (range) {
getRangeForSyntaxNodeContainingRange (range, selector) {

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Jul 20, 2018

Contributor

How do you imagine the compatibility working for this? The legacy TextMate selectors often target multiple scopes like string.quoted, but in a Tree-sitter syntax tree, a node never has more than one type; strings for example are just string.

I'd imagine that we would want the selector .string to match a string node, but what do we do about .string.quoted? It seems like there's no way around changing callers to provide new selectors. Did you envision changing toggle-quotes to just pass .string instead of .string.quoted?

This comment has been minimized.

@queerviolet

queerviolet Jul 20, 2018

Author Contributor

Yeah, the backcompat story here is a bummer.

I did actually change toggle-quotes to do that, and it... well, it works for that use case with the JS grammar ;) Perhaps even for most grammars—I imagine the C family and Ruby grammars call their quoted strings strings. HTML, meanwhile, does not, calling its text text IIRC. (OTOH, HTML has attribute_values, and that's a bummer, because it means that we can't toggle quotes on those (though we couldn't before, either)).

One alternative I considered is looking up the scope map and querying on those. But it would be nice if we could start divesting ourselves of the TextMate CSS classes, particularly for this query interface. We should provide a more expressive query interface than just matching on node type, though—ideally, what toggle-quotes could do is say, "find me the closest node starting with "|' and ending with "|'", which would be quite doable.

I guess the question is, how bad is it to semi-break the behavior of bufferRangeForNodeInRange? I mean, it was completely broken before, and this is the first time someone's noticed.

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Jul 20, 2018

Contributor

We should provide a more expressive query interface than just matching on node type, though—ideally, what toggle-quotes could do is say, "find me the closest node starting with "|' and ending with "|'", which would be quite doable.

Yeah, you're right. That's the logic that toggle-quotes should be using; it probably shouldn't rely on the names of any syntax nodes.

But I don't necessarily think we need to build any special query language for finding that syntax node; we should just allow the package to directly inspect the syntax nodes and walk up the tree using SyntaxNode.parent. What do you think of adding a new experimental API called something like syntaxNodeForPosition() or syntaxNodeForRange() that toggle-quotes can use to inspect the syntax tree manually?

This comment has been minimized.

@queerviolet

queerviolet Jul 21, 2018

Author Contributor

Thing is, there are potentially multiple trees at that point, and the deepest one may not be right. Consider toggling quotes inside the rhs of document.body.innerHTML = "<span>hello</span>".

But this change makes it quite easy for us to accept a node predicate function as a selector, so the language mode can still handle walking the trees and finding the smallest matching node, and extensions can pass in a function describing the node they want.

Updated to support and test that behavior.

This comment has been minimized.

@queerviolet

queerviolet Jul 21, 2018

Author Contributor

On second though, it would be nice to be able to query for support of the new behavior, so I did add getSyntaxNodeForRange and getSyntaxNodeForPosition. (bufferRangeForScopeAtPosition also supports match functions)

It works for toggle-quotes.

This comment has been minimized.

@queerviolet
const startIndex = this.buffer.characterIndexForPosition(range.start)
const endIndex = this.buffer.characterIndexForPosition(range.end)
const searchEndIndex = Math.max(0, endIndex - 1)

const matches = matcherForSelector(selector)

This comment has been minimized.

@Arcanemagus

Arcanemagus Jul 20, 2018

Contributor

Trailing whitespace here 😛

This comment has been minimized.

@queerviolet

queerviolet Jul 20, 2018

Author Contributor

👍

@queerviolet queerviolet changed the title Change TreeSitterLanguageMode::bufferRangeForScopeAtPosition Add TreeSitterLanguageMode::getSyntaxNodeContainingRange and TreeSitterLanguageMode::getSyntaxNodeAtPosition Jul 21, 2018

@queerviolet queerviolet force-pushed the get-range-for-syntax-node-with-selector branch from a0624ce to 8034f3b Jul 23, 2018

queerviolet added some commits Jul 23, 2018

@queerviolet queerviolet force-pushed the get-range-for-syntax-node-with-selector branch from 8034f3b to d6ac437 Jul 23, 2018

Update tree-sitter-language-mode.js
Don't use a default predicate for `getRangeForSyntaxNodeContainingRange`.

@queerviolet queerviolet merged commit f91a4bb into master Jul 24, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daviwil daviwil deleted the get-range-for-syntax-node-with-selector branch Jul 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.