Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.1k
[PEP 695] Fix incorrect Variance Computation with Polymorphic Methods.#19466
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
[PEP 695] Fix incorrect Variance Computation with Polymorphic Methods.#19466
Conversation
Uh oh!
There was an error while loading.Please reload this page.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Stanislav Terliakov <50529348+sterliakov@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sterliakov commentedSep 2, 2025
Also fixes#18334 and maybe something else. |
randolf-scholz commentedSep 16, 2025
@sterliakov I added#18334 as a unit test. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
randolf-scholz commentedOct 6, 2025
@sterliakov This and a few other small PRs (#19517,#19471,#19449) have been sitting since mid-July, I'm guessing you are rather swamped. Who else should I ask for review? |
sterliakov commentedOct 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I am a bit swamped indeed, and also I don't have merge powers here anyway:) This change is really non-trivial. I have already looked at this PR and, tbh, I'm still not 100% certain that binding self to Any-filled self is the correct move here. It makes some sense to me, but I'd love to see more tests with manually verified logic. I just took another look, and IMO the following snippet will be handled incorrectly: classMixed[T,U]:defnew[S](self:"Mixed[S, U]",key:S,val:U)->None: ...x=Mixed[str,int]()x.new(object(),0)# Should error# So this upcast is not really an upcast and should be rejected, T should be contravariant?y:Mixed[object,int]=xy.new(object(),0)# Should not errorcheck_contra:Mixed[str,int]=Mixed[object,int]()check_co:Mixed[object,int]=Mixed[str,int]() It's not strictly incorrect (because resolving I didn't try to compare this to Pyright or other type checkers. I'd suggest to also ping@JukkaL for review - he authored a huge part of PEP695 implementation, including this inference. |
randolf-scholz commentedOct 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I tested a variation of your examplepyright-playground,mypy-playground fromtypingimportcastclassMixed[T,U]:defnew[S](self:"Mixed[S, U]",key:S,val:U)->None: ...deftest_co(x:Mixed[str,int])->Mixed[object,int]:returnx# master: ❌ PR: ✅, pyright: ✅deftest_contra(x:Mixed[object,int])->Mixed[str,int]:returnx# master: ✅ PR: ❌, pyright: ❌deftest_now_sub(y:Mixed[object,int])->None:# str value is assignable to object type.y.new(key=str(),val=0)# master: ✅ PR: ✅, pyright: ✅deftest_new_super(x:Mixed[str,int])->None:# object type is not assignable to str variable.x.new(key=object(),val=1)# master: ❌ PR: ❌, pyright: ❌deftest_new_upcast(x:Mixed[str,int])->None:# technically, one could upcast first:z:Mixed[object,int]=x# master: ❌ PR: ✅, pyright: ✅z=Mixed[object,int]()z.new(key=object(),val=1)# master: ✅ PR: ✅, pyright: ✅ So both |
sterliakov commentedOct 7, 2025
That just feels horribly wrong. LSP asserts that, given two types Please also note that Pyright isn't bug-free, so it makes sense to compare and look closer at any deviations, but it isn't a "golden standard" we aim to match perfectly. Right now I'm moderately certain that Mixed should be contravariant in T (as inferred by current mypy master), not covariant as this PR and Pyright think. I crafted the example to demo the problem with Any substitution. It also means some weird inconsistency: now T is inferred covariant (my vision: contravariant). You can add NB: this is the only category of problems with Any substitution I see right now - when a method has its own type variables parameterizing |
randolf-scholz commentedOct 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
But where is this violated? Given
Technically, in this example Whether So the result is not technically wrong, as explained above Moreover, if we added a covariant constraint like fromtypingimportTypeVar,GenericT=TypeVar("T",covariant=True,contravariant=False)U=TypeVar("U",covariant=False,contravariant=False)S=TypeVar("S",covariant=False,contravariant=False)classMixed(Generic[T,U]):# OK, no variance error detected.defget(self)->T: ...# force covariancedefnew(self:"Mixed[S, U]",key:S,val:U)->None: ...# does not impose constraints on T https://mypy-play.net/?mypy=latest&python=3.12&gist=811ae6429e7e0e05ba06e34d2daed000 |
sterliakov commentedOct 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Here: deftest_new_upcast(x:Mixed[str,int])->None:x.new(object(),1)# master: err, PR: err, pyright: err# technically, one could upcast first:z:Mixed[object,int]=x# master: ❌ PR: ✅, pyright: ✅z=Mixed[object,int]()# Only to counter weird narrowing by pyrightz.new(key=object(),val=1)# master: ✅ PR: ✅, pyright: ✅ So
I'm trying to say that IMO Tshould be inferred invariant in this case to reject the upcast shown above. That |
randolf-scholz commentedOct 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I think that's just incorrect. Again, there's nothing inside the body of Therelevant section of the typing spec states:
So in our case, when we infer being a subtype of Imposes any constraints on |
sterliakov left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Okay, and that was convincing! Seems like this PR implements the spec correctly, and I should move this discussion to typing spec repository or Discourse instead. Thank you for clarification!
(though "replace all type parameters other than the one being inferred by a dummy type instance" definitely does not mean substituting dummies forS, but that will not impact the outcome here)
I'll post a link here when I have enough energy to check the PEP695 history to see if this has already been discussed and, if not, write a detailed overview for Discourse.
randolf-scholz commentedOct 7, 2025
Right. I'll edit my comment; but I do not see how it makes any difference whatsoever. |
This comment has been minimized.
This comment has been minimized.
| def get(self) -> T: ... | ||
| def new[S](self: "Cov[S]", arg: list[S]) -> "Cov[S]": ... | ||
| cov_pos: Cov[object] = Cov[int]() |
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.
This is not safe, if I continue this example like this I get an error at runtime that is not detected:
classSub(Cov[int]):defnew(self,arg:list[int])->Sub:print(arg[0].to_bytes())returnselfcov_pos:Cov[object]=Sub()cov_pos.new([object()])
On a more general level, how exactly this:
classCov[T]:defget(self)->T: ...defnew[S](self:Cov[S],arg:list[S])->Cov[S]: ...
is different from this
classCov[T]:defget(self)->T: ...defnew(self,arg:list[T])->Cov[T]: ...
? Maybe I am missing something but these two are literally identical in terms of semantics. Can you give an example ofuses ofCov where these two classes behave (or should behave) differently?
randolf-scholzOct 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
? Maybe I am missing something but these two are literally identical in terms of semantics.
I think they are only identical when called as a bound method, but not when called on the class and passingself as a regular argument. Note that my original example in#19439 - which is how I came across this issue -- was in the context of classmethods, and theCov test case was really just breaking this down to the most elementary MWE.
classCov[T]:defget(self)->T: ...defnew[S](self:Cov[S],arg:list[S])->Cov[S]: ...x:Cov[int]Cov[str].new(x, [1,2,3])# OK
whereas
classCov[T]:defget(self)->T: ...defnew(self,arg:list[T])->Cov[T]: ...x:Cov[int]Cov[str].new(x, [1,2,3])# not OK, self must be subtype of Cov[str]
randolf-scholzOct 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Consequently, I'd argue that yourSub example is not a proper subclass ofCov.pyright agrees thatSub.new is does not overrideCov.new in a compatible manner. Code sample inpyright playground
So I'd say this is actually a false negative in mypy.https://mypy-play.net/?mypy=latest&python=3.12&gist=f60a4694098406077af0b8627fc78557
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.
Access on class object is not a good argument. For two reasons:
- First, prohibiting overrides that are incompatible on class object only would prohibita lot of use cases that people expect to be treated as safe. Most notably, overrides involving properties and various custom descriptors will stop working.
- Second, my example doesn't involve or rely on class object access in any way. So it is at least weird to say the unsafety is caused by the class object access incompatibility.
Finally, things likeC[int].method are not really well specified (for good reasons, google type erasure). Support for this was added to mypy only recently, following a popular demand.
Btw#18334 is not a bug, it is a true positive. Coming back to your original issue: the two revealed types here are different (and both IMO correct):
classFoo[T](Sequence[T]):@classmethoddefnew[T2](cls:"type[Foo[T2]]",arg:list[T2])->"Foo[T2]": ...@classmethoddefnew2[T2](cls,arg:list[T2])->"Foo[T2]": ...tfi:type[Foo[int]]reveal_type(tfi.new)# def (arg: builtins.list[builtins.int]) -> tstgrp3.Foo[builtins.int]reveal_type(tfi.new2)# def [T2] (arg: builtins.list[T2`2]) -> tstgrp3.Foo[T2`2]
and this is the reason why a class with the first method is considered invariant. I understand why do you want to have the first one: the body should include something likereturn cls(arg) which would give an error with the second method.
TBH I don't think this is solvable without special-casing alternative constructors somehow. Also it looks like a flaw in PEP 695, there should be a simple way to override inferred variance.
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.
@ilevkivskyi your unsafety example is very similar to what I pointed out before, and the problem is roughly "yes, this is obviously unsafe, but the spec says so". What's the official mypy stance on the spec conformance? I prefer to read the spec as an advice, not a mandatory requirement, but IDK if that matches the project attitude.
If mypy considers spec conformance its goal, then this PR is correct, and a typing-sig discussion is needed to fix this spec unsoundness. If not, this PR makes the state of affairs worse, trading false positives for false negatives.
there should be a simple way to override inferred variance
There is, it's calledtyping.TypeVar. AFAIC this ability was one of the reasons to not even consider deprecating "old-style" generics. I think it's rare enough to not warrant any extra syntax (though I'm a bad person to ask about that, IMO PEP695 should have never been implemented at all), so not a PEP omission.
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.
I prefer to read the spec as an advice, not a mandatory requirement, but IDK if that matches the project attitude.
Yes, this is the official mypy stance: internal consistency is more important than consistency with the spec.
AFAIC this ability was one of the reasons to not even consider deprecating "old-style" generics
OK, I see :-)
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.
there should be a simple way to override inferred variance.
Feel free to comment or👍 my suggestion for explicit variance spec for PEP695 type hintsin discourse.
According tomypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
randolf-scholz commentedDec 16, 2025
I still believe the proposed changes here are correct. Thanks to@sterliakov's However, I do want to add that I strongly disagree with@sterliakov's#18920 (comment) in that issue.
This just seems incorrect; the typing spec makes clear examples of using Instead, I strongly agree@jorenham beautiful explanation in his#18334 (comment) why this is not unsound and gives a hint where the source of confusion comes from:
My question now is how to proceed here, because the current setup leads to
Currently, Long story short: should I start an issue inpython/typing to get clarification on this issues (though I personally think the spec is clear enough), or will we be able to come to a conclusion here? |
Uh oh!
There was an error while loading.Please reload this page.
Selftype on an attribute #18334Selfnot accepted when used with adataclass#18920testPEP695InferVariancePolymorphicMethodtestPEP695SelfAttributeMy idea for fixing it was to replace
typ = find_member(member, self_type, self_type)withtyp = find_member(member, self_type, plain_self)inside the functioninfer_variance, whereplain_selfis the type of self without any type variables.To be frank, I do not myself 100% understand why it works / if it is safe, but below is my best effort explanation.
Maybe a better solution is to substitute all function variables with
UninhabitedType()?But I am not sure how to do this directly, since the type is only obtained within
find_member.According to the docstring of
find_member_simple:Since
plain_selfis always a supertype of the self type, however it may be parametrized, thetypwe get this way should be compatible with thetypwe get using the concreteself_type. However, by binding self only toplain_self, it replaces substituted polymorphic variables withNever.Examples:
With this patch:
Foo.newbecomesdef [S] (self: tmp_d.Foo[Never], arg: builtins.list[Never]) -> tmp_d.Foo[Never]intypeops.py#L470def (arg: builtins.list[Never]) -> tmp_d.Foo[Never]insubtypes.py#L2211Bar.newbecomesdef (arg: builtins.list[T`1]) -> tmp_d.Bar[T`1](✅)Without this patch:
Foo.newbecomesdef [S] (self: tmp_d.Foo[T`1], arg: builtins.list[T`1]) -> tmp_d.Foo[T`1]intypeops.py#L470 (❌)def (arg: builtins.list[T`1]) -> tmp_d.Foo[T`1]insubtypes.py#L2211 (❌)Bar.newbecomesdef (arg: builtins.list[T`1]) -> tmp_d.Bar[T`1](✅)Another way to think about it is we can generally assume a signature of the form:
Now, given
self_typeisClass[T], it first solvesClass[T] = Class[TypeForm[S, T]]forSinsidebind_self, giving us some solutionS(T), and then substitutes it giving us some non-polymorphic methoddef method(self: Class[T], arg: TypeForm[T]) -> TypeForm[T]and then drops the first argument, so we get the bound method
method(arg: TypeForm[T]) -> TypeForm[T].By providing the
plain_self, the solution we get isS = Never, which solve the problem.