Consider enhanced for loops as always having side-effects for the pur…

…poses of statement ordering.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=238517556

src/com/google/javascript/jscomp/NodeUtil.java

@@ -1144,13 +1144,21 @@ private static boolean checkForStateChangeHelper(
     // Rather than id which ops may have side effects, id the ones
     // that we know to be safe
     switch (n.getToken()) {
-      // Throws are by definition side effects, and yield and export are similar.
       case THROW:
+        // Throw is a side-effect by definition.
       case YIELD:
-      case EXPORT:
+      case AWAIT:
+      case FOR_AWAIT_OF:
+        // Context switches can conceal side-effects.
+      case FOR_OF:
+      case FOR_IN:
+        // Enhanced for loops are almost always side-effectful; it's not worth checking them
+        // further. Particularly, they represent a kind of assignment op.
       case VAR:
       case LET:
       case CONST:
+      case EXPORT:
+        // Variable declarations are side-effects in practically all contexts.
         return true;
 
       case OBJECTLIT:
@@ -1234,6 +1242,7 @@ && checkForStateChangeHelper(member.getFirstChild(), checkForNewObjects, compile
         return true;
 
       case TAGGED_TEMPLATELIT:
+        // TODO(b/128527671): Inspect the children of the expression for side-effects.
         return functionCallHasSideEffects(n, compiler);
 
       case CAST:

test/com/google/javascript/jscomp/NodeUtilTest.java

@@ -30,6 +30,7 @@
 import static com.google.javascript.rhino.Token.FUNCTION;
 import static com.google.javascript.rhino.Token.REST;
 import static com.google.javascript.rhino.Token.SPREAD;
+import static com.google.javascript.rhino.Token.THROW;
 import static com.google.javascript.rhino.Token.YIELD;
 import static com.google.javascript.rhino.testing.NodeSubject.assertNode;
 
@@ -645,7 +646,7 @@ private void assertSideEffect(boolean se, String js, boolean globalRegExp) {
   }
 
   @Test
-  public void testMayHaveSideEffects() {
+  public void testMayHaveSideEffects_undifferentiatedCases() {
     assertSideEffect(false, "[1]");
     assertSideEffect(false, "[1, 2]");
     assertSideEffect(false, "[...[]]");
@@ -719,7 +720,6 @@ public void testMayHaveSideEffects() {
     assertSideEffect(true, "st = `${name}template`");
     assertSideEffect(true, "tempFunc = templateFunction`template`");
 
-
     assertSideEffect(false, "new RegExp('foobar', 'i')");
     assertSideEffect(true, "new RegExp(SomethingWacky(), 'i')");
     assertSideEffect(false, "new Array()");
@@ -764,7 +764,24 @@ public void testMayHaveSideEffects() {
   }
 
   @Test
-  public void testComputedPropSideEffects() {
+  public void testMayHaveSideEffects_contextSwitch() {
+    assertSideEffect(true, parseFirst(AWAIT, "async function f() { await 0; }"));
+    assertSideEffect(true, parseFirst(FOR_AWAIT_OF, "(async()=>{ for await (let x of []) {} })"));
+    assertSideEffect(true, parseFirst(THROW, "function f() { throw 'something'; }"));
+    assertSideEffect(true, parseFirst(YIELD, "function* f() { yield 'something'; }"));
+    assertSideEffect(true, parseFirst(YIELD, "function* f() { yield* 'something'; }"));
+  }
+
+  @Test
+  public void testMayHaveSideEffects_enhancedForLoop() {
+    // These edge cases are actually side-effect free. We include them to confirm we just give up on
+    // enhanced for loops.
+    assertSideEffect(true, "for (const x in []) { }");
+    assertSideEffect(true, "for (const x of []) { }");
+  }
+
+  @Test
+  public void testMayHaveSideEffects_computedProp() {
     assertSideEffect(false, "({[a]: x})");
     assertSideEffect(true, "({[a()]: x})");
     assertSideEffect(true, "({[a]: x()})");
@@ -792,7 +809,7 @@ public void testComputedPropSideEffects() {
   }
 
   @Test
-  public void testObjectMethodSideEffects() {
+  public void testMayHaveSideEffects_objectMethod() {
     // "toString" and "valueOf" are assumed to be side-effect free
     assertSideEffect(false, "o.toString()");
     assertSideEffect(false, "o.valueOf()");
@@ -802,7 +819,7 @@ public void testObjectMethodSideEffects() {
   }
 
   @Test
-  public void testRegExpSideEffect() {
+  public void testMayHaveSideEffects_regExp() {
     // A RegExp Object by itself doesn't have any side-effects
     assertSideEffect(false, "/abc/gi", true);
     assertSideEffect(false, "/abc/gi", false);
@@ -847,10 +864,7 @@ public void testRegExpSideEffect() {
     // in combination with a RegExp instance "exec" method.
     assertSideEffect(true, "''.match(a)", true);
     assertSideEffect(true, "''.match(a)", false);
-  }
 
-  @Test
-  public void testRegExpSideEffect2() {
     assertSideEffect(true, "'a'.replace(/a/, function (s) {alert(s)})", false);
     assertSideEffect(false, "'a'.replace(/a/, 'x')", false);
   }