Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Operator precedence is not respected #75

Closed
wants to merge 3 commits into from

Conversation

tianyu
Copy link
Contributor

@tianyu tianyu commented Mar 11, 2022

This PR contains a failing test showing that operator precedence is not respected.

No idea how to fix it, though :/ Fixed!

@tianyu
Copy link
Contributor Author

tianyu commented Mar 12, 2022

Actually, I'm having trouble parsing expressions like a - b + c.

  • It should be interpreted as (a - b) + c,
  • Instead it's interpreted as a - (b + c).

It makes a big difference!

@tianyu tianyu changed the title Failing test: Operator precedence is not respected in some cases Failing test: Operator precedence is not respected Mar 13, 2022
Comment on lines +58 to +90
@test
fun testOperatorPrecedence() {
val input = CharStreams.fromString("1 - 2 + 3\n")

/*
* Test that the input is parsed like ((a - b) + c).
* The parse tree should look like:
*
* plus
* / \
* minus \
* / \ \
* 1 2 3
*/

val lexer = MiniCalcLexer(input)
val parser = MiniCalcParser(CommonTokenStream(lexer))

val plus = parser.expression() as MiniCalcParser.BinaryOperationContext
assertNotNull(plus.PLUS())

val minus = plus.left as MiniCalcParser.BinaryOperationContext
assertNotNull(minus.MINUS())

val one = minus.left as MiniCalcParser.IntLiteralContext
assertEquals(one.text, "1")

val two = minus.right as MiniCalcParser.IntLiteralContext
assertEquals(two.text, "2")

val three = plus.right as MiniCalcParser.IntLiteralContext
assertEquals(three.text, "3")
}
Copy link
Contributor Author

@tianyu tianyu Mar 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaccoff I've changed the grammar back to the way it was and also changed to test to show the even more fundamental error.

Here we see that, according to ANTLR, 1 - 2 + 3 is supposed to be parsed as (1 - 2) + 3.
image

But antlr-kotlin generates code that parses 1 - 2 + 3 as 1 - (2 + 3). It does not adhere to the ANTLR standard!

ANTLR's Parse Tree      antlr-kotlin's Parse Tree
-------------------    ---------------------------
         plus                   minus
        /   \                   /   \
     minus   \                 /    plus
    /   \     \               /     /  \
   1     2     3             1     2    3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bug basically makes antlr-kotlin unusable. Anyone can type 1 - 2 + 3 into a calculator (or Google) and get the answer: 2. But if I parse and evaluate that expression using antlr-kotlin, then I'll get -4!

@tianyu
Copy link
Contributor Author

tianyu commented Mar 13, 2022

@yaccoff @ftomassetti I you can point me in the right direction of where the bug could be, I'd happily try to contribute a fix.

@tianyu
Copy link
Contributor Author

tianyu commented Mar 14, 2022

@ftomassetti I was able to apply the patch from @atsushieno and fix the failing unit test! Please take a look!

@tianyu tianyu changed the title Failing test: Operator precedence is not respected Fix: Operator precedence is not respected Mar 14, 2022
@tianyu tianyu requested a review from yaccoff September 21, 2022 15:08
@tianyu
Copy link
Contributor Author

tianyu commented Mar 13, 2023

@ftomassetti, will you take a look?

@ftomassetti
Copy link
Member

Included in #107 , so closing

@ftomassetti ftomassetti closed this Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants