From 89d915a1e34144051815dfcbe60ec2cdeb29909e Mon Sep 17 00:00:00 2001 From: David Peter Date: Fri, 13 Jun 2025 21:50:57 +0200 Subject: [PATCH] [ty] Delay computation of 'unbound' visibility for implicit instance attributes (#18669) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Consider the following example, which leads to a excessively large runtime on `main`. The reason for this is the following. When inferring types for `self.a`, we look up the `a` attribute on `C`. While looking for implicit instance attributes, we go through every method and check for `self.a = …` assignments. There are no such assignments here, but we always have an implicit `self.a = ` binding at the beginning over every method. This binding accumulates a complex visibility constraint in `C.f`, due to the `isinstance` checks. While evaluating that constraint, we need to infer the type of `self.b`. There's no binding for `self.b` either, but there's also an implicit `self.b = ` binding with the same complex visibility constraint (involving `self.b` recursively). This leads to a combinatorial explosion: ```py class C: def f(self: "C"): if isinstance(self.a, str): return if isinstance(self.b, str): return if isinstance(self.b, str): return if isinstance(self.b, str): return # repeat 20 times ``` (note that the `self` parameter here is annotated explicitly because we currently still infer `Unknown` for `self` otherwise) The fix proposed here is rather simple: when there are no `self.name = …` attribute assignments in a given method, we skip evaluating the visibility constraint of the implicit `self.name = ` binding. This should also generally help with performance, because that's a very common case. This is *not* a fix for cases where there *are* actual bindings in the method. When we add `self.a = 1; self.b = 1` to that example above, we still see that combinatorial explosion of runtime. I still think it's worth to make this optimization, as it fixes the problems with `pandas` and `sqlalchemy` reported by users. I will open a ticket to track that separately. closes https://github.com/astral-sh/ty/issues/627 closes https://github.com/astral-sh/ty/issues/641 ## Test Plan * Made sure that `ty` finishes quickly on the MREs in https://github.com/astral-sh/ty/issues/627 * Made sure that `ty` finishes quickly on `pandas` * Made sure that `ty` finishes quickly on `sqlalchemy` --- .../src/semantic_index/definition.rs | 4 --- crates/ty_python_semantic/src/types/class.rs | 29 ++++++++++++------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/crates/ty_python_semantic/src/semantic_index/definition.rs b/crates/ty_python_semantic/src/semantic_index/definition.rs index 9e1e6a89cf..869944eba5 100644 --- a/crates/ty_python_semantic/src/semantic_index/definition.rs +++ b/crates/ty_python_semantic/src/semantic_index/definition.rs @@ -109,10 +109,6 @@ impl<'db> DefinitionState<'db> { || matches!(self, DefinitionState::Defined(def) if f(def)) } - pub(crate) fn is_undefined(self) -> bool { - matches!(self, DefinitionState::Undefined) - } - #[allow(unused)] pub(crate) fn definition(self) -> Option> { match self { diff --git a/crates/ty_python_semantic/src/types/class.rs b/crates/ty_python_semantic/src/types/class.rs index 78532e12de..165a0a3b85 100644 --- a/crates/ty_python_semantic/src/types/class.rs +++ b/crates/ty_python_semantic/src/types/class.rs @@ -1640,19 +1640,21 @@ impl<'db> ClassLiteral<'db> { continue; } - let mut attribute_assignments = attribute_assignments.peekable(); - let unbound_visibility = attribute_assignments - .peek() - .map(|attribute_assignment| { - if attribute_assignment.binding.is_undefined() { - method_map.is_binding_visible(db, attribute_assignment) - } else { - Truthiness::AlwaysFalse - } - }) - .unwrap_or(Truthiness::AlwaysFalse); + // Storage for the implicit `DefinitionState::Undefined` binding. If present, it + // will be the first binding in the `attribute_assignments` iterator. + let mut unbound_binding = None; for attribute_assignment in attribute_assignments { + if let DefinitionState::Undefined = attribute_assignment.binding { + // Store the implicit unbound binding here so that we can delay the + // computation of `unbound_visibility` to the point when we actually + // need it. This is an optimization for the common case where the + // `unbound` binding is the only binding of the `name` attribute, + // i.e. if there is no `self.name = …` assignment in this method. + unbound_binding = Some(attribute_assignment); + continue; + } + let DefinitionState::Defined(binding) = attribute_assignment.binding else { continue; }; @@ -1676,6 +1678,11 @@ impl<'db> ClassLiteral<'db> { // There is at least one attribute assignment that may be visible, // so if `unbound_visibility` is always false then this attribute is considered bound. // TODO: this is incomplete logic since the attributes bound after termination are considered visible. + let unbound_visibility = unbound_binding + .as_ref() + .map(|binding| method_map.is_binding_visible(db, binding)) + .unwrap_or(Truthiness::AlwaysFalse); + if unbound_visibility .negate() .and(is_method_visible)