PEP 705 - TypedDict: Read-only and other keys

Typing
6

@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.

—

Motivation

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.

—

Specification for other_keys flag

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 Any and 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 … (the 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.

—

Generalizing other_keys

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 A (or 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 Any.

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).

—

other_keys versus __extra__

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.

Disadvantages of other_keys

  • 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 Required and NotRequired are not used (although ReadOnly is permitted).

Disadvantages of __extra__

  • It means that __extra__ cannot be used as a legitimate key name.

  • It requires special-case handling in the type checker to disallow Required and NotRequired.

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.

readonly

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=True and ReadOnly[]. I don’t think this limitation makes sense. It’s inconsistent with the precedent set by total=True and 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).

—

ReadOnly

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 ReadOnly[X] (where 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 copy and 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.

—

Update Operations

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.

—

Nomenclature

Do we consider “readonly” to be one word or two? If it’s one word, it should be readonly and Readonly (consistent with typescript, FWIW). If it’s two words, it should be read_only and ReadOnly. Currently the spec uses readonly and 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.

Is 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: other_entries, other_fields, other_items, extra_entries, extra_fields, extra_items, other, extra.

—

Samples

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.

1 Like