Skip to content

Commit 1f5065e

Browse files
lauraharkercopybara-github
lauraharker
authored and
copybara-github
committed
Fix transpilation of a let/const in a loop, used in an object getter/setter
When seeing references to 'let/const' loop vars in functions, transpilation currently wraps the function in an IIFE, to capture the 'let/const's values. This IIFE wrapping is not syntactically possible for getter/setter defs. Instead this change wraps the entire containing object literal in an IIFE. Note: class getters/setters and methods are always transpiled away before this pass runs and are not affected. Fixes #3599. PiperOrigin-RevId: 532132957
1 parent 4a14065 commit 1f5065e

File tree

2 files changed

+144
-19
lines changed

2 files changed

+144
-19
lines changed

‎src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java

+34-19
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
import com.google.common.collect.HashBasedTable;
2525
import com.google.common.collect.HashMultimap;
2626
import com.google.common.collect.LinkedHashMultimap;
27-
import com.google.common.collect.Multimap;
27+
import com.google.common.collect.SetMultimap;
2828
import com.google.common.collect.Table;
2929
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
3030
import com.google.javascript.jscomp.colors.StandardColors;
@@ -34,7 +34,6 @@
3434
import com.google.javascript.rhino.JSDocInfo;
3535
import com.google.javascript.rhino.Node;
3636
import com.google.javascript.rhino.Token;
37-
import java.util.Collection;
3837
import java.util.HashSet;
3938
import java.util.LinkedHashMap;
4039
import java.util.LinkedHashSet;
@@ -243,9 +242,11 @@ private class LoopClosureTransformer extends AbstractPostOrderCallback {
243242
private static final String LOOP_OBJECT_PROPERTY_NAME = "$jscomp$loop$prop$";
244243
private final Map<Node, LoopObject> loopObjectMap = new LinkedHashMap<>();
245244

246-
private final Multimap<Node, LoopObject> functionLoopObjectsMap = LinkedHashMultimap.create();
247-
private final Multimap<Node, String> functionHandledMap = HashMultimap.create();
248-
private final Multimap<Var, Node> referenceMap = LinkedHashMultimap.create();
245+
private final SetMultimap<Node, LoopObject> nodesRequiringloopObjectsClosureMap =
246+
LinkedHashMultimap.create();
247+
private final SetMultimap<Node, String> nodesHandledForLoopObjectClosure =
248+
HashMultimap.create();
249+
private final SetMultimap<Var, Node> referenceMap = LinkedHashMultimap.create();
249250
// Maps from a var to a unique property name for that var
250251
// e.g. 'i' -> '$jscomp$loop$prop$i$0'
251252
private final Map<Var, String> propertyNameMap = new LinkedHashMap<>();
@@ -303,11 +304,26 @@ public void visit(NodeTraversal t, Node n, Node parent) {
303304
}
304305

305306
if (outerMostFunctionScope != null) {
306-
Node function = outerMostFunctionScope.getRootNode();
307-
if (functionHandledMap.containsEntry(function, name)) {
307+
Node enclosingFunction = outerMostFunctionScope.getRootNode();
308+
309+
// There are two categories of functions we might find here:
310+
// 1. a getter or setter in an object literal. We will wrap the entire object literal in
311+
// a closure to capture the value of the let/const.
312+
// 2. a function declaration or expression. We will wrap the function in a closure.
313+
// (At this point, class methods/getters/setters and object literal member functions are
314+
// transpiled away.)
315+
final Node nodeToWrapInClosure;
316+
if (enclosingFunction.getParent().isGetterDef()
317+
|| enclosingFunction.getParent().isSetterDef()) {
318+
nodeToWrapInClosure = enclosingFunction.getGrandparent();
319+
checkState(nodeToWrapInClosure.isObjectLit());
320+
} else {
321+
nodeToWrapInClosure = enclosingFunction;
322+
}
323+
if (nodesHandledForLoopObjectClosure.containsEntry(nodeToWrapInClosure, name)) {
308324
return;
309325
}
310-
functionHandledMap.put(function, name);
326+
nodesHandledForLoopObjectClosure.put(nodeToWrapInClosure, name);
311327

312328
LoopObject object =
313329
loopObjectMap.computeIfAbsent(
@@ -318,8 +334,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
318334
String newPropertyName = createUniquePropertyName(var);
319335
object.vars.add(var);
320336
propertyNameMap.put(var, newPropertyName);
321-
322-
functionLoopObjectsMap.put(function, object);
337+
nodesRequiringloopObjectsClosureMap.put(nodeToWrapInClosure, object);
323338
}
324339
}
325340

@@ -485,9 +500,9 @@ private void transformLoopClosure() {
485500
}
486501

487502
// Create wrapper functions and call them.
488-
for (Node function : functionLoopObjectsMap.keySet()) {
503+
for (Node functionOrObjectLit : nodesRequiringloopObjectsClosureMap.keySet()) {
489504
Node returnNode = IR.returnNode();
490-
Collection<LoopObject> objects = functionLoopObjectsMap.get(function);
505+
Set<LoopObject> objects = nodesRequiringloopObjectsClosureMap.get(functionOrObjectLit);
491506
Node[] objectNames = new Node[objects.size()];
492507
Node[] objectNamesForCall = new Node[objects.size()];
493508
int i = 0;
@@ -505,18 +520,18 @@ private void transformLoopClosure() {
505520
IR.block(returnNode),
506521
type(StandardColors.TOP_OBJECT));
507522
compiler.reportChangeToChangeScope(iife);
508-
Node call = astFactory.createCall(iife, type(function), objectNamesForCall);
523+
Node call = astFactory.createCall(iife, type(functionOrObjectLit), objectNamesForCall);
509524
call.putBooleanProp(Node.FREE_CALL, true);
510525
Node replacement;
511-
if (NodeUtil.isFunctionDeclaration(function)) {
526+
if (NodeUtil.isFunctionDeclaration(functionOrObjectLit)) {
512527
replacement =
513-
IR.var(IR.name(function.getFirstChild().getString()), call)
514-
.srcrefTreeIfMissing(function);
528+
IR.var(IR.name(functionOrObjectLit.getFirstChild().getString()), call)
529+
.srcrefTreeIfMissing(functionOrObjectLit);
515530
} else {
516-
replacement = call.srcrefTreeIfMissing(function);
531+
replacement = call.srcrefTreeIfMissing(functionOrObjectLit);
517532
}
518-
function.replaceWith(replacement);
519-
returnNode.addChildToFront(function);
533+
functionOrObjectLit.replaceWith(replacement);
534+
returnNode.addChildToFront(functionOrObjectLit);
520535
compiler.reportChangeToEnclosingScope(replacement);
521536
}
522537
}

‎test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationTest.java

+110
Original file line numberDiff line numberDiff line change
@@ -1561,4 +1561,114 @@ public void testCatch() {
15611561
" } catch (e$2) { e$2--; }",
15621562
"}"));
15631563
}
1564+
1565+
// Regression test for https://github.com/google/closure-compiler/issues/3599
1566+
@Test
1567+
public void testReferenceToLoopScopedLetInObjectGetterAndSetter() {
1568+
test(
1569+
lines(
1570+
"for (let i = 0; i < 2; i++) {",
1571+
" let bar = 42;",
1572+
" let a = {",
1573+
" get foo() {",
1574+
" return bar",
1575+
" },",
1576+
" set foo(x) {",
1577+
" use(bar);",
1578+
" },",
1579+
" prop: bar",
1580+
" };",
1581+
" bar = 43;",
1582+
" use(a);",
1583+
"}"),
1584+
lines(
1585+
"var $jscomp$loop$0 = {};",
1586+
"var i = 0;",
1587+
"for (; i < 2; $jscomp$loop$0 =",
1588+
" {$jscomp$loop$prop$bar$1:$jscomp$loop$0.$jscomp$loop$prop$bar$1}, i++) {",
1589+
" $jscomp$loop$0.$jscomp$loop$prop$bar$1 = 42;",
1590+
" var a = (function($jscomp$loop$0) {",
1591+
" return {",
1592+
" get foo() {",
1593+
" return $jscomp$loop$0.$jscomp$loop$prop$bar$1;",
1594+
" },",
1595+
" set foo(x) {",
1596+
" use($jscomp$loop$0.$jscomp$loop$prop$bar$1);",
1597+
" },",
1598+
// Note - this statement will be evaluated immediately after this function definition,
1599+
// so will evaluate to 42 and not 43. We don't strictly need this to execute as part
1600+
// of the IIFE body but doing so also doesn't hurt correctness.
1601+
" prop: $jscomp$loop$0.$jscomp$loop$prop$bar$1",
1602+
" };",
1603+
" })($jscomp$loop$0);",
1604+
" $jscomp$loop$0.$jscomp$loop$prop$bar$1 = 43;",
1605+
" use(a);",
1606+
"}"));
1607+
}
1608+
1609+
@Test
1610+
public void testReferenceToMultipleLoopScopedLetConstInObjectWithGetter() {
1611+
test(
1612+
lines(
1613+
"for (let i = 0; i < 2; i++) {",
1614+
" let bar = 42;",
1615+
" const baz = 43;",
1616+
" let a = {",
1617+
" get foo() {",
1618+
" return bar + baz",
1619+
" },",
1620+
" };",
1621+
"}"),
1622+
lines(
1623+
"var $jscomp$loop$0 = {};",
1624+
"var i = 0;",
1625+
"for (; i < 2; $jscomp$loop$0 =",
1626+
" {$jscomp$loop$prop$bar$1:$jscomp$loop$0.$jscomp$loop$prop$bar$1,",
1627+
" $jscomp$loop$prop$baz$2:$jscomp$loop$0.$jscomp$loop$prop$baz$2},",
1628+
" i++) {",
1629+
" $jscomp$loop$0.$jscomp$loop$prop$bar$1 = 42;",
1630+
" /** @const */",
1631+
" $jscomp$loop$0.$jscomp$loop$prop$baz$2 = 43;",
1632+
" var a = (function($jscomp$loop$0) {",
1633+
" return {",
1634+
" get foo() {",
1635+
" return $jscomp$loop$0.$jscomp$loop$prop$bar$1 +",
1636+
" $jscomp$loop$0.$jscomp$loop$prop$baz$2;",
1637+
" },",
1638+
" };",
1639+
" })($jscomp$loop$0);",
1640+
"}"));
1641+
}
1642+
1643+
@Test
1644+
public void testReferenceToMultipleLoopScopedLetsInObjectWithSetter() {
1645+
test(
1646+
lines(
1647+
"for (let i = 0; i < 2; i++) {",
1648+
" let bar = 42;",
1649+
" let baz = 43;",
1650+
" let a = {",
1651+
" set foo(x = bar) {",
1652+
" return x + baz",
1653+
" },",
1654+
" };",
1655+
"}"),
1656+
lines(
1657+
"var $jscomp$loop$0 = {};",
1658+
"var i = 0;",
1659+
"for (; i < 2; $jscomp$loop$0 =",
1660+
" {$jscomp$loop$prop$bar$1:$jscomp$loop$0.$jscomp$loop$prop$bar$1,",
1661+
" $jscomp$loop$prop$baz$2:$jscomp$loop$0.$jscomp$loop$prop$baz$2},",
1662+
" i++) {",
1663+
" $jscomp$loop$0.$jscomp$loop$prop$bar$1 = 42;",
1664+
" $jscomp$loop$0.$jscomp$loop$prop$baz$2 = 43;",
1665+
" var a = (function($jscomp$loop$0) {",
1666+
" return {",
1667+
" set foo(x = $jscomp$loop$0.$jscomp$loop$prop$bar$1) {",
1668+
" return x + $jscomp$loop$0.$jscomp$loop$prop$baz$2;",
1669+
" },",
1670+
" };",
1671+
" })($jscomp$loop$0);",
1672+
"}"));
1673+
}
15641674
}

0 commit comments

Comments
 (0)