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

Preserve __slots__ metadata on Undefined types #2026

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ Unreleased
- Use modern packaging metadata with ``pyproject.toml`` instead of ``setup.cfg``.
:pr:`1793`
- Use ``flit_core`` instead of ``setuptools`` as build backend.
- Fix copy/deepcopy/pickle support for ``Undefined`` objects.
:pr:`2026`


Version 3.1.5
Expand Down
19 changes: 7 additions & 12 deletions src/jinja2/runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -982,10 +982,15 @@ class ChainableUndefined(Undefined):
def __html__(self) -> str:
return str(self)

def __getattr__(self, _: str) -> "ChainableUndefined":
def __getattr__(self, name: str) -> "ChainableUndefined":
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. Can you make the comment clearer?

Copy link
Author

@nitzmahone nitzmahone Oct 1, 2024

Choose a reason for hiding this comment

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

This is the same (incomplete, BTW) heuristic exclusion already used by Undefined.__getattr__. ChainableUndefined needs it for the same reason, but there weren't previously any tests that exhibited the bugs caused by the current unconditional behavior.

There are a number of Python interaction protocols that sniff for the presence of a particular dunder method to support builtin behavior. This is one of the places where Jinja's getattr/getitem equivalency causes problems. In this case, copy uses the presence of a __setstate__ method on the type to decide how it will create and populate the copy. Since hasattr(ChainableUndefined, '__setstate__') is true with the current impl, Python assumes it can call that to populate the empty storage for the copy, but blows up when it actually tries to invoke the Undefined object it receives instead of a bound method to fill in an empty object instance.

In general, __getattr__ should raise AttibuteError on a request for any dunder method it doesn't know about- there are all sorts of weird things that can happen in various places in Python if an object confuses the runtime about its support for a particular interaction protocol.

Happy to include a more detailed inline explanation along those lines in the code. but should probably either copy/paste to the original usage or actually share that logic between them.

Also happy to correct the heuristic in both places to only exclude __XYZ__ instead of the current __.* - while probably unlikely, the current impl would incorrectly exclude, eg __fooattr.

# unimplemented dunder methods returning Undefined break core Python protocols
if name[:2] == "__":
raise AttributeError(name)

return self

__getitem__ = __getattr__ # type: ignore
def __getitem__(self, _: str) -> "ChainableUndefined": # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Don't use _ as an argument name. Use specific ignores, not a bare ignore.

Copy link
Author

Choose a reason for hiding this comment

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

lol, that's what PyCharm coughed up for the override signature- I assumed the base class did the same, but apparently it was just confused by the dynamic _fail_with_undefined_error aliasing.

The base class is already doing the same blanket type: ignore on that method due to the aforementioned dynamic aliasing- I detest overly-broad ignores as well, so happy to figure out a more targeted ignore (and of course use a proper arg name).

Copy link
Member

Choose a reason for hiding this comment

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

Remove the ignore, run mypy, add an ignore for the actual finding. Yeah, there's still a bunch of bare ignores from when we first added typing, but I don't want to persist that going forward.

return self


class DebugUndefined(Undefined):
Expand Down Expand Up @@ -1044,13 +1049,3 @@ class StrictUndefined(Undefined):
__iter__ = __str__ = __len__ = Undefined._fail_with_undefined_error
__eq__ = __ne__ = __bool__ = __hash__ = Undefined._fail_with_undefined_error
__contains__ = Undefined._fail_with_undefined_error


# Remove slots attributes, after the metaclass is applied they are
# unneeded and contain wrong data for subclasses.
del (
Undefined.__slots__,
ChainableUndefined.__slots__,
DebugUndefined.__slots__,
StrictUndefined.__slots__,
)
8 changes: 0 additions & 8 deletions tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,6 @@ def test_default_undefined(self):
assert und1 == und2
assert und1 != 42
assert hash(und1) == hash(und2) == hash(Undefined())
with pytest.raises(AttributeError):
getattr(Undefined, "__slots__") # noqa: B009

def test_chainable_undefined(self):
env = Environment(undefined=ChainableUndefined)
Expand All @@ -335,8 +333,6 @@ def test_chainable_undefined(self):
assert env.from_string("{{ foo.missing }}").render(foo=42) == ""
assert env.from_string("{{ not missing }}").render() == "True"
pytest.raises(UndefinedError, env.from_string("{{ missing - 1}}").render)
with pytest.raises(AttributeError):
getattr(ChainableUndefined, "__slots__") # noqa: B009

# The following tests ensure subclass functionality works as expected
assert env.from_string('{{ missing.bar["baz"] }}').render() == ""
Expand Down Expand Up @@ -368,8 +364,6 @@ def test_debug_undefined(self):
str(DebugUndefined(hint=undefined_hint))
== f"{{{{ undefined value printed: {undefined_hint} }}}}"
)
with pytest.raises(AttributeError):
getattr(DebugUndefined, "__slots__") # noqa: B009

def test_strict_undefined(self):
env = Environment(undefined=StrictUndefined)
Expand All @@ -386,8 +380,6 @@ def test_strict_undefined(self):
env.from_string('{{ missing|default("default", true) }}').render()
== "default"
)
with pytest.raises(AttributeError):
getattr(StrictUndefined, "__slots__") # noqa: B009
assert env.from_string('{{ "foo" if false }}').render() == ""

def test_indexing_gives_undefined(self):
Expand Down
50 changes: 50 additions & 0 deletions tests/test_runtime.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
import copy
import itertools
import pickle

import pytest

from jinja2 import ChainableUndefined
from jinja2 import DebugUndefined
from jinja2 import StrictUndefined
from jinja2 import Template
from jinja2 import TemplateRuntimeError
from jinja2 import Undefined
from jinja2.runtime import LoopContext

TEST_IDX_TEMPLATE_STR_1 = (
Expand Down Expand Up @@ -73,3 +82,44 @@ def __call__(self, *args, **kwargs):
out = t.render(calc=Calc())
# Would be "1" if context argument was passed.
assert out == "0"


_undefined_types = (Undefined, ChainableUndefined, DebugUndefined, StrictUndefined)


@pytest.mark.parametrize("undefined_type", _undefined_types)
def test_undefined_copy(undefined_type):
undef = undefined_type("a hint", ["foo"], "a name", TemplateRuntimeError)
copied = copy.copy(undef)

assert copied is not undef
assert copied._undefined_hint is undef._undefined_hint
assert copied._undefined_obj is undef._undefined_obj
assert copied._undefined_name is undef._undefined_name
assert copied._undefined_exception is undef._undefined_exception


@pytest.mark.parametrize("undefined_type", _undefined_types)
def test_undefined_deepcopy(undefined_type):
undef = undefined_type("a hint", ["foo"], "a name", TemplateRuntimeError)
copied = copy.deepcopy(undef)

assert copied._undefined_hint is undef._undefined_hint
assert copied._undefined_obj is not undef._undefined_obj
assert copied._undefined_obj == undef._undefined_obj
assert copied._undefined_name is undef._undefined_name
assert copied._undefined_exception is undef._undefined_exception


@pytest.mark.parametrize("undefined_type", _undefined_types)
def test_undefined_pickle(undefined_type):
undef = undefined_type("a hint", ["foo"], "a name", TemplateRuntimeError)
copied = pickle.loads(pickle.dumps(undef))

assert copied._undefined_hint is not undef._undefined_hint
assert copied._undefined_hint == undef._undefined_hint
assert copied._undefined_obj is not undef._undefined_obj
assert copied._undefined_obj == undef._undefined_obj
assert copied._undefined_name is not undef._undefined_name
assert copied._undefined_name == undef._undefined_name
assert copied._undefined_exception is undef._undefined_exception