Recursive stage2 parser #2405

hryx · 2019-05-02T08:56:38Z

TODO

🏆 Finish implementation such that CI passes
🥊 Remove as many TODO comments as possible by getting answers to questions in code review and creating follow-ups and tickets as needed
💬 Address all review comments
🚿 Squish several commits via interactive rebase to clean up history a bit
📋 Fill out remaining sections of this pull request description

Keep your eyes open for any TODO comments and leave your suggestions or opinions. Some of these comments will likely be retained and their corresponding changes postponed till a later PR, but maybe some of them can be solved now.

Performance impact

Here are some informal tests of parser_test.zig with perf stat -d on x86_64 Linux. In the absence of visualizations and more formal testing, some quick findings based on my system:

CPU cycles, instructions, branches: -55%
Page faults are roughly the same
Real time elapsed: -45%

Readability impact

Indentation stats of std/zig/parse.zig:

master                          | stage2-recursive-parser
--------------------------------+---------------------------------
indent count                    | indent count
------------                    | ------------
    0     92                    |      0   263
    1    241                    |      1   803
    2    123                    |      2   763
    3    291                    |      3   334
    4    654                    |      4   133
    5    662                    |      5    60
    6    700                    |      6    28
    7    347                    |      7     5
    8    229                    |
    9     42                    |
   10     18                    |
avg indentation level: 4.796999 | avg indentation level: 1.827543
source lines of code:      3399 | source lines of code:      2389

Maximum indentation level went from 10 to 7
Average indentation level went from 4.79 to 1.82
Lines of code (excluding blank/comments) went from 3399 to 2389

Follow-up items

Because this was a heavy change, a few ideas and concerns came up along the way which would unnecessarily increase the scope of this PR or add diff noise but which I would like to address.

Better data modeling of VarDecl initialization: Recursive stage2 parser #2405 (comment)
Fix CompareExpr in stage1: https://github.com/ziglang/zig/pull/2405/files#r282849239
ArrayTypeStart can be either an array or a slice, but const and allowzero only work on pointers; slices are pointers but arrays aren't. The PrefixTypeOp rule might need to be updated to prevent those qualifiers on slices, e.g.:

 PrefixTypeOp
     <- QUESTIONMARK
      / KEYWORD_promise MINUSRARROW
-     / ArrayTypeStart (ByteAlign / KEYWORD_const / KEYWORD_volatile / KEYWORD_allowzero)*
+     / ArrayTypeStart
+     / SliceTypeStart (ByteAlign / KEYWORD_const / KEYWORD_volatile / KEYWORD_allowzero)*
      / PtrTypeStart (KEYWORD_align LPAREN Expr (COLON INTEGER COLON INTEGER)? RPAREN / KEYWORD_const / KEYWORD_volatile / KEYWORD_allowzero)*

Implement disallowing duplicate pointer qualifiers in stage1. Add this test to test/compile_errors.zig:

cases.add(
    "duplicate qualifiers",
    \\test "duplicates" {
    \\    var a: *align(4) align(4) i32 = 0;
    \\    var b: *const const i32 = 0;
    \\    var c: *volatile volatile i32 = 0;
    \\    var d: *allowzero allowzero i32 = 0;
    \\}
,
    "tmp.zig:2:22: error: Extra align qualifier",
    "tmp.zig:3:19: error: Extra const qualifier",
    "tmp.zig:4:22: error: Extra volatile qualifier",
    "tmp.zig:5:23: error: Extra allowzero qualifier",
);

The existing parse error system is somewhat cumbersome: Every possible error is its own type, has entries in the switches for the render() and loc() methods, and exists in the ast.Error union. If it's possible, it would be more flexible, concise, legible, and expressive to generate errors from string literals at the site of the error itself. Something like this:

const expr_node = try expectNode(arena, it, tree, parseExpr, "expression or assignment");

Miscellany

🙇‍♂️ Massive thanks to @tyler569 for unblocking me by finding a memory leak I caused along the way.

🌹 I am so sorry I killed your art @Hejsil

std/zig/parse2.zig

Hejsil · 2019-05-02T14:50:37Z

Exciting. I'll look through all of this once you're done :)

andrewrk · 2019-05-08T22:49:09Z

All tests passing 🎉 🎉

Ready for some review?

hryx · 2019-05-08T22:54:13Z

I still have some work to do to finish the PR description, but since this a big one, I suppose it's time to get started. Let's do it!

std/zig/parse.zig

andrewrk · 2019-05-09T05:21:56Z

This branch has too many commits. How should the git history be cleaned up when ready to merge?

For a branch this large, if there are conflicts, merge master into the branch, then fix the conflicts. If there are no conflicts - which it looks to be the case - no work is needed on your part, I'll merge it into master with a merge commit.

andrewrk

This looks amazing. I left a few comments, but I already consider this ready to merge. I'll give @Hejsil a chance to take a look, and then I think it's good to go!

std/zig/parse.zig

Hejsil

Nice work @hryx. I have a few comments.

std/zig/parse.zig

hryx · 2019-05-10T17:33:28Z

Thanks for the review and great comments @Hejsil & @andrewrk. Been a busy week but I will have time to address all these comments this weekend. Looking forward to wrapping up!

Sahnvour · 2019-05-11T10:59:51Z

* This branch has too many commits. How should the git history be cleaned up when ready to merge?

Looking through the commits, I think you can first reduce their number by doing an interactive rebase and use many small commits as fixups for previous ones.

hryx · 2019-05-11T20:54:45Z

Thanks @Sahnvour, that's my plan now. After addressing all these review comments today, I'll turn a bunch of commits into fixups. But since it sounds like having several commits is allowed, I won't obsess over getting it perfect.

hryx · 2019-05-12T01:30:28Z

Those ❌ marks gave me a good scare; all tests pass locally. I assume Azure cancels a run when a newer commit is pushed on the branch.

The `arena` instance being used bythe parse tree was valid and pointed to valid memory, but existed as a local variable inside the stack frame of the `parse` function (the `const arena`), which was never stored anywhere before leaving the scope. This meant that code above the `parse` function saw a valid instance of an `ArenaAllocator` that pointed to the same backing memory, but didn't posess any of the local state built up after the call to `parseRoot`, basically the caller saw an empty arena. This meant that when `deinit` was called, it saw an Arena with 0 allocations in it's `buffer_list` and wasn't able to destroy any of the memory. This caused it to leak and caused FailingAllocator to balk. The fix is to make sure the parse tree is using the same instance of ArenaAllocator as is reported up the call stack, the one inside the `Tree{}` object. I'm not sure why that field is marked with a comment to remove it, as it's used by the `std.ast.Tree.deinit()` function, but this change seems to solve the problem.

hryx · 2019-05-12T09:34:57Z

The git history has been cleaned up. Please don't ask me how I did it. The original 138 commits can be seen at this tag: master...hryx:stage2-recursive-parser-pre-cleanup

Regarding the most recent changes: some other files did get touched, either to address review comments or to make the parser conform more strictly to the grammar rules. Either way, I now consider this ready to merge if there are no more concerns or blockers!

and now, art

⬜ ⬜ ⬜ ⬜ 🌞 ⬜ ⬜ ⬜
⬜ ⬜ ⬜ ⬜ ⬜ ⬜ ⬜ ⬜
⬜ 🎅 💦 ⬜ ⬜ ⬜ ⬜ ⛵
🌊 🌊 🌊 🌊 🌊 🦈 🌊 🌊

std/zig.zig

std/zig/ast.zig

std/zig/render.zig

tiehuis reviewed May 2, 2019

View reviewed changes

std/zig/parse2.zig Outdated Show resolved Hide resolved

hryx marked this pull request as ready for review May 8, 2019 22:53

hryx commented May 8, 2019

View reviewed changes

std/zig/parse.zig Outdated Show resolved Hide resolved

hryx commented May 9, 2019

View reviewed changes

std/zig/parse.zig Outdated Show resolved Hide resolved

andrewrk approved these changes May 9, 2019

View reviewed changes

std/zig/parse.zig Outdated Show resolved Hide resolved

std/zig/parse.zig Outdated Show resolved Hide resolved

std/zig/parse.zig Outdated Show resolved Hide resolved

std/zig/parse.zig Show resolved Hide resolved

daurnimator reviewed May 10, 2019

View reviewed changes

std/zig/parse.zig Outdated Show resolved Hide resolved

Hejsil reviewed May 10, 2019

View reviewed changes

hryx and others added 5 commits May 12, 2019 01:52

Recursive rewrite of stage2 parser, part 1

4e28c25

Merge branch 'master' into rebased

3787f34

Recursive rewrite of stage2 parser, part 3

3a3a738

Recursive rewrite of stage2 parser, final sync

0d62942

hryx force-pushed the stage2-recursive-parser branch from be0f97e to 0d62942 Compare May 12, 2019 09:33

daurnimator reviewed May 12, 2019

View reviewed changes

std/zig.zig Outdated Show resolved Hide resolved

std/zig/ast.zig Show resolved Hide resolved

std/zig/render.zig Show resolved Hide resolved

Undo parse2 import

173142b

andrewrk merged commit ed5a6d7 into ziglang:master May 12, 2019

hryx mentioned this pull request Aug 2, 2019

Proposal: strict ordering of PtrTypeStart and ArrayTypeStart qualifiers #2992

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursive stage2 parser #2405

Recursive stage2 parser #2405

hryx commented May 2, 2019 •

edited

Loading

Hejsil commented May 2, 2019

andrewrk commented May 8, 2019

hryx commented May 8, 2019

andrewrk commented May 9, 2019 •

edited

Loading

andrewrk left a comment

Hejsil left a comment

hryx commented May 10, 2019

Sahnvour commented May 11, 2019

hryx commented May 11, 2019

hryx commented May 12, 2019

hryx commented May 12, 2019

Recursive stage2 parser #2405

Recursive stage2 parser #2405

Conversation

hryx commented May 2, 2019 • edited Loading

TODO

Performance impact

Readability impact

Follow-up items

Miscellany

Hejsil commented May 2, 2019

andrewrk commented May 8, 2019

hryx commented May 8, 2019

andrewrk commented May 9, 2019 • edited Loading

andrewrk left a comment

Choose a reason for hiding this comment

Hejsil left a comment

Choose a reason for hiding this comment

hryx commented May 10, 2019

Sahnvour commented May 11, 2019

hryx commented May 11, 2019

hryx commented May 12, 2019

hryx commented May 12, 2019

hryx commented May 2, 2019 •

edited

Loading

andrewrk commented May 9, 2019 •

edited

Loading