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 pow and unary operators order #1723

Open
wants to merge 7 commits into
base: stable
Choose a base branch
from

Conversation

krepysh
Copy link
Contributor

@krepysh krepysh commented Sep 19, 2022

Change order of evaluating expressions with pow by Parser.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@krepysh krepysh force-pushed the fix_pow_and_unary_operators_order branch from e22a0d0 to 9ab3dc6 Compare September 19, 2022 19:08
@davidism
Copy link
Member

The changelog entry should be much clearer on what is changing since this affects how these expressions are evaluated.

@bart-nouveau
Copy link

There is still one bug pointed out in #1720 that would not be fixed by this pull request:

expression: '(-1) ** x'
jinja_output: '-1'
python_output: 1

minimal reproducible example

from jinja2 import Environment
env = Environment()
expression = '(-1) ** x'
env.globals = {"x": 2}
jinja_output = env.from_string(f"{{{{ {expression} }}}}").render()
python_output = str(eval(f"{expression}",env.globals))
assert python_output == jinja_output

suggested fix

One possible fix could be to modify the _make_binop function here as follows:

            self.write("(")
            self.visit(node.left, frame)
            self.write(f" {op} ")
            self.visit(node.right, frame)

changed into:

            self.write("((")
            self.visit(node.left, frame)
            self.write(")")
            self.write(f" {op} ")
            self.visit(node.right, frame)

def test_pow_associative_from_left_to_right(self, env):
tmpl = env.from_string("{{ 2 ** 3 ** 2 }}")
assert tmpl.render() == "512"

Copy link

@bart-nouveau bart-nouveau Sep 22, 2022

Choose a reason for hiding this comment

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

In order to verify if issue #1720 is fixed, I would add the following tests:

    def test_pow_negative_base(self, env):
        tmpl = env.from_string("{{ (- 1) ** 2 }}")
        assert tmpl.render() == "1"

    def test_pow_negative_base_variable_exponent(self, env):
        tmpl = env.from_string("{{ (- 1) ** x }}", {"x": 2})
        assert tmpl.render() == "1"

    def test_negative_pow_variable_exponent(self, env):
        tmpl = env.from_string("{{ - 1 ** x }}", {"x": 2})
        assert tmpl.render() == "-1"

All the tests seems to pass adding the change suggested here

``{{ 2**3 }}`` would return ``8``

.. versionchanged:: 3.2
From 3.2, this operator has the same evaluation order as in Python. ``{{ 2 ** 3 ** 2 }}`` will be evaluated as ``2 ** (3 ** 2)``.

Unlike Python, chained pow is evaluated left to right.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidism, should I remove this outdated explanation? Or it will be better to leave it here in order to keep history in docs?

@krepysh krepysh marked this pull request as ready for review November 25, 2022 16:27
@krepysh krepysh requested a review from bart-nouveau November 25, 2022 16:27
Copy link

@bart-nouveau bart-nouveau left a comment

Choose a reason for hiding this comment

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

It's ok for me, it fixes issues :issue:1720 :issue:`1722

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants