GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-43901: Lazy-create an empty annotations dict in all unannotated user classes and modules by larryhastings · Pull Request #25623 · python/cpython

Conversation

@larryhastings

@larryhastings larryhastings commented Apr 26, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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__:

https://mail.python.org/archives/list/[email protected]/thread/AWKVI3NRCHKPIDPCJYGVLW4HBYTEOQYL/

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 use getattr() to get the annotations dict from any object.

https://bugs.python.org/issue43901

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
larryhastings added 3 commits April 25, 2021 22:03
@larryhastings
Change module object to lazy-create annotations.
7c66fc5
@larryhastings
Change class object to lazy-create annotations.
91752e9
@larryhastings
Add news blurb.
da2e6bd
@larryhastings
larryhastings requested review from gvanrossum, ilevkivskyi and markshannon as code owners April 26, 2021 05:55
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 26, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Apr 26, 2021
@larryhastings
larryhastings marked this pull request as draft April 26, 2021 05:56
@larryhastings

larryhastings commented Apr 26, 2021

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

larryhastings added 5 commits April 27, 2021 02:19
@larryhastings
Don't bother raising if PyModule_GetName() fails.
4a5d431
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.
@larryhastings
Unit tests for lazy module annotations dicts.
df7deea
@larryhastings
Add unit tests for lazy user class annotations.
4cd0d92
@larryhastings
Fix pydoc.
c139021
@larryhastings
Test that cls.__annotations__ descriptor works.
f36d27c
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.
@larryhastings
larryhastings marked this pull request as ready for review April 27, 2021 12:37
@larryhastings

larryhastings commented Apr 27, 2021

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@larryhastings
Fix important missing whitespace in important test.
bdf09ac
JelleZijlstra
JelleZijlstra approved these changes Apr 28, 2021
View reviewed changes
@larryhastings
larryhastings merged commit 2f2b698 into python:master Apr 30, 2021
@bedevere-bot

bedevere-bot commented Apr 30, 2021

Copy link
Copy Markdown

@larryhastings: Please replace # with GH- in the commit message next time. Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting core review label Apr 30, 2021
@larryhastings
larryhastings deleted the class_and_module_annotations_jfdi branch April 30, 2021 03:09
@ilevkivskyi ilevkivskyi mentioned this pull request Sep 6, 2022
Can't assign entry to __annotations__ unless a class has existing annotation #95532
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@ilevkivskyi ilevkivskyi Awaiting requested review from ilevkivskyi
@markshannon markshannon Awaiting requested review from markshannon

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@larryhastings @bedevere-bot @JelleZijlstra @the-knights-who-say-ni
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.