gh-119180: Add VALUE_WITH_FAKE_GLOBALS format to annotationlib #124415
Conversation
JelleZijlstra
requested review from
AlexWaygood,
markshannon and
iritkatriel
as code owners
| @@ -144,6 +144,17 @@ Classes | |||
|
|
|||
| The exact values of these strings may change in future versions of Python. | |||
|
|
|||
| .. attribute:: VALUE_WITH_FAKE_GLOBALS | |||
| :value: 4 | |||
There was a problem hiding this comment.
VALUE_WITH_FAKE_GLOBALS is now 2. But why publish their values in the first place?
There was a problem hiding this comment.
A wise man wrote these values in PEP 649 (https://peps.python.org/pep-0649/#overview). They are also relevant if you write an annotate function in C. But yes, we can probably do without them in Python code.
| @@ -413,19 +416,28 @@ def f2(a: undefined): | |||
| annotationlib.get_annotations(f2, format=Format.FORWARDREF), | |||
| {"a": fwd}, | |||
| ) | |||
| self.assertEqual(annotationlib.get_annotations(f2, format=2), {"a": fwd}) | |||
| self.assertEqual(annotationlib.get_annotations(f2, format=3), {"a": fwd}) | |||
There was a problem hiding this comment.
Why is format hard-coded here? I'd prefer you think of it like an enum, an arbitrary value whose actual value doesn't matter. I don't think it's necessary to externally test that the format constants are specific values; they happen to be 1, 2, 3, 4, but I don't want external users to rely on that or ever hard-code them.
There was a problem hiding this comment.
The intent was to test that the function accepts both the integer value and the enum. If you think it's OK to accept only the enum, I can switch to that model.
| @@ -495,6 +496,8 @@ def call_annotate_function(annotate, format, *, owner=None, _is_evaluate=False): | |||
| on the generated ForwardRef objects. | |||
|
|
|||
| """ | |||
| if format == Format.VALUE_WITH_FAKE_GLOBALS: | |||
| raise ValueError("The VALUE_WITH_FAKE_GLOBALS format is for internal use only") | |||
There was a problem hiding this comment.
I was surprised to see this exception. I haven't given the matter a lot of thought, and I don't have a strongly held opinion yet. Can you tell me why we should raise this exception here? (My knee-jerk instinct: "special cases aren't special enough to break the rules.")
p.s. if we do keep these assertions, please drop "The ". The text should read "VALUES_WITH_FAKE_GLOBALS format is for internal use only".
There was a problem hiding this comment.
There is no meaningful value to return for VALUE_WITH_FAKE_GLOBALS; it's only useful within an annotate function.
Concretely, raising an exception here means that if a user-provided annotate function delegates to an API in annotationlib without further thought, it will raise an exception for VALUE_WITH_FAKE_GLOBALS, which is what we'd want.
| @@ -42,11 +42,14 @@ def test_enum(self): | |||
| self.assertEqual(Format.VALUE.value, 1) | |||
| self.assertEqual(Format.VALUE, 1) | |||
|
|
|||
| self.assertEqual(Format.FORWARDREF.value, 2) | |||
| self.assertEqual(Format.FORWARDREF, 2) | |||
| self.assertEqual(Format.VALUE_WITH_FAKE_GLOBALS.value, 2) | |||
There was a problem hiding this comment.
See related comments, I don't think the fact that STRING format is the int object 2 is or should be part of the interface. I don't want people hard-coding it.
Objects/typevarobject.c
Outdated
| @@ -168,7 +168,7 @@ constevaluator_call(PyObject *self, PyObject *args, PyObject *kwargs) | |||
| return NULL; | |||
| } | |||
| PyObject *value = ((constevaluatorobject *)self)->value; | |||
| if (format == 3) { // STRING | |||
| if (format == 4) { // STRING | |||
There was a problem hiding this comment.
The formats should be defined in C too. Macros are fine. I'm expecting FORMAT_STRING here, optionally with some variant of Py_ in front. Worst case, Py_ANNOTATE_FORMAT_STRING.
Refer to #3991.
📚 Documentation preview 📚: https://cpython-previews--124415.org.readthedocs.build/