improve lambda syntax (allow single expression)#18924
Conversation
| setState(449); | ||
| block(); | ||
| setState(451); | ||
| switch ( getInterpreter().adaptivePredict(_input,38,_ctx) ) { |
There was a problem hiding this comment.
All adaptivePredict calls are there because of some ambiguity. In this case I don't expect it to be particularly slow because it has to be after a lambda rather than every expression like ; was.
It is sort of hard review the code and see the ambiguity without tracing inside of adaptivePredict and having a look at the path it takes here. And the path it takes comes from the big string blob.
There was a problem hiding this comment.
yeah, i have no idea about this adaptive prediction, i know we have 36 of these calls today, but no tests are failing (and they fail on the "requires full context" and other stuff like that, that would be super slow). Also unrelated, i have no idea why, in the braces case, the semicolon is still required! Maybe that is related.
There was a problem hiding this comment.
in the braces case, the semicolon is still required!
I can have a look at that after you merge this if you like. The semicolon not required fro braces case is a sneaky lexer trick so it shouldn't have anything to do with the parser....
There was a problem hiding this comment.
ok, the required semicolon is actually independent of this PR (existing issue). I hit that problem first, and tried to figure it out but couldn't. So i worked on this issue to at least make progress. Lemme throw some more tests at it to look for real ambiguities.
There was a problem hiding this comment.
Nik is technically correct, but in this case were just trying to avoid the case where it has to fallback to picking the first rule because two full paths match. As long as k != infinite (looking ahead all the tokens within the script) I think were fine because ANTLR can still make a decision in a reasonable amount of time.
There was a problem hiding this comment.
@nik9000 On the brace issue -- I haven't had much time to investigate this. So if you wanted to take a look it would be much appreciated. I guess to get this bug to work now the lambda would have to use a block.
There was a problem hiding this comment.
I'm finishing up regex flags so after I make a PR for that I'll have a look at braces in lambdas.
|
Neat. It LGTM but I think @jdconrad will want to look too. As for ambiguity - it might be worth testing lambdas with and without in a for loop and comparing the compilation time. I still think it should be ok though. |
|
...I have no idea about antlr, but the syntax improevemnts look good. The main difference in Java is: with |
|
Yes its the same here. |
|
+1 here. If ambiguities come up during testing, we'll resolve them then. Mimicking Java exactly to have expressions return nothing would be a bit more difficult since we would have to turn off auto-return, and it would be really difficult if we didn't make the expression into a statement node with the way things are currently setup. |
|
I added a few more simple tests: lambda inside loop, nested lambda, method taking two lambda parameters. |
Currently lambdas need both braces and semicolons, which is onerous for many situations. Java allows a single expression too, instead of:
I really have no idea if it creates ambiguity or problems, but it seems to work. Someone that actually knows grammars needs to help me with this change :)