Rework and fix Attrpath and Dynamic parsing #106

oxalica · 2022-07-19T15:19:08Z

Summary & Motivation

Rework and fix Attrpath and Dynamic parsing

Current handling of Attrpath and Dynamic ${ is mostly incorrect.

The dynamic attribute ${ can only appear as an Attr inside an Attrpath,
and itself cannot make up an expression.

But currently we parse it as a kind of Expr.

This commit reject this usage.
Dot separated attributes, aka. Attrpath or Key, can appear in 3 places:
1. After a "." in a Select expression
  <expression>.<attrpath>
  
  https://github.com/NixOS/nix/blob/2.10.3/src/libexpr/parser.y#L439
2. After a "?" in a HasAttr expression
  <expression> ? <attrpath>
  
  https://github.com/NixOS/nix/blob/2.10.3/src/libexpr/parser.y#L416
3. As a binding key in Let or Attrset-like expression
  <attrpath> = <expression>;
  
  https://github.com/NixOS/nix/blob/2.10.3/src/libexpr/parser.y#L541
To make it consistent, all usage above are now wrapped in a
NODE_ATTRPATH,
which consists of multiple {NODE_DYNAMIC,NODE_IDENT,NODE_STRING}
interspersed by TOKEN_DOT.

NODE_KEY_VALUE is also renamed to NODE_ATTRPATH_VALUE.

Benefiting from flat Attrpath instead of nested one, we can now get
rid of NODE_OR_DEFAULT, since it can be trivially distinguished by
checking if there is an or in NODE_ATTRPATH_VALUE.

Backwards-incompatible changes

NODE_OR_DEFAULT is removed
NODE_KEY -> NODE_ATTRPATH, NODE_KEY_VALUE -> NODE_ATTRPATH_VALUE
BinOp::IsSet is removed since it's actually not a normal binary operator.
NODE_SELECT is flatten in favor of NODE_ATTRPATH which consists of multiple Attrs, including a.b.c and a.b.c or expr
a or b is Apply, not "OrDefault", which matches the result of Nix. Fixes Doesn't parse valid Nix edge-cases #23
${ is considered invalid at Expr places now, which matches the result of Nix.

Further context

~~The name of NODE_KEY is weird to me. It's called attrpath in Nix and in nixpkgs.~~
~~Should we rename it? This would also cause API breakage.~~

Renamed.

darichey

Awesome! This has been on my todo list for a bit :)

The name of NODE_KEY is weird to me. It's called attrpath in Nix and in nixpkgs.
Should we rename it?

+1 to renaming

a or b is Apply, not "OrDefault", which matches the result of Nix.

Does this fix #23?

src/types.rs

src/kinds.rs

oxalica · 2022-07-20T04:33:14Z

Does this fix #23?

Yes. I referenced it in the description now.

oxalica · 2022-07-20T04:58:09Z

The name of NODE_KEY is weird to me. It's called attrpath in Nix and in nixpkgs.
Should we rename it?

+1 to renaming

I'd prefer to keep this PR focused on parsing fixes, and leave the renaming to another PR later. Maybe after the refactoring of high-level AST in #105.

darichey

Last thing from me: Theimpl TryFrom<SyntaxNode> for ParsedType should be updated to account for ParsedType::HasAttr as well.

src/types.rs

src/parser.rs

oberblastmeister · 2022-07-23T15:38:51Z

@oxalica This looks good. I'll merge after it's rebased

Current handling of Attrpath and Dynamic `${` is mostly incorrect. - The dynamic attribute `${` can only appear as an Attr inside an Attrpath, and itself cannot make up an expression. But currently we parse it as a kind of Expr. This commit reject this usage. - Dot separated attributes, aka. Attrpath or Key, can appear in 3 places: 1. After a "." in a Select expression `<expression>.<attrpath>` https://github.com/NixOS/nix/blob/2.10.3/src/libexpr/parser.y#L439 2. After a "?" in a HasAttr expression `<expression> ? <attrpath>` https://github.com/NixOS/nix/blob/2.10.3/src/libexpr/parser.y#L416 3. As a binding key in Let or Attrset-like expression `<attrpath> = <expression>;` https://github.com/NixOS/nix/blob/2.10.3/src/libexpr/parser.y#L541 To make it consistent, all usage above are now wrapped in a NODE_ATTRPATH, which consists of multiple {NODE_DYNAMIC,NODE_IDENT,NODE_STRING} interspersed by TOKEN_DOT. NODE_KEY_VALUE is also renamed to NODE_ATTRPATH_VALUE. Benefiting from flat Attrpath instead of nested one, we can now get rid of NODE_OR_DEFAULT, since it can be trivially distinguished by checking if there is an `or` in NODE_ATTRPATH_VALUE.

oberblastmeister · 2022-07-23T16:27:29Z

This or business is incredibly sus (in the nix language).

oberblastmeister · 2022-07-23T16:33:36Z

It looks like we are parsing or wrong. In the nix parser or is only an identifier when it is used in an apply expression. In our parser or becomes an identifier when it is not used directly after an attrpath in a select expression which is too liberal.

oberblastmeister · 2022-07-23T16:37:36Z

This pr already does lots of good improvements, so the issue might not be in the scope of this pr.
It might be good to be more liberal, and do a separate pass to warn on using or incorrectly. This would be better for error handling because one incorrect or won't trigger a bunch of parse errors (because of recovery), and will only trigger one precise dedicated error.

oxalica · 2022-07-23T16:40:49Z

It looks like we are parsing or wrong.

Yes, it's still not perfect but maybe it's kind of off-topic for this PR.

This or business is incredibly sus (in the nix language).
In the nix parser or is only an identifier when it is used in an apply expression.

No "only". It's also valid in attrpath let or = 1; in .... Not any "apply expression", it's only valid for the direct RHS of Apply.

or 1 -> no parse
1 or -> ok

And the "fallback to ident" takes precedence over left-associative of Apply. 🤯

fold or false -> (fold or) false
fold false or -> foldr (false or)
fold or or -> no parse

oberblastmeister · 2022-07-23T16:52:32Z

No "only". It's also valid in attrpath let or = 1; in ....

I was only highlighting the differences, we obviously already parse this correctly

No "only". It's also valid in attrpath let or = 1; in .... Not any "apply expression", it's only valid for the direct RHS of Apply.

Thanks bro

And the "fallback to ident" takes precedence over left-associative of Apply.

Yeah unfortunate. That's what happens when you put call inside of inside of expr_select, too high precedence!

oberblastmeister · 2022-07-23T17:05:22Z

src/macros.rs

@@ -7,6 +7,7 @@ macro_rules! T {
    (in)      => ($crate::SyntaxKind::TOKEN_IN);
    (inherit) => ($crate::SyntaxKind::TOKEN_INHERIT);
    (let)     => ($crate::SyntaxKind::TOKEN_LET);
+    (or)      => ($crate::SyntaxKind::TOKEN_OR);


Why is this here?

oberblastmeister · 2022-07-23T18:21:08Z

Either or needs to be added as a new token in the lexer, or a special case is needed. @oxalica can you do this? see #115

oxalica · 2022-07-24T02:33:49Z

@oberblastmeister

Either or needs to be added as a new token in the lexer, or a special case is needed.

I created #116 for the fix.

BTW: You are able to push to my PR branch before merging it, if you have write access to the repo, I think?

darichey suggested changes Jul 19, 2022

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

src/kinds.rs Outdated Show resolved Hide resolved

oxalica force-pushed the fix/attrpath branch from db772e9 to a238ebe Compare July 20, 2022 04:47

darichey suggested changes Jul 20, 2022

View reviewed changes

fogti reviewed Jul 21, 2022

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

src/parser.rs Show resolved Hide resolved

darichey mentioned this pull request Jul 22, 2022

Verifying parser correctness #107

Open

oxalica added 3 commits July 24, 2022 00:04

Format

d0ef891

Add test for multiple ? operators

99b5e9c

oxalica force-pushed the fix/attrpath branch from c8666a7 to 99b5e9c Compare July 23, 2022 16:05

oxalica added 2 commits July 24, 2022 00:06

Remove BinOpKind::IsSet in favor of struct HasAttr

059ddcb

Add test for contextual "or" keyword

34f3ed5

oberblastmeister reviewed Jul 23, 2022

View reviewed changes

oberblastmeister merged commit a22982c into nix-community:master Jul 23, 2022

oberblastmeister pushed a commit to oberblastmeister/rnix-parser that referenced this pull request Jul 23, 2022

Rework and fix Attrpath and Dynamic parsing (nix-community#106)

b01384d

oxalica deleted the fix/attrpath branch July 24, 2022 02:26

oxalica mentioned this pull request Jul 24, 2022

Mimic Nix's behavior of contextual keyword "or" #116

Merged

darichey mentioned this pull request Jul 28, 2022

or is not accepted as an identifier #127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework and fix Attrpath and Dynamic parsing #106

Rework and fix Attrpath and Dynamic parsing #106

oxalica commented Jul 19, 2022 •

edited

Loading

darichey left a comment

oxalica commented Jul 20, 2022

oxalica commented Jul 20, 2022

darichey left a comment

oberblastmeister commented Jul 23, 2022

oberblastmeister commented Jul 23, 2022

oberblastmeister commented Jul 23, 2022

oberblastmeister commented Jul 23, 2022

oxalica commented Jul 23, 2022 •

edited

Loading

oberblastmeister commented Jul 23, 2022

oberblastmeister Jul 23, 2022

oberblastmeister commented Jul 23, 2022

oxalica commented Jul 24, 2022

Rework and fix Attrpath and Dynamic parsing #106

Rework and fix Attrpath and Dynamic parsing #106

Conversation

oxalica commented Jul 19, 2022 • edited Loading

Summary & Motivation

Backwards-incompatible changes

Further context

darichey left a comment

Choose a reason for hiding this comment

oxalica commented Jul 20, 2022

oxalica commented Jul 20, 2022

darichey left a comment

Choose a reason for hiding this comment

oberblastmeister commented Jul 23, 2022

oberblastmeister commented Jul 23, 2022

oberblastmeister commented Jul 23, 2022

oberblastmeister commented Jul 23, 2022

oxalica commented Jul 23, 2022 • edited Loading

oberblastmeister commented Jul 23, 2022

oberblastmeister Jul 23, 2022

Choose a reason for hiding this comment

oberblastmeister commented Jul 23, 2022

oxalica commented Jul 24, 2022

oxalica commented Jul 19, 2022 •

edited

Loading

oxalica commented Jul 23, 2022 •

edited

Loading