Repository: google/closure-compiler (java)

Issue:
Internal error with let inside while loop
This simple code:
```javascript
while (true) {
  let x = null;
  function f() {
    x();
  }
}
```

Gives the following trace (cmd line args: `--language_in=ECMASCRIPT6_STRICT -O SIMPLE`):
```
INTERNAL COMPILER ERROR.
Please report this problem.

Invalid attempt to remove node: NAME $jscomp$loop$0 3 [source_file: bug_closure.js] of CALL 3 [free_call: 1] [source_file: bug_closure.js]
  Node(NAME $jscomp$loop$0): bug_closure.js:3:2
  function f() {
  Parent(CALL): bug_closure.js:3:2
  function f() {

	at com.google.javascript.jscomp.NodeUtil.removeChild(NodeUtil.java:2293)
	at com.google.javascript.jscomp.UnreachableCodeElimination$EliminationPass.removeNode(UnreachableCodeElimination.java:261)
	at com.google.javascript.jscomp.UnreachableCodeElimination$EliminationPass.removeDeadExprStatementSafely(UnreachableCodeElimination.java:251)
	at com.google.javascript.jscomp.UnreachableCodeElimination$EliminationPass.visit(UnreachableCodeElimination.java:104)
	at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:609)
	at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:603)
	at com.google.javascript.jscomp.NodeTraversal.traverseBlockScope(NodeTraversal.java:652)
	at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:597)
	at com.google.javascript.jscomp.NodeTraversal.traverseBlockScope(NodeTraversal.java:652)
	at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:597)
	at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:603)
	at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:603)
	at com.google.javascript.jscomp.NodeTraversal.traverse(NodeTraversal.java:295)
	at com.google.javascript.jscomp.NodeTraversal.traverseEs6(NodeTraversal.java:560)
	at com.google.javascript.jscomp.UnreachableCodeElimination$1.enterFunction(UnreachableCodeElimination.java:81)
```

Note that it works fine with `var` instead of `let`.

Pull request description:
Fix handling of function declarations in loops with let/consts
Convert function declaration to variable declaration so that the IIFE can still be applied.

Unmark calls to captured functions like $jscomp$loop.x() as FREE_CALL.

Fixes #1124 

Patch:
diff --git a/contrib/externs/angular-1.2-q.js b/contrib/externs/angular-1.2-q.js
index a1b6d5432..07473ee2f 100644
--- a/contrib/externs/angular-1.2-q.js
+++ b/contrib/externs/angular-1.2-q.js
@@ -48,10 +48,10 @@ angular.$q.all = function(promises) {};
 angular.$q.defer = function() {};
 
 /**
- * @param {*=} opt_reason
+ * @param {*} reason
  * @return {!angular.$q.Promise}
  */
-angular.$q.reject = function(opt_reason) {};
+angular.$q.reject = function(reason) {};
 
 /**
  * @param {*} value
diff --git a/contrib/externs/angular-1.2-q_templated.js b/contrib/externs/angular-1.2-q_templated.js
index 5b83475cb..fbf36af50 100644
--- a/contrib/externs/angular-1.2-q_templated.js
+++ b/contrib/externs/angular-1.2-q_templated.js
@@ -93,10 +93,10 @@ angular.$q.prototype.all = function(promises) {};
 angular.$q.prototype.defer = function() {};
 
 /**
- * @param {*=} opt_reason
+ * @param {*} reason
  * @return {!angular.$q.Promise}
  */
-angular.$q.prototype.reject = function(opt_reason) {};
+angular.$q.prototype.reject = function(reason) {};
 
 /**
  * @param {RESULT} value
diff --git a/contrib/externs/angular-1.3-q.js b/contrib/externs/angular-1.3-q.js
index 498677646..e7e83b074 100644
--- a/contrib/externs/angular-1.3-q.js
+++ b/contrib/externs/angular-1.3-q.js
@@ -47,10 +47,10 @@ angular.$q.all = function(promises) {};
 angular.$q.defer = function() {};
 
 /**
- * @param {*=} opt_reason
+ * @param {*} reason
  * @return {!angular.$q.Promise}
  */
-angular.$q.reject = function(opt_reason) {};
+angular.$q.reject = function(reason) {};
 
 /**
  * @param {*} value
diff --git a/contrib/externs/angular-1.3-q_templated.js b/contrib/externs/angular-1.3-q_templated.js
index acf8aa3dd..711ada5da 100644
--- a/contrib/externs/angular-1.3-q_templated.js
+++ b/contrib/externs/angular-1.3-q_templated.js
@@ -137,10 +137,10 @@ angular.$q.prototype.all = function(promises) {};
 angular.$q.prototype.defer = function() {};
 
 /**
- * @param {*=} opt_reason
+ * @param {*} reason
  * @return {!angular.$q.Promise}
  */
-angular.$q.prototype.reject = function(opt_reason) {};
+angular.$q.prototype.reject = function(reason) {};
 
 /**
  * @param {RESULT} value
diff --git a/contrib/externs/angular-1.4-q_templated.js b/contrib/externs/angular-1.4-q_templated.js
index 513b8feaf..6894241fa 100644
--- a/contrib/externs/angular-1.4-q_templated.js
+++ b/contrib/externs/angular-1.4-q_templated.js
@@ -135,10 +135,10 @@ angular.$q.prototype.all = function(promises) {};
 angular.$q.prototype.defer = function() {};
 
 /**
- * @param {*=} opt_reason
+ * @param {*} reason
  * @return {!angular.$q.Promise}
  */
-angular.$q.prototype.reject = function(opt_reason) {};
+angular.$q.prototype.reject = function(reason) {};
 
 /**
  * @param {RESULT} value
diff --git a/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java b/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java
index a94208f6c..6ac258637 100644
--- a/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java
+++ b/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java
@@ -342,6 +342,10 @@ public final class Es6RewriteBlockScopedDeclaration extends AbstractPostOrderCal
                 }
               }
 
+              if (reference.getParent().isCall()
+                  && reference.getParent().getFirstChild() == reference) {
+                reference.getParent().putBooleanProp(Node.FREE_CALL, false);
+              }
               // Change reference to GETPROP.
               reference.getParent().replaceChild(
                   reference,
@@ -366,11 +370,19 @@ public final class Es6RewriteBlockScopedDeclaration extends AbstractPostOrderCal
         }
 
         Node iife = IR.function(
-            IR.name(""), IR.paramList(objectNames), IR.block(returnNode));
+            IR.name(""),
+            IR.paramList(objectNames),
+            IR.block(returnNode));
         Node call = IR.call(iife, objectNamesForCall);
         call.putBooleanProp(Node.FREE_CALL, true);
-        function.getParent().replaceChild(
-            function, call.useSourceInfoIfMissingFromForTree(function));
+        Node replacement;
+        if (NodeUtil.isFunctionDeclaration(function)) {
+          replacement = IR.var(IR.name(function.getFirstChild().getString()), call)
+              .useSourceInfoIfMissingFromForTree(function);
+        } else {
+          replacement = call.useSourceInfoIfMissingFromForTree(function);
+        }
+        function.getParent().replaceChild(function, replacement);
         returnNode.addChildToFront(function);
       }
     }
diff --git a/src/com/google/javascript/jscomp/SourceFile.java b/src/com/google/javascript/jscomp/SourceFile.java
index 600228642..e36ead749 100644
--- a/src/com/google/javascript/jscomp/SourceFile.java
+++ b/src/com/google/javascript/jscomp/SourceFile.java
@@ -339,11 +339,7 @@ public class SourceFile implements StaticSourceFile, Serializable {
       while (zipEntries.hasMoreElements()) {
         ZipEntry zipEntry = zipEntries.nextElement();
         URL zipEntryUrl = new URL("jar:file:" + absoluteZipPath + "!/" + zipEntry.getName());
-        sourceFiles.add(
-            builder()
-                .withCharset(inputCharset)
-                .withOriginalPath(zipName + "!/" + zipEntry.getName())
-                .buildFromUrl(zipEntryUrl));
+        sourceFiles.add(builder().withCharset(inputCharset).buildFromUrl(zipEntryUrl));
       }
     }
     return sourceFiles;
@@ -412,7 +408,6 @@ public class SourceFile implements StaticSourceFile, Serializable {
    */
   public static class Builder {
     private Charset charset = UTF_8;
-    private String originalPath = null;
 
     public Builder() {}
 
@@ -422,26 +417,21 @@ public class SourceFile implements StaticSourceFile, Serializable {
       return this;
     }
 
-    public Builder withOriginalPath(String originalPath) {
-      this.originalPath = originalPath;
-      return this;
-    }
-
     public SourceFile buildFromFile(String fileName) {
       return buildFromFile(new File(fileName));
     }
 
     public SourceFile buildFromFile(File file) {
-      return new OnDisk(file, originalPath, charset);
+      return new OnDisk(file, null /** originalPath */, charset);
     }
 
     @GwtIncompatible("java.net.URL")
     public SourceFile buildFromUrl(URL url) {
-      return new AtUrl(url, originalPath, charset);
+      return new AtUrl(url, null /** originalPath */, charset);
     }
 
     public SourceFile buildFromCode(String fileName, String code) {
-      return new Preloaded(fileName, originalPath, code);
+      return new Preloaded(fileName, null /** originalPath */, code);
     }
 
     @GwtIncompatible("java.io.InputStream")
@@ -459,7 +449,7 @@ public class SourceFile implements StaticSourceFile, Serializable {
 
     public SourceFile buildFromGenerator(String fileName,
         Generator generator) {
-      return new Generated(fileName, originalPath, generator);
+      return new Generated(fileName, null /** originalPath */, generator);
     }
   }
 
@@ -622,7 +612,7 @@ public class SourceFile implements StaticSourceFile, Serializable {
     private String inputCharset = UTF_8.name();
 
     AtUrl(URL url, String originalPath, Charset c) {
-      super(originalPath);
+      super(url.getPath());
       this.url = url;
       super.setOriginalPath(originalPath);
       if (c != null) {
diff --git a/src/com/google/javascript/jscomp/deps/DepsGenerator.java b/src/com/google/javascript/jscomp/deps/DepsGenerator.java
index b2c81b0d6..0746e631a 100644
--- a/src/com/google/javascript/jscomp/deps/DepsGenerator.java
+++ b/src/com/google/javascript/jscomp/deps/DepsGenerator.java
@@ -436,15 +436,6 @@ public class DepsGenerator {
     return files;
   }
 
-  static List<SourceFile> createSourceFilesFromZipPaths(
-      Collection<String> paths) throws IOException {
-    List<SourceFile> zipSourceFiles = new ArrayList<>();
-    for (String path : paths) {
-      zipSourceFiles.addAll(SourceFile.fromZipFile(path, UTF_8));
-    }
-    return zipSourceFiles;
-  }
-
   static List<SourceFile> createSourceFilesFromPaths(
       String ... paths) {
     return createSourceFilesFromPaths(Arrays.asList(paths));


Test patch:
diff --git a/test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationTest.java b/test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationTest.java
index 198354701..4bfef1308 100644
--- a/test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationTest.java
+++ b/test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationTest.java
@@ -708,6 +708,66 @@ public final class Es6RewriteBlockScopedDeclarationTest extends CompilerTestCase
             "} while (false);"));
   }
 
+  // https://github.com/google/closure-compiler/issues/1124
+  public void testGithubIssue1124() {
+    test(LINE_JOINER.join(
+        "while (true) {",
+        "  let x = null;",
+        "  var f = function() {",
+        "    x();",
+        "  }",
+        "}"),
+        LINE_JOINER.join(
+        "var $jscomp$loop$0 = {x:undefined};",
+        "while (true) {",
+        "  $jscomp$loop$0.x = null;",
+        "  var f = function($jscomp$loop$0) {",
+        "    return function() {",
+        "      ($jscomp$loop$0.x)();",
+        "    };",
+        "  }($jscomp$loop$0);",
+        "  $jscomp$loop$0 = {x:$jscomp$loop$0.x};",
+        "}"));
+
+    test(LINE_JOINER.join(
+        "while (true) {",
+        "  let x = null;",
+        "  function f() {",
+        "    x();",
+        "  }",
+        "}"),
+        LINE_JOINER.join(
+        "var $jscomp$loop$0 = {x:undefined};",
+        "while (true) {",
+        "  $jscomp$loop$0.x = null;",
+        "  var f = function($jscomp$loop$0) {",
+        "    return function f() {",
+        "      ($jscomp$loop$0.x)();",
+        "    };",
+        "  }($jscomp$loop$0);",
+        "  $jscomp$loop$0 = {x:$jscomp$loop$0.x};",
+        "}"));
+
+    test(LINE_JOINER.join(
+        "while (true) {",
+        "  let x = null;",
+        "  (function() {",
+        "    x();",
+        "  })();",
+        "}"),
+        LINE_JOINER.join(
+        "var $jscomp$loop$0 = {x:undefined};",
+        "while (true) {",
+        "  $jscomp$loop$0.x = null;",
+        "  (function($jscomp$loop$0) {",
+        "    return function () {",
+        "      ($jscomp$loop$0.x)();",
+        "    };",
+        "  })($jscomp$loop$0)();",
+        "  $jscomp$loop$0 = {x:$jscomp$loop$0.x};",
+        "}"));
+  }
+
   public void testDoWhileForOfCapturedLetAnnotated() {
     enableTypeCheck(CheckLevel.WARNING);