GeezerVison is showing github.com — Lazy-create an empty annotations dict in all unannotated user classes and modules · Issue #88067 · python/cpython
Lazy-create an empty annotations dict in all unannotated user classes and modules #88067Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/larryhastings'
closed_at = <Date 2021-04-30.16:27:36.976>
created_at = <Date 2021-04-21.06:44:01.810>
labels = ['interpreter-core', 'type-feature', '3.10', 'release-blocker']
title = 'Lazy-create an empty annotations dict in all unannotated user classes and modules'
updated_at = <Date 2021-04-30.23:27:35.179>
user = 'https://github.com/larryhastings' bugs.python.org fields:
activity = <Date 2021-04-30.23:27:35.179>
actor = 'erlendaasland'
assignee = 'larry'
closed = True
closed_date = <Date 2021-04-30.16:27:36.976>
closer = 'pablogsal'
components = ['Interpreter Core']
creation = <Date 2021-04-21.06:44:01.810>
creator = 'larry'
dependencies = []
files = []
hgrepos = []
issue_num = 43901
keywords = ['patch']
message_count = 39.0
messages = ['391490', '391491', '391495', '391542', '391555', '391556', '391558', '391562', '391572', '391762', '391781', '391802', '391809', '391810', '391812', '391815', '391817', '391819', '391820', '391824', '391825', '392157', '392162', '392373', '392374', '392449', '392450', '392452', '392453', '392454', '392455', '392457', '392458', '392461', '392469', '392471', '392476', '392516', '392542']
nosy_count = 8.0
nosy_names = ['gvanrossum', 'larry', 'eric.smith', 'Guido.van.Rossum', 'JelleZijlstra', 'pablogsal', 'erlendaasland', 'kokxxxxik']
pr_nums = ['25623', '25752', '25754']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue43901'
versions = ['Python 3.10'] Activitylarryhastings commented on Apr 21, 2021 ContributorAuthor More actions 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 is never useful, it is 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.
This won't be a large change. However, there are a lot of changes flying around with respect to __annotations__ right now. My plan is to get the work ready, then when the dust is mostly-settled I'll get it updated, reviewed, and merged. Definitely before Python 3.10b1.
Long term, I'd like to get the other changes to annotations I proposed in that thread:
- del o.__annotations__ always raises an exception.
- o.__annotations__ is permitted to be None.
- You can only set o.__annotations__ to either a dict or None.
But it's probably best to not change everything all at once.
changed the title [-]Add an empty annotations dict to all classes and modules[/-] [+]Add an empty annotations dict to all unannotated classes and modules[/+] on Apr 21, 2021changed the title [-]Add an empty annotations dict to all classes and modules[/-] [+]Add an empty annotations dict to all unannotated classes and modules[/+] on Apr 21, 2021larryhastings commented on Apr 21, 2021 ContributorAuthor More actions Huh. The sample code in my thread got goofed up somewhere along the way--maybe it's my fault, maybe it was done to me by some automated process. Anyway, the example demonstrating classes inheriting annotations was meant to be formatted like this:
class A:
ax:int=3
class B(A):
pass
print(getattr(B, '__annotations__', {}))Maybe some over-smart doodad decided that no paragraph would ever start with spaces, so they helpfully removed them? Hmm. larryhastings commented on Apr 21, 2021 ContributorAuthor More actions Preliminary patch is 17 lines. Ah well. I must be terrible at this! gvanrossum commented on Apr 21, 2021 Sounds like a plan, I agree that the original design misfired here (we were probably worried about million-line code bases with tens of thousands of classes having to pay the price of tens of thousands of empty dicts, but I now think that was an unnecessary worry -- that should be a few megabytes on a very much larger total).
But given that you're not done yet:
-
Is it possible to create __annotations__ lazily? (IIRC in January we came to a conclusion about this, something like yes for modules but for classes, or the other way around?)
-
Why would __annotations__ ever be None? Why do you allow setting it to None? Do we know of people ever write '__annotations__ = None' in their class or write 'cls.__annotations__ = None'?
-
And where's your PR?
larryhastings commented on Apr 21, 2021 ContributorAuthor More actions
- Is it possible to create __annotations__ lazily? (IIRC in January we came to a conclusion about this, something like yes for modules but for classes, or the other way around?)
Maybe for modules, definitely not for classes. The problem is that best practice for classes is to look in the class dict for __annotations__. That sidesteps the inheritance problem.
Functions already lazily create a dict __annotations__ if not set. And it's not stored in the function dict. So nobody ever looks in the function dict for __annotations__. That all works fine.
Would you prefer that *just classes* lazily create __annotations__? It's not hard code to write, but I was being cautious. "always set __annotations__ to an empty dict" is the easiest code to write and get correct. And since modules are not all that numerous even in the largest projects, it seemed like this un-optimized approach would have a minimal contribution to the heat death of the universe.
It's also remotely possible that someone out there does look in the module dict for __annotations__, though I admit I've never seen it. It seems like it'd be tempting to write your code that way though:
if isinstance(o, (type, types.ModuleType)):
ann = o.__dict__.get("__annotations__", None)
elif callable(o):
ann = o.__annotations__
else:
raise ValueError(f"{o!r} doesn't support annotations")
- Why would __annotations__ ever be None?
Simply because it's the cheapest sensible way to indicate "this object has no annotations". It would be nice if the world accepted __annotations__ being None to mean "no annotations". But I don't think we're there.
I note that the function object has a special setter for __annotations__ (func_set_annotations()), since at least Python 3.1, which explicitly only allows setting __annotations__ to either a dict or None. (I didn't have 3.0 handy.)
Why do you allow setting it to None?
Me? I've never contributed code to Python that restricts the permissible types of values one is allowed to set on __annotations__.
Function objects are opinionated as mentioned (either dict or None), classes and modules have no opinion whatsoever, and allow you to set __annotations__ to any value. That was all true long before I started working on annotations.
Do we know of people ever write '__annotations__ = None' in their class or write 'cls.__annotations__ = None'?
AFAIK I've never seen anyone set __annotations__ to None.
Not done yet. I only started it last night, and there were a lot of test failures--it turns out, a *lot* of regression tests do a sanity check that __annotations__ isn't set on classes (and maybe modules too). For example, I was surprised that test_opcodes has two such test failures. larryhastings commented on Apr 21, 2021 ContributorAuthor More actions By the way, here's a tidbit I never got around to posting in c.l.p-d.
I noted in the conversation in January that attrs is an outlier here: it *doesn't* look in the class dict for __annotations__. Instead, it has some complicated code where it asks the class for its annotations, then iterates over the __mro__ and asks every base class for *its* annotations. If the class and a base class have the same class dict (using the "is" operator iirc) then attrs says "oh, that class doesn't have its own annotations, it's inheriting them" and reacts appropriately.
I emailed Hynek to ask him why he did it that way. It turns out, he did that because at the time he didn't know you could peek in the class dict for __annotations__. He literally had a todo item saying "change to looking in the class dict". larryhastings commented on Apr 21, 2021 ContributorAuthor More actions It occurs to me that part of this work should also be a new "best practices for __annotations__" entry in the Python docs.
Best practices for working with annotations, for code that requires a minimum Python version of 3.10+:
Best practice is to call either inspect.get_annotations() or typing.get_type_hints() to access annotations. But if you want to access '__annotations__' on an object directly:
- Always access the annotations on an object using the attribute interface, either o.__annotations__ or getattr(o, '__annotations__'). Accessing '__annotations__' through other mechanisms (e.g. looking in the class dict) is unsupported.
- Assume that o.__annotations__ is always either a dict or None.
- It's best to not assign to o.__annotations__. But if you must, always set it to either a dict or None.
- Never delete o.__annotations__.
- Never modify o.__annotations__.
larryhastings commented on Apr 30, 2021 ContributorAuthor More actions Gee whiz! Sorry about that. I swear, it works fine on my machine.
I'll incorporate import_helper.import_fresh_module helper into the test as you suggest, and once I get it to work I'll send you a PR. I don't know how to test fixing this failure, though, because again it already works fine on my machine.
Why would it fail on Windows? Are there dramatic differences under the covers with respect to the import machinery and caching and such between POSIX and Windows? pablogsal commented on Apr 30, 2021 Oh, actually is workse because commenting out the test shows refleaks:
𓋹 ./python.exe -m test test_module -R :
0:00:00 load avg: 5.03 Run tests sequentially
0:00:00 load avg: 5.03 [1/1] test_module
beginning 9 repetitions
123456789
.........
test_module leaked [47, 47, 47, 47] references, sum=188
test_module leaked [6, 6, 6, 6] memory blocks, sum=24
test_module failed
== Tests result: FAILURE ==
1 test failed:
test_module
Total duration: 2.0 sec pablogsal commented on Apr 30, 2021
> Why would it fail on Windows?
It fails on all refleak buildbots. To reproduce:
% ./configure --with-pydebug -C && make -j -s
% ./python.exe -m test test_module -R : larryhastings commented on Apr 30, 2021 ContributorAuthor More actions Eek! I swear I did a refleak check and got a clean bill of health. Again, sorry about this! pablogsal commented on Apr 30, 2021
Eek! I swear I did a refleak check and got a clean bill of health. Again, sorry about this!
No problem at all! I'm sure we can fix this on time :)
Opened #25754 for the refleaks larryhastings commented on Apr 30, 2021 ContributorAuthor More actions You want a separate PR for the refleak fix, or should I roll them both up into one? pablogsal commented on Apr 30, 2021 I made a fix for everything in #25754. Could you review it? pablogsal commented on Apr 30, 2021 larryhastings commented on Apr 30, 2021 ContributorAuthor More actions Obviously the original assertion failure ("AssertionError: False is not true") wasn't caused by the refleaks. So I'm still puzzled about why that test worked on POSIX and failed on Windows. I admit I was pulling some wacky import shenanigans in the test, and the import_fresh_module change is an improvement. But still... what was going on?! pablogsal commented on Apr 30, 2021
So I'm still puzzled about why that test worked on POSIX and failed on Windows
I was able to reproduce it in my MacOS machine (maybe also it reproduced on Linux). The problem is that when you run with -R, the test runs several times and the import statement is a noop the second time because is cached. We needed to modify the test to import the module always larryhastings commented on Apr 30, 2021 ContributorAuthor More actions Ah, I see. So it wasn't a Windows thing, it was a "we run the test multiple times and that particular test assumed it was only run once" thing. And reflink testing is guaranteed to run every test multiple times. erlend-aasland commented on Apr 30, 2021 test_grammar also needed a fix. It has been updated to use import_helper.import_fresh_module in bpo-43995 (GH-25764).
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|