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(lang): Update Nix grammar & improve queries #2472

Merged
merged 1 commit into from
May 30, 2022

Conversation

nrdxp
Copy link
Contributor

@nrdxp nrdxp commented May 14, 2022

Update grammar and queiries to the latest upstream. Also add improved indents thanks to @oxalica.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helix uses custom scopes (see here), so some of the scopes here need to be renamed. For example: @property becomes @variable.other.member. The changes for booleans and numbers can be reverted except that it looks like integer and float nodes have been renamed to integer_expression and float_expression. It looks like most of the changes in the grammar came from renaming nodes to have a _expression suffix.

runtime/queries/nix/highlights.scm Outdated Show resolved Hide resolved
runtime/queries/nix/indents.scm Show resolved Hide resolved
runtime/queries/nix/locals.scm Outdated Show resolved Hide resolved
@nrdxp
Copy link
Contributor Author

nrdxp commented May 14, 2022

@the-mikedavis, I believe I addressed all your comments with a force push.

@nrdxp nrdxp force-pushed the update-nix branch 3 times, most recently from 530fcd8 to 0ca07a9 Compare May 14, 2022 23:02
runtime/queries/nix/highlights.scm Outdated Show resolved Hide resolved
runtime/queries/nix/highlights.scm Show resolved Hide resolved
Comment on lines -15 to -18
[
"}"
"]"
] @outdent
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably keep these unless the injects work better without them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure exactly why tbh, but I was getting extra indents with the previous definitions. These ones seem to work more accurately

runtime/queries/nix/indents.scm Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 18, 2022
@nrdxp nrdxp force-pushed the update-nix branch 2 times, most recently from 6a19ab6 to 319a4a0 Compare May 18, 2022 21:47
@nrdxp
Copy link
Contributor Author

nrdxp commented May 18, 2022

@the-mikedavis I think I was able to resolve everything. I just recently stumbled on this comment though: nix-community/tree-sitter-nix#13 (comment) and now I'm wondering if I should rebase on oxalica's fork to add the injections. Although what I'd really like to do is get those changes incooperated into upstream properly.

I think I'll give this a shot later. Maybe we should hold off on merging for a bit just in case I'm successful?

@the-mikedavis
Copy link
Member

I think this is a good increment even without shell injections - that can always be a follow-up PR. Would you mind rebasing?

@nrdxp
Copy link
Contributor Author

nrdxp commented May 29, 2022

I went ahead and force pushed a rebase, and added this back:

diff --git a/runtime/queries/nix/highlights.scm b/runtime/queries/nix/highlights.scm
index d15fb9c2..a171a39b 100644
--- a/runtime/queries/nix/highlights.scm
+++ b/runtime/queries/nix/highlights.scm
@@ -72,9 +72,15 @@
 (binary_expression
   operator: _ @operator)
 
+(variable_expression (identifier) @variable)
+
 (binding
   attrpath: (attrpath (identifier)) @variable.other.member)
 
+(identifier) @variable.other.member
+
+(inherit_from attrs: (inherited_attrs attr: (identifier) @variable.other.member) )
+
 [
   ";"
   "."

Without it, I experienced a regression in path type highlighting.

@the-mikedavis
Copy link
Member

The

(variable_expression (identifier) @variable)

stanza is a good catch: it highlights variables in inherit-from statements:

{
  inherit (common) pkgs;
  #         ^ variable
  #                 ^ variable.other.member
}

The

(identifier) @variable.other.member

stanza takes priority over the

(identifier) @variable

on the last line, so let's remove the (identifier) @variable one since it's now redundant - the highlights look much better with the @variable.other.member capture.

The

(inherit_from attrs: (inherited_attrs attr: (identifier) @variable.other.member))

doesn't appear to have an effect because of the (identifier) @variable.other.member above it, I think we're safe to remove it. Based on the old highlights, it looks like the recent refactor upstream combined the old attr_identifier and identifier nodes which is what I think led to the redundant stanzas in the upstream queries like the above one. I'll make a PR to remove the redundant ones upstream.

By path highlights did you mean like ./shell.nix in import ./shell.nix or in attribute paths? Those stanzas shouldn't interact with the file-system path highlights so I think you may have seen #2484 if that's what you're talking about.

@the-mikedavis the-mikedavis self-requested a review May 30, 2022 00:01
Update grammar and queiries to the latest upstream. Also add improved
indents thanks to @oxalica.
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for working on this!

@the-mikedavis the-mikedavis merged commit eba8225 into helix-editor:master May 30, 2022
@nrdxp
Copy link
Contributor Author

nrdxp commented May 30, 2022

Thanks for the feedback. Also, just to confirm, it is #2484 that is the problem. After opening the same file several times I noticed sometimes paths are highlighted correcly, and sometimes they are just hightlighted as regular strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants