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

syntax: correctly highlight paths with interpolation #56

Merged
merged 5 commits into from
Jan 9, 2024

Conversation

Ma27
Copy link
Collaborator

@Ma27 Ma27 commented Jul 30, 2023

OK I nerdsniped myself into doing that now, damn it 🙃
Anyways, this is a draft on purpose, because there's one known case of a wrong highlight (see commit message below) and I haven't tested this under real conditions yet.
cc @LnL7 @figsoda @aszlig


Closes #41

A few implementation notes:

  • nixInterpolatedPath is on purpose below nixPath, otherwise the
    highlighting of top-level expressions, i.e. files containing e.g.

    ./foo/bar${baz}
    

    break where $ is entirely unmatched and {baz} is misinterpreted as
    argument destructuring expression.

  • The region's matching behavior now works like this:

    First, we expect a path start (i.e. either ~/<further path components>
    or <path components>) and then the beginning of a substitution (${), however
    this isn't part of our match anymore (hence \ze) to make sure this
    is matched by the subsequent nixInterpolation.

    Also, nixInterpolation is the only thing allowed in here because
    nixPath would conflict with our end & skip expression and would
    thus make vim expand the nixInterpolatedPath over the rest of the
    file.

    To summarize, after the start, two things are allowed:

    • An interpolation
    • Valid path chars.

    The skip regex is to make sure that the end isn't satisified too
    early, e.g. in

    ./foo/${bar}/${baz}/suff
    

    the path shouldn't terminate after ${bar}/ since another
    substitution is followed by only valid path chars.

    The end regex makes sure that the last substitution was closed
    (nixinterpolatedPath always contains a substitution because of
    start) and only valid path chars are matched after that. This means
    that e.g. in

    ./foo/${bar}!x
    

    everything after ${bar} is not part of the path anymore.

  • In the end a region turned out to be slightly more suitable, so this
    cannot be composed into a single group with the matcher nixPath.

    Instead, nixInterpolatedPath's highlighting group now links to
    nixPath to make sure they look the same in the end.

@Ma27

This comment was marked as outdated.

@aszlig
Copy link
Contributor

aszlig commented Jul 30, 2023

OK, this also breaks string interpolations like "foo ${./bar}" :(

Can you please add a specific test case for this?

Will look into this later.

syntax/nix.vim Outdated
@@ -50,6 +50,7 @@ syn region nixString matchgroup=nixStringDelimiter start=+''+ skip=+''['$\\]+ en
syn match nixFunctionCall "[a-zA-Z_][a-zA-Z0-9_'-]*"

syn match nixPath "[a-zA-Z0-9._+-]*\%(/[a-zA-Z0-9._+-]\+\)\+"
syn match nixInterpolatedPath "\%(\~\|[a-zA-Z0-9._+-]*\)\%\(/\).*\}\%([A-Za-z0-9._+-/]*\)" contains=nixPath,nixInterpolation

This comment was marked as outdated.

Closes LnL7#41

A few implementation notes:

* `nixInterpolatedPath` is on purpose below `nixPath`, otherwise the
  highlighting of top-level expressions, i.e. files containing e.g.

      ./foo/bar${baz}

  break where `$` is entirely unmatched and `{baz}` is misinterpreted as
  argument destructuring expression.

* The region's matching behavior now works like this:

  First, we expect a path start (i.e. either `~/<further path components>`
  or `<path components>`) and then the beginning of a substitution (`${`), however
  this isn't part of our match anymore (hence `\ze`) to make sure this
  is matched by the subsequent `nixInterpolation`.

  Also, `nixInterpolation` is the only thing allowed in here because
  `nixPath` would conflict with our `end` & `skip` expression and would
  thus make vim expand the `nixInterpolatedPath` over the rest of the
  file.

  To summarize, after the start, two things are allowed:

  * An interpolation
  * Valid path chars.

  The `skip` regex is to make sure that the `end` isn't satisified too
  early, e.g. in

      ./foo/${bar}/${baz}/suff

  the path shouldn't terminate after `${bar}/` since another
  substitution is followed by only valid path chars.

  The end regex makes sure that the last substitution was closed
  (`nixinterpolatedPath` always contains a substitution because of
  `start`) and only valid path chars are matched after that. This means
  that e.g. in

      ./foo/${bar}!x

  everything after `${bar}` is not part of the path anymore.

* In the end a region turned out to be slightly more suitable, so this
  cannot be composed into a single group with the matcher `nixPath`.

  Instead, `nixInterpolatedPath`'s highlighting group now links to
  `nixPath` to make sure they look the same in the end.
@Ma27
Copy link
Collaborator Author

Ma27 commented Jul 30, 2023

After playing around with regions, I got something which appears to work more reliably. Also added two new testcases for the issues I encountered with the previous matcher I wrote (path-with-invalid-infix and path-with-recursive-interpolation).

syntax/nix.vim Outdated Show resolved Hide resolved
For instance

    foo/${
      "bar"
    }

is valid Nix code.

Turns out I can simply remove the `oneline` because it doesn't appear to
make a difference for the other rules (I originally introduced it to
avoid matches over multiple lines, but the root cause was a bogus `end`
regex anyways).

Co-authored-by: aszlig <[email protected]>
syntax/nix.vim Outdated Show resolved Hide resolved
syntax/nix.vim Outdated Show resolved Hide resolved
Copy link
Contributor

@aszlig aszlig left a comment

Choose a reason for hiding this comment

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

Thanks a lot :-)

I personally don't like the assertions at the region end match, but they do the job and we can still improve it later iff they become a real issue1.

Footnotes

  1. I highly doubt that anyone would run into the recursion limit, but you'll never know...

* `/[...]+|/` is equivalent to `/[...]*`.
* The surrounding group isn't needed.

Co-authored-by: aszlig <[email protected]>
@Ma27 Ma27 marked this pull request as ready for review August 6, 2023 12:31
@Ma27
Copy link
Collaborator Author

Ma27 commented Aug 6, 2023

Considering that I haven't found any issues in the past week, I decided to mark it as ready to review :)

cc @figsoda @LnL7 :)

Copy link

@figsoda figsoda left a comment

Choose a reason for hiding this comment

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

sorry about the delay, ./${foo}-${bar}.nix still doesn't seem to work, otherwise lgtm

for reference the documentation seems to have moved to https://nixos.org/manual/nix/unstable/language/string-interpolation#path

@Ma27
Copy link
Collaborator Author

Ma27 commented Aug 6, 2023

@figsoda cannot reproduce. Is the issue also observable with the vim from the repo's nix-shell? When I hover with the cursor over the - or the .nix int he editor and issue :Syntax it always prints out ['nixInterpolatedPath'] which seems correct to me.

Copy link

@figsoda figsoda left a comment

Choose a reason for hiding this comment

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

I think I might have messed up something while testing, lgtm

@Ma27
Copy link
Collaborator Author

Ma27 commented Aug 16, 2023

ping @LnL7 I think this is good to go now :)

@Ma27
Copy link
Collaborator Author

Ma27 commented Oct 9, 2023

ping @LnL7

@Ma27
Copy link
Collaborator Author

Ma27 commented Oct 30, 2023

ping @LnL7 I'd still need an approval from you to merge this. Would be cool if we could get this done :)

@Ma27
Copy link
Collaborator Author

Ma27 commented Jan 9, 2024

I'm merging this now, I'm using this patch in my editor for the last months (and I have Nix code open more often than I'd like to admit) without noticing any issues and others seem to have tested it as well.

@Ma27 Ma27 merged commit 048c71f into LnL7:master Jan 9, 2024
1 check passed
@Ma27 Ma27 deleted the path-interpolation branch January 9, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for antiquotations in paths
4 participants