language-servers/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-10-28 02:39:59 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

[ty] Prefer declared base class attribute over inferred attribute on subclass (#20764)

Browse source 
## Summary

When accessing an (instance) attribute on a given class, we were
previously traversing its MRO, and building a union of types (if the
attribute was available on multiple classes in the MRO) until we found a
*definitely bound* symbol. The idea was that possibly unbound symbols in
a subclass might only partially shadow the underlying base class
attribute.

This behavior was problematic for two reasons:
* if the attribute was definitely bound on a class (e.g. `self.x =
None`), we would have stopped iterating, even if there might be a `x:
str | None` declaration in a base class (the bug reported in
https://github.com/astral-sh/ty/issues/1067).
* if the attribute originated from an implicit instance attribute
assignment (e.g. `self.x = 1` in method `Sub.foo`), we might stop
looking and miss another implicit instance attribute assignment in a
base class method (e.g. `self.x = 2` in method `Base.bar`).

With this fix, we still iterate the MRO of the class, but we only stop
iterating if we find a *definitely declared* symbol. In this case, we
only return the declared attribute type. Otherwise, we keep building a
union of inferred attribute types.

The implementation here seemed to be the easiest fix for
https://github.com/astral-sh/ty/issues/1067 that also kept the ecosystem
impact low (the changes that I see all look correct). However, as the
Markdown tests show, there are other things to fix in this area. For
example, we should do a similar thing for *class attributes*. This is
more involved, though (affects many different areas and probably
involves a change to our descriptor protocol implementation), so I'd
like to postpone this to a follow-up.

closes https://github.com/astral-sh/ty/issues/1067

## Test Plan

Updated Markdown tests, including a regression test for
https://github.com/astral-sh/ty/issues/1067.
This commit is contained in:
David Peter 2025-10-13 09:28:57 +02:00 • committed by GitHub
parent c80ee1a50b
commit 9b9c9ae092
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 169 additions and 86 deletions
Download patch file Download diff file Expand all files Collapse all files

1
crates/ty_python_semantic/src/lib.rs
View file

@ -34,6 +34,7 @@ mod db;
mod dunder_all;
pub mod lint;
pub(crate) mod list;
mod member;
mod module_name;
mod module_resolver;
mod node_key;

73
crates/ty_python_semantic/src/member.rs Normal file
View file

@ -0,0 +1,73 @@
use crate::{
place::{Place, PlaceAndQualifiers},
types::Type,
};
/// The return type of certain member-lookup operations. Contains information
/// about the type, type qualifiers, boundness/declaredness, and additional
/// metadata (e.g. whether or not the member was declared)
#[derive(Debug, Clone, PartialEq, Eq, salsa::Update, get_size2::GetSize)]
pub(crate) struct Member<'db> {
/// Type, qualifiers, and boundness information of this member
pub(crate) inner: PlaceAndQualifiers<'db>,
/// Whether or not this member was explicitly declared (e.g. `attr: int = 1`
/// on the class body or `self.attr: int = 1` in a class method), or if the
/// type was inferred (e.g. `attr = 1` on the class body or `self.attr = 1`
/// in a class method).
pub(crate) is_declared: bool,
}
impl Default for Member<'_> {
fn default() -> Self {
Member::inferred(PlaceAndQualifiers::default())
}
}
impl<'db> Member<'db> {
/// Create a new [`Member`] whose type was inferred (rather than explicitly declared).
pub(crate) fn inferred(inner: PlaceAndQualifiers<'db>) -> Self {
Self {
inner,
is_declared: false,
}
}
/// Create a new [`Member`] whose type was explicitly declared (rather than inferred).
pub(crate) fn declared(inner: PlaceAndQualifiers<'db>) -> Self {
Self {
inner,
is_declared: true,
}
}
/// Create a new [`Member`] whose type was explicitly and definitively declared, i.e.
/// there is no control flow path in which it might be possibly undeclared.
pub(crate) fn definitely_declared(ty: Type<'db>) -> Self {
Self::declared(Place::bound(ty).into())
}
/// Represents the absence of a member.
pub(crate) fn unbound() -> Self {
Self::inferred(PlaceAndQualifiers::default())
}
/// Returns `true` if the inner place is unbound (i.e. there is no such member).
pub(crate) fn is_unbound(&self) -> bool {
self.inner.place.is_unbound()
}
/// Returns the inner type, unless it is definitely unbound.
pub(crate) fn ignore_possibly_unbound(&self) -> Option<Type<'db>> {
self.inner.place.ignore_possibly_unbound()
}
/// Map a type transformation function over the type of this member.
#[must_use]
pub(crate) fn map_type(self, f: impl FnOnce(Type<'db>) -> Type<'db>) -> Self {
Self {
inner: self.inner.map_type(f),
is_declared: self.is_declared,
}
}
}

17
crates/ty_python_semantic/src/place.rs
View file

@ -1,6 +1,7 @@
use ruff_db::files::File;
use crate::dunder_all::dunder_all_names;
use crate::member::Member;
use crate::module_resolver::{KnownModule, file_to_module};
use crate::semantic_index::definition::{Definition, DefinitionState};
use crate::semantic_index::place::{PlaceExprRef, ScopedPlaceId};
@ -232,13 +233,9 @@ pub(crate) fn place<'db>(
)
}
/// Infer the public type of a class symbol (its type as seen from outside its scope) in the given
/// Infer the public type of a class member/symbol (its type as seen from outside its scope) in the given
/// `scope`.
pub(crate) fn class_symbol<'db>(
db: &'db dyn Db,
scope: ScopeId<'db>,
name: &str,
) -> PlaceAndQualifiers<'db> {
pub(crate) fn class_member<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Member<'db> {
place_table(db, scope)
.symbol_id(name)
.map(|symbol_id| {
@ -252,7 +249,7 @@ pub(crate) fn class_symbol<'db>(
if !place_and_quals.place.is_unbound() && !place_and_quals.is_init_var() {
// Trust the declared type if we see a class-level declaration
return place_and_quals;
return Member::declared(place_and_quals);
}
if let PlaceAndQualifiers {
@ -267,14 +264,14 @@ pub(crate) fn class_symbol<'db>(
// TODO: we should not need to calculate inferred type second time. This is a temporary
// solution until the notion of Boundness and Declaredness is split. See #16036, #16264
match inferred {
Member::inferred(match inferred {
Place::Unbound => Place::Unbound.with_qualifiers(qualifiers),
Place::Type(_, boundness) => {
Place::Type(ty, boundness).with_qualifiers(qualifiers)
}
}
})
} else {
Place::Unbound.into()
Member::unbound()
}
})
.unwrap_or_default()

155
crates/ty_python_semantic/src/types/class.rs
View file

@ -7,6 +7,7 @@ use super::{
function::FunctionType, infer_expression_type, infer_unpack_types,
};
use crate::FxOrderMap;
use crate::member::Member;
use crate::module_resolver::KnownModule;
use crate::semantic_index::definition::{Definition, DefinitionState};
use crate::semantic_index::scope::{NodeWithScopeKind, Scope};
@ -36,7 +37,7 @@ use crate::{
Db, FxIndexMap, FxOrderSet, Program,
module_resolver::file_to_module,
place::{
Boundness, LookupError, LookupResult, Place, PlaceAndQualifiers, class_symbol,
Boundness, LookupError, LookupResult, Place, PlaceAndQualifiers, class_member,
known_module_symbol, place_from_bindings, place_from_declarations,
},
semantic_index::{
@ -96,12 +97,12 @@ fn inheritance_cycle_initial<'db>(
fn implicit_attribute_recover<'db>(
_db: &'db dyn Db,
_value: &PlaceAndQualifiers<'db>,
_value: &Member<'db>,
_count: u32,
_class_body_scope: ScopeId<'db>,
_name: String,
_target_method_decorator: MethodDecorator,
) -> salsa::CycleRecoveryAction<PlaceAndQualifiers<'db>> {
) -> salsa::CycleRecoveryAction<Member<'db>> {
salsa::CycleRecoveryAction::Iterate
}
@ -110,8 +111,8 @@ fn implicit_attribute_initial<'db>(
_class_body_scope: ScopeId<'db>,
_name: String,
_target_method_decorator: MethodDecorator,
) -> PlaceAndQualifiers<'db> {
Place::Unbound.into()
) -> Member<'db> {
Member::unbound()
}
fn try_mro_cycle_recover<'db>(
@ -744,7 +745,7 @@ impl<'db> ClassType<'db> {
db: &'db dyn Db,
inherited_generic_context: Option<GenericContext<'db>>,
name: &str,
) -> PlaceAndQualifiers<'db> {
) -> Member<'db> {
fn synthesize_getitem_overload_signature<'db>(
index_annotation: Type<'db>,
return_annotation: Type<'db>,
@ -782,7 +783,7 @@ impl<'db> ClassType<'db> {
let synthesized_dunder_method =
CallableType::function_like(db, Signature::new(parameters, Some(return_type)));
Place::bound(synthesized_dunder_method).into()
Member::definitely_declared(synthesized_dunder_method)
};
match name {
@ -956,7 +957,7 @@ impl<'db> ClassType<'db> {
CallableSignature::from_overloads(overload_signatures);
let getitem_type =
Type::Callable(CallableType::new(db, getitem_signature, true));
Place::bound(getitem_type).into()
Member::definitely_declared(getitem_type)
})
.unwrap_or_else(fallback_member_lookup)
}
@ -1028,7 +1029,7 @@ impl<'db> ClassType<'db> {
Signature::new_generic(inherited_generic_context, parameters, None),
);
Place::bound(synthesized_dunder).into()
Member::definitely_declared(synthesized_dunder)
}
_ => fallback_member_lookup(),
@ -1052,7 +1053,7 @@ impl<'db> ClassType<'db> {
/// A helper function for `instance_member` that looks up the `name` attribute only on
/// this class, not on its superclasses.
fn own_instance_member(self, db: &'db dyn Db, name: &str) -> PlaceAndQualifiers<'db> {
fn own_instance_member(self, db: &'db dyn Db, name: &str) -> Member<'db> {
let (class_literal, specialization) = self.class_literal(db);
class_literal
.own_instance_member(db, name)
@ -2017,7 +2018,9 @@ impl<'db> ClassLiteral<'db> {
lookup_result = lookup_result.or_else(|lookup_error| {
lookup_error.or_fall_back_to(
db,
class.own_class_member(db, self.inherited_generic_context(db), name),
class
.own_class_member(db, self.inherited_generic_context(db), name)
.inner,
)
});
}
@ -2089,17 +2092,19 @@ impl<'db> ClassLiteral<'db> {
inherited_generic_context: Option<GenericContext<'db>>,
specialization: Option<Specialization<'db>>,
name: &str,
) -> PlaceAndQualifiers<'db> {
) -> Member<'db> {
if name == "__dataclass_fields__" && self.dataclass_params(db).is_some() {
// Make this class look like a subclass of the `DataClassInstance` protocol
return Place::bound(KnownClass::Dict.to_specialized_instance(
db,
[
KnownClass::Str.to_instance(db),
KnownClass::Field.to_specialized_instance(db, [Type::any()]),
],
))
.with_qualifiers(TypeQualifiers::CLASS_VAR);
return Member::declared(
Place::bound(KnownClass::Dict.to_specialized_instance(
db,
[
KnownClass::Str.to_instance(db),
KnownClass::Field.to_specialized_instance(db, [Type::any()]),
],
))
.with_qualifiers(TypeQualifiers::CLASS_VAR),
);
}
if CodeGeneratorKind::NamedTuple.matches(db, self) {
@ -2113,12 +2118,12 @@ impl<'db> ClassLiteral<'db> {
);
let property_getter = CallableType::single(db, property_getter_signature);
let property = PropertyInstanceType::new(db, Some(property_getter), None);
return Place::bound(Type::PropertyInstance(property)).into();
return Member::definitely_declared(Type::PropertyInstance(property));
}
}
let body_scope = self.body_scope(db);
let symbol = class_symbol(db, body_scope, name).map_type(|ty| {
let member = class_member(db, body_scope, name).map_type(|ty| {
// The `__new__` and `__init__` members of a non-specialized generic class are handled
// specially: they inherit the generic context of their class. That lets us treat them
// as generic functions when constructing the class, and infer the specialization of
@ -2143,15 +2148,15 @@ impl<'db> ClassLiteral<'db> {
}
});
if symbol.place.is_unbound() {
if member.is_unbound() {
if let Some(synthesized_member) = self.own_synthesized_member(db, specialization, name)
{
return Place::bound(synthesized_member).into();
return Member::definitely_declared(synthesized_member);
}
// The symbol was not found in the class scope. It might still be implicitly defined in `@classmethod`s.
return Self::implicit_attribute(db, body_scope, name, MethodDecorator::ClassMethod);
}
symbol
member
}
/// Returns the type of a synthesized dataclass member like `__init__` or `__lt__`, or
@ -2329,7 +2334,6 @@ impl<'db> ClassLiteral<'db> {
.to_class_literal(db)
.into_class_literal()?
.own_class_member(db, self.inherited_generic_context(db), None, name)
.place
.ignore_possibly_unbound()
.map(|ty| {
ty.apply_type_mapping(
@ -2883,6 +2887,7 @@ impl<'db> ClassLiteral<'db> {
let mut union = UnionBuilder::new(db);
let mut union_qualifiers = TypeQualifiers::empty();
let mut is_definitely_bound = false;
for superclass in self.iter_mro(db, specialization) {
match superclass {
@ -2895,27 +2900,32 @@ impl<'db> ClassLiteral<'db> {
);
}
ClassBase::Class(class) => {
if let member @ PlaceAndQualifiers {
place: Place::Type(ty, boundness),
qualifiers,
if let Member {
inner:
member @ PlaceAndQualifiers {
place: Place::Type(ty, boundness),
qualifiers,
},
is_declared,
} = class.own_instance_member(db, name)
{
// TODO: We could raise a diagnostic here if there are conflicting type qualifiers
union_qualifiers |= qualifiers;
if boundness == Boundness::Bound {
if union.is_empty() {
// Short-circuit, no need to allocate inside the union builder
if is_declared {
// We found a definitely-declared attribute. Discard possibly collected
// inferred types from subclasses and return the declared type.
return member;
}
return Place::bound(union.add(ty).build())
.with_qualifiers(union_qualifiers);
is_definitely_bound = true;
}
// If we see a possibly-unbound symbol, we need to keep looking
// higher up in the MRO.
// If the attribute is not definitely declared on this class, keep looking higher
// up in the MRO, and build a union of all inferred types (and possibly-declared
// types):
union = union.add(ty);
// TODO: We could raise a diagnostic here if there are conflicting type qualifiers
union_qualifiers |= qualifiers;
}
}
ClassBase::TypedDict => {
@ -2941,10 +2951,13 @@ impl<'db> ClassLiteral<'db> {
if union.is_empty() {
Place::Unbound.with_qualifiers(TypeQualifiers::empty())
} else {
// If we have reached this point, we know that we have only seen possibly-unbound places.
// This means that the final result is still possibly-unbound.
let boundness = if is_definitely_bound {
Boundness::Bound
} else {
Boundness::PossiblyUnbound
};
Place::Type(union.build(), Boundness::PossiblyUnbound).with_qualifiers(union_qualifiers)
Place::Type(union.build(), boundness).with_qualifiers(union_qualifiers)
}
}
@ -2957,7 +2970,7 @@ impl<'db> ClassLiteral<'db> {
class_body_scope: ScopeId<'db>,
name: &str,
target_method_decorator: MethodDecorator,
) -> PlaceAndQualifiers<'db> {
) -> Member<'db> {
Self::implicit_attribute_inner(
db,
class_body_scope,
@ -2976,7 +2989,7 @@ impl<'db> ClassLiteral<'db> {
class_body_scope: ScopeId<'db>,
name: String,
target_method_decorator: MethodDecorator,
) -> PlaceAndQualifiers<'db> {
) -> Member<'db> {
// If we do not see any declarations of an attribute, neither in the class body nor in
// any method, we build a union of `Unknown` with the inferred types of all bindings of
// that attribute. We include `Unknown` in that union to account for the fact that the
@ -2995,8 +3008,8 @@ impl<'db> ClassLiteral<'db> {
let is_valid_scope = |method_scope: &Scope| {
if let Some(method_def) = method_scope.node().as_function() {
let method_name = method_def.node(&module).name.as_str();
if let Place::Type(Type::FunctionLiteral(method_type), _) =
class_symbol(db, class_body_scope, method_name).place
if let Some(Type::FunctionLiteral(method_type)) =
class_member(db, class_body_scope, method_name).ignore_possibly_unbound()
{
let method_decorator = MethodDecorator::try_from_fn_type(db, method_type);
if method_decorator != Ok(target_method_decorator) {
@ -3048,7 +3061,9 @@ impl<'db> ClassLiteral<'db> {
index.expression(value),
TypeContext::default(),
);
return Place::bound(inferred_ty).with_qualifiers(all_qualifiers);
return Member::inferred(
Place::bound(inferred_ty).with_qualifiers(all_qualifiers),
);
}
// If there is no right-hand side, just record that we saw a `Final` qualifier
@ -3056,7 +3071,7 @@ impl<'db> ClassLiteral<'db> {
continue;
}
return annotation;
return Member::declared(annotation);
}
}
@ -3248,20 +3263,16 @@ impl<'db> ClassLiteral<'db> {
}
}
if is_attribute_bound {
Member::inferred(if is_attribute_bound {
Place::bound(union_of_inferred_types.build()).with_qualifiers(qualifiers)
} else {
Place::Unbound.with_qualifiers(qualifiers)
}
})
}
/// A helper function for `instance_member` that looks up the `name` attribute only on
/// this class, not on its superclasses.
pub(crate) fn own_instance_member(
self,
db: &'db dyn Db,
name: &str,
) -> PlaceAndQualifiers<'db> {
pub(crate) fn own_instance_member(self, db: &'db dyn Db, name: &str) -> Member<'db> {
// TODO: There are many things that are not yet implemented here:
// - `typing.Final`
// - Proper diagnostics
@ -3291,10 +3302,9 @@ impl<'db> ClassLiteral<'db> {
// We ignore `InitVar` declarations on the class body, unless that attribute is overwritten
// by an implicit assignment in a method
if Self::implicit_attribute(db, body_scope, name, MethodDecorator::None)
.place
.is_unbound()
{
return Place::Unbound.into();
return Member::unbound();
}
}
@ -3309,20 +3319,21 @@ impl<'db> ClassLiteral<'db> {
if let Some(implicit_ty) =
Self::implicit_attribute(db, body_scope, name, MethodDecorator::None)
.place
.ignore_possibly_unbound()
{
if declaredness == Boundness::Bound {
// If a symbol is definitely declared, and we see
// attribute assignments in methods of the class,
// we trust the declared type.
declared.with_qualifiers(qualifiers)
Member::declared(declared.with_qualifiers(qualifiers))
} else {
Place::Type(
UnionType::from_elements(db, [declared_ty, implicit_ty]),
declaredness,
Member::declared(
Place::Type(
UnionType::from_elements(db, [declared_ty, implicit_ty]),
declaredness,
)
.with_qualifiers(qualifiers),
)
.with_qualifiers(qualifiers)
}
} else {
// The symbol is declared and bound in the class body,
@ -3331,7 +3342,7 @@ impl<'db> ClassLiteral<'db> {
// has a class-level default value, but it would not be
// found in a `__dict__` lookup.
Place::Unbound.into()
Member::unbound()
}
} else {
// The attribute is declared but not bound in the class body.
@ -3341,7 +3352,7 @@ impl<'db> ClassLiteral<'db> {
// union with the inferred type from attribute assignments.
if declaredness == Boundness::Bound {
declared.with_qualifiers(qualifiers)
Member::declared(declared.with_qualifiers(qualifiers))
} else {
if let Some(implicit_ty) = Self::implicit_attribute(
db,
@ -3349,16 +3360,19 @@ impl<'db> ClassLiteral<'db> {
name,
MethodDecorator::None,
)
.inner
.place
.ignore_possibly_unbound()
{
Place::Type(
UnionType::from_elements(db, [declared_ty, implicit_ty]),
declaredness,
Member::declared(
Place::Type(
UnionType::from_elements(db, [declared_ty, implicit_ty]),
declaredness,
)
.with_qualifiers(qualifiers),
)
.with_qualifiers(qualifiers)
} else {
declared.with_qualifiers(qualifiers)
Member::declared(declared.with_qualifiers(qualifiers))
}
}
}
@ -3563,7 +3577,7 @@ impl<'db> VarianceInferable<'db> for ClassLiteral<'db> {
let attribute_variances = attribute_names
.map(|name| {
let place_and_quals = self.own_instance_member(db, &name);
let place_and_quals = self.own_instance_member(db, &name).inner;
(name, place_and_quals)
})
.chain(attribute_places_and_qualifiers)
@ -5327,6 +5341,7 @@ impl SlotsKind {
fn from(db: &dyn Db, base: ClassLiteral) -> Self {
let Place::Type(slots_ty, bound) = base
.own_class_member(db, base.inherited_generic_context(db), None, "__slots__")
.inner
.place
else {
return Self::NotSpecified;