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

feat(query): add first_retun flag to TSQueryMatch #1372

Closed
wants to merge 1 commit into from
Closed

feat(query): add first_retun flag to TSQueryMatch #1372

wants to merge 1 commit into from

Conversation

vigoux
Copy link
Contributor

@vigoux vigoux commented Aug 31, 2021

Allows for users of the API to determine if the match is returned for the first time, for example to run the predicates only once.

We encountered this little problem when working on tree-sitter intergration on neovim.

cc @bfredl

Allows for users of the API to determine if the match is returned for
the first time, for example to run the predicates only once.
@bfredl
Copy link
Contributor

bfredl commented Sep 13, 2021

ping @maxbrunsfeld is this something you would consider adding? This is needed to effectively highlight regions of a file, if only some captures of a complex match is part of the redrawn region.

@maxbrunsfeld
Copy link
Contributor

The id field on TSQueryMatch exists for a similar purpose (so you can check if the predicates have already been run), but maybe first_return is more useful than id. Did you consider using id to check if the predicates have run?

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld commented Sep 13, 2021

I'm just remembering more about this problem...

The benefit of the id field is that if the predicates fail to match, you can get rid of the match by calling ts_query_cursor_remove_match(cursor, match.id);, which is good because then you never have to deal with that match again, and it doesn't occupy any storage space inside of the TSQueryCursor. Right now, though, it's probably inconvenient to have to keep track of the ids, because the matches are not guaranteed to be returned in ascending order of id. Is that why you're not using id currently?

I think we could actually change the implementation so that the id counter would only be incremented once a match was actually returned. That way, in order to avoid running predicates repeatedly, you could just keep track of a max_match_id. What do you think? Would that work for your use case? If so, I'd prefer that solution, because it avoids having two somewhat redundant fields (id and first_return).

@maxbrunsfeld
Copy link
Contributor

I pushed a small commit to master that ensures the match ids are sequential. Could you take a look and see if this would enable your use case in Neovim?

@bfredl
Copy link
Contributor

bfredl commented Sep 14, 2021

@maxbrunsfeld yes, I think that would work for us, we will give it a try 👍

ahlinc added a commit that referenced this pull request Sep 14, 2021
ahlinc added a commit that referenced this pull request Sep 14, 2021
@ubolonton
Copy link
Contributor

Seems like that commit also fixed an issue I had recently, illustrated using the HCL grammar:

With this code:

resource "random_pet" "this" {
  length = 2
}

resource "tls_private_key" "this" {
  algorithm = "RSA"
}

resource "aws_key_pair" "this" {
  key_name   = random_pet.this.id
  public_key = tls_private_key.this.public_key_openssh
}

and this query:

((block (identifier) @keyword
        . [(string_lit (template_literal) @function)
           (identifier) @function])
 (.eq? @keyword "output"))

((block (identifier) @keyword)
 (.match? @keyword "^(resource|data|output|locals|lifecycle)$"))

The command tree-sitter query --captures did not return the first resource. With that commit, it now returns all 3:

    pattern:  1, capture: 0 - keyword, start: (0, 0), end: (0, 8), text: `resource` # It did not return this.
    pattern:  1, capture: 0 - keyword, start: (4, 0), end: (4, 8), text: `resource`
    pattern:  1, capture: 0 - keyword, start: (8, 0), end: (8, 8), text: `resource`

@vigoux
Copy link
Contributor Author

vigoux commented Oct 1, 2021

Hey, sorry to bother you again, but the fix pushed to master effectively fixes our issue.
Following this, do you think it is possible to make a release following those changes ?

Thanks in advance !

cc @maxbrunsfeld

@maxbrunsfeld
Copy link
Contributor

I published Tree-sitter 0.20.1.

hendrikvanantwerpen pushed a commit to hendrikvanantwerpen/tree-sitter that referenced this pull request Nov 23, 2021
hendrikvanantwerpen pushed a commit to hendrikvanantwerpen/tree-sitter that referenced this pull request Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants