[ty] Check method definitions on subclasses for Liskov violations (#21436)
This commit is contained in:
@@ -737,6 +737,10 @@ impl DefinitionKind<'_> {
|
||||
matches!(self, DefinitionKind::Assignment(_))
|
||||
}
|
||||
|
||||
pub(crate) const fn is_function_def(&self) -> bool {
|
||||
matches!(self, DefinitionKind::Function(_))
|
||||
}
|
||||
|
||||
/// Returns the [`TextRange`] of the definition target.
|
||||
///
|
||||
/// A definition target would mainly be the node representing the place being defined i.e.,
|
||||
|
||||
@@ -95,6 +95,7 @@ mod generics;
|
||||
pub mod ide_support;
|
||||
mod infer;
|
||||
mod instance;
|
||||
mod liskov;
|
||||
mod member;
|
||||
mod mro;
|
||||
mod narrow;
|
||||
@@ -1104,6 +1105,13 @@ impl<'db> Type<'db> {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) const fn as_bound_method(self) -> Option<BoundMethodType<'db>> {
|
||||
match self {
|
||||
Type::BoundMethod(bound_method) => Some(bound_method),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
#[track_caller]
|
||||
pub(crate) const fn expect_class_literal(self) -> ClassLiteral<'db> {
|
||||
self.as_class_literal()
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use std::cell::RefCell;
|
||||
use std::fmt::Write;
|
||||
use std::sync::{LazyLock, Mutex};
|
||||
|
||||
use super::TypeVarVariance;
|
||||
@@ -10,7 +11,7 @@ use super::{
|
||||
use crate::module_resolver::KnownModule;
|
||||
use crate::place::TypeOrigin;
|
||||
use crate::semantic_index::definition::{Definition, DefinitionState};
|
||||
use crate::semantic_index::scope::{NodeWithScopeKind, Scope};
|
||||
use crate::semantic_index::scope::{NodeWithScopeKind, Scope, ScopeKind};
|
||||
use crate::semantic_index::symbol::Symbol;
|
||||
use crate::semantic_index::{
|
||||
DeclarationWithConstraint, SemanticIndex, attribute_declarations, attribute_scopes,
|
||||
@@ -3649,6 +3650,10 @@ impl<'db> ClassLiteral<'db> {
|
||||
.unwrap_or_else(|| class_name.end()),
|
||||
)
|
||||
}
|
||||
|
||||
pub(super) fn qualified_name(self, db: &'db dyn Db) -> QualifiedClassName<'db> {
|
||||
QualifiedClassName { db, class: self }
|
||||
}
|
||||
}
|
||||
|
||||
impl<'db> From<ClassLiteral<'db>> for Type<'db> {
|
||||
@@ -3783,6 +3788,74 @@ impl<'db> VarianceInferable<'db> for ClassLiteral<'db> {
|
||||
}
|
||||
}
|
||||
|
||||
// N.B. It would be incorrect to derive `Eq`, `PartialEq`, or `Hash` for this struct,
|
||||
// because two `QualifiedClassName` instances might refer to different classes but
|
||||
// have the same components. You'd expect them to compare equal, but they'd compare
|
||||
// unequal if `PartialEq`/`Eq` were naively derived.
|
||||
#[derive(Clone, Copy)]
|
||||
pub(super) struct QualifiedClassName<'db> {
|
||||
db: &'db dyn Db,
|
||||
class: ClassLiteral<'db>,
|
||||
}
|
||||
|
||||
impl QualifiedClassName<'_> {
|
||||
/// Returns the components of the qualified name of this class, excluding this class itself.
|
||||
///
|
||||
/// For example, calling this method on a class `C` in the module `a.b` would return
|
||||
/// `["a", "b"]`. Calling this method on a class `D` inside the namespace of a method
|
||||
/// `m` inside the namespace of a class `C` in the module `a.b` would return
|
||||
/// `["a", "b", "C", "<locals of function 'm'>"]`.
|
||||
pub(super) fn components_excluding_self(&self) -> Vec<String> {
|
||||
let body_scope = self.class.body_scope(self.db);
|
||||
let file = body_scope.file(self.db);
|
||||
let module_ast = parsed_module(self.db, file).load(self.db);
|
||||
let index = semantic_index(self.db, file);
|
||||
let file_scope_id = body_scope.file_scope_id(self.db);
|
||||
|
||||
let mut name_parts = vec![];
|
||||
|
||||
// Skips itself
|
||||
for (_, ancestor_scope) in index.ancestor_scopes(file_scope_id).skip(1) {
|
||||
let node = ancestor_scope.node();
|
||||
|
||||
match ancestor_scope.kind() {
|
||||
ScopeKind::Class => {
|
||||
if let Some(class_def) = node.as_class() {
|
||||
name_parts.push(class_def.node(&module_ast).name.as_str().to_string());
|
||||
}
|
||||
}
|
||||
ScopeKind::Function => {
|
||||
if let Some(function_def) = node.as_function() {
|
||||
name_parts.push(format!(
|
||||
"<locals of function '{}'>",
|
||||
function_def.node(&module_ast).name.as_str()
|
||||
));
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(module) = file_to_module(self.db, file) {
|
||||
let module_name = module.name(self.db);
|
||||
name_parts.push(module_name.as_str().to_string());
|
||||
}
|
||||
|
||||
name_parts.reverse();
|
||||
name_parts
|
||||
}
|
||||
}
|
||||
|
||||
impl std::fmt::Display for QualifiedClassName<'_> {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
for parent in self.components_excluding_self() {
|
||||
f.write_str(&parent)?;
|
||||
f.write_char('.')?;
|
||||
}
|
||||
f.write_str(self.class.name(self.db))
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, get_size2::GetSize)]
|
||||
pub(super) enum InheritanceCycle {
|
||||
/// The class is cyclically defined and is a participant in the cycle.
|
||||
|
||||
@@ -10,12 +10,13 @@ use crate::diagnostic::format_enumeration;
|
||||
use crate::lint::{Level, LintRegistryBuilder, LintStatus};
|
||||
use crate::semantic_index::definition::{Definition, DefinitionKind};
|
||||
use crate::semantic_index::place::{PlaceTable, ScopedPlaceId};
|
||||
use crate::semantic_index::{global_scope, place_table};
|
||||
use crate::semantic_index::{global_scope, place_table, use_def_map};
|
||||
use crate::suppression::FileSuppressionId;
|
||||
use crate::types::KnownInstanceType;
|
||||
use crate::types::call::CallError;
|
||||
use crate::types::class::{DisjointBase, DisjointBaseKind, Field};
|
||||
use crate::types::function::KnownFunction;
|
||||
use crate::types::function::{FunctionType, KnownFunction};
|
||||
use crate::types::liskov::{MethodKind, SynthesizedMethodKind};
|
||||
use crate::types::string_annotation::{
|
||||
BYTE_STRING_TYPE_ANNOTATION, ESCAPE_CHARACTER_IN_FORWARD_ANNOTATION, FSTRING_TYPE_ANNOTATION,
|
||||
IMPLICIT_CONCATENATED_STRING_TYPE_ANNOTATION, INVALID_SYNTAX_IN_FORWARD_ANNOTATION,
|
||||
@@ -28,15 +29,18 @@ use crate::types::{
|
||||
};
|
||||
use crate::{Db, DisplaySettings, FxIndexMap, Module, ModuleName, Program, declare_lint};
|
||||
use itertools::Itertools;
|
||||
use ruff_db::diagnostic::{Annotation, Diagnostic, Span, SubDiagnostic, SubDiagnosticSeverity};
|
||||
use ruff_db::source::source_text;
|
||||
use ruff_db::{
|
||||
diagnostic::{Annotation, Diagnostic, Span, SubDiagnostic, SubDiagnosticSeverity},
|
||||
parsed::parsed_module,
|
||||
source::source_text,
|
||||
};
|
||||
use ruff_python_ast::name::Name;
|
||||
use ruff_python_ast::parenthesize::parentheses_iterator;
|
||||
use ruff_python_ast::{self as ast, AnyNodeRef};
|
||||
use ruff_python_trivia::CommentRanges;
|
||||
use ruff_text_size::{Ranged, TextRange};
|
||||
use rustc_hash::FxHashSet;
|
||||
use std::fmt::Formatter;
|
||||
use std::fmt::{self, Formatter};
|
||||
|
||||
/// Registers all known type check lints.
|
||||
pub(crate) fn register_lints(registry: &mut LintRegistryBuilder) {
|
||||
@@ -108,6 +112,7 @@ pub(crate) fn register_lints(registry: &mut LintRegistryBuilder) {
|
||||
registry.register_lint(&REDUNDANT_CAST);
|
||||
registry.register_lint(&UNRESOLVED_GLOBAL);
|
||||
registry.register_lint(&MISSING_TYPED_DICT_KEY);
|
||||
registry.register_lint(&INVALID_METHOD_OVERRIDE);
|
||||
|
||||
// String annotations
|
||||
registry.register_lint(&BYTE_STRING_TYPE_ANNOTATION);
|
||||
@@ -1934,6 +1939,84 @@ declare_lint! {
|
||||
}
|
||||
}
|
||||
|
||||
declare_lint! {
|
||||
/// ## What it does
|
||||
/// Detects method overrides that violate the [Liskov Substitution Principle] ("LSP").
|
||||
///
|
||||
/// The LSP states that an instance of a subtype should be substitutable for an instance of its supertype.
|
||||
/// Applied to Python, this means:
|
||||
/// 1. All argument combinations a superclass method accepts
|
||||
/// must also be accepted by an overriding subclass method.
|
||||
/// 2. The return type of an overriding subclass method must be a subtype
|
||||
/// of the return type of the superclass method.
|
||||
///
|
||||
/// ## Why is this bad?
|
||||
/// Violating the Liskov Substitution Principle will lead to many of ty's assumptions and
|
||||
/// inferences being incorrect, which will mean that it will fail to catch many possible
|
||||
/// type errors in your code.
|
||||
///
|
||||
/// ## Example
|
||||
/// ```python
|
||||
/// class Super:
|
||||
/// def method(self, x) -> int:
|
||||
/// return 42
|
||||
///
|
||||
/// class Sub(Super):
|
||||
/// # Liskov violation: `str` is not a subtype of `int`,
|
||||
/// # but the supertype method promises to return an `int`.
|
||||
/// def method(self, x) -> str: # error: [invalid-override]
|
||||
/// return "foo"
|
||||
///
|
||||
/// def accepts_super(s: Super) -> int:
|
||||
/// return s.method(x=42)
|
||||
///
|
||||
/// accepts_super(Sub()) # The result of this call is a string, but ty will infer
|
||||
/// # it to be an `int` due to the violation of the Liskov Substitution Principle.
|
||||
///
|
||||
/// class Sub2(Super):
|
||||
/// # Liskov violation: the superclass method can be called with a `x=`
|
||||
/// # keyword argument, but the subclass method does not accept it.
|
||||
/// def method(self, y) -> int: # error: [invalid-override]
|
||||
/// return 42
|
||||
///
|
||||
/// accepts_super(Sub2()) # TypeError at runtime: method() got an unexpected keyword argument 'x'
|
||||
/// # ty cannot catch this error due to the violation of the Liskov Substitution Principle.
|
||||
/// ```
|
||||
///
|
||||
/// ## Common issues
|
||||
///
|
||||
/// ### Why does ty complain about my `__eq__` method?
|
||||
///
|
||||
/// `__eq__` and `__ne__` methods in Python are generally expected to accept arbitrary
|
||||
/// objects as their second argument, for example:
|
||||
///
|
||||
/// ```python
|
||||
/// class A:
|
||||
/// x: int
|
||||
///
|
||||
/// def __eq__(self, other: object) -> bool:
|
||||
/// # gracefully handle an object of an unexpected type
|
||||
/// # without raising an exception
|
||||
/// if not isinstance(other, A):
|
||||
/// return False
|
||||
/// return self.x == other.x
|
||||
/// ```
|
||||
///
|
||||
/// If `A.__eq__` here were annotated as only accepting `A` instances for its second argument,
|
||||
/// it would imply that you wouldn't be able to use `==` between instances of `A` and
|
||||
/// instances of unrelated classes without an exception possibly being raised. While some
|
||||
/// classes in Python do indeed behave this way, the strongly held convention is that it should
|
||||
/// be avoided wherever possible. As part of this check, therefore, ty enforces that `__eq__`
|
||||
/// and `__ne__` methods accept `object` as their second argument.
|
||||
///
|
||||
/// [Liskov Substitution Principle]: https://en.wikipedia.org/wiki/Liskov_substitution_principle
|
||||
pub(crate) static INVALID_METHOD_OVERRIDE = {
|
||||
summary: "detects method definitions that violate the Liskov Substitution Principle",
|
||||
status: LintStatus::stable("0.0.1-alpha.20"),
|
||||
default_level: Level::Error,
|
||||
}
|
||||
}
|
||||
|
||||
/// A collection of type check diagnostics.
|
||||
#[derive(Default, Eq, PartialEq, get_size2::GetSize)]
|
||||
pub struct TypeCheckDiagnostics {
|
||||
@@ -3372,6 +3455,173 @@ pub(crate) fn report_rebound_typevar<'db>(
|
||||
}
|
||||
}
|
||||
|
||||
// I tried refactoring this function to placate Clippy,
|
||||
// but it did not improve readability! -- AW.
|
||||
#[expect(clippy::too_many_arguments)]
|
||||
pub(super) fn report_invalid_method_override<'db>(
|
||||
context: &InferContext<'db, '_>,
|
||||
member: &str,
|
||||
subclass: ClassType<'db>,
|
||||
subclass_definition: Definition<'db>,
|
||||
subclass_function: FunctionType<'db>,
|
||||
superclass: ClassType<'db>,
|
||||
superclass_type: Type<'db>,
|
||||
superclass_method_kind: MethodKind,
|
||||
) {
|
||||
let db = context.db();
|
||||
|
||||
let signature_span = |function: FunctionType<'db>| {
|
||||
function
|
||||
.literal(db)
|
||||
.last_definition(db)
|
||||
.spans(db)
|
||||
.map(|spans| spans.signature)
|
||||
};
|
||||
|
||||
let subclass_definition_kind = subclass_definition.kind(db);
|
||||
let subclass_definition_signature_span = signature_span(subclass_function);
|
||||
|
||||
// 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 class_name = subclass.name(db);
|
||||
let superclass_name = superclass.name(db);
|
||||
|
||||
let overridden_method = if class_name == superclass_name {
|
||||
format!(
|
||||
"{superclass}.{member}",
|
||||
superclass = superclass.class_literal(db).0.qualified_name(db),
|
||||
)
|
||||
} else {
|
||||
format!("{superclass_name}.{member}")
|
||||
};
|
||||
|
||||
let Some(builder) = context.report_lint(&INVALID_METHOD_OVERRIDE, diagnostic_range) else {
|
||||
return;
|
||||
};
|
||||
|
||||
let mut diagnostic =
|
||||
builder.into_diagnostic(format_args!("Invalid override of method `{member}`"));
|
||||
|
||||
diagnostic.set_primary_message(format_args!(
|
||||
"Definition is incompatible with `{overridden_method}`"
|
||||
));
|
||||
|
||||
diagnostic.info("This violates the Liskov Substitution Principle");
|
||||
|
||||
if !subclass_definition_kind.is_function_def()
|
||||
&& let Some(span) = subclass_definition_signature_span
|
||||
{
|
||||
diagnostic.annotate(
|
||||
Annotation::secondary(span)
|
||||
.message(format_args!("Signature of `{class_name}.{member}`")),
|
||||
);
|
||||
}
|
||||
|
||||
let superclass_scope = superclass.class_literal(db).0.body_scope(db);
|
||||
|
||||
match superclass_method_kind {
|
||||
MethodKind::NotSynthesized => {
|
||||
if let Some(superclass_symbol) = place_table(db, superclass_scope).symbol_id(member)
|
||||
&& let Some(binding) = use_def_map(db, superclass_scope)
|
||||
.end_of_scope_bindings(ScopedPlaceId::Symbol(superclass_symbol))
|
||||
.next()
|
||||
&& let Some(definition) = binding.binding.definition()
|
||||
{
|
||||
let definition_span = Span::from(
|
||||
definition
|
||||
.full_range(db, &parsed_module(db, superclass_scope.file(db)).load(db)),
|
||||
);
|
||||
|
||||
let superclass_function_span = superclass_type
|
||||
.as_bound_method()
|
||||
.and_then(|method| signature_span(method.function(db)));
|
||||
|
||||
let superclass_definition_kind = definition.kind(db);
|
||||
|
||||
let secondary_span = if superclass_definition_kind.is_function_def()
|
||||
&& let Some(function_span) = superclass_function_span.clone()
|
||||
{
|
||||
function_span
|
||||
} else {
|
||||
definition_span
|
||||
};
|
||||
|
||||
diagnostic.annotate(
|
||||
Annotation::secondary(secondary_span.clone())
|
||||
.message(format_args!("`{overridden_method}` defined here")),
|
||||
);
|
||||
|
||||
if !superclass_definition_kind.is_function_def()
|
||||
&& let Some(function_span) = superclass_function_span
|
||||
&& function_span != secondary_span
|
||||
{
|
||||
diagnostic.annotate(
|
||||
Annotation::secondary(function_span)
|
||||
.message(format_args!("Signature of `{overridden_method}`")),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
MethodKind::Synthesized(synthesized_kind) => {
|
||||
let make_sub =
|
||||
|message: fmt::Arguments| SubDiagnostic::new(SubDiagnosticSeverity::Info, message);
|
||||
|
||||
let mut sub = match synthesized_kind {
|
||||
SynthesizedMethodKind::Dataclass => make_sub(format_args!(
|
||||
"`{overridden_method}` is a generated method created because \
|
||||
`{superclass_name}` is a dataclass"
|
||||
)),
|
||||
SynthesizedMethodKind::NamedTuple => make_sub(format_args!(
|
||||
"`{overridden_method}` is a generated method created because \
|
||||
`{superclass_name}` inherits from `typing.NamedTuple`"
|
||||
)),
|
||||
SynthesizedMethodKind::TypedDict => make_sub(format_args!(
|
||||
"`{overridden_method}` is a generated method created because \
|
||||
`{superclass_name}` is a `TypedDict`"
|
||||
)),
|
||||
};
|
||||
|
||||
sub.annotate(
|
||||
Annotation::primary(superclass.header_span(db))
|
||||
.message(format_args!("Definition of `{superclass_name}`")),
|
||||
);
|
||||
diagnostic.sub(sub);
|
||||
}
|
||||
}
|
||||
|
||||
if superclass.is_object(db) && matches!(member, "__eq__" | "__ne__") {
|
||||
// Inspired by mypy's subdiagnostic at <https://github.com/python/mypy/blob/1b6ebb17b7fe64488a7b3c3b4b0187bb14fe331b/mypy/messages.py#L1307-L1318>
|
||||
let eq_subdiagnostics = [
|
||||
format_args!(
|
||||
"It is recommended for `{member}` to work with arbitrary objects, for example:",
|
||||
),
|
||||
format_args!(""),
|
||||
format_args!(" def {member}(self, other: object) -> bool:",),
|
||||
format_args!(" if not isinstance(other, {class_name}):",),
|
||||
format_args!(" return False"),
|
||||
format_args!(" return <logic to compare two `{class_name}` instances>"),
|
||||
format_args!(""),
|
||||
];
|
||||
|
||||
for subdiag in eq_subdiagnostics {
|
||||
diagnostic.help(subdiag);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// This function receives an unresolved `from foo import bar` import,
|
||||
/// where `foo` can be resolved to a module but that module does not
|
||||
/// have a `bar` member or submodule.
|
||||
|
||||
@@ -14,8 +14,6 @@ use ruff_text_size::{TextLen, TextRange, TextSize};
|
||||
use rustc_hash::{FxHashMap, FxHashSet};
|
||||
|
||||
use crate::Db;
|
||||
use crate::module_resolver::file_to_module;
|
||||
use crate::semantic_index::{scope::ScopeKind, semantic_index};
|
||||
use crate::types::class::{ClassLiteral, ClassType, GenericAlias};
|
||||
use crate::types::function::{FunctionType, OverloadLiteral};
|
||||
use crate::types::generics::{GenericContext, Specialization};
|
||||
@@ -27,7 +25,6 @@ use crate::types::{
|
||||
MaterializationKind, Protocol, ProtocolInstanceType, SpecialFormType, StringLiteralType,
|
||||
SubclassOfInner, Type, UnionType, WrapperDescriptorKind, visitor,
|
||||
};
|
||||
use ruff_db::parsed::parsed_module;
|
||||
|
||||
/// Settings for displaying types and signatures
|
||||
#[derive(Debug, Clone, Default)]
|
||||
@@ -342,8 +339,11 @@ impl<'db> AmbiguousClassCollector<'db> {
|
||||
match value {
|
||||
AmbiguityState::Unambiguous(existing) => {
|
||||
if *existing != class {
|
||||
let qualified_name_components = class.qualified_name_components(db);
|
||||
if existing.qualified_name_components(db) == qualified_name_components {
|
||||
let qualified_name_components =
|
||||
class.qualified_name(db).components_excluding_self();
|
||||
if existing.qualified_name(db).components_excluding_self()
|
||||
== qualified_name_components
|
||||
{
|
||||
*value = AmbiguityState::RequiresFileAndLineNumber;
|
||||
} else {
|
||||
*value = AmbiguityState::RequiresFullyQualifiedName {
|
||||
@@ -358,7 +358,8 @@ impl<'db> AmbiguousClassCollector<'db> {
|
||||
qualified_name_components,
|
||||
} => {
|
||||
if *existing != class {
|
||||
let new_components = class.qualified_name_components(db);
|
||||
let new_components =
|
||||
class.qualified_name(db).components_excluding_self();
|
||||
if *qualified_name_components == new_components {
|
||||
*value = AmbiguityState::RequiresFileAndLineNumber;
|
||||
}
|
||||
@@ -505,52 +506,6 @@ impl<'db> ClassLiteral<'db> {
|
||||
settings,
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns the components of the qualified name of this class, excluding this class itself.
|
||||
///
|
||||
/// For example, calling this method on a class `C` in the module `a.b` would return
|
||||
/// `["a", "b"]`. Calling this method on a class `D` inside the namespace of a method
|
||||
/// `m` inside the namespace of a class `C` in the module `a.b` would return
|
||||
/// `["a", "b", "C", "<locals of function 'm'>"]`.
|
||||
fn qualified_name_components(self, db: &'db dyn Db) -> Vec<String> {
|
||||
let body_scope = self.body_scope(db);
|
||||
let file = body_scope.file(db);
|
||||
let module_ast = parsed_module(db, file).load(db);
|
||||
let index = semantic_index(db, file);
|
||||
let file_scope_id = body_scope.file_scope_id(db);
|
||||
|
||||
let mut name_parts = vec![];
|
||||
|
||||
// Skips itself
|
||||
for (_, ancestor_scope) in index.ancestor_scopes(file_scope_id).skip(1) {
|
||||
let node = ancestor_scope.node();
|
||||
|
||||
match ancestor_scope.kind() {
|
||||
ScopeKind::Class => {
|
||||
if let Some(class_def) = node.as_class() {
|
||||
name_parts.push(class_def.node(&module_ast).name.as_str().to_string());
|
||||
}
|
||||
}
|
||||
ScopeKind::Function => {
|
||||
if let Some(function_def) = node.as_function() {
|
||||
name_parts.push(format!(
|
||||
"<locals of function '{}'>",
|
||||
function_def.node(&module_ast).name.as_str()
|
||||
));
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(module) = file_to_module(db, file) {
|
||||
let module_name = module.name(db);
|
||||
name_parts.push(module_name.as_str().to_string());
|
||||
}
|
||||
|
||||
name_parts.reverse();
|
||||
name_parts
|
||||
}
|
||||
}
|
||||
|
||||
struct ClassDisplay<'db> {
|
||||
@@ -562,14 +517,14 @@ struct ClassDisplay<'db> {
|
||||
impl<'db> FmtDetailed<'db> for ClassDisplay<'db> {
|
||||
fn fmt_detailed(&self, f: &mut TypeWriter<'_, '_, 'db>) -> fmt::Result {
|
||||
let qualification_level = self.settings.qualified.get(&**self.class.name(self.db));
|
||||
|
||||
let ty = Type::ClassLiteral(self.class);
|
||||
if qualification_level.is_some() {
|
||||
for parent in self.class.qualified_name_components(self.db) {
|
||||
f.write_str(&parent)?;
|
||||
f.write_char('.')?;
|
||||
}
|
||||
write!(f.with_type(ty), "{}", self.class.qualified_name(self.db))?;
|
||||
} else {
|
||||
write!(f.with_type(ty), "{}", self.class.name(self.db))?;
|
||||
}
|
||||
f.with_type(Type::ClassLiteral(self.class))
|
||||
.write_str(self.class.name(self.db))?;
|
||||
|
||||
if qualification_level == Some(&QualificationLevel::FileAndLineNumber) {
|
||||
let file = self.class.file(self.db);
|
||||
let path = file.path(self.db);
|
||||
|
||||
@@ -451,7 +451,7 @@ impl<'db> AllMembers<'db> {
|
||||
}
|
||||
|
||||
/// A member of a type with an optional definition.
|
||||
#[derive(Clone, Debug)]
|
||||
#[derive(Clone, Debug, Eq, PartialEq, Hash)]
|
||||
pub struct MemberWithDefinition<'db> {
|
||||
pub member: Member<'db>,
|
||||
pub definition: Option<Definition<'db>>,
|
||||
|
||||
@@ -108,7 +108,7 @@ use crate::types::{
|
||||
TrackedConstraintSet, Truthiness, Type, TypeAliasType, TypeAndQualifiers, TypeContext,
|
||||
TypeQualifiers, TypeVarBoundOrConstraintsEvaluation, TypeVarDefaultEvaluation, TypeVarIdentity,
|
||||
TypeVarInstance, TypeVarKind, TypeVarVariance, TypedDictType, UnionBuilder, UnionType,
|
||||
UnionTypeInstance, binding_type, todo_type,
|
||||
UnionTypeInstance, binding_type, liskov, todo_type,
|
||||
};
|
||||
use crate::types::{ClassBase, add_inferred_python_version_hint_to_diagnostic};
|
||||
use crate::unpack::{EvaluationMode, UnpackPosition};
|
||||
@@ -548,9 +548,10 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
|
||||
"Inferring deferred types should not add more deferred definitions"
|
||||
);
|
||||
|
||||
// TODO: Only call this function when diagnostics are enabled.
|
||||
self.check_class_definitions();
|
||||
self.check_overloaded_functions(node);
|
||||
if self.db().should_check_file(self.file()) {
|
||||
self.check_class_definitions();
|
||||
self.check_overloaded_functions(node);
|
||||
}
|
||||
}
|
||||
|
||||
/// Iterate over all class definitions to check that the definition will not cause an exception
|
||||
@@ -949,6 +950,9 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
|
||||
}
|
||||
}
|
||||
|
||||
// (8) Check for Liskov violations
|
||||
liskov::check_class(&self.context, class);
|
||||
|
||||
if let Some(protocol) = class.into_protocol_class(self.db()) {
|
||||
protocol.validate_members(&self.context);
|
||||
}
|
||||
|
||||
167
crates/ty_python_semantic/src/types/liskov.rs
Normal file
167
crates/ty_python_semantic/src/types/liskov.rs
Normal file
@@ -0,0 +1,167 @@
|
||||
//! Checks relating to the [Liskov Substitution Principle].
|
||||
//!
|
||||
//! [Liskov Substitution Principle]: https://en.wikipedia.org/wiki/Liskov_substitution_principle
|
||||
|
||||
use rustc_hash::FxHashSet;
|
||||
|
||||
use crate::{
|
||||
place::Place,
|
||||
semantic_index::place_table,
|
||||
types::{
|
||||
ClassBase, ClassLiteral, ClassType, KnownClass, Type,
|
||||
class::CodeGeneratorKind,
|
||||
context::InferContext,
|
||||
diagnostic::report_invalid_method_override,
|
||||
ide_support::{MemberWithDefinition, all_declarations_and_bindings},
|
||||
},
|
||||
};
|
||||
|
||||
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();
|
||||
|
||||
for member in own_class_members {
|
||||
check_class_declaration(context, class_specialized, &member);
|
||||
}
|
||||
}
|
||||
|
||||
fn check_class_declaration<'db>(
|
||||
context: &InferContext<'db, '_>,
|
||||
class: ClassType<'db>,
|
||||
member: &MemberWithDefinition<'db>,
|
||||
) {
|
||||
let db = context.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;
|
||||
};
|
||||
|
||||
// TODO: classmethods and staticmethods
|
||||
if function.is_classmethod(db) || function.is_staticmethod(db) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Constructor methods are not checked for Liskov compliance
|
||||
if matches!(
|
||||
&*member.name,
|
||||
"__init__" | "__new__" | "__post_init__" | "__init_subclass__"
|
||||
) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Synthesized `__replace__` methods on dataclasses are not checked
|
||||
if &member.name == "__replace__"
|
||||
&& matches!(
|
||||
CodeGeneratorKind::from_class(db, class.class_literal(db).0, None),
|
||||
Some(CodeGeneratorKind::DataclassLike(_))
|
||||
)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
let Place::Defined(type_on_subclass_instance, _, _) =
|
||||
Type::instance(db, class).member(db, &member.name).place
|
||||
else {
|
||||
return;
|
||||
};
|
||||
|
||||
for superclass in class.iter_mro(db).skip(1).filter_map(ClassBase::into_class) {
|
||||
let superclass_symbol_table =
|
||||
place_table(db, superclass.class_literal(db).0.body_scope(db));
|
||||
|
||||
let mut method_kind = MethodKind::NotSynthesized;
|
||||
|
||||
// If the member is not defined on the class itself, skip it
|
||||
if let Some(superclass_symbol) = superclass_symbol_table.symbol_by_name(&member.name) {
|
||||
if !(superclass_symbol.is_bound() || superclass_symbol.is_declared()) {
|
||||
continue;
|
||||
}
|
||||
} else {
|
||||
let (superclass_literal, superclass_specialization) = superclass.class_literal(db);
|
||||
if superclass_literal
|
||||
.own_synthesized_member(db, superclass_specialization, None, &member.name)
|
||||
.is_none()
|
||||
{
|
||||
continue;
|
||||
}
|
||||
let class_kind =
|
||||
CodeGeneratorKind::from_class(db, superclass_literal, superclass_specialization);
|
||||
|
||||
method_kind = match class_kind {
|
||||
Some(CodeGeneratorKind::NamedTuple) => {
|
||||
MethodKind::Synthesized(SynthesizedMethodKind::NamedTuple)
|
||||
}
|
||||
Some(CodeGeneratorKind::DataclassLike(_)) => {
|
||||
MethodKind::Synthesized(SynthesizedMethodKind::Dataclass)
|
||||
}
|
||||
// It's invalid to define a method on a `TypedDict` (and this should be
|
||||
// reported elsewhere), but it's valid to override other things on a
|
||||
// `TypedDict`, so this case isn't relevant right now but may become
|
||||
// so when we expand Liskov checking in the future
|
||||
Some(CodeGeneratorKind::TypedDict) => {
|
||||
MethodKind::Synthesized(SynthesizedMethodKind::TypedDict)
|
||||
}
|
||||
None => MethodKind::NotSynthesized,
|
||||
};
|
||||
}
|
||||
|
||||
let Place::Defined(superclass_type, _, _) = Type::instance(db, superclass)
|
||||
.member(db, &member.name)
|
||||
.place
|
||||
else {
|
||||
// If not defined on any superclass, nothing to check
|
||||
break;
|
||||
};
|
||||
|
||||
let Some(superclass_type_as_callable) = superclass_type.try_upcast_to_callable(db) else {
|
||||
continue;
|
||||
};
|
||||
|
||||
if type_on_subclass_instance.is_assignable_to(db, superclass_type_as_callable) {
|
||||
continue;
|
||||
}
|
||||
|
||||
report_invalid_method_override(
|
||||
context,
|
||||
&member.name,
|
||||
class,
|
||||
*definition,
|
||||
function,
|
||||
superclass,
|
||||
superclass_type,
|
||||
method_kind,
|
||||
);
|
||||
|
||||
// Only one diagnostic should be emitted per each invalid override,
|
||||
// even if it overrides multiple superclasses incorrectly!
|
||||
// It's possible `report_invalid_method_override` didn't emit a diagnostic because there's a
|
||||
// suppression comment, but that too should cause us to exit early here.
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub(super) enum MethodKind {
|
||||
Synthesized(SynthesizedMethodKind),
|
||||
NotSynthesized,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub(super) enum SynthesizedMethodKind {
|
||||
NamedTuple,
|
||||
Dataclass,
|
||||
TypedDict,
|
||||
}
|
||||
Reference in New Issue
Block a user