Skip to content

Commit c33badb

Browse files
stereotype441dart-scoped@luci-project-accounts.iam.gserviceaccount.com
stereotype441
authored and
dart-scoped@luci-project-accounts.iam.gserviceaccount.com
committed
[analyzer] Don't write capture for-in loop variables at loop entry
The analyzer declared a for-in loop variable to flow analysis before the loop's conservative join, so a closure that wrote to the loop variable anywhere in the loop body prevented it from being promoted anywhere in the body. The CFE doesn't have this problem: for (var x in xs) { // List<Object?> xs if (x is int) { x.isEven; // Analyzer: error. CFE: OK. } () { x = null; }; } This change makes the analyzer match the CFE. It also updates `mini_ast` to declare for-in loop variables the same way. (Previously, `mini_ast` didn't declare these variables at all; this code path was untested.) Fixes #64536. Bug: #64536 TAG=agy CONV=0f865fb0-0d0a-4ef9-a59f-e0dee650ec2d Change-Id: Ia7f263cc365f42744933814af1a211826a6a6964 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/561800 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
1 parent a4dd1f9 commit c33badb

6 files changed

Lines changed: 258 additions & 16 deletions

File tree

‎pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -807,8 +807,14 @@ abstract class FlowAnalysis<
807807
/// The order of visiting a "for-in" statement or collection element should
808808
/// be:
809809
/// - Visit the iterable expression.
810-
/// - Call [forEach_bodyBegin] or [patternForIn_bodyBegin] (depending whether
811-
/// the loop is a pattern for-in or an ordinary for-in).
810+
/// - If the loop is a pattern for-in, call [patternForIn_beforePattern],
811+
/// visit the pattern (declaring the pattern variables), and then call
812+
/// [patternForIn_bodyBegin]. Otherwise, call [forEach_bodyBegin], and
813+
/// then:
814+
/// - If the loop declares a loop variable (`for (var x in ...)`), call
815+
/// [declare] for it.
816+
/// - If the loop assigns to an existing variable (`for (x in ...)`), call
817+
/// [write] for it (or visit an equivalent synthetic assignment).
812818
/// - Visit the body.
813819
/// - Call [forEach_end].
814820
///

‎pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -872,6 +872,47 @@ main() {
872872
]);
873873
});
874874

875+
test(
876+
'forEach_bodyBegin() does not write capture declared loop variable',
877+
() {
878+
// Each iteration gets a fresh copy of `x`, so a closure created later
879+
// in the loop body can't affect the promotion.
880+
var x = Var('x');
881+
h.run([
882+
forEachWithVariableDecl(x, expr('List<int?>'), [
883+
if_(x.notEq(nullLiteral), [checkPromoted(x, 'int')]),
884+
localFunction([x.write(expr('int?'))]),
885+
]),
886+
]);
887+
},
888+
);
889+
890+
test('forEach_bodyBegin() declared loop variable: write capture earlier '
891+
'in the iteration', () {
892+
var x = Var('x');
893+
h.run([
894+
forEachWithVariableDecl(x, expr('List<int?>'), [
895+
localFunction([x.write(expr('int?'))]),
896+
if_(x.notEq(nullLiteral), [checkNotPromoted(x)]),
897+
]),
898+
]);
899+
});
900+
901+
test('forEach_bodyBegin() declared loop variable: write capture in a '
902+
'nested loop', () {
903+
// A closure created in an earlier iteration of the inner loop writes
904+
// the same copy of `x`.
905+
var x = Var('x');
906+
h.run([
907+
forEachWithVariableDecl(x, expr('List<int?>'), [
908+
while_(expr('bool'), [
909+
if_(x.notEq(nullLiteral), [checkNotPromoted(x)]),
910+
localFunction([x.write(expr('int?'))]),
911+
]),
912+
]),
913+
]);
914+
});
915+
875916
test('forEach_bodyBegin() pushes conservative join state', () {
876917
var x = Var('x');
877918
h.run([

‎pkg/_fe_analyzer_shared/test/flow_analysis/type_promotion/data/write_capture.dart‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,44 @@ forEachElement(Object a, Object b, Object c) {
114114
});
115115
}
116116

117+
forEachLoopVariable(List<Object> list) {
118+
for (var x in list) {
119+
// Each iteration has a fresh copy of `x`, so the closure below (which is
120+
// created later in the iteration) can't invalidate this promotion.
121+
if (x is int) /*int*/ x;
122+
f(0, () {
123+
x = '';
124+
});
125+
}
126+
for (var y in list) {
127+
f(0, () {
128+
y = '';
129+
});
130+
if (y is int) y;
131+
}
132+
}
133+
134+
forEachElementLoopVariable(List<Object> list) {
135+
[
136+
for (var x in list)
137+
[
138+
x is int ? /*int*/ x : null,
139+
f(0, () {
140+
x = '';
141+
}),
142+
],
143+
];
144+
[
145+
for (var y in list)
146+
[
147+
f(0, () {
148+
y = '';
149+
}),
150+
y is int ? y : null,
151+
],
152+
];
153+
}
154+
117155
switchWithoutLabels(Object a, Object b, Object c) {
118156
switch (f(0, () {
119157
a = '';

‎pkg/_fe_analyzer_shared/test/mini_ast.dart‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ Statement forEachWithVariableDecl(
258258
return new ForEach._(
259259
variable,
260260
iterable.asExpression(location: location),
261-
block(body),
261+
Block._(body, location: location),
262262
true,
263263
location: location,
264264
);
@@ -1898,14 +1898,26 @@ class ForEach extends Statement {
18981898
);
18991899
h.flow.forEach_bodyBegin(this, offset: _syntheticBodyBeginOffset!);
19001900
var variable = this.variable;
1901-
if (variable != null && !declaresVariable) {
1902-
h.flow.write(
1903-
this,
1904-
variable,
1905-
SharedTypeView(iteratedType),
1906-
null,
1907-
offset: _syntheticBodyBeginOffset!,
1908-
);
1901+
if (variable != null) {
1902+
if (declaresVariable) {
1903+
// As in both front ends, the loop variable is declared after
1904+
// `forEach_bodyBegin`, since each iteration gets a fresh copy of it.
1905+
variable._type ??= iteratedType;
1906+
h.flow.declare(
1907+
variable,
1908+
SharedTypeView(variable.type),
1909+
initialized: true,
1910+
offset: _syntheticBodyBeginOffset!,
1911+
);
1912+
} else {
1913+
h.flow.write(
1914+
this,
1915+
variable,
1916+
SharedTypeView(iteratedType),
1917+
null,
1918+
offset: _syntheticBodyBeginOffset!,
1919+
);
1920+
}
19091921
}
19101922
h.typeAnalyzer._visitLoopBody(this, body);
19111923
h.flow.forEach_end(offset: _syntheticEndOffset!);

‎pkg/analyzer/lib/src/dart/resolver/for_resolver.dart‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,17 @@ class ForResolver {
217217
loopVariableElement.type = elementType;
218218
}
219219

220+
_typeAnalyzer.flowAnalysis.flow?.forEach_bodyBegin(
221+
node,
222+
offset: rightParenthesisOffset,
223+
);
220224
if (loopVariable != null) {
225+
// Each iteration of the loop gets a fresh copy of the loop variable, so
226+
// the loop variable is declared after `forEach_bodyBegin`. This ensures
227+
// that the conservative join performed by `forEach_bodyBegin` doesn't
228+
// affect it. In particular, a write to the loop variable inside a
229+
// closure in the loop body doesn't prevent the loop variable from being
230+
// promoted before the closure is created.
221231
var declaredElement = loopVariable.declaredFragment!.element;
222232
_typeAnalyzer.flowAnalysis.flow?.declare(
223233
declaredElement,
@@ -226,11 +236,6 @@ class ForResolver {
226236
offset: rightParenthesisOffset,
227237
);
228238
}
229-
230-
_typeAnalyzer.flowAnalysis.flow?.forEach_bodyBegin(
231-
node,
232-
offset: rightParenthesisOffset,
233-
);
234239
if (target != null) {
235240
if (target.write?.elementOrRecovery case PromotableElementImpl element) {
236241
_typeAnalyzer.flowAnalysis.flow?.write(

‎tests/language/type_promotion/for_in_loop_variable_write_captured_test.dart‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
// Copyright (c) 2026, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
// Tests that a variable declared by a for-in loop can be promoted inside the
6+
// loop even if a closure later in the loop body writes to it.
7+
//
8+
// Each iteration of the loop gets a fresh copy of the loop variable, so a
9+
// closure created in one iteration can only write to that iteration's copy.
10+
// Therefore the closure can't invalidate a promotion that happens in the same
11+
// iteration before the closure is created, nor can it invalidate promotions in
12+
// other iterations.
13+
14+
import 'package:expect/expect.dart';
15+
import 'package:expect/static_type_helper.dart';
16+
17+
void statementWriteCapturedLater(List<Object?> xs) {
18+
var closures = <void Function()>[];
19+
for (var x in xs) {
20+
if (x is int) {
21+
x.expectStaticType<Exactly<int>>();
22+
// Invoking closures from earlier iterations doesn't affect `x`.
23+
for (var closure in closures) {
24+
closure();
25+
}
26+
Expect.isTrue(x.isEven);
27+
}
28+
closures.add(() {
29+
x = null;
30+
});
31+
}
32+
}
33+
34+
void statementWriteCapturedEarlier(List<Object?> xs) {
35+
for (var x in xs) {
36+
() {
37+
x = null;
38+
};
39+
if (x is int) {
40+
x.expectStaticType<Exactly<Object?>>();
41+
}
42+
}
43+
}
44+
45+
void statementWriteCapturedInNestedLoop(List<Object?> xs) {
46+
for (var x in xs) {
47+
for (var i = 0; i < 2; i++) {
48+
// A closure created in an earlier iteration of the inner loop writes the
49+
// same copy of `x`, so `x` is considered write captured here and does
50+
// not promote.
51+
if (x is int) {
52+
x.expectStaticType<Exactly<Object?>>();
53+
}
54+
() {
55+
x = null;
56+
};
57+
}
58+
}
59+
}
60+
61+
void identifierWriteCaptured(List<Object?> xs) {
62+
// In this form, every iteration assigns to the same variable, so the
63+
// promotion must be invalidated.
64+
Object? x;
65+
for (x in xs) {
66+
if (x is int) {
67+
x.expectStaticType<Exactly<Object?>>();
68+
}
69+
() {
70+
x = null;
71+
};
72+
}
73+
}
74+
75+
Future<void> asyncStatementWriteCapturedLater(Stream<Object?> xs) async {
76+
await for (var x in xs) {
77+
if (x is int) {
78+
x.expectStaticType<Exactly<int>>();
79+
}
80+
() {
81+
x = null;
82+
};
83+
}
84+
}
85+
86+
void listElementWriteCapturedLater(List<Object?> xs) {
87+
[
88+
for (var x in xs) ...[
89+
if (x is int) x.expectStaticType<Exactly<int>>(),
90+
() {
91+
x = null;
92+
},
93+
],
94+
];
95+
}
96+
97+
void listElementWriteCapturedEarlier(List<Object?> xs) {
98+
[
99+
for (var x in xs) ...[
100+
() {
101+
x = null;
102+
},
103+
if (x is int) x.expectStaticType<Exactly<Object?>>(),
104+
],
105+
];
106+
}
107+
108+
void setElementWriteCapturedLater(List<Object?> xs) {
109+
<Object?>{
110+
for (var x in xs) ...[
111+
if (x is int) x.expectStaticType<Exactly<int>>(),
112+
() {
113+
x = null;
114+
},
115+
],
116+
};
117+
}
118+
119+
void mapElementWriteCapturedLater(List<Object?> xs) {
120+
<Object?, Object?>{
121+
for (var x in xs) ...{
122+
if (x is int) x.expectStaticType<Exactly<int>>(): null,
123+
() {
124+
x = null;
125+
}: null,
126+
},
127+
};
128+
}
129+
130+
main() async {
131+
statementWriteCapturedLater([0, null, 2]);
132+
statementWriteCapturedEarlier([0, null]);
133+
statementWriteCapturedInNestedLoop([0, null]);
134+
identifierWriteCaptured([0, null]);
135+
await asyncStatementWriteCapturedLater(Stream.fromIterable([0, null]));
136+
listElementWriteCapturedLater([0, null]);
137+
listElementWriteCapturedEarlier([0, null]);
138+
setElementWriteCapturedLater([0, null]);
139+
mapElementWriteCapturedLater([0, null]);
140+
}

0 commit comments

Comments
 (0)