Skip to content

Commit 89d915a

Browse files
authored
[ty] Delay computation of 'unbound' visibility for implicit instance attributes (#18669)
## 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 = <unbound>` 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 = <unbound>` 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 = <unbound>` 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 astral-sh/ty#627 closes astral-sh/ty#641 ## Test Plan * Made sure that `ty` finishes quickly on the MREs in astral-sh/ty#627 * Made sure that `ty` finishes quickly on `pandas` * Made sure that `ty` finishes quickly on `sqlalchemy`
1 parent 1889a5e commit 89d915a

File tree

2 files changed

+18
-15
lines changed

2 files changed

+18
-15
lines changed

crates/ty_python_semantic/src/semantic_index/definition.rs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,6 @@ impl<'db> DefinitionState<'db> {
109109
|| matches!(self, DefinitionState::Defined(def) if f(def))
110110
}
111111

112-
pub(crate) fn is_undefined(self) -> bool {
113-
matches!(self, DefinitionState::Undefined)
114-
}
115-
116112
#[allow(unused)]
117113
pub(crate) fn definition(self) -> Option<Definition<'db>> {
118114
match self {

crates/ty_python_semantic/src/types/class.rs

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1640,19 +1640,21 @@ impl<'db> ClassLiteral<'db> {
16401640
continue;
16411641
}
16421642

1643-
let mut attribute_assignments = attribute_assignments.peekable();
1644-
let unbound_visibility = attribute_assignments
1645-
.peek()
1646-
.map(|attribute_assignment| {
1647-
if attribute_assignment.binding.is_undefined() {
1648-
method_map.is_binding_visible(db, attribute_assignment)
1649-
} else {
1650-
Truthiness::AlwaysFalse
1651-
}
1652-
})
1653-
.unwrap_or(Truthiness::AlwaysFalse);
1643+
// Storage for the implicit `DefinitionState::Undefined` binding. If present, it
1644+
// will be the first binding in the `attribute_assignments` iterator.
1645+
let mut unbound_binding = None;
16541646

16551647
for attribute_assignment in attribute_assignments {
1648+
if let DefinitionState::Undefined = attribute_assignment.binding {
1649+
// Store the implicit unbound binding here so that we can delay the
1650+
// computation of `unbound_visibility` to the point when we actually
1651+
// need it. This is an optimization for the common case where the
1652+
// `unbound` binding is the only binding of the `name` attribute,
1653+
// i.e. if there is no `self.name = …` assignment in this method.
1654+
unbound_binding = Some(attribute_assignment);
1655+
continue;
1656+
}
1657+
16561658
let DefinitionState::Defined(binding) = attribute_assignment.binding else {
16571659
continue;
16581660
};
@@ -1676,6 +1678,11 @@ impl<'db> ClassLiteral<'db> {
16761678
// There is at least one attribute assignment that may be visible,
16771679
// so if `unbound_visibility` is always false then this attribute is considered bound.
16781680
// TODO: this is incomplete logic since the attributes bound after termination are considered visible.
1681+
let unbound_visibility = unbound_binding
1682+
.as_ref()
1683+
.map(|binding| method_map.is_binding_visible(db, binding))
1684+
.unwrap_or(Truthiness::AlwaysFalse);
1685+
16791686
if unbound_visibility
16801687
.negate()
16811688
.and(is_method_visible)

0 commit comments

Comments
 (0)