Fix dataclass/protocol crash on joining types #15629
Conversation
ikonst
force-pushed
the
07-09-Fix_dataclass/protocol_crash_on_joining
branch
from
00fd471
to
d46b01c
Compare
ikonst
force-pushed
the
07-09-Fix_dataclass/protocol_crash_on_joining
branch
from
d46b01c
to
afecb8a
Compare
| def to_argument( | ||
| self, current_info: TypeInfo, *, of: Literal["__init__", "replace", "__post_init__"] | ||
| ) -> Argument: | ||
| if of == "__init__": |
There was a problem hiding this comment.
Now that we're using to_argument for all use cases, it's the arg_pos that differs between the use cases.
| @@ -86,7 +86,7 @@ | |||
| field_specifiers=("dataclasses.Field", "dataclasses.field"), | |||
| ) | |||
| _INTERNAL_REPLACE_SYM_NAME: Final = "__mypy-replace" | |||
| _INTERNAL_POST_INIT_SYM_NAME: Final = "__mypy-__post_init__" | |||
| _INTERNAL_POST_INIT_SYM_NAME: Final = "__mypy-post_init" | |||
There was a problem hiding this comment.
@sobolevn btw, __mypy-replace was intentionally class-private (starts with __ but doesn't end with __), aligning the post_init sym to be the same
| ideal_sig = ideal_sig_method.type | ||
| assert isinstance(ideal_sig, ProperType) # we set it ourselves | ||
| assert isinstance(ideal_sig, CallableType) | ||
| ideal_sig = ideal_sig.copy_modified(name="__post_init__") |
There was a problem hiding this comment.
Have to copy with new name so that it appears correctly in the error message. Fortunately we do it just once per class.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
| T_co = TypeVar("T_co", covariant=True) | ||
|
|
||
| @dataclass | ||
| class MyDataclass(Generic[T_co]): | ||
| a: T_co |
There was a problem hiding this comment.
I'm struggling to find an example where mypy's current behaviour actually causes some unsafe behaviour, but it feels weird to me that mypy will reject this:
from typing import TypeVar, Protocol
T_co = TypeVar("T_co", covariant=True)
class Foo(Protocol[T_co]): # error: Covariant type variable "T_co" used in protocol where invariant one is expected
x: T_coBut is fine with this:
from dataclasses import dataclass
from typing import TypeVar, Generic
T_co = TypeVar("T_co", covariant=True)
@dataclass
class Bar(Generic[T_co]):
x: T_coIt feels like the argument that the former is unsafe should probably also apply to the latter? But as I say, I'm struggling to come up with a real example where mypy is currently not flagging unsafe behaviour with generic dataclasses.
There was a problem hiding this comment.
Both are unsafe, see also #3208. But for regular (i.e. nominal) classes it is hard to prohibit this, since it would force people to use @property or Final explicitly everywhere if the attribute is effectively read-only. Protocols were a new thing, so we were able to implement them safely from the start (also an additional argument is that bugs with structural subtyping are usually trickier to find, so our handling of structural subtyping should be stricter that our handling of nominal subtyping).
Btw I was thinking about actually trying to prohibit covariant mutable overrides in --strict mode first, and see what will be the fallout and how people will react to this.
There was a problem hiding this comment.
Btw I'm merely testing here a regression that surfaced in mypy_primer.
There was a problem hiding this comment.
Btw I'm merely testing here a regression that surfaced in mypy_primer.
Yes, I was questioning whether it was actually a regression, or whether the new behaviour that we had with your PR before your latest commit was actually a feature rather than a bug :-)
Before your latest commit, though, mypy was flagging frozen dataclasses with covariant attributes, and that was a false positive, since all attributes on frozen dataclasses are readonly
There was a problem hiding this comment.
Even if we went to report it, probably not like this.
What was being reported were covariant parameters of the synthetic __mypy-replace and __mypy-post_init methods (which are added in a less hacky manner now and probably that's why they hit this check).
The root cause is hacky creation of incomplete symbols; instead switching to
add_method_to_classwhich does the necessary housekeeping.Fixes #15618.