Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Open
randolf-scholz wants to merge11 commits intopython:master
base:master
Choose a base branch
Loading
fromrandolf-scholz:fix_variance_polymorphic_constructor

Conversation

@randolf-scholz
Copy link
Contributor

@randolf-scholzrandolf-scholz commentedJul 16, 2025
edited
Loading

My idea for fixing it was to replacetyp = find_member(member, self_type, self_type) withtyp = find_member(member, self_type, plain_self) inside the functioninfer_variance, whereplain_self is 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 withUninhabitedType()?
But I am not sure how to do this directly, since the type is only obtained withinfind_member.

According to the docstring offind_member_simple:

Find the member type after applying type arguments from 'itype', and binding 'self' to 'subtype'. Return None if member was not found.

Sinceplain_self is always a supertype of the self type, however it may be parametrized, thetyp we get this way should be compatible with thetyp we get using the concreteself_type. However, by binding self only toplain_self, it replaces substituted polymorphic variables withNever.

Examples:

classFoo[T]:defnew[S](self:"Foo[S]",arg:list[S])->"Foo[S]": ...classBar[T]:defnew(self,arg:list[T])->"Foo[T]": ...

With this patch:

  • Foo.new becomes
    • def [S] (self: tmp_d.Foo[Never], arg: builtins.list[Never]) -> tmp_d.Foo[Never] intypeops.py#L470
    • def (arg: builtins.list[Never]) -> tmp_d.Foo[Never] insubtypes.py#L2211
  • Bar.new becomesdef (arg: builtins.list[T`1]) -> tmp_d.Bar[T`1] (✅)

Without this patch:

  • Foo.new becomes
    • def [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.new becomesdef (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:

class Class[T]:    def method[S](self: Class[TypeForm[S, T]], arg: TypeForm[S, T]) -> TypeForm[S, T]: ...

Now, givenself_type isClass[T], it first solvesClass[T] = Class[TypeForm[S, T]] forS insidebind_self, giving us some solutionS(T), and then substitutes it giving us some non-polymorphic method

def method(self: Class[T], arg: TypeForm[T]) -> TypeForm[T]

and then drops the first argument, so we get the bound methodmethod(arg: TypeForm[T]) -> TypeForm[T].

By providing theplain_self, the solution we get isS = Never, which solve the problem.

jorenham reacted with thumbs up emoji
@github-actions

This comment has been minimized.

randolf-scholzand others added2 commitsJuly 16, 2025 17:42
Co-authored-by: Stanislav Terliakov <50529348+sterliakov@users.noreply.github.com>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@sterliakov
Copy link
Collaborator

Also fixes#18334 and maybe something else.

@randolf-scholz
Copy link
ContributorAuthor

@sterliakov I added#18334 as a unit test.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@randolf-scholz
Copy link
ContributorAuthor

@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
Copy link
Collaborator

sterliakov commentedOct 6, 2025
edited
Loading

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 resolvingS toobject would be valid forx.new() call), but is handled by mypy that way, and errors should never disappear when a value is upcasted (assigned to without ignore, type error orcast) to some wider type.

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
Copy link
ContributorAuthor

randolf-scholz commentedOct 7, 2025
edited
Loading

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 bothpyright and the PR treatT as covariant, whereas on mastermypy treats it as contravariant. The new-cast still fails because object cannot be assigned tokey. On PR andpyright, we can make it work by upcasting first, which is how it should be, since calling a method isn't allowed to mutate the type.

@sterliakov
Copy link
Collaborator

On PR and pyright, we can make it work by upcasting first, which is how it should be, since calling a method isn't allowed to mutate the type.

That just feels horribly wrong. LSP asserts that, given two typesT <: U, any valid operation on U must be valid on T as well. If an upcast removes type checking errors, then something went wrong.

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.test_co should produce an error,test_contra should check cleanly - at least until mypy starts inferringS to object in aMixed[str, int]().new(object(), 0) call, which would render polymorphic methods with unbound vars completely useless and probably isn't going to happen (not sure if this behavior is specced, but it is the most natural one anyway).

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 adddef run(self) -> T, and T will remain covariant (my vision: should become invariant). Replace it withdef run(self, _: T) -> None method with T in contravariant position, and T will be inferred contravariant (my vision: still contravariant).

NB: this is the only category of problems with Any substitution I see right now - when a method has its own type variables parameterizingself type. This PR is already a huge improvement over current behavior on master, so maybe it's a good idea to merge this as-is, but I'm not ready to say "yes, this is definitely correct" now.

@randolf-scholz
Copy link
ContributorAuthor

randolf-scholz commentedOct 7, 2025
edited
Loading

That just feels horribly wrong. LSP asserts that, given two types T <: U, any valid operation on U must be valid on T as well. If an upcast removes type checking errors, then something went wrong.

But where is this violated? GivenT@Mixed is treated as covariant, thentest_new_sub passes. What doesn't pass without upcasting is the contravariant casetest_new_super, where the key is a supertype ofT, and that is fine, because upcasting here would mean we start treating theMixed[str, int] instance as if it wereMixed[object, int].

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.

Technically, in this exampleT should be both co- and contravariant simultaneously ("bi-variant"), since there is nothing inside the body ofMixed that would impose variance constraints onT. Usually in this case, bothmypy andpyright would prefer covariance over contravariance by convention, for example both treat an empty genericclass Foo[T]: pass as covariant (mypy-play,pyright-play.).

WhetherMixed ought to be co- or contravariant inT depends on howT is used within its body. ButT is absent. polymorphic methods, likenew in the example, that use a method-bound typevar do not impose constraints on the variance. (and this is the bug, mypy incorrectly infers a constraint here).

So the result is not technically wrong, as explained aboveT is technicallyboth co- and contravariant, but (A) the way it is inferred is incorrect, and (B) it goes against the usual preference for covariance in the case of no constraints.

Moreover, if we added a covariant constraint likedef get(self) -> T, then master would incorrectly inferMixed as invariant, when it should be covariant. We can double-check this by using old-style typevars. IfMixed ought to be contravariant inT, thenmypy should complain here, but it doesn't: (see also the original example of#19439)

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
Copy link
Collaborator

sterliakov commentedOct 7, 2025
edited
Loading

But where is this violated?

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: ✅

Soz still points to the samex object, and upcast (allowed in this PR) removes a type error. In this PR and pyright,Mixed[str, int] <: Mixed[object, int], and the outcome is incorrect (safe upcast removes a typing error), so probablyMixed shouldnot be covariant inT.

Moreover, if we added a covariant constraint like def get(self) -> T, then master would incorrectly infer Mixed as invariant

I'm trying to say that IMO Tshould be inferred invariant in this case to reject the upcast shown above. Thatnew definition should already impose contravariance, and only invariance remains if other uses reject that.

@randolf-scholz
Copy link
ContributorAuthor

randolf-scholz commentedOct 7, 2025
edited
Loading

I'm trying to say that IMO T should be inferred invariant in this case to reject the upcast shown above.

I think that's just incorrect. Again, there's nothing inside the body ofMixed that should impose any constraints on the variance ofT, so it should be covariant by default.

Therelevant section of the typing spec states:

  1. Create two specialized versions of the class. We’ll refer to these as upper and lower specializations. In both of these specializations, replace all type parameters other than the one being inferred by a dummy type instance (a concrete anonymous class that is assumed to meet the bounds or constraints of the type parameter). In the upper specialized class, specialize the target type parameter with an object instance. This specialization ignores the type parameter’s upper bound or constraints. In the lower specialized class, specialize the target type parameter with itself (i.e. the corresponding type argument is the type parameter itself).
  2. Determine whether lower can be assigned to upper using normal assignability rules. If so, the target type parameter is covariant. If not, determine whether upper can be assigned to lower. If so, the target type parameter is contravariant. If neither of these combinations are assignable, the target type parameter is invariant.

So in our case, when we inferT, that means we should check whether

def [S] (self: Mixed[S, DummyU], key: S, val: DummyU) -> None: ...

being a subtype of

def [S] (self: Mixed[S, DummyU], key: S, val: DummyU) -> None: ...

Imposes any constraints onT. It doesn't,T doesn't even appear in the expression, and they are identical regardless.

Copy link
Collaborator

@sterliakovsterliakov left a comment
edited
Loading

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
Copy link
ContributorAuthor

(though "replace all type parameters other than the one being inferred by a dummy type instance" definitely does not mean substituting dummies for S, but that will not impact the outcome here)

Right. I'll edit my comment; but I do not see how it makes any difference whatsoever.

@github-actions

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]()
Copy link
Member

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?

Copy link
ContributorAuthor

@randolf-scholzrandolf-scholzOct 15, 2025
edited
Loading

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]

Copy link
ContributorAuthor

@randolf-scholzrandolf-scholzOct 15, 2025
edited
Loading

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

Copy link
Member

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.

Copy link
Collaborator

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.

Copy link
Member

Choose a reason for hiding this comment

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

@sterliakov

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

sterliakov reacted with thumbs up emoji
Copy link
ContributorAuthor

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.

ilevkivskyi reacted with thumbs up emoji
@github-actions
Copy link
Contributor

According tomypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@randolf-scholz
Copy link
ContributorAuthor

I still believe the proposed changes here are correct. Thanks to@sterliakov'smypy-issues I found it also fixes#18920 (at least without type defaults).

However, I do want to add that I strongly disagree with@sterliakov's#18920 (comment) in that issue.

I'm big +1 on rejecting such Self usage in instance attributes right at the definition site. Self is only well-defined in covariant positions, so any use in writeable attributes and parameters is immediately unsafe and confusing - it violates all the rules we know (like "upcasting to a supertype should never make an ill-typed program well-typed") and can hide a lot of soundness issues.

This just seems incorrect; the typing spec makes clear examples of usingSelf in contravariant positions, such as usingselfas a parameter or as amutable attribute.

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:

Mypy seems to assume that Self is class-scoped. It isn't; Self is method- or property-scoped. So Self cannot influence the (variance of) class-scoped type parameters.

My question now is how to proceed here, because the current setup leads to

  1. annoying divergence between type checkers
  2. internal inconsistencies in mypy itself, where it treats PEP695 code differently from equivalent old-style code. (see my original example[PEP 695] Incorrect Variance Computation with Polymorphic Constructor. #19439)

Currently,mypy-primer doesn't find much, but I doubt there are many repos in the list that use PEP695 syntax.


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?

jorenham reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ilevkivskyiilevkivskyiilevkivskyi left review comments

+1 more reviewer

@sterliakovsterliakovsterliakov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

3 participants

@randolf-scholz@sterliakov@ilevkivskyi

[8]ページ先頭

©2009-2025 Movatter.jp