Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Commit bcd24e3

Browse files
author
John Messerly
committed
improve cascades
* better code for the case we already supported * support the general case [email protected] Review URL: https://chromereviews.googleplex.com/154757013
1 parent c817ff2 commit bcd24e3

File tree

11 files changed

+379
-46
lines changed

11 files changed

+379
-46
lines changed

‎lib/src/codegen/js_codegen.dart

Lines changed: 135 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1127,26 +1127,97 @@ $name.prototype[Symbol.iterator] = function() {
11271127
// respective visit methods.
11281128
@override
11291129
void visitCascadeExpression(CascadeExpression node) {
1130-
// TODO(jmesserly): we need to handle the cascade target better. Ideally
1131-
// it should be assigned to a temp. Note that even simple identifier isn't
1132-
// safe in the face of getters.
1133-
if (node.target is! SimpleIdentifier) {
1134-
out.write('/* Unimplemented cascade on non-simple identifier: $node */');
1135-
return;
1130+
var savedCascadeTemp = _cascadeTarget;
1131+
1132+
var parent = node.parent;
1133+
var grandparent = parent.parent;
1134+
if (_isStateless(node.target, node)) {
1135+
// Special case: target is stateless, so we can just reuse it.
1136+
_cascadeTarget = node.target;
1137+
1138+
if (parent is ExpressionStatement) {
1139+
_visitNodeList(node.cascadeSections, separator: ';\n');
1140+
} else {
1141+
// Use comma expression. For example:
1142+
// (sb.write(1), sb.write(2), sb)
1143+
out.write('(');
1144+
_visitNodeList(node.cascadeSections, separator: ', ', suffix: ', ');
1145+
_cascadeTarget.accept(this);
1146+
out.write(')');
1147+
}
1148+
} else if (parent is AssignmentExpression &&
1149+
grandparent is ExpressionStatement &&
1150+
_isStateless(parent.leftHandSide, node)) {
1151+
1152+
// Special case: assignment to a variable in a statement.
1153+
// We can reuse the variable to desugar it:
1154+
// result = []..length = length;
1155+
// becomes:
1156+
// result = [];
1157+
// result.length = length;
1158+
_cascadeTarget = parent.leftHandSide;
1159+
node.target.accept(this);
1160+
out.write(';\n');
1161+
_visitNodeList(node.cascadeSections, separator: ';\n');
1162+
} else if (parent is VariableDeclaration &&
1163+
grandparent is VariableDeclarationList &&
1164+
grandparent.variables.last == parent) {
1165+
1166+
// Special case: variable declaration
1167+
// We can reuse the variable to desugar it:
1168+
// var result = []..length = length;
1169+
// becomes:
1170+
// var result = [];
1171+
// result.length = length;
1172+
_cascadeTarget = parent.name;
1173+
node.target.accept(this);
1174+
out.write(';\n');
1175+
_visitNodeList(node.cascadeSections, separator: ';\n');
1176+
} else {
1177+
// In the general case we need to capture the target expression into
1178+
// a temporary. This uses a lambda to get a temporary scope, and it also
1179+
// remains valid in an expression context.
1180+
// TODO(jmesserly): need a better way to handle temps.
1181+
// TODO(jmesserly): special case for parent is ExpressionStatement?
1182+
_cascadeTarget =
1183+
new SimpleIdentifier(new StringToken(TokenType.IDENTIFIER, '_', 0));
1184+
_cascadeTarget.staticElement =
1185+
new LocalVariableElementImpl.forNode(_cascadeTarget);
1186+
_cascadeTarget.staticType = node.target.staticType;
1187+
1188+
out.write('((${_cascadeTarget.name}) => {\n', 2);
1189+
_visitNodeList(node.cascadeSections, separator: ';\n', suffix: ';\n');
1190+
if (node.parent is! ExpressionStatement) {
1191+
out.write('return ${_cascadeTarget.name};\n');
1192+
}
1193+
out.write('})(', -2);
1194+
node.target.accept(this);
1195+
out.write(')');
11361196
}
11371197

1138-
var savedCascadeTemp = _cascadeTarget;
1139-
_cascadeTarget = node.target;
1140-
out.write('(', 2);
1141-
_visitNodeList(node.cascadeSections, separator: ',\n');
1142-
if (node.parent is! ExpressionStatement) {
1143-
if (node.cascadeSections.isNotEmpty) out.write(',\n');
1144-
_cascadeTarget.accept(this);
1145-
}
1146-
out.write(')', -2);
11471198
_cascadeTarget = savedCascadeTemp;
11481199
}
11491200

1201+
/// True is the expression can be evaluated multiple times without causing
1202+
/// code execution. This is true for final fields. This can be true for local
1203+
/// variables, if:
1204+
/// * they are not assigned within the [context].
1205+
/// * they are not assigned in a function closure anywhere.
1206+
bool _isStateless(Expression node, [AstNode context]) {
1207+
if (node is SimpleIdentifier) {
1208+
var e = node.staticElement;
1209+
if (e is PropertyAccessorElement) e = e.variable;
1210+
if (e is VariableElementImpl && !e.isSynthetic) {
1211+
if (e.isFinal) return true;
1212+
if (e is LocalVariableElementImpl || e is ParameterElementImpl) {
1213+
// make sure the local isn't mutated in the context.
1214+
return !_isPotentiallyMutated(e, context);
1215+
}
1216+
}
1217+
}
1218+
return false;
1219+
}
1220+
11501221
@override
11511222
void visitParenthesizedExpression(ParenthesizedExpression node) {
11521223
out.write('(');
@@ -1676,6 +1747,55 @@ $name.prototype[Symbol.iterator] = function() {
16761747
}
16771748
}
16781749

1750+
/// Returns true if the local variable is potentially mutated within [context].
1751+
/// This accounts for closures that may have been created outside of [context].
1752+
bool _isPotentiallyMutated(VariableElementImpl e, [AstNode context]) {
1753+
if (e.isPotentiallyMutatedInClosure) {
1754+
// TODO(jmesserly): this returns true incorrectly in some cases, because
1755+
// VariableResolverVisitor only checks that enclosingElement is not the
1756+
// function element, but enclosingElement can be something else in some
1757+
// cases (the block scope?). So it's more conservative than it could be.
1758+
return true;
1759+
}
1760+
if (e.isPotentiallyMutatedInScope) {
1761+
// Need to visit the context looking for assignment to this local.
1762+
if (context != null) {
1763+
var visitor = new _AssignmentFinder(e);
1764+
context.accept(visitor);
1765+
return visitor._potentiallyMutated;
1766+
}
1767+
return true;
1768+
}
1769+
return false;
1770+
}
1771+
1772+
/// Adapted from VariableResolverVisitor. Finds an assignment to a given
1773+
/// local variable.
1774+
class _AssignmentFinder extends RecursiveAstVisitor {
1775+
final VariableElementImpl _variable;
1776+
bool _potentiallyMutated = false;
1777+
1778+
_AssignmentFinder(this._variable);
1779+
1780+
@override
1781+
visitSimpleIdentifier(SimpleIdentifier node) {
1782+
// Ignore if qualified.
1783+
AstNode parent = node.parent;
1784+
if (parent is PrefixedIdentifier &&
1785+
identical(parent.identifier, node)) return;
1786+
if (parent is PropertyAccess &&
1787+
identical(parent.propertyName, node)) return;
1788+
if (parent is MethodInvocation &&
1789+
identical(parent.methodName, node)) return;
1790+
if (parent is ConstructorName) return;
1791+
if (parent is Label) return;
1792+
1793+
if (node.inSetterContext() && node.staticElement == _variable) {
1794+
_potentiallyMutated = true;
1795+
}
1796+
}
1797+
}
1798+
16791799
class JSGenerator extends CodeGenerator {
16801800
JSGenerator(String outDir, Uri root, TypeRules rules)
16811801
: super(outDir, root, rules);

‎test/codegen/cascade.dart

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
class A {
2+
var x;
3+
}
4+
5+
void test_closure_with_mutate() {
6+
var a = new A();
7+
a.x = () {
8+
print("hi");
9+
a = null;
10+
};
11+
a
12+
..x()
13+
..x();
14+
print(a);
15+
}
16+
17+
void test_closure_without_mutate() {
18+
var a = new A();
19+
a.x = () {
20+
print(a);
21+
};
22+
a
23+
..x()
24+
..x();
25+
print(a);
26+
}
27+
28+
void test_mutate_inside_cascade() {
29+
var a;
30+
a = new A()
31+
..x = (a = null)
32+
..x = (a = null);
33+
print(a);
34+
}
35+
36+
void test_mutate_outside_cascade() {
37+
var a, b;
38+
a = new A()
39+
..x = (b = null)
40+
..x = (b = null);
41+
a = null;
42+
print(a);
43+
}
44+
45+
void test_VariableDeclaration_single() {
46+
var a = []
47+
..length = 2
48+
..add(42);
49+
print(a);
50+
}
51+
52+
void test_VariableDeclaration_last() {
53+
var a = 42,
54+
b = []
55+
..length = 2
56+
..add(a);
57+
print(b);
58+
}
59+
60+
void test_VariableDeclaration_first() {
61+
var a = []
62+
..length = 2
63+
..add(3),
64+
b = 2;
65+
print(a);
66+
}

‎test/codegen/expect/_internal/_internal.js

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,10 @@ var _internal;
171171
let growable = opt$.growable === undefined ? true : opt$.growable;
172172
let result = null;
173173
if (growable) {
174-
result = /* Unimplemented cascade on non-simple identifier: new List<E>()..length = length */;
174+
result = ((_) => {
175+
_.length = this.length;
176+
return _;
177+
})(new core.List());
175178
} else {
176179
result = new core.List(this.length);
177180
}
@@ -253,7 +256,10 @@ var _internal;
253256
if (this._endOrLength !== null && this._endOrLength < end) end = this._endOrLength;
254257
let length = end - start;
255258
if (length < 0) length = 0;
256-
let result = growable ? (/* Unimplemented cascade on non-simple identifier: new List<E>()..length = length */) : new core.List(length);
259+
let result = growable ? (((_) => {
260+
_.length = length;
261+
return _;
262+
})(new core.List())) : new core.List(length);
257263
for (let i = 0; i < length; i++) {
258264
result.set(i, this._iterable.elementAt(start + i));
259265
if (this._iterable.length < end) throw new core.ConcurrentModificationError(this);

‎test/codegen/expect/async/async.js

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2042,7 +2042,10 @@ var async;
20422042
}
20432043
catch (e) {
20442044
let s = dart.stackTrace(e);
2045-
result = /* Unimplemented cascade on non-simple identifier: new _Future().._asyncCompleteError(e, s) */;
2045+
result = ((_) => {
2046+
_._asyncCompleteError(e, s);
2047+
return _;
2048+
})(new _Future());
20462049
}
20472050
} else {
20482051
result = result.whenComplete(this._onCancel);
@@ -2536,7 +2539,10 @@ var async;
25362539
_createSubscription(onData, onError, onDone, cancelOnError) {
25372540
if (this._isUsed) throw new core.StateError("Stream has already been listened to.");
25382541
this._isUsed = true;
2539-
return /* Unimplemented cascade on non-simple identifier: new _BufferingStreamSubscription(onData, onError, onDone, cancelOnError).._setPendingEvents(_pending()) */;
2542+
return ((_) => {
2543+
_._setPendingEvents(this._pending());
2544+
return _;
2545+
})(new _BufferingStreamSubscription(onData, onError, onDone, cancelOnError));
25402546
}
25412547
}
25422548

‎test/codegen/expect/cascade.txt

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// Messages from compiling cascade.dart
2+
warning: line 12, column 7 of test/codegen/cascade.dart: ..x() requires dynamic invoke
3+
..x()
4+
^^^^^
5+
warning: line 13, column 7 of test/codegen/cascade.dart: ..x() requires dynamic invoke
6+
..x();
7+
^^^^^
8+
warning: line 23, column 7 of test/codegen/cascade.dart: ..x() requires dynamic invoke
9+
..x()
10+
^^^^^
11+
warning: line 24, column 7 of test/codegen/cascade.dart: ..x() requires dynamic invoke
12+
..x();
13+
^^^^^

‎test/codegen/expect/cascade/cascade.js

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
var cascade;
2+
(function (cascade) {
3+
'use strict';
4+
class A {
5+
constructor() {
6+
this.x = null;
7+
super();
8+
}
9+
}
10+
11+
// Function test_closure_with_mutate: () → void
12+
function test_closure_with_mutate() {
13+
let a = new A();
14+
a.x = () => {
15+
core.print("hi");
16+
a = null;
17+
};
18+
((_) => {
19+
dart.dinvoke(_, "x");
20+
dart.dinvoke(_, "x");
21+
})(a);
22+
core.print(a);
23+
}
24+
25+
// Function test_closure_without_mutate: () → void
26+
function test_closure_without_mutate() {
27+
let a = new A();
28+
a.x = () => {
29+
core.print(a);
30+
};
31+
dart.dinvoke(a, "x");
32+
dart.dinvoke(a, "x");
33+
core.print(a);
34+
}
35+
36+
// Function test_mutate_inside_cascade: () → void
37+
function test_mutate_inside_cascade() {
38+
let a = null;
39+
a = ((_) => {
40+
_.x = (a = null);
41+
_.x = (a = null);
42+
return _;
43+
})(new A());
44+
core.print(a);
45+
}
46+
47+
// Function test_mutate_outside_cascade: () → void
48+
function test_mutate_outside_cascade() {
49+
let a = null, b = null;
50+
a = new A();
51+
dart.dput(a, "x", (b = null));
52+
dart.dput(a, "x", (b = null));
53+
a = null;
54+
core.print(a);
55+
}
56+
57+
// Function test_VariableDeclaration_single: () → void
58+
function test_VariableDeclaration_single() {
59+
let a = new List.from([]);
60+
dart.dput(a, "length", 2);
61+
a.add(42);
62+
core.print(a);
63+
}
64+
65+
// Function test_VariableDeclaration_last: () → void
66+
function test_VariableDeclaration_last() {
67+
let a = 42, b = new List.from([]);
68+
dart.dput(b, "length", 2);
69+
b.add(a);
70+
core.print(b);
71+
}
72+
73+
// Function test_VariableDeclaration_first: () → void
74+
function test_VariableDeclaration_first() {
75+
let a = ((_) => {
76+
_.length = 2;
77+
_.add(3);
78+
return _;
79+
})(new List.from([])), b = 2;
80+
core.print(a);
81+
}
82+
83+
// Exports:
84+
cascade.A = A;
85+
cascade.test_closure_with_mutate = test_closure_with_mutate;
86+
cascade.test_closure_without_mutate = test_closure_without_mutate;
87+
cascade.test_mutate_inside_cascade = test_mutate_inside_cascade;
88+
cascade.test_mutate_outside_cascade = test_mutate_outside_cascade;
89+
cascade.test_VariableDeclaration_single = test_VariableDeclaration_single;
90+
cascade.test_VariableDeclaration_last = test_VariableDeclaration_last;
91+
cascade.test_VariableDeclaration_first = test_VariableDeclaration_first;
92+
})(cascade || (cascade = {}));

0 commit comments

Comments
 (0)