| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Co-authored-by: Stanislav Terliakov <50529348+sterliakov@users.noreply.github.com>
|
Also fixes #18334 and maybe something else. |
Sorry, something went wrong.
|
@sterliakov I added #18334 as a unit test. |
Sorry, something went wrong.
|
@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? |
Sorry, something went wrong.
|
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: class Mixed[T, U]:
def new[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] = x
y.new(object(), 0) # Should not error
check_contra: Mixed[str, int] = Mixed[object, int]()
check_co: Mixed[object, int] = Mixed[str, int]()It's not strictly incorrect (because resolving S to object would be valid for x.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 or cast) 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. |
Sorry, something went wrong.
|
I tested a variation of your example pyright-playground, mypy-playground from typing import cast
class Mixed[T, U]:
def new[S](self: "Mixed[S, U]", key: S, val: U) -> None: ...
def test_co(x: Mixed[str, int]) -> Mixed[object, int]:
return x # master: ❌ PR: ✅, pyright: ✅
def test_contra(x: Mixed[object, int]) -> Mixed[str, int]:
return x # master: ✅ PR: ❌, pyright: ❌
def test_now_sub(y: Mixed[object, int]) -> None:
# str value is assignable to object type.
y.new(key=str(), val=0) # master: ✅ PR: ✅, pyright: ✅
def test_new_super(x: Mixed[str, int]) -> None:
# object type is not assignable to str variable.
x.new(key=object(), val=1) # master: ❌ PR: ❌, pyright: ❌
def test_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 pyright and the PR treat T as covariant, whereas on master mypy treats it as contravariant. The new-cast still fails because object cannot be assigned to key. 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. |
Sorry, something went wrong.
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. 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 inferring S to object in a Mixed[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 add def run(self) -> T, and T will remain covariant (my vision: should become invariant). Replace it with def 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 parameterizing self 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. |
Sorry, something went wrong.
But where is this violated? Given T@Mixed is treated as covariant, then test_new_sub passes. What doesn't pass without upcasting is the contravariant case test_new_super, where the key is a supertype of T, and that is fine, because upcasting here would mean we start treating the Mixed[str, int] instance as if it were Mixed[object, int].
Technically, in this example T should be both co- and contravariant simultaneously ("bi-variant"), since there is nothing inside the body of Mixed that would impose variance constraints on T. Usually in this case, both mypy and pyright would prefer covariance over contravariance by convention, for example both treat an empty generic class Foo[T]: pass as covariant (mypy-play, pyright-play.). Whether Mixed ought to be co- or contravariant in T depends on how T is used within its body. But T is absent. polymorphic methods, like new 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 above T is technically both 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 like def get(self) -> T, then master would incorrectly infer Mixed as invariant, when it should be covariant. We can double-check this by using old-style typevars. If Mixed ought to be contravariant in T, then mypy should complain here, but it doesn't: (see also the original example of #19439) from typing import TypeVar, Generic
T = TypeVar("T", covariant=True, contravariant=False)
U = TypeVar("U", covariant=False, contravariant=False)
S = TypeVar("S", covariant=False, contravariant=False)
class Mixed(Generic[T, U]): # OK, no variance error detected.
def get(self) -> T: ... # force covariance
def new(self: "Mixed[S, U]", key: S, val: U) -> None: ... # does not impose constraints on Thttps://mypy-play.net/?mypy=latest&python=3.12&gist=811ae6429e7e0e05ba06e34d2daed000 |
Sorry, something went wrong.
Here: def test_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 pyright
z.new(key=object(), val=1) # master: ✅ PR: ✅, pyright: ✅So z still points to the same x 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 probably Mixed should not be covariant in T.
I'm trying to say that IMO T should be inferred invariant in this case to reject the upcast shown above. That new definition should already impose contravariance, and only invariance remains if other uses reject that. |
Sorry, something went wrong.
I think that's just incorrect. Again, there's nothing inside the body of Mixed that should impose any constraints on the variance of T, so it should be covariant by default. The relevant section of the typing spec states:
So in our case, when we infer T, 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 on T. It doesn't, T doesn't even appear in the expression, and they are identical regardless. |
Sorry, something went wrong.
There was a problem hiding this comment.
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 for S, 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.
Sorry, something went wrong.
Right. I'll edit my comment; but I do not see how it makes any difference whatsoever. |
Sorry, something went wrong.
| 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.
This is not safe, if I continue this example like this I get an error at runtime that is not detected:
class Sub(Cov[int]):
def new(self, arg: list[int]) -> Sub:
print(arg[0].to_bytes())
return self
cov_pos: Cov[object] = Sub()
cov_pos.new([object()])On a more general level, how exactly this:
class Cov[T]:
def get(self) -> T: ...
def new[S](self: Cov[S], arg: list[S]) -> Cov[S]: ...is different from this
class Cov[T]:
def get(self) -> T: ...
def new(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 of uses of Cov where these two classes behave (or should behave) differently?
Sorry, something went wrong.
There was a problem hiding this comment.
? 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 passing self 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 the Cov test case was really just breaking this down to the most elementary MWE.
class Cov[T]:
def get(self) -> T: ...
def new[S](self: Cov[S], arg: list[S]) -> Cov[S]: ...
x: Cov[int]
Cov[str].new(x, [1,2,3]) # OKwhereas
class Cov[T]:
def get(self) -> T: ...
def new(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]
Sorry, something went wrong.
There was a problem hiding this comment.
Consequently, I'd argue that your Sub example is not a proper subclass of Cov. pyright agrees that Sub.new is does not override Cov.new in a compatible manner. Code sample in pyright playground
So I'd say this is actually a false negative in mypy. https://mypy-play.net/?mypy=latest&python=3.12&gist=f60a4694098406077af0b8627fc78557
Sorry, something went wrong.
There was a problem hiding this comment.
Access on class object is not a good argument. For two reasons:
Finally, things like C[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):
class Foo[T](Sequence[T]):
@classmethod
def new[T2](cls: "type[Foo[T2]]", arg: list[T2]) -> "Foo[T2]": ...
@classmethod
def new2[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 like return 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.
Sorry, something went wrong.
There was a problem hiding this comment.
@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 called typing.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.
Sorry, something went wrong.
There was a problem hiding this comment.
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 :-)
Sorry, something went wrong.
There was a problem hiding this comment.
there should be a simple way to override inferred variance.
Feel free to comment or👍 my suggestion for explicit variance spec for PEP695 type hints in discourse.
Sorry, something went wrong.
|
I still believe the proposed changes here are correct. Thanks to @sterliakov's mypy-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.
This just seems incorrect; the typing spec makes clear examples of using Self in contravariant positions, such as using self as a parameter or as a mutable 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:
My question now is how to proceed here, because the current setup leads to
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 in python/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? |
Sorry, something went wrong.
|
@ilevkivskyi I found another bug that is also solved by this PR: (#20997) from typing import Self
class FloatScalar[FloatT]: # contravariant
def pow(self, arg: FloatT) -> Self | complex: ...
def upcast(arg: FloatScalar[object]) -> FloatScalar[float]:
return arg # ❌️ Incompatible return value typeWhat's the way forward here? |
Sorry, something went wrong.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
My idea for fixing it was to replace typ = find_member(member, self_type, self_type) with typ = find_member(member, self_type, plain_self) inside the function infer_variance, where plain_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 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_self is always a supertype of the self type, however it may be parametrized, the typ we get this way should be compatible with the typ we get using the concrete self_type. However, by binding self only to plain_self, it replaces substituted polymorphic variables with Never.
Examples:
With this patch:
Without this patch:
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, given self_type is Class[T], it first solves Class[T] = Class[TypeForm[S, T]] for S inside bind_self, giving us some solution S(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 method method(arg: TypeForm[T]) -> TypeForm[T].
By providing the plain_self, the solution we get is S = Never, which solve the problem.