Refactor the parser somewhat #9776

pennae · 2024-01-15T15:53:46Z

Motivation

the parser is currently not in great shape. it's tightly coupled to EvalState, it does more than just parsing, and it contains heaps of not entirely related code in the core parser.y file. let's refactor it a bit to make it easier to work with.

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

this is a proper subset of Formals anyway, so let's just use those and avoid the extra allocations and moves.

since nix doesn't use the bison `error` terminal anywhere any invocation of yyerror will immediately cause a failure. since we're *already* leaking tons of memory whatever little bit bison allocates internally doesn't much matter any more, and we'll be replacing the parser soon anyway. coincidentally this now also matches the error behavior of URIs when they are disabled or ~/ paths in pure eval mode, duplicate attr detection etc.

all of them need access to parser state in some way. make them members to allow this without fussing so much.

ParserState better describes what this struct really is. the parser really does modify its state (most notably position and symbol tables), so calling it that rather than obliquely "data" (which implies being input only) makes sense.

most instances of this being used do not refer to the "current" position, sometimes not even to one reasonably close by. it could also be called `makePos` instead, but `at` seems clear in context.

most EvalState and Expr members defined here could be elsewhere, where they'd be easier to maintain (not being embedded in a file with arcane syntax) and *somewhat* more faithfully placed according to the path of the file they're defined in.

there's no reason the parser itself should be doing semantic analysis like bindVars. split this bit apart (retaining the previous name in EvalState) and have the parser really do *only* parsing, decoupled from EvalState.

these symbols are used a *lot*, so it makes sense to cache them. this mostly increases clarity of the code (however clear one may wish to call the parser desugaring here), but it also provides a small performance benefit.

src/libexpr/eval.cc

Co-authored-by: John Ericson <[email protected]>

tomberek · 2024-01-24T01:43:37Z

src/libexpr/eval.cc

+    }
+
+    else if (hasPrefix(value, "flake:")) {
+        experimentalFeatureSettings.require(Xp::Flakes);


I was not aware of this feature. (Ref: #7026)

tomberek · 2024-01-24T01:49:46Z

src/libexpr/eval.cc

@@ -20,6 +20,9 @@
 #include "gc-small-vector.hh"
 #include "url.hh"
 #include "fetch-to-store.hh"
+#include "tarball.hh"
+#include "flake/flakeref.hh"


This oddity was already in the parser. Moving it here is directionally correct, further future work might refactor this more. It is for the NIX_PATH handling of "flake:something" URLs.

tomberek

This looks to be exactly as advertised, refactoring the parser and separting concerns a bit better than before.

thufschmitt · 2024-01-26T08:55:37Z

Discussed during the Nix maintainers meeting on 2024-01-22.
Idea approved, assigned to @Ericson2314 and @tomberek for further review

- Talked about @pennae's larger plans for a parser rewrite

Exciting stuff: correctness, big performance improvements too.
- Often 20% faster (just parsing, to be clear)
- Not more memory usage
@edolstra says in some ways this is similar to previous iterations of the parser implementation
@roberth: We'll hold of changes to the parser if needed and sync with them and pennae. (also parser PRs are rare)
@tomberek, @roberth: Let's make a sort of announcement issue.
- Explain the intent
- Make sure other Nix implementations (tvix, rnix) are informed.
- We don't get too many parser PRs, but this could be hint for anyone thinking of making one to wait while such big changes are in flight.
Refactor the parser somewhat #9776 assigned to @tomberek & @Ericson2314 for joint review.
@pennae: notes more prep PRs are on the way; looking forward to seeing them!
@tomberek: are we OK dependencies wise?
- Loose 2 gain 1 sounds like a good trade!
@roberth: How is compile time performance?
- @pennae: Haven't tested, but feels fine
- Splitting the very large file (from this prep PR) also helps a lot.
- @roberth: sounds good. If it feels reasonable, that's good enough :).
@Ericson2314: Also excited for the test coverage improvements, independent of parser rewriting.

Goals/requirements

Do not allow users to write new syntax that's currently invalid
- at least not without any warning
Unblock the new parser

Decision Options (decision is #2):

Merge 9722 first
- easiest
Close 9722 (picked!!!)
- next-easiest, but creates larger parser rules
re-write 9722 to disallow arbitrary numbers of "/"s
- most work and changes behavior

Decision: the new PEGTL parser is "Idea Approved"

nixos-discourse · 2024-01-26T09:06:15Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-22-nix-team-meeting-minutes-117/38838/1

src/libexpr/parser-state.hh

Ericson2314

I tried confirming things were just moved around with git diff --color-moved. Looks good!

pennae added 8 commits January 15, 2024 16:52

remove ParserFormals

f07388b

this is a proper subset of Formals anyway, so let's just use those and avoid the extra allocations and moves.

make parser utility functions members of ParseData

1b09b80

all of them need access to parser state in some way. make them members to allow this without fussing so much.

move ParseData to own header, rename to ParserState

0076056

ParserState better describes what this struct really is. the parser really does modify its state (most notably position and symbol tables), so calling it that rather than obliquely "data" (which implies being input only) makes sense.

rename ParserState::{makeCurPos -> at}

835a6c7

most instances of this being used do not refer to the "current" position, sometimes not even to one reasonably close by. it could also be called `makePos` instead, but `at` seems clear in context.

slim down parser.y

e1aa585

most EvalState and Expr members defined here could be elsewhere, where they'd be easier to maintain (not being embedded in a file with arcane syntax) and *somewhat* more faithfully placed according to the path of the file they're defined in.

decouple parser and EvalState

b596cc9

there's no reason the parser itself should be doing semantic analysis like bindVars. split this bit apart (retaining the previous name in EvalState) and have the parser really do *only* parsing, decoupled from EvalState.

don't repeatedly look up ast internal symbols

09a1128

these symbols are used a *lot*, so it makes sense to cache them. this mostly increases clarity of the code (however clear one may wish to call the parser desugaring here), but it also provides a small performance benefit.

pennae requested a review from edolstra as a code owner January 15, 2024 15:53

Ericson2314 reviewed Jan 22, 2024

View reviewed changes

src/libexpr/eval.cc Outdated Show resolved Hide resolved

Update src/libexpr/eval.cc

80b8471

Co-authored-by: John Ericson <[email protected]>

Ericson2314 changed the title ~~refactor the parser somewhat~~ Refactor the parser somewhat Jan 22, 2024

Ericson2314 assigned tomberek and Ericson2314 Jan 22, 2024

tomberek reviewed Jan 24, 2024

View reviewed changes

pennae mentioned this pull request Jan 25, 2024

deduplicate inherit-from source expr work #9847

Merged