AzureMarker / intellij-lalrpop

Jetbrains plugin for the LALRPOP parser-generator
MIT License
16 stars 2 forks source link

Remove Context and simplify LpActionCodeEscaper.kt #30

Closed dnbln closed 3 years ago

dnbln commented 3 years ago

First of all, happy new year! I'd like to discuss this change, which came after a realization that the whole Context enum might have been useless(we have field initialization shorthand after all), and the whole commit simplifies LpActionCodeEscaper.kt by removing it. It looks like the only valid usage of {a:a, b:b, c:c} where expanding {<>} to {a, b, c} will fail in the entire rust grammar is in a struct declaration, e.g.:

// Expanding
struct Something {<>}
// to the invalid
struct Something {a, b, c}
// rather than the valid
struct Something {a: a, b: b, c: c}

Though it seems very very unlikely anyone would use <> in a struct declaration, and I do hope nobody uses something like this.

I also tried to change this from lalrpop's sources to use the field init shorthand and ran all of lalrpop's tests; the only failing one was verify_lalrpop_generates_itself but it would fail even without the change(no idea why).

So @Mcat12 what do you think?

Edit: someone mentioned macros and they would indeed make for very weird cases

AzureMarker commented 3 years ago

Happy new year! I think it would be worthwhile to make an issue on lalrpop's repo to get more context on why they don't use field init shorthand (maybe it's just a legacy thing), and what impact it might have if they switch to the shorthand.

I think it's fine for us as an IDE plugin to experiment and make this change. It's pretty easy to roll back. It will probably help inform lalrpop of issues that might might arise if they made the change. The fact that the lalrpop test results didn't change after making the switch is encouraging.

dnbln commented 3 years ago

Created https://github.com/lalrpop/lalrpop/issues/579

AzureMarker commented 3 years ago

Closed by #42