From 67db4ca68f80158fa79d5368a0500187bb06fee2 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 25 Oct 2026 16:17:03 -0400 Subject: [PATCH] [ty] Check for instance storage without inferring method assignments (#29709) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Checking Home Assistant’s [HomeKit light module](https://github.com/home-assistant/core/blob/a0162d2ff0ed061501405a617961be149b15bd1f/homeassistant/components/homekit_controller/light.py#L53) opened 38 Python files instead of 45 and used 10% fewer instructions. When a class declares `value: int = 0`, ty may still need to know whether a method writes `self.value`. It used to infer `make()` in `self.value = make()` just to answer that question. Ty now stops once it finds a reachable assignment. The annotation supplies the type; checking the method still validates the assigned value. --- .../resources/mdtest/attributes.md | 51 +++++++++++ .../src/types/class/implicit_attributes.rs | 86 +++++++++++++------ .../src/types/class/static_literal.rs | 53 ++++++------ .../src/types/infer/tests.rs | 39 +++++++++ 4 files changed, 179 insertions(+), 50 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/attributes.md b/crates/ty_python_semantic/resources/mdtest/attributes.md index de3efd715a..398f086d2a 100644 --- a/crates/ty_python_semantic/resources/mdtest/attributes.md +++ b/crates/ty_python_semantic/resources/mdtest/attributes.md @@ -1303,6 +1303,57 @@ reveal_type(C.variable_with_class_default1) # revealed: Literal["overwritten on reveal_type(c_instance.variable_with_class_default1) # revealed: Literal["value set on instance"] ``` +#### Instance storage for annotated descriptor defaults + +A non-data descriptor can be shadowed by instance storage. An ordinary receiver assignment or +annotation establishes that storage; assignments in static or class methods do not. An augmented +assignment also reads the descriptor before writing its result. + +```py +def flag() -> bool: + return True + +class Descriptor: + def __get__(self, instance: object, owner: type) -> str: + return "" + + def __iadd__(self, other: str): + return self + +class Model: + ordinary: Descriptor = Descriptor() + annotated: Descriptor = Descriptor() + static_only: Descriptor = Descriptor() + class_only: Descriptor = Descriptor() + augmented: Descriptor = Descriptor() + if flag(): + conditional: Descriptor = Descriptor() + + def assign(self, value: Descriptor): + self.ordinary = value + self.annotated: Descriptor + self.augmented += "!" # error: [invalid-assignment] + # error: [possibly-missing-attribute] + self.conditional = b"instance" # error: [invalid-assignment] + + @staticmethod + def assign_static(receiver): + receiver.static_only = Descriptor() + + @classmethod + def assign_class(cls): + cls.class_only = Descriptor() + +m = Model() +reveal_type(m.ordinary) # revealed: str | Descriptor +reveal_type(m.annotated) # revealed: str | Descriptor +reveal_type(m.static_only) # revealed: str +reveal_type(m.class_only) # revealed: str +reveal_type(m.augmented) # revealed: str | Descriptor +# error: [possibly-missing-attribute] +reveal_type(m.conditional) # revealed: str | Descriptor | bytes +``` + #### Augmented assignments to overriding class-level defaults An unannotated class-level default retains the inherited declaration, including through a diamond. diff --git a/crates/ty_python_semantic/src/types/class/implicit_attributes.rs b/crates/ty_python_semantic/src/types/class/implicit_attributes.rs index 19cc5c5405..ab98f7194c 100644 --- a/crates/ty_python_semantic/src/types/class/implicit_attributes.rs +++ b/crates/ty_python_semantic/src/types/class/implicit_attributes.rs @@ -1,5 +1,7 @@ //! Implicit instance and class attributes inferred from method assignments. +use std::{convert::Infallible, ops::ControlFlow}; + use super::{MethodDecorator, static_literal::StaticClassLiteral}; use crate::{ Db, ProgramEnvironment, TypeQualifiers, attribute_assignments, attribute_declarations, @@ -55,27 +57,41 @@ impl<'db> StaticClassLiteral<'db> { name: &str, target_method_decorator: MethodDecorator, ) -> ImplicitAttribute<'db> { + self.implicit_attribute_name(db, name, target_method_decorator) + .map_or_else( + || ImplicitAttribute { + member: Member::unbound(), + augmented_bindings: None, + }, + |attribute| Self::implicit_attribute_inner(db, attribute), + ) + } + + /// Does a method establish instance storage for an attribute already typed by the class? + pub(super) fn has_implicit_instance_attribute(self, db: &'db dyn Db, name: &str) -> bool { + self.implicit_attribute_name(db, name, MethodDecorator::None) + .is_some_and(|attribute| Self::has_implicit_attribute_inner(db, attribute)) + } + + fn implicit_attribute_name( + self, + db: &'db dyn Db, + name: &str, + target_method_decorator: MethodDecorator, + ) -> Option> { let class_body_scope = self.body_scope(db); // Collect names in a tracked query so unrelated edits can preserve dependent member // lookups, and avoid retaining query entries for names that no method can define. let names = implicit_attribute_definition_names(db, class_body_scope); - let Ok(name_index) = names.binary_search_by(|candidate| candidate.as_str().cmp(name)) - else { - return ImplicitAttribute { - member: Member::unbound(), - augmented_bindings: None, - }; - }; - - Self::implicit_attribute_inner( + let name_index = names + .binary_search_by(|candidate| candidate.as_str().cmp(name)) + .ok()?; + Some(ImplicitAttributeName::new( db, - ImplicitAttributeName::new( - db, - class_body_scope, - &names[name_index], - target_method_decorator, - ), - ) + class_body_scope, + &names[name_index], + target_method_decorator, + )) } #[salsa::tracked( @@ -93,13 +109,31 @@ impl<'db> StaticClassLiteral<'db> { db: &'db dyn Db, attribute: ImplicitAttributeName<'db>, ) -> ImplicitAttribute<'db> { - Self::implicit_attribute_impl(db, attribute) + let ControlFlow::Continue(result) = + Self::implicit_attribute_impl(db, attribute, |binding| { + ControlFlow::::Continue(implicit_attribute_binding_type(db, binding)) + }); + result } - fn implicit_attribute_impl( + #[salsa::tracked(returns(copy), cycle_initial=|_, _, _| true, heap_size=ruff_memory_usage::heap_size)] + fn has_implicit_attribute_inner( db: &'db dyn Db, attribute: ImplicitAttributeName<'db>, - ) -> ImplicitAttribute<'db> { + ) -> bool { + match Self::implicit_attribute_impl(db, attribute, |_| ControlFlow::Break(())) { + ControlFlow::Break(()) => true, + ControlFlow::Continue(result) => !result.member.is_undefined(), + } + } + + /// An ordinary reachable assignment establishes the attribute even when its value's type + /// is unknown. Callers that only need presence can stop before inferring that value. + fn implicit_attribute_impl( + db: &'db dyn Db, + attribute: ImplicitAttributeName<'db>, + mut binding_type: impl FnMut(Definition<'db>) -> ControlFlow>>, + ) -> ControlFlow> { let class_body_scope = attribute.class_body_scope(db); let name = attribute.name(db).as_str(); let target_method_decorator = attribute.target_method_decorator(db); @@ -208,14 +242,14 @@ impl<'db> StaticClassLiteral<'db> { index.expression(value), TypeContext::default(), ); - return ImplicitAttribute { + return ControlFlow::Continue(ImplicitAttribute { member: Member { inner: Place::bound(inferred_ty) .with_definition(declaration) .with_qualifiers(all_qualifiers), }, augmented_bindings: None, - }; + }); } // If there is no right-hand side, just record that we saw a `Final` qualifier @@ -223,10 +257,10 @@ impl<'db> StaticClassLiteral<'db> { continue; } - return ImplicitAttribute { + return ControlFlow::Continue(ImplicitAttribute { member: Member { inner: annotation }, augmented_bindings: None, - }; + }); } } @@ -293,7 +327,7 @@ impl<'db> StaticClassLiteral<'db> { is_attribute_bound = true; } - let inferred_ty = implicit_attribute_binding_type(db, binding); + let inferred_ty = binding_type(binding)?; if let Some(inferred_ty) = inferred_ty { provenance = provenance.or(Provenance::SingleDefinition(binding)); @@ -317,11 +351,11 @@ impl<'db> StaticClassLiteral<'db> { Member::unbound() }; - ImplicitAttribute { + ControlFlow::Continue(ImplicitAttribute { member, augmented_bindings: (!augmented_bindings.is_empty()) .then(|| AugmentedBindings::new(db, augmented_bindings.into_boxed_slice())), - } + }) } } diff --git a/crates/ty_python_semantic/src/types/class/static_literal.rs b/crates/ty_python_semantic/src/types/class/static_literal.rs index 3e30843615..f93656138d 100644 --- a/crates/ty_python_semantic/src/types/class/static_literal.rs +++ b/crates/ty_python_semantic/src/types/class/static_literal.rs @@ -3045,36 +3045,41 @@ impl<'db> StaticClassLiteral<'db> { if has_binding { // The attribute is declared and bound in the class body. - let implicit = self.implicit_attribute(db, name, MethodDecorator::None); + let implicit = match declaredness { + Definedness::AlwaysDefined + if self.has_implicit_instance_attribute(db, name) => + { + // A method assigns to this attribute, but its declared type + // already determines what an instance can read. + return Member { + inner: declared.with_qualifiers(qualifiers), + }; + } + Definedness::AlwaysDefined => Member::unbound(), + Definedness::PossiblyUndefined => { + self.implicit_attribute(db, name, MethodDecorator::None) + } + }; if let Place::Defined(DefinedPlace { ty: implicit_ty, provenance: implicit_provenance, .. }) = implicit.inner.place { - if declaredness == Definedness::AlwaysDefined { - // If a symbol is definitely declared, and we see - // attribute assignments in methods of the class, - // we trust the declared type. - Member { - inner: declared.with_qualifiers(qualifiers), - } - } else { - Member { - inner: Place::Defined(DefinedPlace { - ty: UnionType::from_two_elements( - db, - env, - declared_ty, - implicit_ty, - ), - origin: TypeOrigin::Declared, - definedness: declaredness, - public_type_policy: PublicTypePolicy::Raw, - provenance: implicit_provenance.or(declared_provenance), - }) - .with_qualifiers(qualifiers), - } + Member { + inner: Place::Defined(DefinedPlace { + ty: UnionType::from_two_elements( + db, + env, + declared_ty, + implicit_ty, + ), + origin: TypeOrigin::Declared, + definedness: declaredness, + public_type_policy: PublicTypePolicy::Raw, + provenance: implicit_provenance.or(declared_provenance), + }) + .with_qualifiers(qualifiers), } } else if self.is_own_dataclass_instance_field(db, name) && declared_ty diff --git a/crates/ty_python_semantic/src/types/infer/tests.rs b/crates/ty_python_semantic/src/types/infer/tests.rs index 6e8778b57d..d2acdc23f7 100644 --- a/crates/ty_python_semantic/src/types/infer/tests.rs +++ b/crates/ty_python_semantic/src/types/infer/tests.rs @@ -1644,6 +1644,45 @@ fn declared_instance_attribute_avoids_inferring_method_assignments() -> anyhow:: Ok(()) } +#[test] +fn inherited_class_defaults_avoid_inferring_instance_assignments() -> anyhow::Result<()> { + let mut db = setup_db(); + db.write_file("/src/dependency.py", "value = 'updated'")?; + db.write_dedented( + "/src/bases.py", + r#" + class First: + value: int = 0 + + class Second: + value: str = "" + + def reset(self): + from dependency import value + self.value = value + "#, + )?; + db.write_dedented( + "/src/main.py", + r#" + from bases import First, Second + + class Child(First, Second): ... + "#, + )?; + + let dependency = system_path_to_file(&db, "/src/dependency.py")?; + db.clear_salsa_events(); + assert_file_diagnostics( + &db, + "/src/main.py", + &["Incompatible inherited attribute `value`"], + ); + let events = db.take_salsa_events(); + assert_function_query_was_not_run(&db, semantic_index, program_file(&db, dependency), &events); + Ok(()) +} + #[test] fn dependency_implicit_instance_attribute() -> anyhow::Result<()> { fn x_rhs_expression(db: &TestDb) -> Expression<'_> {