Skip to content
This repository was archived by the owner on Oct 31, 2024. It is now read-only.

Fix precedence of negation vs comparison#70

Merged
iconara merged 1 commit into
burtcorp:masterfrom
mntnorv:fix-incorrect-parsing-of-negation-and-comparison
Oct 24, 2023
Merged

Fix precedence of negation vs comparison#70
iconara merged 1 commit into
burtcorp:masterfrom
mntnorv:fix-incorrect-parsing-of-negation-and-comparison

Conversation

@mntnorv

@mntnorv mntnorv commented May 3, 2022

Copy link
Copy Markdown
Contributor

Fixed antlr grammar to correctly prioritize negation vs comparison when parsing.

Before fix, expression !foo == 'bar' was parsed into:

Negate(Comparison(==, Property(Foo), String(bar)))

After fix, expression !foo == 'bar' is parsed into:

Comparison(==, Negate(Property(Foo)), String(bar))

Taking the JS implementation as the source of truth, because it prioritizes negation vs comparison. Example:

search(!foo == `true`, {"foo":1}) // false

First foo (which is equal to 1) is negated, resulting in false. Then, the result is compared to true, so the final result is false. Before the fix, this Java implementation would return true for the same expression, because first foo is compared to true, which results in false (because 1 != true) and the result is negated, resulting in true.

@iconara

iconara commented May 3, 2022

Copy link
Copy Markdown
Collaborator

Hi. I'm not sure what makes this different from #69, but thank you for fixing this! Will this fix #65?

I'm sorry that I'm not very responsive when PRs come in, I don't have much time to spend on this, but I will get this merged and release, I just can't promise you when.

@iconara iconara self-requested a review May 3, 2022 09:05
@mntnorv

mntnorv commented May 3, 2022

Copy link
Copy Markdown
Contributor Author

#69 had some unintended changes pushed and then removed, otherwise the change is the same. Regarding issue #65, I think it should have been solved by PR #66.

@iconara iconara merged commit 2002a7f into burtcorp:master Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants