Preserve __slots__ metadata on Undefined types#2026
Conversation
2eced62 to
5a95332
Compare
| 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.
| 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.
020dd78 to
d4fb0e8
Compare
Restores erroneous deletions of
__slots__metadata fromUndefinedtypes that breaks various serialization/copy mechanisms on Python >= 3.6. Also:fixes #2025