Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-119180: PEP 649 compiler changes #119361

Merged
merged 77 commits into from
Jun 11, 2024

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented May 21, 2024

This implements the compiler changes in PEP-649, creating a compiler-generated __annotate__ function that holds the function, class, and module annotations.

I commented on Discuss on some of the CPython tests I needed to change: https://discuss.python.org/t/pep-649-deferred-evaluation-of-annotations-tentatively-accepted/21331/60

I tried this branch on a few prominent runtime typing projects; my notes are in the issue description (#119180). Mostly there are few issues, except a few places that need adjustment because they were checking for annotations in the __dict__ directly.

@bedevere-app bedevere-app bot mentioned this pull request May 21, 2024
29 tasks
Lib/test/test_type_annotations.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Python/symtable.c Outdated Show resolved Hide resolved
Python/symtable.c Outdated Show resolved Hide resolved
Python/symtable.c Outdated Show resolved Hide resolved
Python/symtable.c Show resolved Hide resolved
Python/compile.c Outdated Show resolved Hide resolved
Python/compile.c Outdated Show resolved Hide resolved
Python/compile.c Outdated Show resolved Hide resolved
…s on

This makes it so functools.update_wrapper can always copy __annotate__ without having
to worry about whether or not the future is enabled.
@JelleZijlstra
Copy link
Member Author

Any further feedback on this PR? Getting this in will make it possible to start working on the Python-level parts of the PEP, which can be done in smaller chunks.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bits I feel qualified to comment on all look good to me!

@@ -76,6 +76,11 @@ class A(builtins.object)
| __weakref__%s

class B(builtins.object)
| Methods defined here:
|
| __annotate__(...)
Copy link
Member

@AlexWaygood AlexWaygood Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It would be great if we could generate a nice __text_signature__ for these generated methods, but that definitely doesn't need to be done in this PR)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're actually Python function, so I wonder why pydoc doesn't pick them up. Possibly because the parameter is called .format.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed:

>>> inspect.signature(f.__annotate__)
Traceback (most recent call last):
  File "<python-input-2>", line 1, in <module>
    inspect.signature(f.__annotate__)
    ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/Users/jelle/py/cpython/Lib/inspect.py", line 3329, in signature
    return Signature.from_callable(obj, follow_wrapped=follow_wrapped,
           ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                   globals=globals, locals=locals, eval_str=eval_str)
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jelle/py/cpython/Lib/inspect.py", line 3055, in from_callable
    return _signature_from_callable(obj, sigcls=cls,
                                    follow_wrapper_chains=follow_wrapped,
                                    globals=globals, locals=locals, eval_str=eval_str)
  File "/Users/jelle/py/cpython/Lib/inspect.py", line 2558, in _signature_from_callable
    return _signature_from_function(sigcls, obj,
                                    skip_bound_arg=skip_bound_arg,
                                    globals=globals, locals=locals, eval_str=eval_str)
  File "/Users/jelle/py/cpython/Lib/inspect.py", line 2403, in _signature_from_function
    parameters.append(Parameter(name, annotation=annotation,
                      ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                kind=kind))
                                ^^^^^^^^^^
  File "/Users/jelle/py/cpython/Lib/inspect.py", line 2750, in __init__
    raise ValueError('{!r} is not a valid parameter name'.format(name))
ValueError: '.format' is not a valid parameter name

Maybe in a followup PR we can figure out a way to make this work. The parameter has to have an illegal name so it doesn't shadow any symbol name that the user might have used.

Copy link
Member

@AlexWaygood AlexWaygood Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter has to have an illegal name so it doesn't shadow any symbol name that the user might have used.

I'm not sure I 100% understand this point, and no tests seem to fail if I make this change locally to your PR branch and recompile (and I verified that it means that __annotate__ methods have valid signatures as per inspect.signature):

diff --git a/Include/internal/pycore_global_strings.h b/Include/internal/pycore_global_strings.h
index 009802c4416..899b19ae941 100644
--- a/Include/internal/pycore_global_strings.h
+++ b/Include/internal/pycore_global_strings.h
@@ -45,7 +45,7 @@ struct _Py_global_strings {
         STRUCT_FOR_STR(dot, ".")
         STRUCT_FOR_STR(dot_locals, ".<locals>")
         STRUCT_FOR_STR(empty, "")
-        STRUCT_FOR_STR(format, ".format")
+        STRUCT_FOR_STR(format, "format")
         STRUCT_FOR_STR(generic_base, ".generic_base")
         STRUCT_FOR_STR(json_decoder, "json.decoder")
         STRUCT_FOR_STR(kwdefaults, ".kwdefaults")
diff --git a/Include/internal/pycore_runtime_init_generated.h b/Include/internal/pycore_runtime_init_generated.h
index ff5b6ee8e0f..e8b5173a457 100644
--- a/Include/internal/pycore_runtime_init_generated.h
+++ b/Include/internal/pycore_runtime_init_generated.h
@@ -554,7 +554,7 @@ extern "C" {
     INIT_STR(dot, "."), \
     INIT_STR(dot_locals, ".<locals>"), \
     INIT_STR(empty, ""), \
-    INIT_STR(format, ".format"), \
+    INIT_STR(format, "format"), \
     INIT_STR(generic_base, ".generic_base"), \
     INIT_STR(json_decoder, "json.decoder"), \
     INIT_STR(kwdefaults, ".kwdefaults"), \
diff --git a/Python/compile.c b/Python/compile.c
index c3372766d0b..7535067ded0 100644
--- a/Python/compile.c
+++ b/Python/compile.c
@@ -1447,7 +1447,7 @@ compiler_setup_annotations_scope(struct compiler *c, location loc,
     }
     c->u->u_metadata.u_posonlyargcount = 1;
     // if .format != 1: raise NotImplementedError
-    _Py_DECLARE_STR(format, ".format");
+    _Py_DECLARE_STR(format, "format");
     ADDOP_I(c, loc, LOAD_FAST, 0);
     ADDOP_LOAD_CONST(c, loc, _PyLong_GetOne());
     ADDOP_I(c, loc, COMPARE_OP, (Py_NE << 5) | compare_masks[Py_NE]);
diff --git a/Python/symtable.c b/Python/symtable.c
index 23fc4a0ec03..0154092a905 100644
--- a/Python/symtable.c
+++ b/Python/symtable.c
@@ -2511,7 +2511,7 @@ symtable_visit_annotation(struct symtable *st, expr_ty annotation, void *key)
             }
         }
 
-        _Py_DECLARE_STR(format, ".format");
+        _Py_DECLARE_STR(format, "format");
         // The generated __annotate__ function takes a single parameter with the
         // internal name ".format".
         if (!symtable_add_def(st, &_Py_STR(format), DEF_PARAM,
@@ -2570,7 +2570,7 @@ symtable_visit_annotations(struct symtable *st, stmt_ty o, arguments_ty a, expr_
             return 0;
         }
     }
-    _Py_DECLARE_STR(format, ".format");
+    _Py_DECLARE_STR(format, "format");
     // We need to insert code that reads this "parameter" to the function.
     if (!symtable_add_def(st, &_Py_STR(format), DEF_PARAM, LOCATION(o))) {
         return 0;

This also might be worth a brief mention in PEP-749, since PEP-649 seems to specify that the signature of __annotate__ should be __annotate__(format: int).

In any event, this can definitely be tackled in a followup; I don't want to block this PR!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try something like this:

class format: pass

def f(x: format): pass

print(f.__annotations__)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, makes sense! Worth adding a test like that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, the problem is that any such test can only tell us about one specific name.

Copy link
Member

@AlexWaygood AlexWaygood Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it doesn't verify any fundamental invariants about __annotate__ — but I still think it's useful to have such a test, so that the CI goes obviously red if someone else comes along in the future and tries to do the "obvious fix" to get inspect.signature() working for these methods.

Thanks for adding it!

Lib/symtable.py Show resolved Hide resolved
Lib/test/test_grammar.py Outdated Show resolved Hide resolved
Python/compile.c Outdated Show resolved Hide resolved
Lib/inspect.py Outdated
@@ -173,6 +177,13 @@
TPFLAGS_IS_ABSTRACT = 1 << 20


@enum.global_enum
class AnnotationsFormat(enum.IntEnum):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this PR relate to PEP 749? Are you intending for this PR to implement PEP 649 as written, or the updated proposals in PEP 749?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will just take out this part; I only use the enum values in one test. The companion PR #119891 will implement PEP 749's proposed new module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And thanks for reviewing!

@@ -105,7 +105,7 @@ def test_runsource_shows_syntax_error_for_failed_compilation(self):

def test_no_active_future(self):
console = InteractiveColoredConsole()
source = "x: int = 1; print(__annotations__)"
source = "x: int = 1; print(__annotate__(1))"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does printing __annotations__ not work here? Shouldn't it call __annotate__ under the hood anyway?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Printing a global doesn't go through the descriptor protocol, so printing __annotations__ will just throw a NameError; it is added to the namespace only when the __annotate__ descriptor is called.

@JelleZijlstra JelleZijlstra enabled auto-merge (squash) June 11, 2024 05:07
@JelleZijlstra JelleZijlstra merged commit 9b8611e into python:main Jun 11, 2024
52 of 56 checks passed
@JelleZijlstra JelleZijlstra deleted the pep649-compile branch June 11, 2024 13:08
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants