@alicederyn, thanks for all your hard work on this PEP. It has come a long way since the earlier drafts, and I like how it’s shaping up!
I’ve organized my feedback by topic area roughly corresponding to the order in which these topics appear in the PEP.
I think this section is looking really good. One small nit is that the “type discrimination” example assumes mypy’s current type narrowing logic. Mypy is using a very conservative approach here — one that is arguably inconsistent with the approach it takes with other type narrowing patterns. By contrast, pyright handles type narrowing for the provided example in the manner in which most users would expect, even though it opens up a small type safety hole.
This feature feels incomplete because it accepts only
Never. It defines the behaviors for
Never and alludes to additional behaviors that could be added in the future. This is a case where I think it’s important for the feature to be fully fleshed out. If not, we risk getting it wrong. I also think this is a case where generalizing the feature will actually simplify the spec and the implementation rather than complicating them. Special-casing the behavior for
Never opens up a bunch of questions and makes it harder to understand the intent of the feature going forward. My preference is that we combine the concepts in PEP 728 and 705 into a single PEP. The resulting PEP will be easier to understand and more coherent. Alternatively, we could separate the
ReadOnly functionality from the
other_keys functionality and make them “concurrent PEPs” that need to be considered as a “linked pair".
The spec says that
other_keys is a flag (implying that it takes a boolean value), but it’s not a flag. Perhaps it should be called a “parameter” instead?
The spec says that
other_keys is optional, but it doesn’t say what it defaults to. The challenge here is that
None are legitimate types, so there is a danger in using either of them as a default value. We could say that
None isn’t a useful type for additional keys, but that seems somewhat arbitrary. We could maybe convince ourselves that a default value of
Any is the correct behavior (more on this below). Another option is to use
ellipsis singleton) as a sentry value. (I’ll note that this is a good example of a detail that we could get wrong if we don’t consider the feature in its fullness.)
I would think that the
other_keys type, if provided, would be inherited by subclasses, but the spec says otherwise. It indicates that if a base class specifies
other_keys, a subclass must likewise specify
other_keys (and presumably always specify the same value as the superclass). Wouldn’t it be better to inherit this property rather than forcing the user to specify a redundant piece of information? It could be argued that specifying
other_keys in a subclass should be considered an error if the parent class already specifies it because it cannot be redefined without breaking type safety.
As I mentioned above, I think the spec could be simplified (and provide additional value in the process) if the
other_keys feature was fully fleshed out. I think this can be done with a few additional rules copied from PEP 728. This would make the “type consistency” section much easier to understand. I find the current bulleted list in the “Type consistency” section very difficult to follow, and it’s important for this section of the spec to be crystal clear.
Here are the modified rules I’d propose:
For each key in B that is not in A, the type of the key must be compatible with the
other_key type if it is specified for
Any if it is not specified).
A TypedDict is consistent with
Mapping[str, V] where
V is the union of all its value types and the
other_key type if specified (or
Any if not specified).
Would it make sense to say that
other_keys defaults to
Any if no class within the class hierarchy defines it? I think that works as long as we allow subclasses to override the
other_keys with a value other than
I think it’s important to allow
ReadOnly[T] to be used for an
other_keys type. This enables various use cases and is required for future type features (like mapping types, which have been discussed in the past).
PEP 705 specifies an
other_keys parameter whereas PEP 728 specifies a dundered
__extra__ field. Neither of these formulations is ideal. I see disadvantages to both. Some of these disadvantages have not yet been discussed in either PEP, and I think it’s important for us to consider the full list of pros and cons before deciding.
It’s not clear how this would work with inlined TypedDict type definitions (a feature that is not yet spec’ed but is in high demand).
It’s not clear how to “spell”
other_keys in a type synthesized within the type checker (e.g. using the process described in the PEP for the
| “merge” operator). How should these types appear in error messages, hover text, etc.? What if two types differ only in their
other_keys value, and the error indicates that one is not consistent with the other?
The argument expression for
other_keys is evaluated at runtime, which means that forward references will need to be quoted even when deferred annotation evaluation becomes the norm.
It requires special-case handling in the type checker to verify that the expression is a valid static type expression and that
NotRequired are not used (although
ReadOnly is permitted).
Given these tradeoffs, I slightly prefer
__extra__, although I’m not crazy about either of them. I wonder if there are other options here that we haven’t yet considered.
I think the spec should make it very clear that
readonly=True is simply a shorthand way to mark all fields as
ReadOnly. This is not a property associated with a class. It’s a property of individual entries. I think it would be clearer if the spec first described
ReadOnly and only later talk about the
readonly parameter (and define it simply as a shorthand). The current spec is confusing because it says at one point that "The
readonly flag is equivalent to marking all entries as
ReadOnly”, but it then spells out rules that contradict this statement. Any place the spec says that the “class” or the “type” is “read only” adds to this confusion. This distinction is especially important if we start to synthesize new classes (e.g. as suggested in the in the PEP for the
| operator). I think it’s likely that we’ll add more ways to modify TypedDict definitions in the future, and the notion of a “read-only class” will get in the way of those efforts.
Here are some specific areas of the spec that this affects:
The spec says that “a read-only type cannot extend a non-read-only type". I contend that there’s no such thing as a “read-only type”, only read-only fields. This restriction should be removed. It’s perfectly legitimate to add read-only fields (or redefine existing fields as read-only) when a base class contains non-read-only fields.
The spec says that
readonly=True “indicates that no mutator operations will be permitted” and that it “removes mutator operations”. These statements are confusing (and even incorrect) because mutator operations are synthesized on a per-field basis as individual overloads. Mutator methods are not synthesized for the entire class.
The spec uses the phrase "The subclass will not be read-only…”. Once again, this is confusing because classes cannot be “read-only”; this property applies only to individual fields within a class.
The spec says that “it is an error to use both
ReadOnly. I don’t think this limitation makes sense. It’s inconsistent with the precedent set by
Required. I don’t see any good justification for imposing this restriction because it doesn’t result in a logical conflict, and its meaning is clear. It seems like something that should be handled by a linter, not the runtime or a type checker. It’s akin to redundant subtypes within a union annotation (
int | str | int).
As I mentioned above, I think the spec would be clearer and build a better mental model for the reader if it leads with a definition of
ReadOnly and then defines
readonly=True as simply a shorthand. Currently, these concepts are presented in the opposite order, and this serves to build the wrong mental model for the reader.
I wonder if we should support
X is a TypedDict) anywhere that a type annotation is allowed? This would allow the TypedDict
X to be defined as writable but used in specific situations where you want all of its fields to be ReadOnly. This would be comparable to
Readonly<X> in typescript. I see that this was considered in the “Rejected Ideas” section, but I think it’s worth considering.
Merge Operations (Union Operation)
The section titled “Union Operation” is confusing. I needed to read it a couple of times before I realized that it’s not talking about “unions” (which is a term that has a well-defined meaning in the type system). Instead, it’s talking about a dictionary “merges” — the operator
|, which is implemented with the
__or__ magic method. I recommend replacing all use of the term “union” with “merge” to avoid confusion.
I found the wording in this section hard to understand. In particular, it says “This is different from the usual compatibility rules…”. I’m not sure what “compatibility rules” means here. Does it mean “type consistency” rules? I don’t think that’s actually different here. If I understand the spec correctly, it’s saying that “type checkers should treat the
| operator specially when the LHS and RHS operands are both TypedDict instances. Rather than applying the typeshed-supplied definition of
_TypedDict.__or__, it should apply special-case logic to compute the type of this operator. I presume the same is true of
__ior__? If my interpretation is correct, I recommend rephrasing it to avoid confusion.
The section also mentions
deepcopy. Is this referring to the
copy method on
_TypedDict or the
copy function exported by the
copy module? I presume
deepcopy refers to the function exported by the
copy module? Is the spec suggesting that type checkers should special-case these functions, effectively overriding the behavior specified in the stubs? I prefer not to do this in pyright unless there’s really strong justification. The spec should be clear about its meaning — and whether this is required or optional for type checkers.
I’m not sure what is being proposed in this section. As the spec points out, type checkers are currently more permissive here (practicality over purity). The spec then indicates that it allows type checkers to be less permissive. Won’t this result in more false positives? Or are you proposing that type checkers should be less permissive only under certain circumstances? Or in certain modes (e.g. when “strict” is requested)? It’s unclear what is being required of type checkers here.
I’m not sure what the spec means by “declare a new type”. A type declaration (at least as I tend to use the word) refers to a code statement that declares the presence of a symbol and describes its type. I don’t think that’s what you mean. Perhaps what you mean is “internally synthesize a new type”? I’d also avoid using the term “copy” and instead use “include”.
I was initially confused by “Union this with an iterable of matching key-value pairs”. I wasn’t aware that
_TypedDict.update supported an iterable of pairs. Neither mypy nor pyright support this today. Also, typeshed’s
_TypedDict.update definition does not include an overload that supports this form. I now understand why you included this statement, but I wonder if it will generate similar confusion for other readers. Perhaps consider deleting it. If you want to keep it, then you may need to specify additional ways in which a type checker needs to accommodate the full range of capabilities of the
update method — including support for keyword parameters. I filed this issue to track this in pyright.
Do we consider “readonly” to be one word or two? If it’s one word, it should be
Readonly (consistent with typescript, FWIW). If it’s two words, it should be
ReadOnly. Currently the spec uses
ReadOnly, which are inconsistent.
In general, the text of the PEP is very inconsistent when talking about TypedDict fields. It alternates between “field”, “entry”, and “key”. FWIW, the term used in the Python
dict class is “item”. I think “key” is probably the least accurate, since that refers to just the name of an entry, not the entire entry. I don’t have a strong opinion here, but I think the spec should try to be more consistent.
other_keys the right name? “Key” doesn’t seem to be the right term here. The value passed to the
other_keys parameter is the type of the “value”, not the “key”, so this name strikes me as especially confusing. I don’t have a strong opinion on the name, but I think we could do better than
other_keys. Here are some options:
I noticed minor errors in a few of the code samples.
The sample beneath the statement "Subclasses can also require keys that are read-only but not required in the superclass:” does not mark the
name field in the subclass as
ReadOnly. That means that it is no longer ReadOnly in the subclass, right? I don’t think that’s what the sample is trying to show. So I recommend changing it to
name: Required[ReadOnly[str]]. Or maybe I’m misunderstanding what this sample is intending to show.
In the “Keyword argument typing” example, there is a bug in the
impl function. It shouldn’t have a
self parameter because it’s not a method. This prevents the assignment from working on the last line of the example.