-
Notifications
You must be signed in to change notification settings - Fork 2k
[ty] make Type::BoundMethod include instances of same-named methods bound to a subclass
#24039
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ use crate::types::constraints::{ | |
| }; | ||
| use crate::types::cyclic::PairVisitor; | ||
| use crate::types::enums::is_single_member_enum; | ||
| use crate::types::function::FunctionDecorators; | ||
| use crate::types::set_theoretic::RecursivelyDefined; | ||
| use crate::types::{ | ||
| CallableType, ClassBase, ClassType, CycleDetector, DynamicType, KnownBoundMethodType, | ||
|
|
@@ -1877,15 +1878,13 @@ impl<'a, 'c, 'db> DisjointnessChecker<'a, 'c, 'db> { | |
| ( | ||
| // note `LiteralString` is not single-valued, but we handle the special case above | ||
| left @ (Type::FunctionLiteral(..) | ||
| | Type::BoundMethod(..) | ||
| | Type::KnownBoundMethod(..) | ||
| | Type::WrapperDescriptor(..) | ||
| | Type::ModuleLiteral(..) | ||
| | Type::ClassLiteral(..) | ||
| | Type::SpecialForm(..) | ||
| | Type::KnownInstance(..)), | ||
| right @ (Type::FunctionLiteral(..) | ||
| | Type::BoundMethod(..) | ||
| | Type::KnownBoundMethod(..) | ||
| | Type::WrapperDescriptor(..) | ||
| | Type::ModuleLiteral(..) | ||
|
|
@@ -2197,6 +2196,58 @@ impl<'a, 'c, 'db> DisjointnessChecker<'a, 'c, 'db> { | |
| .negate(db, self.constraints) | ||
| } | ||
|
|
||
| // A `BoundMethod` type includes instances of the same method bound to a | ||
| // subtype/subclass of the self type. | ||
| (Type::BoundMethod(a), Type::BoundMethod(b)) => { | ||
| if a.function(db).name(db) != b.function(db).name(db) { | ||
| // We typically ask about `BoundMethod` disjointness when we're looking at a | ||
| // method call on an intersection type like `A & B`. In that case, the same | ||
| // method name would show up on both sides of this check. However for | ||
| // completeness, if we're ever comparing `BoundMethod` types with different | ||
| // method names, then they're clearly disjoint. | ||
| self.always() | ||
| } else if a.function(db) != b.function(db) | ||
| && a.function(db) | ||
| .has_known_decorator(db, FunctionDecorators::FINAL) | ||
| && b.function(db) | ||
| .has_known_decorator(db, FunctionDecorators::FINAL) | ||
| { | ||
| // If *both* methods are `@final` (and they're not literally the same | ||
| // definition), they must be disjoint. | ||
| // | ||
| // Note that we can't establish disjointness when only one side is `@final`, | ||
| // because we have to worry about cases like this: | ||
| // | ||
| // ``` | ||
| // class A: | ||
| // def f(self): ... | ||
| // class B: | ||
| // @final | ||
| // def f(self): ... | ||
| // # Valid in this order, though `C(A, B)` would be invalid. | ||
| // class C(B, A): ... | ||
| // ``` | ||
| self.always() | ||
| } else { | ||
| // The names match, so `BoundMethod` disjointness depends on whether the bound | ||
| // self types are disjoint. Note that this can produce confusing results in the | ||
| // face of Liskov violations. For example: | ||
| // ``` | ||
| // class A: | ||
| // def f(self) -> int: ... | ||
| // class B: | ||
| // def f(self) -> str: ... | ||
| // def _(x: Intersection[A, B]): | ||
| // x.f() | ||
| // ``` | ||
| // `class C(A, B)` could inhabit that intersection, but `int` and `str` are | ||
| // disjoint, so the type of `x.f()` there is going to be inferred as `Never`. | ||
| // That's probably not correct in practice, but the right way to address it is | ||
| // to emit a diagnostic on the definition of `C.f`. | ||
| self.check_type_pair(db, a.self_instance(db), b.self_instance(db)) | ||
|
Comment on lines
+2208
to
+2247
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When two overlapping classes define methods with the same name but different implementations, this branch now keeps their bound-method types non-disjoint purely because the names match and the instance types overlap. That is too permissive: Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should (though currently don't) reject both I think enforcing Liskov is the thing that makes it OK for this PR to not consider the actual signatures of the methods. That may be worth a comment here. |
||
| } | ||
| } | ||
|
|
||
| (Type::BoundMethod(_), other) | (other, Type::BoundMethod(_)) => { | ||
| self.check_type_pair(db, KnownClass::MethodType.to_instance(db), other) | ||
| } | ||
|
|
||
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.
Do we need to also consider method finality here (not just class finality, which is implicitly considered by the self-type disjointness check)? A
@finalmethod cannot be overridden in a subclass.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.
Playing with this a bit, there are some corner cases. Here's an example where a non-final method and a
@finalmethod are not disjoint, because the latter inherits from the former:Multiple inheritance can also come into play here:
The
@finalmethod wins in the MRO in that case, so it presumably doesn't violate the rules for@final, but the opposite inheritance order presumably would violate those rules? On the other hand only Mypy currently gives a diagnostic for that.That said, if both sides are
@final(and they're not literally the same method), it does seem fair to conclude that they should be disjoint?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.
@carljm could you take a look at 327fdcb and tell me if it makes sense to you?
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.
Looks good!