GeezerVison is showing github.com — Incorrect format passed to `__annotate__` function · Issue #125507 · python/cpython
Incorrect format passed to __annotate__ function #125507DescriptionBug report
Consider this example:
class My:
def __annotate__(format):
assert format == 2, format
return {}
import annotationlib
annotationlib.get_annotations(My, format=2)This will fail with:
Traceback (most recent call last):
File "/Users/sobolev/Desktop/cpython2/ex.py", line 7, in <module>
annotationlib.get_annotations(My, format=2)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
File "/Users/sobolev/Desktop/cpython2/Lib/annotationlib.py", line 688, in get_annotations
return dict(_get_dunder_annotations(obj))
~~~~~~~~~~~~~~~~~~~~~~~^^^^^
File "/Users/sobolev/Desktop/cpython2/Lib/annotationlib.py", line 817, in _get_dunder_annotations
ann = _BASE_GET_ANNOTATIONS(obj)
File "/Users/sobolev/Desktop/cpython2/ex.py", line 3, in __annotate__
assert format == 2, format
^^^^^^^^^^^
AssertionError: 1
I think that we should still pass the format in _BASE_GET_ANNOTATIONS.
I will try to fix this, since this affects my other PR: #122262
CC @JelleZijlstra
Linked PRs
Activitysobolevn commented on Oct 15, 2024 Ok, this is not so easy. What happens here:
- We try to get
__annotations__ first with format=VALUE via
|
ann = _BASE_GET_ANNOTATIONS(obj) |
- If it fails with a
NameError we actually try to call __annotate__ with format=FORWARDREF:
|
# But if __annotations__ threw a NameError, we try calling __annotate__ |
|
ann = _get_and_call_annotate(obj, format) |
My code in #122262 never had any NameErrors, so it never actually received the second call
But the question is: is this a good API? For me it feels like a very nuanced one with a lot of corner-cases. sobolevn commented on Oct 15, 2024 Practical question, how can I differentiate two calls in
|
def annotate_method(format): |
|
typing = sys.modules.get("typing") |
|
if typing is None and format == annotationlib.Format.FORWARDREF: |
|
typing_any = annotationlib.ForwardRef("Any", module="typing") |
|
return { |
|
ann: typing_any if t is _ANY_MARKER else t |
|
for ann, t in annotations.items() |
|
} |
|
|
|
from typing import Any |
|
ann_dict = { |
|
ann: Any if t is _ANY_MARKER else t |
|
for ann, t in annotations.items() |
|
} |
|
if format == annotationlib.Format.STRING: |
|
return annotationlib.annotations_to_string(ann_dict) |
|
return ann_dict |
- When calling with
format=VALUE the first time, so it can be later called with format=FORWARDREF
- When calling with
format=VALUE the only time, because this is what was asked by the end user
JelleZijlstra commented on Oct 15, 2024 sobolevn commented on Oct 15, 2024 @JelleZijlstra hm, I don't think so. Here's my simplified reproducer with #124415 applied:
import annotationlib
class My:
def __annotate__(format):
assert format == annotationlib.Format.FORWARDREF, format
# Fails with: `AssertionError: 1`
return {}
annotationlib.get_annotations(My, format=annotationlib.Format.FORWARDREF)So, looks like it does still make the first call with format=1, even when asked to get forward refs explicitly. JelleZijlstra commented on Oct 15, 2024 I understand the issue now. You want to return typing.Any in VALUE format, but in FORWARDREF format you want to avoid importing typing and return ForwardRef("typing.Any") directly.
For FORWARDREF format, we currently try using __annotations__ directly before we call __annotate__(FORWARDREF). If we change the order in which these operations happen, we could avoid your issue. I'll work on a PR for that. JelleZijlstra commented on Oct 15, 2024 Though I'm curious if @larryhastings and @carljm have opinions here: If an object has __annotations__ that can be retrieved without error, but it also has an __annotate__ that directly works with the FORWARDREF format, which should we prefer?
At the sprint, we changed it so we preferred returning __annotations__ over evaluating __annotate__ in the fake globals environment to implement the FORWARDREF format, which makes sense to me. But if a user-written __annotate__ supports FORWARDREF directly, shouldn't we believe it? carljm commented on Oct 15, 2024 To me it seems like a useful, and sensible, invariant that if you successfully get VALUES annotations (the default), then FORWARDREFS annotations will be identical to that. FORWARDREFS is intended for cases where VALUES would fail with NameError, and the API is designed around that assumption. I would prefer not to permit objects where FORWARDREFS annotations permanently differ from VALUES annotations. So I would rather not make changes here.
To me, the proposed approach for dataclasses seems like over-optimization. I think it's fine to always import typing -- someone is introspecting an annotation that uses typing.Any, after all. There may be other possibilities to avoid this, but I would need to look at the problem more. larryhastings commented on Oct 19, 2024 What Carl said. This should be documented behavior: if __annotations__ is set, that value will be returned for VALUES or FORWARDREF formats and __annotate__ won't be called. JelleZijlstra commented on Nov 20, 2024 Thanks, then let's stick with the current behavior.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|