-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
PEP 692: Using TypedDict for more precise **kwargs
typing
#2620
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
Conversation
You can take 692. A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix CI:
A
Thank you @AA-Turner! |
**kwargs
typing
You will also need to sign the CLA -- you've used your Goldman email in commits, you may need to check if you need to use the corporate CLA vs or if you can use the individual CLA. A |
I need to be using a corporate CLA that AFAIK has been signed - however I'll need to verify if there is still something for me to do, still new to our internal process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to make the distinction between runtime and static type checking much clearer -- it is usually clear what you mean, but I have to work somewhat to get to your intent.
I'd strongly reccomend adding a rejected ideas section, as well as how to teach this, especially as this is primarily a user facing change.
Your rationale section is very thin -- a simple counterargument would be "why not use dataclasses
" -- you should address why specifically using kwargs
with TypedDict
is a useful addition to the language, especially given it connotes grammar changes.
A
pep-0692.rst
Outdated
Function calls with standard dictionaries | ||
----------------------------------------- | ||
|
||
Calling a function that has ``**kwargs`` typed using the ``**kwargs: **Movie`` sytnax with a standard dictionary must raise an error. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must raise an error at runtime (the python level) or when type checking? (The former doesn't make sense, but it is unspecified currently)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I presume you mean "a type checker should generate an error". The term raise
implies a runtime error.
pep-0692.rst
Outdated
|
||
def foo(**kwargs: **Movie) -> None: ... | ||
|
||
would mean that the ``**kwargs`` comprise two keyword arguments specified by ``Movie`` (i.e. a ``name`` keyword of type ``str`` and a ``year`` keyword of type ``int``). Then, inside the function itself, appropriate type checking would take place: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"appropriate type checking would take place" at type checking time, not at runtime.
pep-0692.rst
Outdated
Keyword collisions | ||
------------------ | ||
|
||
A ``TypedDict`` that is used to type ``**kwargs`` could potentially contain keys that are already defined in the function's signature. If the duplicate name is a standard argument, an error should be raised. If the duplicate name is a positional only argument, no errors should be raised. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"an error should be raised" / "no errors should be raised" -- again, unspecified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this should be type checking-only. Maybe just put a note at the beginning of the spec saying that the new annotation has no runtime effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, I'd avoid using the term "raise" when talking about errors emitted by a type checker. Use "generated" or "emitted" or "reported" instead. The term "raise" implies a runtime exception.
pep-0692.rst
Outdated
title: str | ||
year: NotRequired[int] | ||
|
||
When using a ``TypedDict`` to type ``**kwargs`` all of the required and non-required keys should correspond to required and non-required function keyword parameters. Therefore, if a required key is not supported by the caller, then an error must be raised. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"an error must be raised" -- by whom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above — avoid the term "raise".
pep-0692.rst
Outdated
By default all keys in a ``TypedDict`` are required. This behaviour can be overriden by setting the dictionary's ``total`` parameter as ``False``: | ||
|
||
:: | ||
|
||
class Movie(TypedDict, total=False): | ||
title: str | ||
year: int | ||
|
||
dictionary: Movie = {"title": "Life of Brian"} # OK! "year" is not required. | ||
dictionary = {"title": "Life of Brian", "year": 1979} # OK! | ||
|
||
Moreover, PEP 655 introduced new type qualifiers - ``typing.Required`` and ``typing.NotRequired`` - that enable specyfing whether a particular key is required or not: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would combine these two paragraphs and remove the example -- in the Specification section, a level of familiarity with the type hinting system can be assumed.
pep-0692.rst
Outdated
|
||
Assignment | ||
---------- | ||
A function typed using the ``**kwargs: **Movie`` construct can be assigned to another callable type only if they are compatible. This can happen in the following cases: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"assigned to another callable type" -- I think this could be clearer. You're talking about valid assignments under static type checking, not runtime assignments. Perhaps "validly assigned" or "pass type checking" or similar.
pep-0692.rst
Outdated
Additional keys | ||
--------------- | ||
|
||
A big advantage of using ``**kwargs`` is the ability to pass arbitrary dictionaries containing keyword-value pairs to the function and let the function pick and use only the ones it needs. However, currently, there is no way to define a ``TypedDict`` that would allow additional keys, so supporting this feature would require an addition to the ``TypedDict`` specification. If this limitation blocks the acceptance of this PEP, then the PEP's scope would have to be extended or a new PEP proposing additional keys for ``TypedDict`` would have to be proposed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section seems speculative, I would remove personally, or add as a limitation / rejected idea. It shouldn't be in Specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
Also cc @AlexWaygood for interest -- I've seen you doing various typing things but you might not be subscribed to the peps repo. A |
Too slow, I've already read it ;) I'm excited to see movement in this area! Will try to give some comments in the next few days, either here and/or on typing-sig. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think the PEP text still needs some work. I have a few suggestions, but I'll probably need another pass.
Also, please make your lines shorter by breaking them to around 70-80 characters. That will make editing easier.
pep-0692.rst
Outdated
Motivation | ||
========== | ||
|
||
Currently annotating ``**kwargs`` with a type ``T`` means that the ``kwargs`` type is in fact ``Dict[str, T]``. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently annotating ``**kwargs`` with a type ``T`` means that the ``kwargs`` type is in fact ``Dict[str, T]``. For example: | |
Currently annotating ``**kwargs`` with a type ``T`` means that the ``kwargs`` type is in fact ``dict[str, T]``. For example: |
pep-0692.rst
Outdated
|
||
def foo(**kwargs: str) -> None: ... | ||
|
||
means that all keyword arguments in ``foo`` are strings (i.e. ``kwargs`` is of type ``Dict[str, str]``). This behaviour limits the ability to type annotate ``**kwargs`` only to the cases where all of them are of the same type. However, it is often the case that keyword arguments conveyed by ``**kwargs`` have different types that are dependent on the keyword's name. In those cases type annotating ``**kwargs`` is not possible. This is especially a problem for already existing codebases where the need of refactoring the code in order to introduce proper type annotations may be considered not worth the effort. This in turn prevents the project from getting all of the benefits that type hinting can provide. As a consequence, there has been a lot of discussion around supporting more precise ``**kwargs`` typing [#mypyIssue4441]_ and it became a feature that is necessary for a large part of Python community. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
means that all keyword arguments in ``foo`` are strings (i.e. ``kwargs`` is of type ``Dict[str, str]``). This behaviour limits the ability to type annotate ``**kwargs`` only to the cases where all of them are of the same type. However, it is often the case that keyword arguments conveyed by ``**kwargs`` have different types that are dependent on the keyword's name. In those cases type annotating ``**kwargs`` is not possible. This is especially a problem for already existing codebases where the need of refactoring the code in order to introduce proper type annotations may be considered not worth the effort. This in turn prevents the project from getting all of the benefits that type hinting can provide. As a consequence, there has been a lot of discussion around supporting more precise ``**kwargs`` typing [#mypyIssue4441]_ and it became a feature that is necessary for a large part of Python community. | |
means that all keyword arguments in ``foo`` are strings (i.e., ``kwargs`` is of type ``dict[str, str]``). This behaviour limits the ability to type annotate ``**kwargs`` only to the cases where all of them are of the same type. However, it is often the case that keyword arguments conveyed by ``**kwargs`` have different types that are dependent on the keyword's name. In those cases type annotating ``**kwargs`` is not possible. This is especially a problem for already existing codebases where the need of refactoring the code in order to introduce proper type annotations may be considered not worth the effort. This in turn prevents the project from getting all of the benefits that type hinting can provide. As a consequence, there has been a lot of discussion around supporting more precise ``**kwargs`` typing [#mypyIssue4441]_ and it became a feature that is necessary for a large part of Python community. |
pep-0692.rst
Outdated
|
||
def foo(**kwargs: Movie) -> None: ... | ||
|
||
means that each keyword argument in ``foo`` is itself a ``Movie`` dictionary that has a ``name`` key with a string type value and a ``year`` key with an integer type value. Therefore, in order to support specifing ``kwargs`` type as a ``TypedDict`` without breaking current behaviour, a new syntax has to be introduced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
means that each keyword argument in ``foo`` is itself a ``Movie`` dictionary that has a ``name`` key with a string type value and a ``year`` key with an integer type value. Therefore, in order to support specifing ``kwargs`` type as a ``TypedDict`` without breaking current behaviour, a new syntax has to be introduced. | |
means that each keyword argument in ``foo`` is itself a ``Movie`` dictionary that has a ``name`` key with a string type value and a ``year`` key with an integer type value. Therefore, in order to support specifing ``kwargs`` type as a ``TypedDict`` without breaking current behaviour, a new syntax has to be introduced. |
means that each keyword argument in ``foo`` is itself a ``Movie`` dictionary that has a ``name`` key with a string type value and a ``year`` key with an integer type value. Therefore, in order to support specifing ``kwargs`` type as a ``TypedDict`` without breaking current behaviour, a new syntax has to be introduced. | |
means that each keyword argument in ``foo`` is itself a ``Movie`` dictionary that has a ``name`` key with a string type value and a ``year`` key with an integer type value. Therefore, in order to support specifying ``kwargs`` type as a ``TypedDict`` without breaking current behaviour, a new syntax has to be introduced. |
pep-0692.rst
Outdated
:: | ||
|
||
def foo(**kwargs: **Movie) -> None: | ||
kwargs["name"].capitalize() # OK! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use assert_type(kwargs, Movie)
to communicate the point more succinctly.
pep-0692.rst
Outdated
Function calls with standard dictionaries | ||
----------------------------------------- | ||
|
||
Calling a function that has ``**kwargs`` typed using the ``**kwargs: **Movie`` sytnax with a standard dictionary must raise an error. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling a function that has ``**kwargs`` typed using the ``**kwargs: **Movie`` sytnax with a standard dictionary must raise an error. For example: | |
Calling a function that has ``**kwargs`` typed using the ``**kwargs: **Movie`` syntax with a standard dictionary must raise an error. For example: |
pep-0692.rst
Outdated
---------- | ||
A function typed using the ``**kwargs: **Movie`` construct can be assigned to another callable type only if they are compatible. This can happen in the following cases: | ||
|
||
1. Both destination and source have a ``**kwargs: **TypedDict`` parameter, the destination ``TypedDict`` is assignable to the source ``TypedDict`` and the rest of the parameters are the compatible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. Both destination and source have a ``**kwargs: **TypedDict`` parameter, the destination ``TypedDict`` is assignable to the source ``TypedDict`` and the rest of the parameters are the compatible. | |
1. Both destination and source have a ``**kwargs: **TypedDict`` parameter, the destination ``TypedDict`` is assignable to the source ``TypedDict`` and the rest of the parameters are compatible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think this gets the variance wrong. Consider two callables f1: (int) -> bool
and f2: (object) -> bool
. f1
is not assignable to f2
, because when you have an f2
, you can pass it any object, but f1
must have an int. On the other hand, f2
is assignable to f1
. Functions parameters behave contravariantly.
For analogous reasons, for two functions (**TD1) -> ...
and (**TD2) -> ...
, the first function is assignable to the second if TD2
is assignable to TD1
, not the other way around.
(2) also gets the variance wrong.
Last, you ignore the return type, but it also affects assignability. However, it is covariant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way I worded it was confusing.
For analogous reasons, for two functions
(**TD1) -> ...
and(**TD2) -> ...
, the first function is assignable to the second ifTD2
is assignable toTD1
, not the other way around.
The first function is assignable to the second, so the first function is the "source" and the second one is the "destination". So the destination TypedDict
(TD2
) has to be assignable to the source TypedDict (TD1
) - I think this is the source (pun not intended) of the error - what I meant is:
Both destination and source have a ``**kwargs: **TypedDict`` parameter, the destination function's ``TypedDict`` is assignable to the source function's ``TypedDict`` and the rest of the parameters are compatible.
Let me try to find a better way to phrase it.
pep-0692.rst
Outdated
|
||
5. If the destination callable contains ``**kwargs: **TypedDict`` then source callable should have keyword parameters assignable to the key value pairs in the ``TypedDict``. Again, the rest of the parameters should be compatible. | ||
|
||
``TypedDict`` unions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go in "Rejected Ideas"
pep-0692.rst
Outdated
Additional keys | ||
--------------- | ||
|
||
A big advantage of using ``**kwargs`` is the ability to pass arbitrary dictionaries containing keyword-value pairs to the function and let the function pick and use only the ones it needs. However, currently, there is no way to define a ``TypedDict`` that would allow additional keys, so supporting this feature would require an addition to the ``TypedDict`` specification. If this limitation blocks the acceptance of this PEP, then the PEP's scope would have to be extended or a new PEP proposing additional keys for ``TypedDict`` would have to be proposed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
pep-0692.rst
Outdated
>>> foo.__annotations__ | ||
{'kwargs': **Movie} | ||
|
||
In order to make the runtime behaviour consistent with the AST as shown above we propose to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't specify what goes in typing.py: that's an implementation detail. But we should specify what the **
operator does, which is call the __unpack__
special method. We should make it explicit that def f(**kwargs: **T):
is equivalent to def f(**kwargs: T.__unpack__()):
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree, I feel like I had to scroll way too far to be notified that this adds a new dunder
pep-0692.rst
Outdated
return self._unpacked | ||
|
||
|
||
Implications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we even need this section.
pep-0692.rst
Outdated
Specification | ||
============= | ||
|
||
To support the aforementioned use case we propose to use the double asterisk syntax inside of the type annotation. The required grammar change is discussed in more detail in section `Grammar Changes`_. Continuing the previous example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize that you were thinking of proposing a grammar change. The SC has indicated that the bar is very high when it comes to grammar changes for features that are specific to typing. I therefore think it's likely that this proposal will be rejected if it proposes a grammar change. I thought the proposal was to support Unpack[T]
, which doesn't require any new syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to engage with the Steering Council on this prior to submitting the PEP, since they did, after all, accept PEP 646 (also required syntax changes, also very typing-specific). To me, the changes in syntax here feel like a fairly natural extension of the changes made by PEP 646, and don't feel nearly as jarring as those proposed by PEP 677 (a PEP I initially supported, but had mixed feelings about by the end). So I don't think these proposed syntax changes necessarily mean it definitely won't get accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm hopeful this will be acceptable because PEP 646's similar change seemed uncontroversial. It's true though that the SC at the PyCon panel recommended to split out syntax changes from typing ecosystem changes, so we could consider having one PEP for the new type system feature (initially spelled with Unpack
) and another for the grammar change. Let me think about this some more and ask for other opinions.
pep-0692.rst
Outdated
Function calls with standard dictionaries | ||
----------------------------------------- | ||
|
||
Calling a function that has ``**kwargs`` typed using the ``**kwargs: **Movie`` sytnax with a standard dictionary must raise an error. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I presume you mean "a type checker should generate an error". The term raise
implies a runtime error.
pep-0692.rst
Outdated
foo(**typedMovie) # OK! | ||
|
||
|
||
However, if the dictionary is completely untyped it is up to the type checker whether to raise an error or not: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this should be omitted. This gets into inference behaviors, which are not specified in any other PEPs.
pep-0692.rst
Outdated
Keyword collisions | ||
------------------ | ||
|
||
A ``TypedDict`` that is used to type ``**kwargs`` could potentially contain keys that are already defined in the function's signature. If the duplicate name is a standard argument, an error should be raised. If the duplicate name is a positional only argument, no errors should be raised. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, I'd avoid using the term "raise" when talking about errors emitted by a type checker. Use "generated" or "emitted" or "reported" instead. The term "raise" implies a runtime exception.
pep-0692.rst
Outdated
title: str | ||
year: NotRequired[int] | ||
|
||
When using a ``TypedDict`` to type ``**kwargs`` all of the required and non-required keys should correspond to required and non-required function keyword parameters. Therefore, if a required key is not supported by the caller, then an error must be raised. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above — avoid the term "raise".
pep-0692.rst
Outdated
|
||
1. Both destination and source have a ``**kwargs: **TypedDict`` parameter, the destination ``TypedDict`` is assignable to the source ``TypedDict`` and the rest of the parameters are the compatible. | ||
|
||
2. If the destination callable contains traditionally typed ``**kwargs: T`` and the source callable is typed using ``**kwargs: **TypedDict`` then each ``TypedDict`` value has to be assignable to type ``T``. Again, the rest of the parameters has to be compatible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section also gets the variance backward.
pep-0692.rst
Outdated
|
||
bar = foo # OK! | ||
|
||
4. If the destination callable contains ``**kwargs: **TypedDict`` and the source callable containes either untyped or traditionally typed ``**kwargs: T`` then an error should be raised. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid the term "raised" here.
pep-0692.rst
Outdated
|
||
2. If the destination callable contains traditionally typed ``**kwargs: T`` and the source callable is typed using ``**kwargs: **TypedDict`` then each ``TypedDict`` value has to be assignable to type ``T``. Again, the rest of the parameters has to be compatible. | ||
|
||
3. If the destination callable doesn't contain ``**kwargs`` and the source callable contains ``**kwargs: **TypedDict`` then all of the ``TypedDict`` fields should be assignable to individual named parameters. Again, the rest of the parameters has to be compatible. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: "has to be" should be "have to be".
pep-0692.rst
Outdated
Alternatives | ||
------------ | ||
|
||
Instead of making the grammar change, ``Unpack`` could be the only way to annotate ``**kwargs`` of different types. However, introducing the double asterisk syntax has two advantages. Namely, it is more concise and more intuitive than using ``Unpack``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "more concise and more intuitive" argument is unlikely to be enough to justify a grammar change here. This is an advanced typing feature that will be used by very few Python programmers, so making it slightly more concise and intuitive is not a strong argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, here is PEP 646's justification for its proposed grammar changes (PEP 646 was accepted by the Steering Council): https://peps.python.org/pep-0646/#alternatives-why-not-just-use-unpack
Thank you for feedback and all the comments! I'll try to address them by Thursday/Friday. |
Thanks for all the comments once again, they are very helpful! I addressed a part of them, will push the changes once I have more. |
I'll also echo the sentiment that I'm excited to see a concrete proposal for TypedDict for **kwargs typing. 😁 I'll plan to take a look at the next draft as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, it appears that I forgot to press the "Submit review" button
pep-0692.rst
Outdated
``TypedDict`` unions | ||
-------------------- | ||
|
||
It is possible to create unions of typed dictionaries. However, supporting typing ``**kwargs`` with a union of typed dicts would greatly increase the complexity of the implementation of this PEP and there seems to be no compelling use case to justify the support for this. Therefore, using unions of typed dictionaries to type ``**kwargs`` as described in the context of this PEP should result in an error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really need to be mentioned?
Wouldn't it be better to leave it open, as perhaps future refactors may make it easier for type checkers to implement this even though (assuming you've asked) currently it doesn't? If a type checker decides to allow this in the future they would be going against this PEP.
I think it would be best to either remove this or somehow reword it to not explicitly say that type checkers should disallow it. Perhaps it could just recommend using overloads instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that makes more sense - changed
pep-0692.rst
Outdated
|
||
TypedDictUnion = Movie | Book | ||
|
||
def foo(**kwargs: **TypedDictUnion) -> None: ... # WRONG! Cannot use a union of TypedDicts to type **kwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going with my other comment, yes this would be wrong and this PEP could recommend using overloads instead:
@overload
def foo(**kwargs: **Movie) -> None: ...
@overload
def foo(**kwargs: **Book) -> None: ...
>>> def foo(**kwargs: **Movie): ... | ||
... | ||
>>> foo.__annotations__ | ||
{'kwargs': **Movie} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What type is the value here? I don't understand, this is invalid Python syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading more of the PEP closer, I see that this is just the string-representation of the value, hence the value should be wrapped in quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might suggest having the example output be instead:
{'kwargs': Unpack[Movie]}
which avoids introducing the new expression form **expr
.
Update: I understand now that **expr
is intended to be the repr() of Unpack[Movie]
. It might be worth saying that explicitly, because it's now confused at least two reviewers after a first reading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't an expression form, it's repr
output, analogous to that for PEP 646:
>>> from typing import TypeVarTuple
>>> Ts = TypeVarTuple("Ts")
>>> def f(*args: *Ts): pass
...
>>> f.__annotations__
{'args': *Ts}
The PEP isn't proposing a new general expression form.
pep-0692.rst
Outdated
>>> foo.__annotations__ | ||
{'kwargs': **Movie} | ||
|
||
In order to make the runtime behaviour consistent with the AST as shown above we propose to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree, I feel like I had to scroll way too far to be notified that this adds a new dunder
pep-0692.rst
Outdated
Backwards Compatibility | ||
----------------------- | ||
|
||
Using the double asterisk operator for annotating ``**kwargs`` is a syntax that would be only available in new versions of Python. PEP 646 dealt with the similar problem and its authors introduced a new type operator ``Unpack``. For the purposes of this PEP, the proposition is to reuse ``Unpack`` for more precise ``**kwargs`` typing. For example: | ||
|
||
:: | ||
|
||
def foo(**kwargs: Unpack[Movie]) -> None: ... | ||
|
||
There are several reasons for reusing PEP 646's ``Unpack``. Firstly, the name is quite suitable and intuitive for the ``**kwargs`` typing use case as the keywords arguments are "unpacked" from the ``TypedDict``. Secondly, there would be no need to introduce any new special forms. Lastly, the use of ``Unpack`` for the purposes described in this PEP does not interfere with the use cases described in PEP 646. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will Unpack
be changed to call __unpack__()
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a need for that - in the case of Unpack
the AST should be consistent with the runtime annotations. The argument for doing that would be to keep Unpack[Movie]
and **kwargs: **Movie
consistent, but then Unpack
would probably have to call that as well when being used in different contexts - it feels like it might be worth it to keep it as so that Unpack
had the flexibility for other use cases?
pep-0692.rst
Outdated
class _UnpackedTypedDict: | ||
def __init__(self, name): | ||
self._name = name | ||
|
||
def __repr__(self): | ||
return '**' + self._name | ||
|
||
class _TypedDictMeta(type): | ||
def __init__(self, name): | ||
self._name = name | ||
self._unpacked = _UnpackedTypedDict(name) | ||
|
||
def __unpack__(self): | ||
return self._unpacked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on how things are done, can't __unpack__()
return Unpack[self]
? Unpack
would in turn be changed to detect TypedDicts and instead show **self.name
in its __str__()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this section altogether as PEPs don't specify implementation details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great first draft!
My most significant feedback is around a resistance to introducing a new **expr
operator and related __unpack__
dunder method. I think this PEP would have a better chance of passing core devs and the SC with a more narrow proposal that only accepts the new **expr
syntax in the position of a kwargs type annotation.
pep-0692.rst
Outdated
Abstract | ||
======== | ||
|
||
Current specification enables for type hinting ``**kwargs`` as long as all of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I had a bit of trouble getting into the first few words with the given phrasing. Perhaps something like:
Current specification enables for type hinting ``**kwargs`` as long as all of | |
Currently ``**kwargs`` can be type hinted so long as |
would be a bit more digestable?
pep-0692.rst
Outdated
in turn prevents the project from getting all of the benefits that type hinting | ||
can provide. As a consequence, there has been a lot of discussion around | ||
supporting more precise ``**kwargs`` typing [#mypyIssue4441]_ and it became a | ||
feature that is necessary for a large part of Python community. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feature that is necessary for a large part of Python community. | |
feature that would be valuable for a large part of the Python community. |
Nit: Missing "the".
Did also weaken the phrasing slightly ("necessary -> "valuable"). (Feel free to ignore me on this aspect.)
pep-0692.rst
Outdated
|
||
|
||
Using the new annotation will not have any runtime effect - it should only be | ||
taken into account by the type checkers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
taken into account by the type checkers. | |
taken into account by type checkers. |
Nit: Remove "the"
pep-0692.rst
Outdated
----------------------------------------- | ||
|
||
Calling a function that has ``**kwargs`` typed using the ``**kwargs: **Movie`` | ||
syntax with a standard dictionary must generate an error. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax with a standard dictionary must generate an error. For example: | |
syntax with a standard dictionary must generate a type checker error. For example: |
It may be worth spelling out that "errors" (unqualified) always mean "type checker errors" as opposed to "runtime errors".
I know this pattern can be inferred from the statement above "Using the new annotation will not have any runtime effect - it should only be taken into account by the type checkers." however I've received similar feedback from non #typing-sig members on my own recent typing PEP.
pep-0692.rst
Outdated
typedMovie: Movie = {"name": "The Meaning of Life", "year": 1983} | ||
foo(**typedMovie) # OK! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typedMovie: Movie = {"name": "The Meaning of Life", "year": 1983} | |
foo(**typedMovie) # OK! | |
typed_movie: Movie = {"name": "The Meaning of Life", "year": 1983} | |
foo(**typed_movie) # OK! |
pep-0692.rst
Outdated
To sum up points, functions' parameters should behave contravariantly. In | ||
addition, functions' return types should behave covariantly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To sum up points, functions' parameters should behave contravariantly. In | |
addition, functions' return types should behave covariantly. | |
To summarize, function parameters should behave contravariantly and | |
function return types should behave covariantly. |
Nit: Wordsmithing
enable more precise ``**kwargs`` typing. The new approach revolves around using | ||
``TypedDict`` to type ``**kwargs`` that comprise keyword arguments of different | ||
types. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth highlighting near the beginning that:
- a Python grammar change is being proposed
a new operator**expr
is being proposed- a new dunder
__unpack__
is being proposed
Since that is useful context for a reader to determine whether they need to respond to changes in the PEP (and thus read it fully).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if a note could be added that
Unpack
for*args
andTypeVarTuple
was introduced inPEP 646
.
to link the two PEP
s and provide greater traceability. It's a little unclear to me from reading whether or not Unpack
will work for all 4 (*args
, TypeVarTuple
, **dict
, TypedDict
) and appropriately handle *args
differently from **kwargs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unpack
isn't mentioned until the Backwards Compatibility section where it is linked to PEP 646. Added a note that the new use cases of Unpack
should not interfere with the behaviour described in 646.
annotation=DoubleStarred( | ||
value=Name(id='Movie', ctx=Load()), | ||
ctx=Load())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So a DoubleStarred(...)
would appear here if you wrote def foo(**kwargs: **Movie): ...
.
What would appear instead if you wrote def foo(**kwargs: Unpack[Movie]): ...
? (This may be a useful example to distinguish how both forms would appear in the AST, if they were to appear differently.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the AST with Unpack in the Backwards Compatibility section.
pep-0692.rst
Outdated
The double asterisk operator should call the ``__unpack__`` special method on | ||
the object it was used on. This means that ``def foo(**kwargs: **T): ...`` is | ||
equivalent to ``def foo(**kwargs: T.__unpack__()): ...``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to imply that an entirely new **expr
operator (with related __unpack__
dunder) is being introduced, that could potentially be used anywhere. I would advocate narrowing the scope of the PEP to only allow the new **expr
syntax in the position of a kwarg annotation and not (yet) elsewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on reading other comments it sounds like there is not an intent to introduce a new general operator. In that case I might suggest replacing the word "operator" here with some other word like "syntax":
The double asterisk operator should call the ``__unpack__`` special method on | |
the object it was used on. This means that ``def foo(**kwargs: **T): ...`` is | |
equivalent to ``def foo(**kwargs: T.__unpack__()): ...``. | |
The double asterisk syntax should call the ``__unpack__`` special method on | |
the object it was used on. This means that ``def foo(**kwargs: **T): ...`` is | |
equivalent to ``def foo(**kwargs: T.__unpack__()): ...``. |
>>> def foo(**kwargs: **Movie): ... | ||
... | ||
>>> foo.__annotations__ | ||
{'kwargs': **Movie} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might suggest having the example output be instead:
{'kwargs': Unpack[Movie]}
which avoids introducing the new expression form **expr
.
Update: I understand now that **expr
is intended to be the repr() of Unpack[Movie]
. It might be worth saying that explicitly, because it's now confused at least two reviewers after a first reading.
On Fri, Jun 10, 2022 at 08:39 David Foster ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Great first draft!
My most significant feedback is around a resistance to introducing a new
**expr operator and related __unpack__ dunder method. I think this PEP
would have a better chance of passing core devs and the SC with a more
narrow proposal that *only* accepts the new **expr syntax in the position
of a kwargs type annotation.
Indeed. What was the reason for introducing this?
…--
--Guido (mobile)
|
pep-0692.rst
Outdated
dest = src # WRONG! | ||
dest(**animal) # Fails at runtime. | ||
|
||
However, when the ``TypedDict`` in the function signature is final, we are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: The idea that decorating a TD class with @Final ensures its values will not have additional keys doesn’t make sense to me. PEP 589 says that TD compatibility is based on structural, not nominal, subtyping. So whether another TD subclasses the intended one (as prevented by final) is irrelevant.
Here’s an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was thinking about that, thanks for mentioning that. At the time it seemed like a good idea because I thought that as a user you would have to "hack around" to break that intentionally, but probably that's not a good premise to base the specification on. On second thought it seems like it would make more sense to disallow that. Let me make the changes.
…ntended Usage section
Added a section on Intended Usage and modified sections mentioning final typed dicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. 👍 Just left a few remaining comments.
pep-0692.rst
Outdated
By default all keys in a ``TypedDict`` are required. This behaviour can be | ||
overridden by setting the dictionary's ``total`` parameter as ``False``. | ||
Moreover, :pep:`655` introduced new type qualifiers - ``typing.Required`` and | ||
``typing.NotRequired`` - that enable specyfing whether a particular key is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
``typing.NotRequired`` - that enable specyfing whether a particular key is | |
``typing.NotRequired`` - that enable specifing whether a particular key is |
Nit: Spelling
pep-0692.rst
Outdated
contract via type hints. | ||
|
||
Adding type hints directly in the source code as opposed to the ``*.pyi`` | ||
stubs benefits to anyone who reads the code as it is easier to understand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stubs benefits to anyone who reads the code as it is easier to understand. | |
stubs benefits anyone who reads the code as it is easier to understand. |
Nit: Wordsmith
pep-0692.rst
Outdated
A new AST node needs to be created so that type checkers can differentiate the | ||
semantics of the new syntax and take into account that the ``**kwargs`` should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
differentiate the semantics of the new syntax
I didn't understand what this fragment was trying to say. Differentiate what from what else?
@davidfstr Thanks for the review! Added suggested changes. |
FYI, @franekmagiera — you can apply a single suggested change by simply clicking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @franekmagiera , thanks for submitting this PEP! It looks like its getting close to ready. Here I have a smattering of minor changes, nearly all as GitHub suggestions, covering PEP formatting, reST syntax and a couple related things.
I did notice some grammar, typographical, clarity and phrasing issues, but I only opportunistically fixed unambiguous ones on the lines that I otherwise touched. If you'd like me to do a full copyedit, I'd be happy to; to avoid delaying and further cluttering this PR, I can make them in a separate PR as a followup to this one, which you can then review.
Note: You can apply changes automatically by clicking Add to batch
on each suggestion in the Files
tab, and then once all the ones you want are selected, clinking Commit
at the top). This will ensure there are no unintentional mistakes when transposing the change, and will auto-resolve the associated review comments. Thanks!
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@CAM-Gerlach thanks for all the tips!
That would be great, all the suggestions really improved the quality of the PR, thank you! |
Where can I find the discussion (if there's been one) behind the naming of the dunder? It occured to me today that while I mean, if you were to read I do not immediately have any suggestions apart from working around it by not adding a new dunder - because even using for example One work-around though, is also using |
The language reference states that all dunder names are reserved for use by the core language and the stdlib; there are no backwards-compatibility guarantees with respect to dunder names. If third-party libraries or apps are using |
@Bluenix2 unfortunately there hasn't been any discussion around naming the dunder. The idea was that similarly to |
Reference for this: https://docs.python.org/3/reference/lexical_analysis.html#reserved-classes-of-identifiers. The docs state:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR has run its course and it's time to merge, but the CLA check is failing. @franekmagiera did you sign the CLA?
pep-0692.rst
Outdated
``Unpack`` for the purposes described in this PEP does not interfere with the | ||
use cases described in :pep:`646`. | ||
|
||
It is worth pointing out that the AST generated for ``Unpack`` would differ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unnecessary since this AST is standard for all Python 3 versions (Unpack isn't syntactically special) and we're not changing anything to it. I'd remove this paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, makes sense, removed.
@JelleZijlstra the corporate CLA was signed, I was told that it is ok for me to sign the individual contributor license agreement to unblock the pipeline, so went ahead and did that. Also removed the section on AST for Unpack. |
This is besides my point - I am aware of this policy. @franekmagiera answered my question though, which was about the specific name of the dunder. |
@CAM-Gerlach now that this is merged, could I take you up on your offer to do a full copyedit? |
Sure, added to my queue. I have some other things to take care, but I'll try to get to it today or tomorrow. Thanks! |
Why not using Protocol for this? It would allow extra arguments |
@rafalkrupinski you might want to bring that up on the PEP's canonical discussion thread instead |
The first version of the "Using TypedDict for more precise **kwargs typing" PEP