Skip to content

Allow redefining TypedDict keys while still throwing an error #8109

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

Merged
merged 3 commits into from
Jan 17, 2020

Conversation

ckarnell
Copy link
Contributor

@ckarnell ckarnell commented Dec 8, 2019

Creating a TypedDict type that is similar to an existing one but has different types for one or two keys is currently impossible and requires redefining the TypedDict completely, which can be very verbose in the case of complex types. Throwing a type error in the same way as before, but allowing the overwrite to go through, seems much more reasonable for quality of life purposes.

I understand this PR basically amounts to a design decision about how mypy handles TypedDicts, but I definitely think it should be considered. The lack of ability to redefine TypedDict types slightly causes me lots of headache and seems like something that should be supported. Let me know if we think this is a reasonable change but we want to go about it slightly differently, I'm happy to change this.

To save myself a lot of headache later, I finally got the subset of tests I wanted to run locally with pytest -n 0 -p no:flaky -k "TypeCheckSuite"

@ilevkivskyi
Copy link
Member

But why don't you want to use composition? For example:

class CommonDict(TypedDict):
    # <dozen keys here>
    ...

class OneDict(CommonDict):
    'differing_key': float
class OtherDict(CommonDict):
    'differing_key': int

Anyway, I am not against this, but will not have time to review the PR.

@ckarnell
Copy link
Contributor Author

ckarnell commented Dec 9, 2019

@ilevkivskyi There are cases where it's hard to use composition, like when complex types are being programmatically generated and I want to change the type of one of the keys on it in a certain context.

In any case, since the error is still being thrown, it seems to me like this is just a superset of the functionality we had before, and is a pretty decent QOL feature I would expect TypedDict to support if I were a new user.

@jkillian
Copy link

jkillian commented Dec 9, 2019

But why don't you want to use composition?

Composition might also be impossible if the original TypedDict definition comes from a third-party library, right?


Anyways, to me, the issue comes down to the following question: Is TypedDict type A extends TypedDict type B, is A supposed to be consistent with B? If A is supposed to be consistent with B, then it probably doesn't make sense to allow overwriting keys. If A doesn't have to be consistent with B, then might as well allow overwriting keys.

In favor of A does not have to be consistent with B:

  • The mypy docs say: "[TypedDict] inheritance... is primarily a notational shortcut. Since mypy uses structural compatibility with TypedDicts, inheritance is not required for compatibility." If TypedDict inheritance is purely a notational shortcut, and isn't supposed to reflect type consistency, then making it as fully-featured as possible makes sense and it might as well also allow redefining keys.

In favor of A should be consistent with B:

  • It's somewhat natural to assume that if A extends B, then A is consistent with B
  • PEP 589's rules seems to be around the idea of preserving consistency: "Changing a field type of a parent TypedDict class in a subclass is not allowed" and "Multiple inheritance does not allow conflict types for the same name field" seem

FWIW, in TypeScript (which is what my background is in) if you extend an interface (essentially equivalent to TypedDict), the new type can always be used where the supertype is expected. TypeScript is a bit less strict here though and you can redefine keys to be a more specific subtype, but not an entirely different type:

interface Animal { numLegs: number };
interface Dog extends Animal { numLegs: 4 /* Equivalent to `Literal[4]` in Python */ };
interface Invalid extends Animal { numLegs: string /* Error! */ }

This feels pretty natural, but I don't think it's compatible with PEP 589 since it states: "Value types behave invariantly, since TypedDict objects are mutable." Which is fair, the above constructs can lead to some unsoundness in TypeScript.

TypeScript also lets you work on types in much more granular ways via a variety of operators, but of course when you do this, you have no guarantee that the resulting type is compatible with anything.

type Dog = Omit<Animal, 'numLegs'> & { numLegs: 4 };

@jkillian
Copy link

jkillian commented Dec 9, 2019

In any case, since the error is still being thrown, it seems to me like this is just a superset of the functionality we had before, and is a pretty decent QOL feature I would expect TypedDict to support if I were a new user.

The bad thing here is that this is sort of like relying on undefined behavior, right? Even if we did make this change to MyPy (which I agree, perhaps could be done since the type error still gets thrown), I'd think it'd be very shaky ground for a user to rely on the fact that the type-checker will still proceed as if redefining type was valid. It feels like it might encourage bad behavior to have the type-checker say "Error: you can't do X" but then proceed to type-check as if X were allowed.

So maybe it's more of a philosophical question about how heavy-handed mypy should be with forcing users to act a certain way then.

@ckarnell
Copy link
Contributor Author

ckarnell commented Dec 9, 2019

In favor of A should be consistent with B:

  • It's somewhat natural to assume that if A extends B, then A is consistent with B
  • PEP 589's rules seems to be around the idea of preserving consistency: "Changing a field type of a parent TypedDict class in a subclass is not allowed" and "Multiple inheritance does not allow conflict types for the same name field" seem

I want to note that what is and isn't allowed by the type checker is sort of out of the scope of this PR, since the error is still being thrown. I would argue that since the error is still thrown, and nothing that was previously disallowed is now allowed by the type checker, this change creates a simple API for changing a TypedDict definition without violating PEP 589.

The bad thing here is that this is sort of like relying on undefined behavior, right? Even if we did make this change to MyPy (which I agree, perhaps could be done since the type error still gets thrown), I'd think it'd be very shaky ground for a user to rely on the fact that the type-checker will still proceed as if redefining type was valid.

I would agree that you're on shaky ground if you ignore a type error, yep. I just think this is a better (read: much less astonishing) default behavior in the case that you intentionally ignore the error than completely disallowing redefinition of the types. This is a simple quality of life thing, people don't have to and won't be encouraged to use it (it throws an error after all), but in a pinch, they can!

I think we're mostly in agreement on the rest of your points! And yeah, it's good to mention that composition would also be impossible if the TypedDict definition comes from a third party library.

@ckarnell
Copy link
Contributor Author

Hi @JukkaL and @msullivan , Ivan said he won't be able to review this, so does one of you mind? The change is pretty small :)

@ckarnell ckarnell requested a review from JukkaL January 15, 2020 20:14
pass

b: Bad
reveal_type(b) # N: Revealed type is 'TypedDict('__main__.Bad', {'x': builtins.int})'
reveal_type(b) # N: Revealed type is 'TypedDict('__main__.Bad', {'x': builtins.float})'
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency with how normal multiple inheritance works, the leftmost base types should probably take precedence. That is, definition of x in Point1 should overwrite the definition in Point2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I've fixed this now.

@ckarnell ckarnell requested a review from JukkaL January 17, 2020 02:17
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now!

@JukkaL JukkaL merged commit 7495340 into python:master Jan 17, 2020
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.

4 participants