bpo-43901: Lazy-create an empty annotations dict in all unannotated user classes and modules #25623
Conversation
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
larryhastings
requested review from
gvanrossum,
ilevkivskyi and
markshannon
as code owners
|
I've marked it as draft because it's not ready for merging. In particular, it needs unit tests for this new behavior. I uploaded the PR so we might get a head start on reviewing the changes. In its current state it does pass the unit test suite. |
I copied some code from module_dir into module_{get|set}_annotations().
It seemed like it had a minor flaw: if the module dict failed a
PyDict_Check(), it wanted to raise an exception. But to raise the
exception, it wanted the name of the module, and if getting the name
failed it skipped raising the exception. It turns out, if getting
the name of the module fails, *that* raises an exception too. So,
rather than stomp on that exception, we should do the same thing
module_dir() does: leave that exception alone and return an error.
Added a test to ensure that the new C getsetdef for __annotations__ doesn't interfere with classes (or metaclasses!) redefining __annotations__ using the descriptor protocol.
|
I think this is ready for review. I have test coverage for all the new lines of code. Everything seems to work. It's as good as I can make it without adult supervision. |
JelleZijlstra
approved these changes
Apr 28, 2021
|
@larryhastings: Please replace |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The behavior of the
__annotations__member is wildly different between the three objects (function, class, module) that support it. Many of these differences are minor, but one in particular has been an awful wart for years: classes can inherit__annotations__from their base classes. This is incontestably a misfire of the original design--it's never useful, it's only confusing, and libraries who examine annotations have had to work around it for years.I started a thread in January 2021 in which I brought up this and other inconsistencies with respect to
__annotations__:The reaction was positive: yes, that's a genuine problem, and there's an easy, highly compatible fix that everybody liked: classes should always have an
__annotations__dict set. So that the behavior is consistent, modules should always have one set too.Happily, lazy-creating the annotations dict is an adequate fix, as long as the annotations dict is still stored in the object's
__dict__. Code written using the old best practice, which peeks in the__dict__for annotations on classes, will still see (or not see) the dict as it did before, so it will continue to work unchanged. (Lazy-creating the annotations dict doesn't actually fix the inherited annotations problem for such code--but this code is already working around the problem, so this is fine.) Going forward, code written for 3.10+ can always usegetattr()to get the annotations dict from any object.https://bugs.python.org/issue43901