Preserve __slots__ metadata on Undefined types
#2026
Conversation
* fix copy/deepcopy/pickle of Undefined objects on Python >= 3.6 * fix ChainedUndefined to raise AttributeError on unimplemented dunder methods to prevent breaking core Python protocols (including copy) * add tests
* base impl/assignment is improperly annotated, but it was already ignored
nitzmahone
force-pushed
the
undefined_slots_fix
branch
from
2eced62
to
5a95332
Compare
| @@ -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": | |||
There was a problem hiding this comment.
I don't understand this change. Can you make the comment clearer?
There was a problem hiding this comment.
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.
src/jinja2/runtime.py
Outdated
| return self | ||
|
|
||
| __getitem__ = __getattr__ # type: ignore | ||
| def __getitem__(self, _: str) -> "ChainableUndefined": # type: ignore |
There was a problem hiding this comment.
Don't use _ as an argument name. Use specific ignores, not a bare ignore.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
* tighten up dunder method exclusion heuristic and clarify raison d'etre in Undefined/ChainableUndefined.__getattr__ * use a more granular type ignore for incorrect base class declaration on ChainableUndefined.__getattr__
Restores erroneous deletions of
__slots__metadata fromUndefinedtypes that breaks various serialization/copy mechanisms on Python >= 3.6. Also:fixes #2025