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

Bind custom operators between mul and unary #895

Merged
merged 8 commits into from
Jul 21, 2022

Conversation

tkindy
Copy link
Contributor

@tkindy tkindy commented Jul 20, 2022

Closes #893

Currently, our custom operators like | and is bind tighter than unary operators. This means -1 is integer incorrectly evaluates like -(1 is integer), leading to an error when Jinjava tries to make true negative. This is because these operators are defined within value which is called by unary, and therefore have precedence greater than the unary operators.

This PR pulls our custom operators up from the unary level of the JUEL grammar to the mul level. This means they bind tighter than mul but less tightly than unary. From my testing, this aligns with Jinja's precedence rules.

@tkindy tkindy requested a review from jasmith-hs July 20, 2022 17:44
@tkindy tkindy mentioned this pull request Jul 20, 2022
Copy link
Contributor

@jasmith-hs jasmith-hs left a comment

Choose a reason for hiding this comment

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

assertThat(interpreter.render("{{ (-5 * -4 | abs) }}")).isEqualTo("-20");

I think the filter operation should be at the same level as unary rather than the same level as mul.
Might make sense to add a new filter(boolean required) method and have mul() point to that instead of unary() and have the filter method call unary and do the | and is logic

@tkindy
Copy link
Contributor Author

tkindy commented Jul 21, 2022

assertThat(interpreter.render("{{ (-5 * -4 | abs) }}")).isEqualTo("-20");

Thanks for the extra test case! I confirmed Jinja indeed returns -20 here.

image

I think the filter operation should be at the same level as unary rather than the same level as mul.

I think you mean filters should bind tighter than mul but looser than unary; that is, that we should introduce a new level of precedence. They currently bind at the same level as unary, and that's the problem.

Your implementation suggestion sounds good to me, I'll try that out.

@tkindy tkindy changed the title Apply custom operators at mul precedence Bind custom operators between mul and unary Jul 21, 2022
@tkindy tkindy requested a review from jasmith-hs July 21, 2022 20:13
Copy link
Contributor

@jasmith-hs jasmith-hs left a comment

Choose a reason for hiding this comment

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

LGTM

@tkindy tkindy merged commit d2756ef into master Jul 21, 2022
@tkindy tkindy deleted the tk/fix-operator-precedence branch July 21, 2022 20:27
tkindy added a commit that referenced this pull request Jul 22, 2022
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.

Unary minus binds too loosely
2 participants