nix-community / rnix-parser

A Nix parser written in Rust [maintainer=@oberblastmeister]
MIT License
365 stars 44 forks source link

DuplicatedArgs error is semantic, not syntactic #132

Closed darichey closed 2 years ago

darichey commented 2 years ago

I noticed that rnix-parser will throw an error when encountering a duplicated arg in a pattern (the reference impl calls these "formal" args):

fn main() {
    let nix_expr = "{ a, a }: a";
    println!(
        "{:#?}",
        rnix::Root::parse(&nix_expr).ok().unwrap()
    );
}
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: DuplicatedArgs(5..6, "a")', src/main.rs:5:43

In my opinion, the parser is overextending a bit here. I would argue that the program is syntactically valid, but semantically invalid, and therefore should only be rejected later (e.g., in rnix-lsp).

The original justification for the error (see #45 and #53) was to be consistent with the behavior of the reference implementation. But the reference implementation tightly couples parsing and evaluation in a way I do not think we should replicate. For example, even checking for bound variables is done at parse time in the reference impl (see https://github.com/NixOS/nix/blob/e9178d7d4a9d3f689f911440fa71f135e55b570b/src/libexpr/parser.y#L677 and https://github.com/NixOS/nix/blob/e9178d7d4a9d3f689f911440fa71f135e55b570b/src/libexpr/nixexpr.cc#L324)

I would propose removing the check in rnix-parser and reimplementing it in rnix-lsp.

cc'ing the authors of the original issue and PR: @nerdypepper @Ma27

oberblastmeister commented 2 years ago

@darichey This is already removed on my branch, I haven't published it yet though.

oberblastmeister commented 2 years ago

Addressed in #135