Detect Feature.BLOCK_SCOPED_FUNCTION_DECLARATIONS correctly and set i…

…t as an ES6 feature.

This change has two main consequences:
1. Some code that was previously considered to be ES5 will now be marked as ES6, resulting in transpilation occurring in test runners, bundlers, and debug loaders.  If LANGUAGE_IN is explicitly set to ES5, or if ES6 externs are missing, it's possible that builds could break.
2. A possible behavior change for ambiguous block-scoped declarations.  The code `function f(){return 1;} {function f(){return 2;}} console.log(f());` will print '2' in sloppy mode, but '1' in ES6 strict mode (and is an error in ES5 strict mode).  Depending on a variety of other factors, the compiler may output ES5 code printing either result; this change will make it always obey the strict-mode behavior and print '1'.

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

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

@@ -23,7 +23,6 @@
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Table;
 import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
-import com.google.javascript.jscomp.Normalize.NormalizeStatements;
 import com.google.javascript.jscomp.parsing.parser.FeatureSet;
 import com.google.javascript.jscomp.parsing.parser.FeatureSet.Feature;
 import com.google.javascript.rhino.IR;
@@ -124,25 +123,6 @@ public void process(Node externs, Node root) {
     NodeTraversal.traverseEs6(compiler, root, transformer);
     transformer.transformLoopClosure();
     rewriteDeclsToVars();
-
-    // Block scoped function declarations can occur in any language mode, however for
-    // transpilation to ES3 and ES5, we want to hoist the functions from the block-scope by
-    // redeclaring them in var assignments
-    //
-    // If block-scope-declared function is the only "ES6 feature" for which we want to transpile,
-    // then the transpilation process will not rewriteFunctions. Thus, we manually check whether
-    // we need to rewrite in that case.
-    if (compiler.getOptions().needsTranspilationFrom(FeatureSet.ES6)) {
-      RewriteBlockScopedFunctionDeclaration rewriteFunction =
-          new RewriteBlockScopedFunctionDeclaration();
-
-      for (Node singleRoot : root.children()) {
-        FeatureSet features = (FeatureSet) singleRoot.getProp(Node.FEATURE_SET);
-        if (features.has(Feature.BLOCK_SCOPED_FUNCTION_DECLARATION)) {
-          NodeTraversal.traverseEs6(compiler, singleRoot, rewriteFunction);
-        }
-      }
-    }
   }
 
   @Override
@@ -154,7 +134,6 @@ public void hotSwapScript(Node scriptRoot, Node originalRoot) {
     NodeTraversal.traverseEs6(compiler, scriptRoot, transformer);
     transformer.transformLoopClosure();
     rewriteDeclsToVars();
-    NodeTraversal.traverseEs6(compiler, scriptRoot, new RewriteBlockScopedFunctionDeclaration());
   }
 
   /**
@@ -230,15 +209,6 @@ private void rewriteDeclsToVars() {
     }
   }
 
-  private class RewriteBlockScopedFunctionDeclaration extends AbstractPostOrderCallback {
-    @Override
-    public void visit(NodeTraversal t, Node n, Node parent) {
-      if (n.isFunction()) {
-        NormalizeStatements.visitFunction(n, compiler);
-      }
-    }
-  }
-
   /**
    * Records undeclared names and aggressively rename possible references to them.
    * Eg: In "{ let inner; } use(inner);", we rename the let declared variable.

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

@@ -0,0 +1,100 @@
+/*
+ * Copyright 2017 The Closure Compiler Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.javascript.jscomp;
+
+import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
+import com.google.javascript.jscomp.parsing.parser.FeatureSet;
+import com.google.javascript.jscomp.parsing.parser.FeatureSet.Feature;
+import com.google.javascript.rhino.IR;
+import com.google.javascript.rhino.Node;
+import com.google.javascript.rhino.Token;
+
+/**
+ * Rewrite block-scoped function declarations as "let"s.  This pass must happen before
+ * Es6RewriteBlockScopedDeclaration, which rewrites "let" to "var".
+ */
+public final class Es6RewriteBlockScopedFunctionDeclaration extends AbstractPostOrderCallback
+    implements HotSwapCompilerPass {
+
+  private final AbstractCompiler compiler;
+  private static final FeatureSet transpiledFeatures =
+      FeatureSet.BARE_MINIMUM.with(Feature.BLOCK_SCOPED_FUNCTION_DECLARATION);
+
+  public Es6RewriteBlockScopedFunctionDeclaration(AbstractCompiler compiler) {
+    this.compiler = compiler;
+  }
+
+  @Override
+  public void process(Node externs, Node root) {
+    TranspilationPasses.processTranspile(compiler, externs, transpiledFeatures, this);
+    TranspilationPasses.processTranspile(compiler, root, transpiledFeatures, this);
+  }
+
+  @Override
+  public void hotSwapScript(Node scriptRoot, Node originalRoot) {
+    TranspilationPasses.hotSwapTranspile(compiler, scriptRoot, transpiledFeatures, this);
+  }
+
+  @Override
+  public void visit(NodeTraversal t, Node n, Node parent) {
+    if (n.isFunction()
+        && parent != null
+        && parent.isNormalBlock()
+        && !parent.getParent().isFunction()) {
+      // Only consider declarations (all expressions have non-block parents) that are not directly
+      // within a function or top-level.
+      visitBlockScopedFunctionDeclaration(n, parent);
+    }
+  }
+
+  /**
+   * Rewrite the function declaration from:
+   * <pre>
+   *   function f() {}
+   *   FUNCTION
+   *     NAME x
+   *     PARAM_LIST
+   *     BLOCK
+   * </pre> to <pre>
+   *   let f = function() {};
+   *   LET
+   *     NAME f
+   *       FUNCTION
+   *         NAME (w/ empty string)
+   *         PARAM_LIST
+   *         BLOCK
+   * </pre>
+   * This is similar to {@link Normalize.NormalizeStatements#rewriteFunctionDeclaration} but
+   * rewrites to "let" instead of "var".
+   */
+  private void visitBlockScopedFunctionDeclaration(Node n, Node parent) {
+    // Prepare a spot for the function.
+    Node oldNameNode = n.getFirstChild();
+    Node fnNameNode = oldNameNode.cloneNode();
+    Node let = IR.declaration(fnNameNode, Token.LET).srcref(n);
+
+    // Prepare the function.
+    oldNameNode.setString("");
+    compiler.reportChangeToEnclosingScope(oldNameNode);
+
+    // Move the function to the front of the parent.
+    parent.removeChild(n);
+    parent.addChildToFront(let);
+    compiler.reportChangeToEnclosingScope(let);
+    fnNameNode.addChildToFront(n);
+  }
+}

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

@@ -67,6 +67,7 @@ public static void addEs6LatePasses(List<PassFactory> passes) {
     passes.add(es6RewriteClass);
     passes.add(earlyConvertEs6ToEs3);
     passes.add(lateConvertEs6ToEs3);
+    passes.add(rewriteBlockScopedFunctionDeclaration);
     passes.add(rewriteBlockScopedDeclaration);
     passes.add(rewriteGenerators);
   }
@@ -84,6 +85,7 @@ public static void addEs6PassesBeforeNTI(List<PassFactory> passes) {
     passes.add(es6ExtractClasses);
     passes.add(es6RewriteClass);
     passes.add(earlyConvertEs6ToEs3);
+    passes.add(rewriteBlockScopedFunctionDeclaration);
     passes.add(rewriteBlockScopedDeclaration);
   }
 
@@ -320,6 +322,19 @@ protected FeatureSet featureSet() {
     }
   };
 
+  static final HotSwapPassFactory rewriteBlockScopedFunctionDeclaration =
+      new HotSwapPassFactory("Es6RewriteBlockScopedFunctionDeclaration") {
+    @Override
+    protected HotSwapCompilerPass create(final AbstractCompiler compiler) {
+      return new Es6RewriteBlockScopedFunctionDeclaration(compiler);
+    }
+
+    @Override
+    protected FeatureSet featureSet() {
+      return ES8;
+    }
+  };
+
   static final HotSwapPassFactory rewriteBlockScopedDeclaration =
       new HotSwapPassFactory("Es6RewriteBlockScopedDeclaration") {
     @Override

src/com/google/javascript/jscomp/parsing/IRFactory.java

@@ -420,6 +420,7 @@ private void validate(Node n) {
     validateReturn(n);
     validateNewDotTarget(n);
     validateLabel(n);
+    validateBlockScopedFunctions(n);
   }
 
   private void validateReturn(Node n) {
@@ -572,6 +573,12 @@ private void validateParameters(Node n) {
     }
   }
 
+  private void validateBlockScopedFunctions(Node n) {
+    if (n.isFunction() && n.getParent().isNormalBlock() && !n.getGrandparent().isFunction()) {
+      maybeWarnForFeature(n, Feature.BLOCK_SCOPED_FUNCTION_DECLARATION);
+    }
+  }
+
   JSDocInfo recordJsDoc(SourceRange location, JSDocInfo info) {
     if (info != null && info.hasTypeInformation()) {
       hasJsDocTypeAnnotations = true;
@@ -880,6 +887,19 @@ void maybeWarnForFeature(
     }
   }
 
+  void maybeWarnForFeature(Node node, Feature feature) {
+    features = features.with(feature);
+    if (!isSupportedForInputLanguageMode(feature)) {
+      errorReporter.warning(
+          "this language feature is only supported for "
+              + LanguageMode.minimumRequiredFor(feature)
+              + " mode or better: "
+              + feature,
+          sourceName,
+          node.getLineno(), node.getCharno());
+    }
+  }
+
   void setSourceInfo(Node node, Node ref) {
     node.setLineno(ref.getLineno());
     node.setCharno(ref.getCharno());
@@ -1302,12 +1322,6 @@ Node processFunction(FunctionDeclarationTree functionTree) {
         maybeWarnForFeature(functionTree, Feature.ASYNC_FUNCTIONS);
       }
 
-      // Add a feature so that transpilation process hoists block scoped functions through
-      // var redeclaration in ES3 and ES5
-      if (isDeclaration) {
-        features = features.with(Feature.BLOCK_SCOPED_FUNCTION_DECLARATION);
-      }
-
       IdentifierToken name = functionTree.name;
       Node newName;
       if (name != null) {

src/com/google/javascript/jscomp/parsing/parser/FeatureSet.java

@@ -104,15 +104,6 @@ private Set<Feature> features() {
 
   /** Specific features that can be included in a FeatureSet. */
   public enum Feature {
-    // ES3 features
-    // Functions can be declared in the block scope for all ES versions. However, for ES3 and ES5,
-    // we want to hoist the functions from the block scope by redeclaring them as vars (i.e.
-    // { function f() {} } becomes { var f = function {} }. To prevent this feature from causing
-    // code to run additional transpilation passes beyond rewriting block scoped functions, we mark
-    // block scoped functions as an ES3 feature and then manually determine whether to rewrite
-    // functions inside the processTranspile method of TranspilationPasses.java
-    BLOCK_SCOPED_FUNCTION_DECLARATION("block function", LangVersion.ES3),
-
     // ES5 features
     ES3_KEYWORDS_AS_IDENTIFIERS("ES3 keywords as identifiers", LangVersion.ES5),
     GETTER("getters", LangVersion.ES5),
@@ -125,6 +116,7 @@ public enum Feature {
     ARRAY_PATTERN_REST("array pattern rest", LangVersion.ES6),
     ARROW_FUNCTIONS("arrow function", LangVersion.ES6),
     BINARY_LITERALS("binary literal", LangVersion.ES6),
+    BLOCK_SCOPED_FUNCTION_DECLARATION("block-scoped function declaration", LangVersion.ES6),
     CLASSES("class", LangVersion.ES6),
     COMPUTED_PROPERTIES("computed property", LangVersion.ES6),
     CONST_DECLARATIONS("const declaration", LangVersion.ES6),

src/com/google/javascript/jscomp/parsing/parser/testing/FeatureSetSubject.java

@@ -29,6 +29,7 @@
  *   import static com.google.javascript.jscomp.parsing.parser.testing.FeatureSetSubject.assertFS;
  *   ...
  *   assertFS(features).contains(otherFeatures);
+ *   assertFS(features).containsNoneOf(otherFeatures);
  * </pre>
  */
 public class FeatureSetSubject extends Subject<FeatureSetSubject, FeatureSet> {
@@ -50,6 +51,13 @@ public void contains(FeatureSet other) {
     }
   }
 
+  public void containsNoneOf(FeatureSet other) {
+    if (!other.without(actual()).equals(other)) {
+      failWithRawMessage("Expected a FeatureSet containing none of: %s\nBut got: %s",
+          other, actual());
+    }
+  }
+
   public void has(Feature feature) {
     if (!actual().has(feature)) {
       failWithRawMessage("Expected a FeatureSet that has: %s\nBut got: %s", feature, actual());