nix-community / rnix-parser

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

Wrap lambda ident params with different node #124

Closed darichey closed 2 years ago

darichey commented 2 years ago

Summary & Motivation

If a lambda's parameter is a NODE_IDENT, it is now wrapped in NODE_IDENT_PARAM.

Backwards-incompatible changes

ast::Param::Ident -> ast::Param::IdentParam which wraps an ast::Ident.

Further context

Fixes #121

aaronjanse commented 2 years ago

Thanks for the PR! Does this handle parameters of lambdas with patterns like { x ? 1, y ? 2 }: x+y?

aaronjanse commented 2 years ago

Oh, I now see the original problem this was meant to address.

@oberblastmeister what do you think?

oberblastmeister commented 2 years ago

I think it would be better to parse as many parameters as we can, and also parse as many applications as we can. So param1: param2: param3: 1 would be

NODE_LAMBDA
  NODE_PARAMS
    TOKEN_IDENT
    TOKEN_IDENT
    TOKEN_IDENT
  ...

And the same for application. Also see that we don't need an extra NODE_IDENT. Would you be willing to do this?

aaronjanse commented 2 years ago

Hmm, I'm not entirely sure if I agree. For example, foo: bar: foo+bar and foo: (bar: foo+bar) would parse very differently despite being syntactically very similar. It might also be awkward for expressions like foo: { bar }: foo+bar.

darichey commented 2 years ago

I think I agree with @aaronjanse. I would say that foo: bar: foo+bar is two lambdas both syntactically and semantically.

Interestingly, the reference implementation squashes nested Apply nodes (e.g., f x y is a single node), but it does not squash nested Lambda nodes.

oberblastmeister commented 2 years ago

Yeah maybe we should not squash parameters. I guess I was thinking of haskell where you can put multiple parameters together, and ghc will parse them together. But nix only has single lambdas, so it makes sense not to parse them together. I'm not sure whether we should squash apply nodes then because doing so makes it asymmetric.

darichey commented 2 years ago

I agree, I don't like the asymmetry, and I don't think we should copy it.

darichey commented 2 years ago

@oberblastmeister What do you want to do here?

oberblastmeister commented 2 years ago

I think we should remove the NODE_IDENT indirection and just use TOKEN_IDENT. But that can be a separate pr.