-
-
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
Open
nitzmahone
wants to merge
5
commits into
pallets:main
Choose a base branch
from
nitzmahone:undefined_slots_fix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
532a0eb
preserve __slots__ on Undefined et al
nitzmahone 7d0b64c
remove test assertions for missing `__slots__`
nitzmahone 5a95332
ignore `ChainedUndefined.__getitem__` return-type annotation
nitzmahone d2f6333
[pre-commit.ci lite] apply automatic fixes
pre-commit-ci-lite[bot] 020dd78
address review requests
nitzmahone File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
.