Skip to content

Commit 9dd666d

Browse files
mtshibaMichaReisercarljm
authored
[ty] fix global symbol lookup from eager scopes (#21317)
## Summary cf. #20962 In the following code, `foo` in the comprehension was not reported as unresolved: ```python # error: [unresolved-reference] "Name `foo` used when not defined" foo foo = [ # no error! # revealed: Divergent reveal_type(x) for _ in () for x in [foo] ] baz = [ # error: [unresolved-reference] "Name `baz` used when not defined" # revealed: Unknown reveal_type(x) for _ in () for x in [baz] ] ``` In fact, this is a more serious bug than it looks: for `foo`, [`explicit_global_symbol` is called](https://github.com/astral-sh/ruff/blob/6cc3393ccd9059439d9c1325e0e041db1d7481af/crates/ty_python_semantic/src/types/infer/builder.rs#L8052), causing a symbol that should actually be `Undefined` to be reported as being of type `Divergent`. This PR fixes this bug. As a result, the code in `mdtest/regression/pr_20962_comprehension_panics.md` no longer panics. ## Test Plan `corpus\cyclic_symbol_in_comprehension.py` is added. New tests are added in `mdtest/comprehensions/basic.md`. --------- Co-authored-by: Micha Reiser <micha@reiser.io> Co-authored-by: Carl Meyer <carl@astral.sh>
1 parent a1d9cb5 commit 9dd666d

File tree

8 files changed

+136
-24
lines changed

8 files changed

+136
-24
lines changed
Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,7 @@
1-
# Documentation of two fuzzer panics involving comprehensions
1+
# Regression test for https://github.com/astral-sh/ruff/pull/20962
2+
# error message:
3+
# `place_by_id: execute: too many cycle iterations`
24

3-
Type inference for comprehensions was added in <https://github.com/astral-sh/ruff/pull/20962>. It
4-
added two new fuzzer panics that are documented here for regression testing.
5-
6-
## Too many cycle iterations in `place_by_id`
7-
8-
<!-- expect-panic: too many cycle iterations -->
9-
10-
```py
115
name_5(name_3)
126
[0 for unique_name_0 in unique_name_1 for unique_name_2 in name_3]
137

@@ -34,4 +28,3 @@ def name_3():
3428
@name_3
3529
async def name_5():
3630
pass
37-
```

crates/ty_python_semantic/resources/mdtest/annotations/deferred.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,23 @@ class Foo:
8787
class Baz[T: Foo]:
8888
pass
8989

90+
# error: [unresolved-reference] "Name `Foo` used when not defined"
91+
# error: [unresolved-reference] "Name `Bar` used when not defined"
92+
class Qux(Foo, Bar, Baz):
93+
pass
94+
95+
# error: [unresolved-reference] "Name `Foo` used when not defined"
96+
# error: [unresolved-reference] "Name `Bar` used when not defined"
97+
class Quux[_T](Foo, Bar, Baz):
98+
pass
99+
90100
# error: [unresolved-reference]
91101
type S = a
92102
type T = b
103+
type U = Foo
104+
# error: [unresolved-reference]
105+
type V = Bar
106+
type W = Baz
93107

94108
def h[T: Bar]():
95109
# error: [unresolved-reference]
@@ -141,9 +155,23 @@ class Foo:
141155
class Baz[T: Foo]:
142156
pass
143157

158+
# error: [unresolved-reference] "Name `Foo` used when not defined"
159+
# error: [unresolved-reference] "Name `Bar` used when not defined"
160+
class Qux(Foo, Bar, Baz):
161+
pass
162+
163+
# error: [unresolved-reference] "Name `Foo` used when not defined"
164+
# error: [unresolved-reference] "Name `Bar` used when not defined"
165+
class Quux[_T](Foo, Bar, Baz):
166+
pass
167+
144168
# error: [unresolved-reference]
145169
type S = a
146170
type T = b
171+
type U = Foo
172+
# error: [unresolved-reference]
173+
type V = Bar
174+
type W = Baz
147175

148176
def h[T: Bar]():
149177
# error: [unresolved-reference]

crates/ty_python_semantic/resources/mdtest/comprehensions/basic.md

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,24 @@ Iterating over an unbound iterable yields `Unknown`:
5858
# error: [not-iterable] "Object of type `int` is not iterable"
5959
# revealed: tuple[int, Unknown]
6060
[reveal_type((x, z)) for x in range(3) for z in x]
61+
62+
# error: [unresolved-reference] "Name `foo` used when not defined"
63+
foo
64+
foo = [
65+
# revealed: tuple[int, Unknown]
66+
reveal_type((x, z))
67+
for x in range(3)
68+
# error: [unresolved-reference] "Name `foo` used when not defined"
69+
for z in [foo]
70+
]
71+
72+
baz = [
73+
# revealed: tuple[int, Unknown]
74+
reveal_type((x, z))
75+
for x in range(3)
76+
# error: [unresolved-reference] "Name `baz` used when not defined"
77+
for z in [baz]
78+
]
6179
```
6280

6381
## Starred expressions

crates/ty_python_semantic/resources/mdtest/generics/scoping.md

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,43 @@ class C[T]:
288288
class Bad2(Iterable[T]): ...
289289
```
290290

291+
## Class bases are evaluated within the type parameter scope
292+
293+
```py
294+
class C[_T](
295+
# error: [unresolved-reference] "Name `C` used when not defined"
296+
C
297+
): ...
298+
299+
# `D` in `list[D]` is resolved to be a type variable of class `D`.
300+
class D[D](list[D]): ...
301+
302+
# error: [unresolved-reference] "Name `E` used when not defined"
303+
if E:
304+
class E[_T](
305+
# error: [unresolved-reference] "Name `E` used when not defined"
306+
E
307+
): ...
308+
309+
# error: [unresolved-reference] "Name `F` used when not defined"
310+
F
311+
312+
# error: [unresolved-reference] "Name `F` used when not defined"
313+
class F[_T](F): ...
314+
315+
def foo():
316+
class G[_T](
317+
# error: [unresolved-reference] "Name `G` used when not defined"
318+
G
319+
): ...
320+
# error: [unresolved-reference] "Name `H` used when not defined"
321+
if H:
322+
class H[_T](
323+
# error: [unresolved-reference] "Name `H` used when not defined"
324+
H
325+
): ...
326+
```
327+
291328
## Class scopes do not cover inner scopes
292329

293330
Just like regular symbols, the typevars of a generic class are only available in that class's scope,

crates/ty_python_semantic/resources/mdtest/narrow/complex_target.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,15 @@ d.x = 1
5858
reveal_type(d.x) # revealed: Literal[1]
5959
d.x = unknown()
6060
reveal_type(d.x) # revealed: Unknown
61+
62+
class E:
63+
x: int | None = None
64+
65+
e = E()
66+
67+
if e.x is not None:
68+
class _:
69+
reveal_type(e.x) # revealed: int
6170
```
6271

6372
Narrowing can be "reset" by assigning to the attribute:

crates/ty_python_semantic/src/semantic_index/builder.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,9 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
314314

315315
// Records snapshots of the place states visible from the current eager scope.
316316
fn record_eager_snapshots(&mut self, popped_scope_id: FileScopeId) {
317+
let popped_scope = &self.scopes[popped_scope_id];
318+
let popped_scope_is_annotation_scope = popped_scope.kind().is_annotation();
319+
317320
// If the scope that we just popped off is an eager scope, we need to "lock" our view of
318321
// which bindings reach each of the uses in the scope. Loop through each enclosing scope,
319322
// looking for any that bind each place.
@@ -328,6 +331,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
328331
// ```
329332
for enclosing_scope_info in self.scope_stack.iter().rev() {
330333
let enclosing_scope_id = enclosing_scope_info.file_scope_id;
334+
let is_immediately_enclosing_scope = popped_scope.parent() == Some(enclosing_scope_id);
331335
let enclosing_scope_kind = self.scopes[enclosing_scope_id].kind();
332336
let enclosing_place_table = &self.place_tables[enclosing_scope_id];
333337

@@ -355,6 +359,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
355359
enclosing_place_id,
356360
enclosing_scope_kind,
357361
enclosing_place,
362+
popped_scope_is_annotation_scope && is_immediately_enclosing_scope,
358363
);
359364
self.enclosing_snapshots.insert(key, eager_snapshot);
360365
}
@@ -429,6 +434,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
429434
enclosed_symbol_id.into(),
430435
enclosing_scope_kind,
431436
enclosing_place.into(),
437+
false,
432438
);
433439
self.enclosing_snapshots.insert(key, lazy_snapshot);
434440
}

crates/ty_python_semantic/src/semantic_index/use_def.rs

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1187,17 +1187,21 @@ impl<'db> UseDefMapBuilder<'db> {
11871187
pub(super) fn snapshot_enclosing_state(
11881188
&mut self,
11891189
enclosing_place: ScopedPlaceId,
1190-
scope: ScopeKind,
1190+
enclosing_scope: ScopeKind,
11911191
enclosing_place_expr: PlaceExprRef,
1192+
is_parent_of_annotation_scope: bool,
11921193
) -> ScopedEnclosingSnapshotId {
11931194
let bindings = match enclosing_place {
11941195
ScopedPlaceId::Symbol(symbol) => self.symbol_states[symbol].bindings(),
11951196
ScopedPlaceId::Member(member) => self.member_states[member].bindings(),
11961197
};
11971198

1198-
// Names bound in class scopes are never visible to nested scopes (but attributes/subscripts are visible),
1199-
// so we never need to save eager scope bindings in a class scope.
1200-
if (scope.is_class() && enclosing_place.is_symbol()) || !enclosing_place_expr.is_bound() {
1199+
let is_class_symbol = enclosing_scope.is_class() && enclosing_place.is_symbol();
1200+
// Names bound in class scopes are never visible to nested scopes (but
1201+
// attributes/subscripts are visible), so we never need to save eager scope bindings in a
1202+
// class scope. There is one exception to this rule: annotation scopes can see names
1203+
// defined in an immediately-enclosing class scope.
1204+
if (is_class_symbol && !is_parent_of_annotation_scope) || !enclosing_place_expr.is_bound() {
12011205
self.enclosing_snapshots.push(EnclosingSnapshot::Constraint(
12021206
bindings.unbound_narrowing_constraint(),
12031207
))

crates/ty_python_semantic/src/types/infer/builder.rs

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8319,6 +8319,12 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
83198319
let mut nonlocal_union_builder = UnionBuilder::new(db);
83208320
let mut found_some_definition = false;
83218321
for (enclosing_scope_file_id, _) in self.index.ancestor_scopes(file_scope_id).skip(1) {
8322+
// If the current enclosing scope is global, no place lookup is performed here,
8323+
// instead falling back to the module's explicit global lookup below.
8324+
if enclosing_scope_file_id.is_global() {
8325+
break;
8326+
}
8327+
83228328
// Class scopes are not visible to nested scopes, and we need to handle global
83238329
// scope differently (because an unbound name there falls back to builtins), so
83248330
// check only function-like scopes.
@@ -8349,6 +8355,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
83498355
// registering eager bindings for nested scopes that are actually eager, and for
83508356
// enclosing scopes that actually contain bindings that we should use when
83518357
// resolving the reference.)
8358+
let mut eagerly_resolved_place = None;
83528359
if !self.is_deferred() {
83538360
match self.index.enclosing_snapshot(
83548361
enclosing_scope_file_id,
@@ -8360,6 +8367,11 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
83608367
enclosing_scope_file_id,
83618368
ConstraintKey::NarrowingConstraint(constraint),
83628369
));
8370+
// If the current scope is eager, it is certain that the place is undefined in the current scope.
8371+
// Do not call the `place` query below as a fallback.
8372+
if scope.scope(db).is_eager() {
8373+
eagerly_resolved_place = Some(Place::Undefined.into());
8374+
}
83638375
}
83648376
EnclosingSnapshotResult::FoundBindings(bindings) => {
83658377
let place = place_from_bindings(db, bindings).map_type(|ty| {
@@ -8421,18 +8433,20 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
84218433
// `nonlocal` variable, but we don't enforce that here. See the
84228434
// `ast::Stmt::AnnAssign` handling in `SemanticIndexBuilder::visit_stmt`.)
84238435
if enclosing_place.is_bound() || enclosing_place.is_declared() {
8424-
let local_place_and_qualifiers = place(
8425-
db,
8426-
enclosing_scope_id,
8427-
place_expr,
8428-
ConsideredDefinitions::AllReachable,
8429-
)
8430-
.map_type(|ty| {
8431-
self.narrow_place_with_applicable_constraints(
8436+
let local_place_and_qualifiers = eagerly_resolved_place.unwrap_or_else(|| {
8437+
place(
8438+
db,
8439+
enclosing_scope_id,
84328440
place_expr,
8433-
ty,
8434-
&constraint_keys,
8441+
ConsideredDefinitions::AllReachable,
84358442
)
8443+
.map_type(|ty| {
8444+
self.narrow_place_with_applicable_constraints(
8445+
place_expr,
8446+
ty,
8447+
&constraint_keys,
8448+
)
8449+
})
84368450
});
84378451
// We could have `Place::Undefined` here, despite the checks above, for example if
84388452
// this scope contains a `del` statement but no binding or declaration.
@@ -8475,6 +8489,9 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
84758489
FileScopeId::global(),
84768490
ConstraintKey::NarrowingConstraint(constraint),
84778491
));
8492+
// Reaching here means that no bindings are found in any scope.
8493+
// Since `explicit_global_symbol` may return a cycle initial value, we return `Place::Undefined` here.
8494+
return Place::Undefined.into();
84788495
}
84798496
EnclosingSnapshotResult::FoundBindings(bindings) => {
84808497
let place = place_from_bindings(db, bindings).map_type(|ty| {

0 commit comments

Comments
 (0)