-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Allow redefining TypedDict keys while still throwing an error #8109
Conversation
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. |
@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 |
Composition might also be impossible if the original Anyways, to me, the issue comes down to the following question: Is In favor of
In favor of
FWIW, in TypeScript (which is what my background is in) if you extend an 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 }; |
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. |
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.
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 |
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 :) |
test-data/unit/check-typeddict.test
Outdated
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})' |
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 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
?
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.
Good point, I've fixed this now.
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, looks good now!
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 theTypedDict
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
TypedDict
s, but I definitely think it should be considered. The lack of ability to redefineTypedDict
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"