-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
532a0eb
7d0b64c
5a95332
d2f6333
020dd78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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": | ||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The base class is already doing the same blanket There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
@@ -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__, | ||
) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. Sincehasattr(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 theUndefined
object it receives instead of a bound method to fill in an empty object instance.In general,
__getattr__
should raiseAttibuteError
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
.