Skip to content

Commit 877fcd2

Browse files
committed
disambiguate class names where necessary
1 parent 5c4284c commit 877fcd2

File tree

5 files changed

+130
-31
lines changed

5 files changed

+130
-31
lines changed

crates/ty_python_semantic/resources/mdtest/liskov.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,26 @@ class D(C):
183183
def get(self, my_default): ... # error: [invalid-method-override]
184184
```
185185

186+
## Fully qualified names are used in diagnostics where appropriate
187+
188+
<!-- snapshot-diagnostics -->
189+
190+
`a.pyi`:
191+
192+
```pyi
193+
class A:
194+
def foo(self, x): ...
195+
```
196+
197+
`b.pyi`:
198+
199+
```pyi
200+
import a
201+
202+
class A(a.A):
203+
def foo(self, y): ... # error: [invalid-method-override]
204+
```
205+
186206
## Excluded methods
187207

188208
Certain special constructor methods are excluded from Liskov checks. None of the following classes
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
---
2+
source: crates/ty_test/src/lib.rs
3+
expression: snapshot
4+
---
5+
---
6+
mdtest name: liskov.md - The Liskov Substitution Principle - Fully qualified names are used in diagnostics where appropriate
7+
mdtest path: crates/ty_python_semantic/resources/mdtest/liskov.md
8+
---
9+
10+
# Python source files
11+
12+
## a.pyi
13+
14+
```
15+
1 | class A:
16+
2 | def foo(self, x): ...
17+
```
18+
19+
## b.pyi
20+
21+
```
22+
1 | import a
23+
2 |
24+
3 | class A(a.A):
25+
4 | def foo(self, y): ... # error: [invalid-method-override]
26+
```
27+
28+
# Diagnostics
29+
30+
```
31+
error[invalid-method-override]: Invalid override of method `foo`
32+
--> src/b.pyi:4:9
33+
|
34+
3 | class A(a.A):
35+
4 | def foo(self, y): ... # error: [invalid-method-override]
36+
| ^^^^^^^^^^^^ Definition is incompatible with `a.A.foo`
37+
|
38+
::: src/a.pyi:2:9
39+
|
40+
1 | class A:
41+
2 | def foo(self, x): ...
42+
| ------------ `a.A.foo` defined here
43+
|
44+
info: This violates the Liskov Substitution Principle
45+
info: rule `invalid-method-override` is enabled by default
46+
47+
```

crates/ty_python_semantic/src/types/diagnostic.rs

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ use crate::types::KnownInstanceType;
1616
use crate::types::call::CallError;
1717
use crate::types::class::{DisjointBase, DisjointBaseKind, Field};
1818
use crate::types::function::KnownFunction;
19+
use crate::types::ide_support::Member;
1920
use crate::types::string_annotation::{
2021
BYTE_STRING_TYPE_ANNOTATION, ESCAPE_CHARACTER_IN_FORWARD_ANNOTATION, FSTRING_TYPE_ANNOTATION,
2122
IMPLICIT_CONCATENATED_STRING_TYPE_ANNOTATION, INVALID_SYNTAX_IN_FORWARD_ANNOTATION,
@@ -3273,6 +3274,54 @@ pub(crate) fn report_rebound_typevar<'db>(
32733274
}
32743275
}
32753276

3277+
pub(super) fn report_invalid_method_override<'db>(
3278+
context: &InferContext<'db, '_>,
3279+
range: impl Ranged,
3280+
member: &Member<'db>,
3281+
class: ClassType<'db>,
3282+
supercls: ClassType<'db>,
3283+
type_on_supercls: Type<'db>,
3284+
) {
3285+
let Some(builder) = context.report_lint(&INVALID_METHOD_OVERRIDE, range) else {
3286+
return;
3287+
};
3288+
3289+
let db = context.db();
3290+
3291+
let mut diagnostic =
3292+
builder.into_diagnostic(format_args!("Invalid override of method `{}`", member.name));
3293+
3294+
let supercls_name = supercls.name(db);
3295+
3296+
let overridden_method = if class.name(db) != supercls_name {
3297+
format!("{}.{}", supercls_name, member.name)
3298+
} else {
3299+
let mut buffer = supercls.class_literal(db).0.qualified_name(db);
3300+
buffer.push('.');
3301+
buffer.push_str(&member.name);
3302+
buffer
3303+
};
3304+
3305+
diagnostic.set_primary_message(format_args!(
3306+
"Definition is incompatible with `{overridden_method}`"
3307+
));
3308+
3309+
diagnostic.info("This violates the Liskov Substitution Principle");
3310+
3311+
if let Type::BoundMethod(method_on_supercls) = type_on_supercls
3312+
&& let Some(spans) = method_on_supercls
3313+
.function(db)
3314+
.literal(db)
3315+
.last_definition(db)
3316+
.spans(db)
3317+
{
3318+
diagnostic.annotate(
3319+
Annotation::secondary(spans.signature)
3320+
.message(format_args!("`{overridden_method}` defined here")),
3321+
);
3322+
}
3323+
}
3324+
32763325
/// This function receives an unresolved `from foo import bar` import,
32773326
/// where `foo` can be resolved to a module but that module does not
32783327
/// have a `bar` member or submodule.

crates/ty_python_semantic/src/types/display.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,16 @@ impl<'db> ClassLiteral<'db> {
321321
name_parts.reverse();
322322
name_parts
323323
}
324+
325+
pub(super) fn qualified_name(self, db: &'db dyn Db) -> String {
326+
let mut qualified_name = String::new();
327+
for parent in self.qualified_name_components(db) {
328+
qualified_name.push_str(&parent);
329+
qualified_name.push('.');
330+
}
331+
qualified_name.push_str(self.name(db));
332+
qualified_name
333+
}
324334
}
325335

326336
struct ClassDisplay<'db> {

crates/ty_python_semantic/src/types/infer/builder/liskov.rs

Lines changed: 4 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
use ruff_db::diagnostic::Annotation;
2-
31
use ruff_text_size::Ranged;
42
use rustc_hash::FxHashSet;
53

@@ -10,7 +8,7 @@ use crate::{
108
ClassBase, ClassLiteral, ClassType, KnownClass, Type,
119
class::CodeGeneratorKind,
1210
context::InferContext,
13-
diagnostic::INVALID_METHOD_OVERRIDE,
11+
diagnostic::report_invalid_method_override,
1412
ide_support::{MemberWithDefinition, all_declarations_and_bindings},
1513
},
1614
};
@@ -114,37 +112,12 @@ fn check_class_declaration<'db>(
114112
definition.focus_range(db, context.module()).range()
115113
};
116114

117-
let Some(builder) = context.report_lint(&INVALID_METHOD_OVERRIDE, range) else {
118-
continue;
119-
};
120-
121-
let mut diagnostic =
122-
builder.into_diagnostic(format_args!("Invalid override of method `{}`", member.name));
123-
124-
diagnostic.set_primary_message(format_args!(
125-
"Definition is incompatible with `{}.{}`",
126-
supercls.name(db),
127-
member.name
128-
));
129-
130-
diagnostic.info("This violates the Liskov Substitution Principle");
131-
132-
if let Type::BoundMethod(method_on_supercls) = type_on_supercls
133-
&& let Some(spans) = method_on_supercls
134-
.function(db)
135-
.literal(db)
136-
.last_definition(db)
137-
.spans(db)
138-
{
139-
diagnostic.annotate(Annotation::secondary(spans.signature).message(format_args!(
140-
"`{}.{}` defined here",
141-
supercls.name(db),
142-
member.name
143-
)));
144-
}
115+
report_invalid_method_override(context, range, member, class, supercls, type_on_supercls);
145116

146117
// Only one diagnostic should be emitted per each invalid override,
147118
// even if it overrides multiple superclasses incorrectly!
119+
// It's possible `report_invalid_method_override` didn't emit a diagnostic because there's a
120+
// suppression comment, but that too should cause us to exit early here.
148121
break;
149122
}
150123
}

0 commit comments

Comments
 (0)