Skip to content
This repository has been archived by the owner on Oct 6, 2021. It is now read-only.

Special-case _unused and __MODULE__ #19

Closed
nifoc opened this issue May 15, 2021 · 4 comments · Fixed by #20
Closed

Special-case _unused and __MODULE__ #19

nifoc opened this issue May 15, 2021 · 4 comments · Fixed by #20

Comments

@nifoc
Copy link

nifoc commented May 15, 2021

I'm working on integrating this awesome parser into nvim-treesitter. While working on the queries, I too ran into #4.

At least for me, the issue is caused by the following queries (that I wrote):

; Unused variables
((identifier) @comment
 (#lua-match? @comment "^_[%a%d_]*$"))

; Stuff like __MODULE__ or __STACKTRACE__
((identifier) @constant.builtin
 (#lua-match? @constant.builtin "^__[%a%d_]+__$"))

Would it be a good idea to add two new identifier types (e.g. unused_identifier and special_identifier) to handle these cases?

(I tried adding support for this myself and submitting a PR, but I'm afraid I don't know enough about writing tree-sitter gammars yet …)

@ananthakumaran
Copy link
Owner

Thanks for starting the work on integrating with nvim-treesitter. I
just had a quick look at the PR, looks like nvim implementation is far
ahead compared to emacs (which I have been mostly testing with).

As for the implementation status, there are two unsolved issues

  1. handling identifiers inside pattern match.

Could this be solved by #has-ancestor? predicate in
nvim-treesitter?

  1. tree-sitter has some limits on how many capture groups
    it would keep in memory, this in turn limits the number of
    parameters highlighted.

Would it be a good idea to add two new identifier types
(e.g. unused_identifier and special_identifier) to handle these
cases?

I played around with these and removing queries on identifier seems to
improve the number of parameters getting highlighted.

so we are basically asking whether it's OK to specialize the AST (like
differentiating identifier/unused_identifier/special_identifier) to
fix the issue?

Initially, I tried to avoid this direction, but I don't see any other
option. I will start with adding new AST nodes for unused_identifier
and special_identifier (we might have to do the same for def*
identifiers as well I guess)

Apart from these, I am still not clear how the queries will be
maintained? What would happen if I make any breaking changes to the
AST?

@nifoc
Copy link
Author

nifoc commented May 15, 2021

Could this be solved by #has-ancestor? predicate in
nvim-treesitter?

I started looking into this, but I'm not sure if the current implementation would help. I don't think you can "scope" #has-ancestor? @_test binary_op to left (and a specific operator), for example.

; Try to match: foo = 1
((identifier) @_test
 (#has-ancestor? @_test binary_op))

This will match all identifiers nested under binary_op.

so we are basically asking whether it's OK to specialize the AST (like
differentiating identifier/unused_identifier/special_identifier) to
fix the issue?
[…]

Yeah, this is sadly what I'm asking/proposing.

I can understand why you wanted to avoid doing this, but (with my very limited understanding) I think this is the only way to work around the current "limitations" of tree-sitter …

Apart from these, I am still not clear how the queries will be
maintained? What would happen if I make any breaking changes to the
AST?

nvim-treesitter locks parsers to a certain commit. So it will not (by default at least) install a newer version of the parser automatically. This way the queries could be updated to match the new AST before pulling in a newer version of the parser.

@ananthakumaran
Copy link
Owner

This will match all identifiers nested under binary_op.

Introducing something like the assign_op AST node would solve this.
To solve 100% of the cases, we might also need to have
(not (#has-parent? @identifier module_attribute_op))

I am still unsure how to handle pattern in function argument position, even
if we are ok with adding new AST node types.

@nifoc
Copy link
Author

nifoc commented May 16, 2021

I am still unsure how to handle pattern in function argument position, even
if we are ok with adding new AST node types.

I played around with the has-ancestor? predicate in nvim-treesitter a bit, but I always end up with something like:

((identifier) @_test
 (#has-ancestor? @_test arguments))

Which obviously doesn't work for a variety of reasons:

  1. Since this is matching (identifier), it will inevitably run into the match limit
  2. It matches all arguments passed to function calls

I also tried (#has-ancestor? @_test arguments call call), because I thought that might narrow it down to just the function definition, but it looks like has-ancestor? really just looks at any ancestor(s), regardless of the nesting. So this sadly also matches functions calls, too.

At the moment I don't have any good ideas on how to make this work …

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants