Incorrect format passed to `__annotate__` function

[sobolevn]

Bug 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

• gh-125507: Call annotate(FORWARDREF) before trying __annotations__ #125534

[sobolevn]

Ok, this is not so easy. What happens here:

1. We try to get __annotations__ first with format=VALUE via

   cpython/Lib/annotationlib.py

   Line 817 in aa18fd5

   ann = _BASE_GET_ANNOTATIONS(obj)

2. If it fails with a NameError we actually try to call __annotate__ with format=FORWARDREF:

   cpython/Lib/annotationlib.py

   Lines 692 to 693 in aa18fd5

   	# 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]

Practical question, how can I differentiate two calls in

cpython/Lib/dataclasses.py

Lines 1614 to 1630 in 3b26e36

	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

1. When calling with format=VALUE the first time, so it can be later called with format=FORWARDREF
2. When calling with format=VALUE the only time, because this is what was asked by the end user

[JelleZijlstra]

Would #124415 help here?

[sobolevn]

@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]

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]

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]

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.