rust-lang / wg-grammar

Where the work of WG-grammar, aiming to provide a canonical grammar for Rust, resides
Apache License 2.0
99 stars 20 forks source link

Update with %% grammar #38

Closed ehuss closed 5 years ago

ehuss commented 5 years ago

Updates using the %% operator.

ehuss commented 5 years ago

This requires the gll version from https://github.com/rust-lang/gll/pull/121 Updated to bump GLL version.

Note: This is really just an experiment, I'm not proposing that this test structure be final. This only includes a few tests.

It would be easier to read/review the snapshots if the debug output included the actual text matched in the leaves.

I think it would be nice to have better support for error testing, possibly something similar to compiletest where there would be annotations on the source to indicate where the error happened. It would also be nice to place multiple errors in one file.

I'm concerned about creating hundreds of tiny files to exercise everything. I don't immediately have good suggestions. Maybe something like having a more compact format for the forest, and being able to place multiple tests in the same file with the text and the result together.

There are a few surprising ambiguities (like 'a being treated as a dyn trait type). I'm curious how rustc disambiguates that.

I left an example of something that should fail in 2018 (Type-dyn-2015.input). Not sure how we're going to handle edition differences.

Compile times are painful. Minimum 2 minutes for any change.

Some rules won't compile with %%. I get a strange error about "expected Option". I didn't really investigate too much. Examples: StructExprFieldsAndBase, StructPatFieldsAndEllipsis. Fixed by removing {...}.

eddyb commented 5 years ago

The issues you hit with StructExprFieldsAndBase are because you replaced:

    | Fields:{ fields:StructExprField* % "," ","? }

with

    | Fields:{ fields:StructExprField* %% "," }

So instead of Fields:{ fields:A B }, you then had just Fields:{ fields:A }. And since {...} is merely grouping syntax, you're left with Fields:fields:A. Two names for one field works (modulo whatever bugs you hit), but it won't trigger the enum-with-fields-in-variants special-casing.

This is why I was contemplating disallowing {...} with a single rule inside (no A B or A|B), although it might be friendlier to disallow giving the same thing two field names.

ehuss commented 5 years ago

Hm, CI failed though it passed locally for me. Looks like the order of elements is not consistent (at least for the error case). Maybe it needs to sort things? Or maybe we'll need a completely different strategy for error tests?

eddyb commented 5 years ago

If something is not deterministic, that's a bug.

qmx commented 5 years ago

build fails on my machine too, investigating

eddyb commented 5 years ago

FWIW, this condition is wrong: if input_consume_left is called in various positions of the input, as opposed to monotonically LTR, it would keep resetting the expected_pats.