Fix precedence issue with unary ops and labelled args

[anmonteiro]

fixes #2200

[bsansouci]

Awesome! Thanks for fixing. Any way we can avoid the parens around the number with unary minus since we know how to parse it correctly?

[jordwalke]

The reason why this is getting so messy, is because unary parsing precedence is very tricky and has special behavior. You can see how it is designated as special in all the precedence rules. To lex a token, which is then parsed as an infix operator =-., and then split it up into an equal sign but then try to treat it as a unary -., is going to be full of tons of bugs because it's too late in the parser - the opportunity to receive the especial behavior of unary parsing precedence is lost. For these kinds of things, the set_lexeme_length is a really powerful hack that can keep the rest of the parser sane.

Diff 1642 which special cased ~lbl= was pretty clean. There were other unrelated parsing issues like let x=-.1 so 1945 was created. That diff 1945 tried to kill two birds with one stone, and maybe that just wasn't the best fit - maybe let x=-.1 warrants a different solution - one that won't require us to put all these patches into the parser .mly phase?

[jordwalke]

When we see let x=-.1, we really do wish we could split the = from the -.1, but not split it in the case of let _ = x=-.1. That seems like a really hard problem.

[jordwalke]

Would it be cleaner to bring back the special token for ~label= since that avoided problems with labeled args, then introduce another special token for let ident =? That's pretty out there, but I think it would cover all the cases.

[anmonteiro]

Indeed #1642 doesn't suffer from this bug. I'll refactor the PR to include that approach.

[anmonteiro]

After some more investigation, I think this is indeed a very hard problem.

With the approach employed by #1642, these wouldn't parse:

let x = (~a: int=- 1) => a;

let x: float=-.1;

The reason being the type constraint. And making yet another special case in the lexer / parser to support this would IMO not justify the cost.

[IwanKaramazow]

What if remove all special logic in lexing/parsing and delay this "feature" to the error stage and recover there?

[anmonteiro]

@jordwalke My latest commit employs the suggestion given by @IwanKaramazow, delaying the parsing of labeled unary ops to the error recovery section.

I feel like the code is much cleaner now as it removes the weird special tokens from the parser / lexer. I think this is ready for another round of review!

[anmonteiro]

Awesome! Thanks for fixing. Any way we can avoid the parens around the number with unary minus since we know how to parse it correctly?

If everyone agrees, I'll address this in a future PR to keep the change set here amenable for review.

[bsansouci]

Whatever works, you guys are the specialists! What I would say is that I hope nobody ever ever ever uses =-. as an infix. And if we wanted to ban that infix forever, I wouldn't mind ;)

[jordwalke]

I'm glad you went with this general approach (thanks for the suggestion @IwanKaramazow). This helps avoid messing with the complicated precedence rules of shift/reduce and handles the squashing at an earlier stage. These earlier hacks to cover less important edge cases keep the overall grammar rules comprehensible. Similar to the approach with optional semicolons we could say that ~label=-.1 isn't actually valid Reason code, but that we go and do the user a favor by correcting some common pitfalls. Thanks as always @anmonteiro

[anmonteiro]

@jordwalke that's a good point, but if you think ~label=-.1 shouldn't be valid Reason code perhaps we shouldn't print it as such:

$ echo 'foo(~x=-.1);' | ./_build/install/default/bin/refmt
foo(~x=-. 1);