mirror of https://github.com/astral-sh/ruff
[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 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`
This commit is contained in:
parent
1889a5e6eb
commit
89d915a1e3
|
|
@ -109,10 +109,6 @@ impl<'db> DefinitionState<'db> {
|
||||||
|| matches!(self, DefinitionState::Defined(def) if f(def))
|
|| matches!(self, DefinitionState::Defined(def) if f(def))
|
||||||
}
|
}
|
||||||
|
|
||||||
pub(crate) fn is_undefined(self) -> bool {
|
|
||||||
matches!(self, DefinitionState::Undefined)
|
|
||||||
}
|
|
||||||
|
|
||||||
#[allow(unused)]
|
#[allow(unused)]
|
||||||
pub(crate) fn definition(self) -> Option<Definition<'db>> {
|
pub(crate) fn definition(self) -> Option<Definition<'db>> {
|
||||||
match self {
|
match self {
|
||||||
|
|
|
||||||
|
|
@ -1640,19 +1640,21 @@ impl<'db> ClassLiteral<'db> {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
let mut attribute_assignments = attribute_assignments.peekable();
|
// Storage for the implicit `DefinitionState::Undefined` binding. If present, it
|
||||||
let unbound_visibility = attribute_assignments
|
// will be the first binding in the `attribute_assignments` iterator.
|
||||||
.peek()
|
let mut unbound_binding = None;
|
||||||
.map(|attribute_assignment| {
|
|
||||||
if attribute_assignment.binding.is_undefined() {
|
|
||||||
method_map.is_binding_visible(db, attribute_assignment)
|
|
||||||
} else {
|
|
||||||
Truthiness::AlwaysFalse
|
|
||||||
}
|
|
||||||
})
|
|
||||||
.unwrap_or(Truthiness::AlwaysFalse);
|
|
||||||
|
|
||||||
for attribute_assignment in attribute_assignments {
|
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 {
|
let DefinitionState::Defined(binding) = attribute_assignment.binding else {
|
||||||
continue;
|
continue;
|
||||||
};
|
};
|
||||||
|
|
@ -1676,6 +1678,11 @@ impl<'db> ClassLiteral<'db> {
|
||||||
// There is at least one attribute assignment that may be visible,
|
// There is at least one attribute assignment that may be visible,
|
||||||
// so if `unbound_visibility` is always false then this attribute is considered bound.
|
// 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.
|
// 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
|
if unbound_visibility
|
||||||
.negate()
|
.negate()
|
||||||
.and(is_method_visible)
|
.and(is_method_visible)
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue