-
Notifications
You must be signed in to change notification settings - Fork 56
update sshd config grammar so quotedString node is not hidden #1281
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR makes the quotedString grammar node visible in the parse tree by removing the underscore prefix from _quotedString to quotedString. This change enables proper differentiation between regular strings and quoted strings when parsing space-separated arguments in SSH server configuration files (e.g., allowgroups admins devs "openssh users").
- Updated grammar rule from hidden
_quotedStringto visiblequotedString - Updated all test expectations to include the
quotedStringnode in parse trees
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| grammars/tree-sitter-ssh-server-config/grammar.js | Changed _quotedString to quotedString in both the rule definition and the arguments choice, making the node visible in the parse tree |
| grammars/tree-sitter-ssh-server-config/test/corpus/valid_expressions.txt | Updated 6 test cases to expect quotedString nodes wrapping string nodes in the parse tree output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| _commaSeparatedString: $ => seq($.string, repeat(seq(',', $.string))), | ||
| _quotedString: $ => seq('\"', $.string, '\"'), | ||
| quotedString: $ => seq('\"', $.string, '\"'), |
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.
For the parser, you don't care if it's quoted or not, but simply the result is the contents between the quotes (if provided), right? Can't you define a string as a choice between quoted and not quoted where the quoted is just the contents between the quotes?
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.
The full scenario the Rust parser is running into:
The existing parser has a test for:
allowgroups group1 "group with spaces"
However, the existing parser would parse this scenario:
allowgroups group1 group2
as a single string, "group1 group2" instead of "group1" and "group2"
To support both, I plan to update the Rust parser to be able to split "string" nodes by spaces but not "quoted string nodes"
It may be easier to consolidate the grammar and parser changes because this has broken the parser anyway
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.
another option would probably be to update the grammar to not declare space in extras but that may have broader impact
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.
I think space in extras is fine to allow arbitrary whitespace, but the Rust parsing code should just handle the tokens returned by the grammar and not have to worry about the enclosing quotes and just the contents which can contain whitespace. I think this should be possible by defining as (pseudo-grammar):
[keyword] [parameter]+
where [parameter] is choice([string], [number], [yes/no], etc...) and [string] is either a bareString or quotedString, but the parser just gets it as a string.
PR Summary
PR Context
allowgroups admins devs "openssh users")