-
-
Notifications
You must be signed in to change notification settings - Fork 143
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
Tree-sitter rolling fixes, 1.123 edition #1118
Conversation
…where it got flummoxed by URLs with subdomains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much a rubber-stamping this PR.
If I knew a lot more I could be more insightful. But speaking as a reviewer: Short and small change sets are generally good!
Didn't see anything spooky or objectionable, not that I would know, TBH, but there's less diff to inspect and in which to miss anything, so I feel maybe more confident in that than usual. At least.
Anyway, caveats aside, thanks for keeping up maintenance of these grammars and such!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems legit. I do a bit of parameter expansion here and there myself.
Docs for reference: https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping the parser seems probably desirable I assume 👍
Linking the diff in the actual parser package repo, in case anyone better at reading this stuff (or just curious) wants to read through it: savetheclocktower/tree-sitter-hyperlink@04c3a66...0704b3e
; The "foo" in `int &foo` with in a parameter list. | ||
(parameter_declaration | ||
declarator: (reference_declarator | ||
(identifier) @variable.parameter.cpp)) | ||
; The "foo" in `const char *foo` within a parameter list. | ||
; (Should work no matter how many pointers deep we are.) | ||
(reference_declarator (identifier) @variable.parameter.cpp | ||
(#is? test.descendantOfType "parameter_declaration")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wish I understood the syntax better but haven't achieved that enlightenment yet. (Both for C itself and for tree-sitter grammars.)
But... presumably this is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“When a reference_declarator
node has a child named identifier
, apply the variable.parameter.cpp
scope name to the identifier
node… but only if identifier
is a descendant of parameter_declaration
.”
Tree-sitter has an annoying oversight in its query syntax: you can't directly query ancestor/descendant relationships — only parent/child relationships. A parameter_declaration
like int foo
can have an identifier
as a child…
int main (int foo) {
…or it can have a reference_declarator
as a child, which itself has an identifier
child…
int main (int *foo) {
…or it can be doubly-nested in reference_declarators
and be an extra layer deep…
int main (int **foo) {
…and, though this is probably as deep as it gets, at this point one looks around for a way to match all identifier
s anywhere underneath all parameter_declaration
nodes — instead of having to handle these three cases in three separate queries.
The test.descendantOfType
query predicate allows us to do that. It traverses upward from identifier
and returns true
as soon as it finds a node of type parameter_declaration
, or false
if it gets to the root without finding one. This would be pretty costly — executing against all identifier
s, and failing most of the time! — except that a reference_declarator
node is pretty much guaranteed to exist inside of a parameter_declaration
, so the initial query winnows out nearly all of the identifier
s that would fail this test.
Indeed the one "Editor Tests" failing spec is known problematic across multiple unrelated PRs at the moment, so it's not this PR's fault, so safe to ignore with regard to merging this particular PR or not. |
Diligence in bug reporting means I have two new small fixes even though we just released! More to come, I'm sure.