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

queries/nix: add injection rule for python test scripts, fix rules #5815

Conversation

pacien
Copy link
Contributor

@pacien pacien commented Feb 3, 2023

No description provided.

@pacien
Copy link
Contributor Author

pacien commented Feb 3, 2023

This adds a missing injection rule for the NixOS VM test scripts written in
Python.

However, I could not get this rule to apply without commenting the one that
preceeds it.

Re-ordering the rules in the file does not seem to change anything either.

Is there a way to set some priority on the rules?

This would also be useful to allow the comment rule to take precedence, so that
syntax highlighting can be manually overridden.

(CC past file committers: @nrdxp, @the-mikedavis, @farwyler)

@pacien pacien changed the title queries/nix: add injection rule for python test scripts queries/nix: add injection rule for python test scripts (broken, help needed) Feb 3, 2023
@pacien
Copy link
Contributor Author

pacien commented Feb 8, 2023

Here's an example of the issue I'm observing due to the lack of
priority/ordering of the rules: some snippets seem to have some incorrect
syntax highlighting.

16758893636434

{ pkgs, ... }:

{
  # highlighted as python as it should ("writePython3Bin" rule)
  package = pkgs.writers.writePython3Bin "test" { } ''
    from sys import stdin
    print(i**2 for i in range(1, 10))
  '';

  # highlighted as python as it should (comment rule)
  fromComment = /* python */ ''
    from sys import stdin
    print(i**2 for i in range(1, 10))
  '';

  # highlighted as bash as it should ("*Script" rule)
  some.dummyScript = ''
    from sys import stdin
    print(i**2 for i in range(1, 10))
  '';

  # highlighted weirdly? Should be highlighted as python (comment rule)
  another.dummyScript = /* python */ ''
    from sys import stdin
    print(i**2 for i in range(1, 10))
  '';

  # highlighted weirdly? Should be highlighted as python ("testScript" rule)
  some.testScript = ''
    from sys import stdin
    print(i**2 for i in range(1, 10))
  '';

  # highleighted weirdly? Should be highligted as python (comment rule)
  another.testScript = /* python */ ''
    from sys import stdin
    print(i**2 for i in range(1, 10))
  '';
}

@nrdxp
Copy link
Contributor

nrdxp commented Feb 9, 2023

I think I saw something like this in my own queries too. IIRC this might have something to do with the injections.combined, which does a little bit more than I want it to. perhaps @the-mikedavis has some insight?

@archseer
Copy link
Member

I think all of these snippets should drop injections.combined it's intended for filetypes like erb or handlebars where all the fragments in the file combine into one large document, whereas here every individual case should be parsed separately

@pacien pacien force-pushed the queries-nix-add-nixos-python-test-scripts branch from abe223c to 16fafce Compare September 15, 2023 06:31
@pacien
Copy link
Contributor Author

pacien commented Sep 15, 2023

I think all of these snippets should drop injections.combined
it's intended for filetypes like erb or handlebars where all the
fragments in the file combine into one large document, whereas here
every individual case should be parsed separately

With injections.combined removed, the last three examples do not seem to be
affected by the Python rule at all anymore.

What's the correct way to set overrides / priorities without combining?

16947594436335

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.

Stanzas that are higher in the file will have higher precedence but we will also need to tune the comment injection rule on L5-7. The dummyScript with a comment case is failing because the stanza on L11 starts matching earlier than the comment stanza (L5). We can add another stanza right next to the comment stanza that starts capturing at the same time, so it will have higher precedence:

; mark arbitary languages with a comment
((((comment) @injection.language) .
  (indented_string_expression (string_fragment) @injection.content)))
(binding
  (comment) @injection.language
  expression: (indented_string_expression (string_fragment) @injection.content))

runtime/queries/nix/injections.scm Outdated Show resolved Hide resolved
@pacien pacien force-pushed the queries-nix-add-nixos-python-test-scripts branch from 16fafce to e164e03 Compare September 15, 2023 15:17
@pacien
Copy link
Contributor Author

pacien commented Sep 15, 2023

Thanks, I've applied the suggestions;
Everything is now working as expected!

16947910301975

@pacien pacien marked this pull request as ready for review September 15, 2023 15:19
@pacien pacien changed the title queries/nix: add injection rule for python test scripts (broken, help needed) queries/nix: add injection rule for python test scripts, fix rules Sep 15, 2023
the-mikedavis
the-mikedavis previously approved these changes Sep 15, 2023
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-language-support Area: Support for programming/text languages labels Sep 15, 2023
@nrdxp
Copy link
Contributor

nrdxp commented Sep 15, 2023

fwiw, without injections.combined highlighting stops at the first instance of string iterpolation, which is why I added it in.

@pacien
Copy link
Contributor Author

pacien commented Sep 16, 2023

fwiw, without injections.combined highlighting stops at the first
instance of string iterpolation, which is why I added it in.

This still seems to be working without:

16948443283056

@pascalkuthe
Copy link
Member

interpolation like this is difficult since tis not really valid python and instead essentially an injection insice injection.

I guess the nixos grammar does actually endup producing multiple syntax nodes here so you would essentially want combining injections within a single TS node (but not globally). That doesn't currently exist in helix however.

I think using combined injection does actually represent that more correctly for now. The issues you showed seem odd I don't have time to investigate right now but that looks like an actual highlighting bug. This seems similar to some of the issues I saw (and fixed) in #7621. Jjust to be sure:

Are you using the latest master build?
Otherwire if you could upload a minimal reporduciable example file (with some more details) that would be helpful so I can look into whats causing that.

This rule failed to override other ones because it started its
matching later.
@pacien pacien force-pushed the queries-nix-add-nixos-python-test-scripts branch from e164e03 to 42c3ba8 Compare September 16, 2023 14:26
@pacien
Copy link
Contributor Author

pacien commented Sep 16, 2023

I just tested again rebased on master and without removing the combining,
applying @the-mikedavis' fixes for the order and start of the matching.

It seems to work as expected with the example above in this thread!

@the-mikedavis the-mikedavis merged commit 37e48f4 into helix-editor:master Sep 16, 2023
6 checks passed
@the-mikedavis
Copy link
Member

Having scopes for combined injections so that they're limited to some part of the subtree that you select with queries could be useful for other languages (in particular HEEx). That's a feature that we might want to suggest upstream first though since it would benefit any tree-sitter consumer using injections

@pascalkuthe
Copy link
Member

I think we would only need to agree on the query syntax tough. The implementation should be entirely possible on our end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-support Area: Support for programming/text languages S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants