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

Support paths with dynamic interpolation from Nix 2.4+ #52

Merged
merged 1 commit into from
Nov 23, 2021
Merged

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Nov 10, 2021

⚠️ Warning: not ready to be merged. This is my first deep-dive and I'm a little afraid of regressions, so I'd first like to use this with rnix-lsp locally to test it a bit :)

Summary & Motivation

Since Nix 2.4 it's possible to perform string interpolations within
paths:

nix-repl> let a = "rnix"; in /home/ma27/Projects/${a}-parser
/home/ma27/Projects/rnix-parser

Please note that the following expressions are not supported (by both
Nix and this patch):

  • NIX_PATH imports, i.e. <nixpkgs/${foo}>
  • Interpolations before the first slash, i.e. foo${bar}/baz.

The newly introduced token NODE_PATH_WITH_INTERPOL is only used for
these interpolated paths. Literal paths are still TOKEN_PATH inside
NODE_LITERAL for backwards compatibility.

Backwards-incompatible changes

none

Further context

Closes #44


cc @nerdypepper

Since Nix 2.4 it's possible to perform string interpolations within
paths:

    nix-repl> let a = "rnix"; in /home/ma27/Projects/${a}-parser
    /home/ma27/Projects/rnix-parser

Please note that the following expressions are not supported (by both
Nix and this patch):

* `NIX_PATH` imports, i.e. `<nixpkgs/${foo}>`
* Interpolations before the first slash, i.e. `foo${bar}/baz`.

The newly introduced token `NODE_PATH_WITH_INTERPOL` is only used for
these interpolated paths. Literal paths are still `TOKEN_PATH` inside
`NODE_LITERAL` for backwards compatibility.

Closes #44
@Ma27 Ma27 added the enhancement New feature or request label Nov 10, 2021
@Ma27 Ma27 added this to the 0.10.0 milestone Nov 10, 2021
@Ma27 Ma27 requested a review from aaronjanse November 10, 2021 23:16
@Ma27 Ma27 marked this pull request as draft November 10, 2021 23:16
@Ma27
Copy link
Member Author

Ma27 commented Nov 10, 2021

The changes can be tested via https://github.com/Ma27/rnix-lsp/tree/experimental.

@Ma27 Ma27 marked this pull request as ready for review November 11, 2021 22:59
@Ma27
Copy link
Member Author

Ma27 commented Nov 11, 2021

Seems to be working for me.

cc @nerdypepper @aaronjanse for a review.

@oppiliappan
Copy link
Contributor

Amazing, thanks! I'll take a look over the weekend.

@aaronjanse
Copy link
Member

Thank you @Ma27!! I'll do some review & local testing over the next few days. I'm excited to have support for this syntax

NODE_IDENT 0..1 {
TOKEN_IDENT("a") 0..1
}
NODE_ERROR 1..7 {
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if the error were more isolated (rather than consuming the rest of the expression in code like a${b}/c + /tmp), but I think we can figure that out once we merge some of the very helpful error handling code that @oberblastmeister has been working on

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll need to think of this later today, but actually we should split 0.10 in 0.10 and 0.11. The reason for this is that I'd like to have a rnix-parser which doesn't provide invalid syntax-errors for new language-features that are part of the current stable Nix (2.4) which will be stable Nix in the upcoming 21.11.

That said, yes, I'd love to have @oberblastmeister's changes at some point, definitely, but I think we should cut 0.10 a bit earlier than planned IMHO :)

Copy link
Member

@aaronjanse aaronjanse left a comment

Choose a reason for hiding this comment

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

This all LGTM. I want to later re-visit error handling in this code, but in the meantime I think that parsing correct Nix syntax is a higher priority.

I'm in favor of merging this PR. I think we should consider releasing 0.10 quickly with just this PR, moving the already-merged breaking-change commits like 56131e0 to 0.11 (then maybe assigning 0.12 to things currently assigned to 0.11).

@Ma27 Ma27 merged commit 1a45713 into master Nov 23, 2021
@Ma27 Ma27 deleted the nix24-paths branch November 23, 2021 00:24
@ncfavier
Copy link
Member

ncfavier commented May 28, 2022

This is not completely in sync with the syntax supported by Nix, for example ./${"a"}.b gives

error: unexpected TOKEN_PATH at 9..10, wanted any of [TOKEN_IDENT]
error: unexpected end of file, wanted any of [TOKEN_IDENT]
NODE_ROOT 0..10 {
  NODE_SELECT 0..10 {
    NODE_PATH_WITH_INTERPOL 0..8 {
      TOKEN_PATH("./") 0..2
      NODE_STRING_INTERPOL 2..8 {
        TOKEN_INTERPOL_START("${") 2..4
        NODE_STRING 4..7 {
          TOKEN_STRING_START("\"") 4..5
          TOKEN_STRING_CONTENT("a") 5..6
          TOKEN_STRING_END("\"") 6..7
        }
        TOKEN_INTERPOL_END("}") 7..8
      }
    }
    TOKEN_DOT(".") 8..9
    NODE_ERROR 9..10 {
      TOKEN_PATH("b") 9..10
    }
  }
}

whereas this syntax is supported by Nix since 2.4 (NixOS/nix#5066).

@Ma27
Copy link
Member Author

Ma27 commented May 28, 2022

Yes, I realized that this is a problem ~a week ago, I started working on a fix, but my time is slightly constrained unfortunately.

@Ma27
Copy link
Member Author

Ma27 commented May 28, 2022

Opened a ticket to keep track of it: #85 :)

@ncfavier
Copy link
Member

Fixed in #86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diversion from nix with splices in paths
4 participants