GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-119180: PEP 649 compiler changes by JelleZijlstra · Pull Request #119361 · python/cpython Conversation
|
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| class B(builtins.object) | ||
| | Methods defined here: | ||
| | | ||
| | __annotate__(...) |
There was an error while loading. Please reload this page.
There was a problem 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)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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__)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
|
||
|
|
||
| @enum.global_enum | ||
| class AnnotationsFormat(enum.IntEnum): |
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
And thanks for reviewing!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| def test_no_active_future(self): | ||
| console = InteractiveColoredConsole() | ||
| source = "x: int = 1; print(__annotations__)" | ||
| source = "x: int = 1; print(__annotate__(1))" |
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.