mirror of https://github.com/astral-sh/ruff
[ty] Extend Liskov checks to cover cases where methods are incompatibly overridden by non-methods
This commit is contained in:
Alex Waygood
parent
b19ddca69b
commit
67a54e6b9a
|
|
@ -583,3 +583,54 @@ class GoodChild2(Parent):
|
|||
@staticmethod
|
||||
def static_method(x: object) -> bool: ...
|
||||
```
|
||||
|
||||
## Method incompatibly overridden by a non-method
|
||||
|
||||
<!-- snapshot-diagnostics -->
|
||||
|
||||
```pyi
|
||||
from typing import ClassVar, Final, Callable, Any
|
||||
|
||||
class Parent:
|
||||
def method(self): ...
|
||||
|
||||
class Child1(Parent):
|
||||
method = None # error: [invalid-method-override]
|
||||
|
||||
class Child2(Parent):
|
||||
method: ClassVar = None # error: [invalid-method-override]
|
||||
|
||||
class Child3(Parent):
|
||||
method: None = None # error: [invalid-method-override]
|
||||
|
||||
class Child4(Parent):
|
||||
# error: [invalid-assignment] "Object of type `None` is not assignable to `(...) -> Unknown`"
|
||||
# error: [invalid-method-override]
|
||||
method: Callable = None
|
||||
|
||||
class Child5(Parent):
|
||||
method: Callable | None = None # error: [invalid-method-override]
|
||||
|
||||
class Child6(Parent):
|
||||
method: Final = None # error: [invalid-method-override]
|
||||
|
||||
class Child7(Parent):
|
||||
method: None # error: [invalid-method-override]
|
||||
|
||||
class Child8(Parent):
|
||||
# A `Callable` type is insufficient for the type to be assignable to
|
||||
# the superclass definition: the definition on the superclass is a
|
||||
# *function-like* callable, which can be used as a method descriptor,
|
||||
# and which has `types.FunctionType` attributes such as `__name__`,
|
||||
# `__kwdefaults__`, etc.
|
||||
method: Callable # error: [invalid-method-override]
|
||||
|
||||
class Child9(Parent):
|
||||
method: Any
|
||||
|
||||
class Child10(Parent):
|
||||
# fine: although the local-narrowed type is `None`,
|
||||
# the type as accessed on the class object or on instances of the class
|
||||
# will be `Any`, which is assignable to the declared type on the superclass
|
||||
method: Any = None
|
||||
```
|
||||
|
|
|
|||
|
|
@ -2668,7 +2668,7 @@ violates the Liskov principle (this also matches the behaviour of other type che
|
|||
from typing import Iterable
|
||||
|
||||
class Foo(Iterable[int]):
|
||||
__iter__ = None
|
||||
__iter__ = None # error: [invalid-method-override]
|
||||
|
||||
static_assert(is_subtype_of(Foo, Iterable[int]))
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,270 @@
|
|||
---
|
||||
source: crates/ty_test/src/lib.rs
|
||||
expression: snapshot
|
||||
---
|
||||
---
|
||||
mdtest name: liskov.md - The Liskov Substitution Principle - Method incompatibly overridden by a non-method
|
||||
mdtest path: crates/ty_python_semantic/resources/mdtest/liskov.md
|
||||
---
|
||||
|
||||
# Python source files
|
||||
|
||||
## mdtest_snippet.pyi
|
||||
|
||||
```
|
||||
1 | from typing import ClassVar, Final, Callable, Any
|
||||
2 |
|
||||
3 | class Parent:
|
||||
4 | def method(self): ...
|
||||
5 |
|
||||
6 | class Child1(Parent):
|
||||
7 | method = None # error: [invalid-method-override]
|
||||
8 |
|
||||
9 | class Child2(Parent):
|
||||
10 | method: ClassVar = None # error: [invalid-method-override]
|
||||
11 |
|
||||
12 | class Child3(Parent):
|
||||
13 | method: None = None # error: [invalid-method-override]
|
||||
14 |
|
||||
15 | class Child4(Parent):
|
||||
16 | # error: [invalid-assignment] "Object of type `None` is not assignable to `(...) -> Unknown`"
|
||||
17 | # error: [invalid-method-override]
|
||||
18 | method: Callable = None
|
||||
19 |
|
||||
20 | class Child5(Parent):
|
||||
21 | method: Callable | None = None # error: [invalid-method-override]
|
||||
22 |
|
||||
23 | class Child6(Parent):
|
||||
24 | method: Final = None # error: [invalid-method-override]
|
||||
25 |
|
||||
26 | class Child7(Parent):
|
||||
27 | method: None # error: [invalid-method-override]
|
||||
28 |
|
||||
29 | class Child8(Parent):
|
||||
30 | # A `Callable` type is insufficient for the type to be assignable to
|
||||
31 | # the superclass definition: the definition on the superclass is a
|
||||
32 | # *function-like* callable, which can be used as a method descriptor,
|
||||
33 | # and which has `types.FunctionType` attributes such as `__name__`,
|
||||
34 | # `__kwdefaults__`, etc.
|
||||
35 | method: Callable # error: [invalid-method-override]
|
||||
36 |
|
||||
37 | class Child9(Parent):
|
||||
38 | method: Any
|
||||
39 |
|
||||
40 | class Child10(Parent):
|
||||
41 | # fine: although the local-narrowed type is `None`,
|
||||
42 | # the type as accessed on the class object or on instances of the class
|
||||
43 | # will be `Any`, which is assignable to the declared type on the superclass
|
||||
44 | method: Any = None
|
||||
```
|
||||
|
||||
# Diagnostics
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/mdtest_snippet.pyi:4:9
|
||||
|
|
||||
3 | class Parent:
|
||||
4 | def method(self): ...
|
||||
| ------------ `Parent.method` defined here
|
||||
5 |
|
||||
6 | class Child1(Parent):
|
||||
7 | method = None # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^ Definition is incompatible with `Parent.method`
|
||||
8 |
|
||||
9 | class Child2(Parent):
|
||||
|
|
||||
info: `Child1.method` has type `None`, which is not callable
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/mdtest_snippet.pyi:10:5
|
||||
|
|
||||
9 | class Child2(Parent):
|
||||
10 | method: ClassVar = None # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Parent.method`
|
||||
11 |
|
||||
12 | class Child3(Parent):
|
||||
|
|
||||
::: src/mdtest_snippet.pyi:4:9
|
||||
|
|
||||
3 | class Parent:
|
||||
4 | def method(self): ...
|
||||
| ------------ `Parent.method` defined here
|
||||
5 |
|
||||
6 | class Child1(Parent):
|
||||
|
|
||||
info: `Child2.method` has type `Unknown | None`, which is not callable
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/mdtest_snippet.pyi:13:5
|
||||
|
|
||||
12 | class Child3(Parent):
|
||||
13 | method: None = None # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Parent.method`
|
||||
14 |
|
||||
15 | class Child4(Parent):
|
||||
|
|
||||
::: src/mdtest_snippet.pyi:4:9
|
||||
|
|
||||
3 | class Parent:
|
||||
4 | def method(self): ...
|
||||
| ------------ `Parent.method` defined here
|
||||
5 |
|
||||
6 | class Child1(Parent):
|
||||
|
|
||||
info: `Child3.method` has type `None`, which is not callable
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/mdtest_snippet.pyi:18:5
|
||||
|
|
||||
16 | # error: [invalid-assignment] "Object of type `None` is not assignable to `(...) -> Unknown`"
|
||||
17 | # error: [invalid-method-override]
|
||||
18 | method: Callable = None
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Parent.method`
|
||||
19 |
|
||||
20 | class Child5(Parent):
|
||||
|
|
||||
::: src/mdtest_snippet.pyi:4:9
|
||||
|
|
||||
3 | class Parent:
|
||||
4 | def method(self): ...
|
||||
| ------------ `Parent.method` defined here
|
||||
5 |
|
||||
6 | class Child1(Parent):
|
||||
|
|
||||
info: `Child4.method` has type `(...) -> Unknown`
|
||||
info: `(...) -> Unknown` is a callable type with the correct signature, but objects with this type cannot necessarily be used as method descriptors
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-assignment]: Object of type `None` is not assignable to `(...) -> Unknown`
|
||||
--> src/mdtest_snippet.pyi:18:13
|
||||
|
|
||||
16 | # error: [invalid-assignment] "Object of type `None` is not assignable to `(...) -> Unknown`"
|
||||
17 | # error: [invalid-method-override]
|
||||
18 | method: Callable = None
|
||||
| -------- ^^^^ Incompatible value of type `None`
|
||||
| |
|
||||
| Declared type
|
||||
19 |
|
||||
20 | class Child5(Parent):
|
||||
|
|
||||
info: rule `invalid-assignment` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/mdtest_snippet.pyi:21:5
|
||||
|
|
||||
20 | class Child5(Parent):
|
||||
21 | method: Callable | None = None # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Parent.method`
|
||||
22 |
|
||||
23 | class Child6(Parent):
|
||||
|
|
||||
::: src/mdtest_snippet.pyi:4:9
|
||||
|
|
||||
3 | class Parent:
|
||||
4 | def method(self): ...
|
||||
| ------------ `Parent.method` defined here
|
||||
5 |
|
||||
6 | class Child1(Parent):
|
||||
|
|
||||
info: `Child5.method` has type `((...) -> Unknown) | None`, which is not callable
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/mdtest_snippet.pyi:24:5
|
||||
|
|
||||
23 | class Child6(Parent):
|
||||
24 | method: Final = None # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Parent.method`
|
||||
25 |
|
||||
26 | class Child7(Parent):
|
||||
|
|
||||
::: src/mdtest_snippet.pyi:4:9
|
||||
|
|
||||
3 | class Parent:
|
||||
4 | def method(self): ...
|
||||
| ------------ `Parent.method` defined here
|
||||
5 |
|
||||
6 | class Child1(Parent):
|
||||
|
|
||||
info: `Child6.method` has type `None`, which is not callable
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/mdtest_snippet.pyi:27:5
|
||||
|
|
||||
26 | class Child7(Parent):
|
||||
27 | method: None # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^ Definition is incompatible with `Parent.method`
|
||||
28 |
|
||||
29 | class Child8(Parent):
|
||||
|
|
||||
::: src/mdtest_snippet.pyi:4:9
|
||||
|
|
||||
3 | class Parent:
|
||||
4 | def method(self): ...
|
||||
| ------------ `Parent.method` defined here
|
||||
5 |
|
||||
6 | class Child1(Parent):
|
||||
|
|
||||
info: `Child7.method` has type `None`, which is not callable
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/mdtest_snippet.pyi:35:5
|
||||
|
|
||||
33 | # and which has `types.FunctionType` attributes such as `__name__`,
|
||||
34 | # `__kwdefaults__`, etc.
|
||||
35 | method: Callable # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^ Definition is incompatible with `Parent.method`
|
||||
36 |
|
||||
37 | class Child9(Parent):
|
||||
|
|
||||
::: src/mdtest_snippet.pyi:4:9
|
||||
|
|
||||
3 | class Parent:
|
||||
4 | def method(self): ...
|
||||
| ------------ `Parent.method` defined here
|
||||
5 |
|
||||
6 | class Child1(Parent):
|
||||
|
|
||||
info: `Child8.method` has type `(...) -> Unknown`
|
||||
info: `(...) -> Unknown` is a callable type with the correct signature, but objects with this type cannot necessarily be used as method descriptors
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
|
@ -37,7 +37,7 @@ reveal_type(().__class__()) # revealed: tuple[()]
|
|||
reveal_type((1, 2).__class__((1, 2))) # revealed: tuple[Literal[1], Literal[2]]
|
||||
|
||||
class LiskovUncompliantIterable(Iterable[int]):
|
||||
# TODO we should emit an error here about the Liskov violation
|
||||
# error: [invalid-method-override]
|
||||
__iter__ = None
|
||||
|
||||
def f(x: Iterable[int], y: list[str], z: Never, aa: list[Never], bb: LiskovUncompliantIterable):
|
||||
|
|
|
|||
|
|
@ -737,6 +737,13 @@ impl DefinitionKind<'_> {
|
|||
matches!(self, DefinitionKind::Assignment(_))
|
||||
}
|
||||
|
||||
pub(crate) const fn as_function_def(&self) -> Option<&AstNodeRef<ast::StmtFunctionDef>> {
|
||||
match self {
|
||||
DefinitionKind::Function(function) => Some(function),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) const fn is_function_def(&self) -> bool {
|
||||
matches!(self, DefinitionKind::Function(_))
|
||||
}
|
||||
|
|
|
|||
|
|
@ -27,7 +27,7 @@ use crate::types::{
|
|||
ProtocolInstanceType, SpecialFormType, SubclassOfInner, Type, TypeContext, binding_type,
|
||||
infer_isolated_expression, protocol_class::ProtocolClass,
|
||||
};
|
||||
use crate::types::{KnownInstanceType, MemberLookupPolicy};
|
||||
use crate::types::{CallableType, KnownInstanceType, MemberLookupPolicy};
|
||||
use crate::{Db, DisplaySettings, FxIndexMap, Module, ModuleName, Program, declare_lint};
|
||||
use itertools::Itertools;
|
||||
use ruff_db::{
|
||||
|
|
@ -3464,14 +3464,14 @@ pub(super) fn report_invalid_method_override<'db>(
|
|||
member: &str,
|
||||
subclass: ClassType<'db>,
|
||||
subclass_definition: Definition<'db>,
|
||||
subclass_function: FunctionType<'db>,
|
||||
subclass_type: Type<'db>,
|
||||
superclass: ClassType<'db>,
|
||||
superclass_type: Type<'db>,
|
||||
superclass_method_kind: MethodKind,
|
||||
) {
|
||||
let db = context.db();
|
||||
|
||||
let signature_span = |function: FunctionType<'db>| {
|
||||
let signature_span_inner = |function: FunctionType<'db>| {
|
||||
function
|
||||
.literal(db)
|
||||
.last_definition(db)
|
||||
|
|
@ -3479,23 +3479,31 @@ pub(super) fn report_invalid_method_override<'db>(
|
|||
.map(|spans| spans.signature)
|
||||
};
|
||||
|
||||
let signature_span = |ty| match ty {
|
||||
Type::FunctionLiteral(function) => signature_span_inner(function),
|
||||
Type::BoundMethod(method) => signature_span_inner(method.function(db)),
|
||||
_ => None,
|
||||
};
|
||||
|
||||
let subclass_definition_kind = subclass_definition.kind(db);
|
||||
let subclass_definition_signature_span = signature_span(subclass_function);
|
||||
let subclass_definition_signature_span = signature_span(subclass_type);
|
||||
|
||||
// If the function was originally defined elsewhere and simply assigned
|
||||
// in the body of the class here, we cannot use the range associated with the `FunctionType`
|
||||
let diagnostic_range = if subclass_definition_kind.is_function_def() {
|
||||
subclass_definition_signature_span
|
||||
.as_ref()
|
||||
.and_then(Span::range)
|
||||
.unwrap_or_else(|| {
|
||||
subclass_function
|
||||
.node(db, context.file(), context.module())
|
||||
.range
|
||||
})
|
||||
} else {
|
||||
subclass_definition.full_range(db, context.module()).range()
|
||||
};
|
||||
let diagnostic_range = subclass_definition_kind
|
||||
.as_function_def()
|
||||
.map(|node| {
|
||||
let function = node.node(context.module());
|
||||
TextRange::new(
|
||||
function.name.start(),
|
||||
function
|
||||
.returns
|
||||
.as_deref()
|
||||
.map(Ranged::end)
|
||||
.unwrap_or(function.parameters.end()),
|
||||
)
|
||||
})
|
||||
.unwrap_or_else(|| subclass_definition.full_range(db, context.module()).range());
|
||||
|
||||
let class_name = subclass.name(db);
|
||||
let superclass_name = superclass.name(db);
|
||||
|
|
@ -3539,6 +3547,33 @@ pub(super) fn report_invalid_method_override<'db>(
|
|||
superclass_function_kind = superclass_function_kind.description(),
|
||||
subclass_function_kind = subclass_function_kind.description(),
|
||||
));
|
||||
} else if !matches!(
|
||||
subclass_type,
|
||||
Type::FunctionLiteral(_) | Type::BoundMethod(_)
|
||||
) {
|
||||
if let Some(callables) = subclass_type.try_upcast_to_callable(db) {
|
||||
diagnostic.info(format_args!(
|
||||
"`{class_name}.{member}` has type `{}`",
|
||||
subclass_type.display(db)
|
||||
));
|
||||
if let Some(callable) = callables.exactly_one()
|
||||
&& !callable.is_function_like(db)
|
||||
&& let Some(superclass_callable) = superclass_type.try_upcast_to_callable(db)
|
||||
&& Type::Callable(CallableType::new(db, callable.signatures(db), true))
|
||||
.is_assignable_to(db, superclass_callable.into_type(db))
|
||||
{
|
||||
diagnostic.info(format_args!(
|
||||
"`{}` is a callable type with the correct signature, \
|
||||
but objects with this type cannot necessarily be used as method descriptors",
|
||||
subclass_type.display(db)
|
||||
));
|
||||
}
|
||||
} else {
|
||||
diagnostic.info(format_args!(
|
||||
"`{class_name}.{member}` has type `{}`, which is not callable",
|
||||
subclass_type.display(db)
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
diagnostic.info("This violates the Liskov Substitution Principle");
|
||||
|
|
@ -3568,8 +3603,8 @@ pub(super) fn report_invalid_method_override<'db>(
|
|||
);
|
||||
|
||||
let superclass_function_span = match superclass_type {
|
||||
Type::FunctionLiteral(function) => signature_span(function),
|
||||
Type::BoundMethod(method) => signature_span(method.function(db)),
|
||||
Type::FunctionLiteral(function) => signature_span_inner(function),
|
||||
Type::BoundMethod(method) => signature_span_inner(method.function(db)),
|
||||
_ => None,
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@ use crate::{
|
|||
place::Place,
|
||||
semantic_index::place_table,
|
||||
types::{
|
||||
ClassBase, ClassLiteral, ClassType, KnownClass, Type,
|
||||
ClassBase, ClassLiteral, ClassType, Type,
|
||||
class::CodeGeneratorKind,
|
||||
context::InferContext,
|
||||
diagnostic::report_invalid_method_override,
|
||||
|
|
@ -18,10 +18,6 @@ use crate::{
|
|||
|
||||
pub(super) fn check_class<'db>(context: &InferContext<'db, '_>, class: ClassLiteral<'db>) {
|
||||
let db = context.db();
|
||||
if class.is_known(db, KnownClass::Object) {
|
||||
return;
|
||||
}
|
||||
|
||||
let class_specialized = class.identity_specialization(db);
|
||||
let own_class_members: FxHashSet<_> =
|
||||
all_declarations_and_bindings(db, class.body_scope(db)).collect();
|
||||
|
|
@ -40,11 +36,6 @@ fn check_class_declaration<'db>(
|
|||
|
||||
let MemberWithDefinition { member, definition } = member;
|
||||
|
||||
// TODO: Check Liskov on non-methods too
|
||||
let Type::FunctionLiteral(function) = member.ty else {
|
||||
return;
|
||||
};
|
||||
|
||||
let Some(definition) = definition else {
|
||||
return;
|
||||
};
|
||||
|
|
@ -121,14 +112,17 @@ fn check_class_declaration<'db>(
|
|||
break;
|
||||
};
|
||||
|
||||
let Some(superclass_type_as_callable) = superclass_type
|
||||
.try_upcast_to_callable(db)
|
||||
.map(|callables| callables.into_type(db))
|
||||
else {
|
||||
continue;
|
||||
let superclass_type_as_callable = match superclass_type {
|
||||
Type::FunctionLiteral(function) => function.into_callable_type(db),
|
||||
Type::BoundMethod(method) => method.into_callable_type(db),
|
||||
Type::Callable(callable) => callable,
|
||||
// TODO: check subclass overrides even if the definition on the superclass is not a method
|
||||
_ => continue,
|
||||
};
|
||||
|
||||
if type_on_subclass_instance.is_assignable_to(db, superclass_type_as_callable) {
|
||||
if type_on_subclass_instance
|
||||
.is_assignable_to(db, Type::Callable(superclass_type_as_callable))
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
|
|
@ -137,7 +131,7 @@ fn check_class_declaration<'db>(
|
|||
&member.name,
|
||||
class,
|
||||
*definition,
|
||||
function,
|
||||
type_on_subclass_instance,
|
||||
superclass,
|
||||
superclass_type,
|
||||
method_kind,
|
||||
|
|
|
|||
Loading…
Reference in New Issue