Strumenta / antlr-kotlin

Support for Kotlin as a target for ANTLR 4
Apache License 2.0
224 stars 47 forks source link

Wrong precedence in left-recursive rules #22

Closed mstrohbach closed 8 months ago

mstrohbach commented 5 years ago

It seems that antlr-kotlin does not properly handle precedence in left-recursive rules. Consider for instance the expression rule in the MiniCalc example:

expression : left=expression operator=(DIVISION|ASTERISK) right=expression # binaryOperation
           | left=expression operator=(PLUS|MINUS) right=expression        # binaryOperation
           | value=expression AS targetType=type                           # typeConversion
           | LPAREN expression RPAREN                                      # parenExpression
           | ID                                                            # valueReference
           | MINUS expression                                              # minusExpression
           | STRING_OPEN (parts+=stringLiteralContent)* STRING_CLOSE       # stringLiteral
           | INTLIT                                                        # intLiteral
           | DECLIT                                                        # decimalLiteral ;

The expression b/3+2 is parsed as expected by the Intellij ANTLR v4 grammar plugin (v1.10): plugin-tree But antlr-kotlin groups 3+2 as a child to the / operator: antlr-kotlin-tree

I get the same behaviour in my own grammars.

I am using the antlr-kotlin plugin with the default settings antlrKotlinVersion = "0.0.4" and antlrVersion = "4.7.1"

ftomassetti commented 5 years ago

Hi Martin, thank you for reporting this.

I am not sure when I will be able to work on this but a PR would be appreciated at any time :)

This issue surprise me a bit because ANTLR Kotlin is basically a translation of the original ANTLR implementation written in Java, so it is very curious we introduced this bug while translating.

mstrohbach commented 5 years ago

Hi Federico,

Thanks for your reply. I looked into the issue and the problem seems to be that a method in Parser.kt has been commented out.

Regards

Martin

ftomassetti commented 5 years ago

Thank you Martin for your investigation!

edwinRNDR commented 4 years ago

Has this ever been resolved? I seem to still run into the issue using 94f8764bcf

edwinRNDR commented 4 years ago

I guess not. I tried the same grammar (something very similar to MiniCalc) with the Java based Antlr tooling and there the operator precedence works as expected.

atsushieno commented 3 years ago

I had been similarly stuck at false-positive ambiguity detection failure that IDEA ANTLR v4 plugin never complains. I tried to fix it by actually uncommenting various code bits around Parser.kt, but it did not make it any better for me.

I leave my patch here so in case anyone finds it useful feel free to take it. https://gist.github.com/atsushieno/e56e451eb3fd42270ed7af5c21e55b86

sergioazua-pc commented 2 years ago

Also experiencing this issue. Antlr-kotlin parsing incorrectly, but getting the correct precedence using antlr4 generated parsers.

sergioazua-pc commented 2 years ago

I had been similarly stuck at false-positive ambiguity detection failure that IDEA ANTLR v4 plugin never complains. I tried to fix it by actually uncommenting various code bits around Parser.kt, but it did not make it any better for me.

I leave my patch here so in case anyone finds it useful feel free to take it. https://gist.github.com/atsushieno/e56e451eb3fd42270ed7af5c21e55b86

This patch seems to fix my precedence issues. Thank you!

jay-karimi commented 2 years ago

Any updates on this? ~I've tried the patch with no success.~

tianyu commented 2 years ago

Thank you @atsushieno for your patch! I've applied it to my repo: https://github.com/tianyu/antlr-kotlin And you can use the maven package here: https://jitpack.io/#tianyu/antlr-kotlin/3bb1661562 And there's a Pull Request here: https://github.com/Strumenta/antlr-kotlin/pull/75

ftomassetti commented 8 months ago

Solved in https://github.com/Strumenta/antlr-kotlin/pull/107, closing (thank you @lppedd for pointing it out)