Fix Es6SyntacticScopeCreator to not generate duplicate declarations

across function parameter scope and function block scope

Before: we generate two declarations of "x" for "function f(x) { var x; }".
Fix it to only generate the param declaration.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=98523125

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

@@ -2107,7 +2107,7 @@ static boolean createsBlockScope(Node n) {
           return false;
         }
         Node child = n.getFirstChild();
-        return child != null && !child.isScript();
+        return child != null && !child.isScript(); // Optimization for empty blocks
       case Token.FOR:
       case Token.FOR_OF:
         return true;

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

@@ -647,7 +647,7 @@ boolean isDeclaration() {
     private static boolean isDeclarationHelper(Node node) {
       Node parent = node.getParent();
 
-      // Special case for class B extends A, A is not a redeclaration.
+      // Special case for class B extends A, A is not a declaration.
       if (parent.isClass() && node != parent.getFirstChild()) {
         return false;
       }
@@ -672,6 +672,11 @@ private static boolean isDeclarationHelper(Node node) {
         return isDeclarationHelper(parent);
       }
 
+      // Special case for arrow function
+      if (parent.isArrowFunction()) {
+        return node == parent.getFirstChild();
+      }
+
       return DECLARATION_PARENTS.contains(parent.getType());
     }
 
@@ -687,6 +692,10 @@ boolean isConstDeclaration() {
       return getParent().isConst();
     }
 
+    boolean isClassDeclaration() {
+      return getParent().isClass() && getNode() == getParent().getFirstChild();
+    }
+
     boolean isHoistedFunction() {
       return NodeUtil.isHoistedFunctionDeclaration(getParent());
     }

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

@@ -173,14 +173,30 @@ public boolean isDeclared(String name, boolean recurse) {
       if (scope.vars.containsKey(name)) {
         return true;
       }
-      if (scope.parent != null && recurse) {
+
+      // In ES6, we create a separate "function parameter scope" above the function block scope to
+      // handle default parameters. Since nothing in the function block scope is allowed to shadow
+      // the variables in the function scope, we treat the two scopes as one in this method.
+      if (scope.isFunctionBlockScope() || (scope.parent != null && recurse)) {
         scope = scope.parent;
         continue;
       }
       return false;
     }
   }
 
+  public boolean isDeclaredInFunction(String name) {
+    if (vars.containsKey(name)) {
+      return true;
+    }
+    Scope parent = getParent();
+    if (parent != null && parent.getRootNode().isFunction()
+        && parent.isDeclared(name, false)) {
+      return true;
+    }
+    return false;
+  }
+
   /**
    * Return an iterator over all of the variables declared in this scope.
    */

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

@@ -185,10 +185,20 @@ boolean isConst() {
     return declarationType() == Token.CONST;
   }
 
+  boolean isClass() {
+    return declarationType() == Token.CLASS;
+  }
+
   boolean isParam() {
     return declarationType() == Token.PARAM_LIST;
   }
 
+  boolean isDefaultParam() {
+    Node parent = nameNode.getParent();
+    return parent.getParent().isParamList() && parent.isDefaultValue()
+        && parent.getFirstChild() == nameNode;
+  }
+
   private int declarationType() {
     final Set<Integer> types = ImmutableSet.of(
         Token.VAR,

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

@@ -115,10 +115,10 @@ public void afterExitScope(NodeTraversal t, ReferenceMap referenceMap) {
         ReferenceCollection referenceCollection = referenceMap.getReferences(v);
         // TODO(moz): Figure out why this could be null
         if (referenceCollection != null) {
-          if (scope.getRootNode().isFunction() && v.getParentNode().isDefaultValue()
-              && v.getParentNode().getFirstChild() == v.getNode()) {
+          if (scope.getRootNode().isFunction() && v.isDefaultParam()) {
             checkDefaultParam(v, scope);
-          } else if (scope.isFunctionBlockScope()) {
+          }
+          if (scope.getRootNode().isFunction()) {
             checkShadowParam(v, scope, referenceCollection.references);
           }
           checkVar(v, referenceCollection.references);
@@ -156,23 +156,22 @@ private void checkDefaultParam(Var v, Scope scope) {
       }
     }
 
-    private void checkShadowParam(Var v, Scope scope, List<Reference> references) {
-      Scope functionScope = scope.getParent();
+    private void checkShadowParam(Var v, Scope functionScope, List<Reference> references) {
       Var maybeParam = functionScope.getVar(v.getName());
       if (maybeParam != null && maybeParam.isParam()
           && maybeParam.getScope() == functionScope) {
         for (Reference r : references) {
-          if (!r.isDeclaration() || r.getScope() != scope) {
-            continue;
+          if (r.isDeclaration() && !r.getNode().equals(v.getNameNode())) {
+            if (!compiler.getLanguageMode().isEs6OrHigher() && r.getParent().isCatch()) {
+              continue;
+            }
+            compiler.report(
+                JSError.make(
+                    r.getNode(),
+                    (r.isVarDeclaration() || r.isHoistedFunction())
+                    ? REDECLARED_VARIABLE
+                    : PARAMETER_SHADOWED_ERROR, v.name));
           }
-          compiler.report(
-              JSError.make(
-                  r.getNode(),
-                  (r.isVarDeclaration() || r.isHoistedFunction())
-                      && !(maybeParam.getNode().getParent().isDefaultValue()
-                          || maybeParam.getNode().isRest())
-                  ? REDECLARED_VARIABLE
-                  : PARAMETER_SHADOWED_ERROR, v.name));
         }
       }
     }
@@ -231,9 +230,11 @@ private void checkVar(Var v, List<Reference> references) {
               // better yet, make sure the generated code never violates
               // the requirement to pass aggressive var check!
               DiagnosticType diagnosticType;
-              if (v.isLet() || v.isConst()
+              if (v.isLet() || v.isConst() || v.isClass()
                   || letConstShadowsVar || shadowCatchVar) {
                 diagnosticType = REDECLARED_VARIABLE_ERROR;
+              } else if (reference.getNode().getParent().isCatch()) {
+                return;
               } else {
                 diagnosticType = REDECLARED_VARIABLE;
               }
@@ -286,7 +287,7 @@ private void checkVar(Var v, List<Reference> references) {
               isUndeclaredReference = true;
               compiler.report(
                   JSError.make(reference.getNode(),
-                               (v.isLet() || v.isConst() || v.isParam())
+                               (v.isLet() || v.isConst() || v.isClass() || v.isParam())
                                    ? EARLY_REFERENCE_ERROR
                                    : EARLY_REFERENCE, v.name));
             }

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

@@ -201,12 +201,7 @@ public void testObjectDestructuringComputedPropParam() {
 
     Node functionNode = root.getFirstChild();
     Scope functionScope = scopeCreator.createScope(functionNode, globalScope);
-
-    Node functionBlock = functionNode.getLastChild();
-    Scope functionBlockScope = scopeCreator.createScope(functionBlock, functionScope);
-
     assertTrue(functionScope.isDeclared("a", false));
-    assertFalse(functionBlockScope.isDeclared("a", false));
   }
 
   public void testObjectDestructuringNested() {
@@ -376,7 +371,7 @@ public void testFunctionArgument() {
 
     Node ifBlock = functionBlock.getLastChild().getLastChild().getLastChild();
     Scope blockScope = scopeCreator.createScope(ifBlock, functionScope);
-    assertFalse(blockScope.isDeclared("x", false));
+    assertTrue(blockScope.isDeclared("x", false));
     assertTrue(blockScope.isDeclared("y", false));
   }
 
@@ -454,4 +449,52 @@ public void testVarAfterLet() {
     assertTrue(ifBlockScope.isDeclared("x", false));
     assertFalse(ifBlockScope.isDeclared("y", false));
   }
+
+  public void testSimpleFunctionParam() {
+    String js = "function f(x) {}";
+    Node root = getRoot(js);
+    Node fNode = root.getFirstChild();
+
+    Scope globalScope = scopeCreator.createScope(root, null);
+    Scope fScope = scopeCreator.createScope(fNode, globalScope);
+    assertTrue(fScope.isDeclared("x", false));
+
+    Node fBlock = fNode.getLastChild();
+    Scope fBlockScope = scopeCreator.createScope(fBlock, null);
+    assertFalse(fBlockScope.isDeclared("x", false));
+  }
+
+  public void testOnlyOneDeclaration() {
+    String js = "function f(x) { if (!x) var x = 6; }";
+    Node root = getRoot(js);
+    Node fNode = root.getFirstChild();
+    Scope globalScope = scopeCreator.createScope(root, null);
+    Scope fScope = scopeCreator.createScope(fNode, globalScope);
+    assertTrue(fScope.isDeclared("x", false));
+
+    Node fBlock = fNode.getLastChild();
+    Scope fBlockScope = scopeCreator.createScope(fBlock, fScope);
+    assertTrue(fBlockScope.isDeclared("x", false));
+
+    Node ifBlock = fBlock.getFirstChild().getLastChild();
+    Scope ifBlockScope = scopeCreator.createScope(ifBlock, fBlockScope);
+    assertFalse(ifBlockScope.isDeclared("x", false));
+  }
+
+  public void testCatchInFunction() {
+    String js = "function f(e) { try {} catch (e) {} }";
+    Node root = getRoot(js);
+    Node fNode = root.getFirstChild();
+    Scope globalScope = scopeCreator.createScope(root, null);
+    Scope fScope = scopeCreator.createScope(fNode, globalScope);
+    assertTrue(fScope.isDeclared("e", false));
+
+    Node fBlock = fNode.getLastChild();
+    Scope fBlockScope = scopeCreator.createScope(fBlock, fScope);
+    Node tryBlock = fBlock.getFirstChild().getFirstChild();
+    Scope tryScope = scopeCreator.createScope(tryBlock, fBlockScope);
+    Node catchBlock = tryBlock.getNext();
+    Scope catchScope = scopeCreator.createScope(catchBlock, tryScope);
+    assertTrue(catchScope.isDeclared("e", false));
+  }
 }

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

@@ -25,9 +25,6 @@
  */
 public final class Es6VariableReferenceCheckTest extends CompilerTestCase {
 
-  private static final String VARIABLE_RUN =
-      "var a = 1; var b = 2; var c = a + b, d = c;";
-
   private static final String LET_RUN =
       "let a = 1; let b = 2; let c = a + b, d = c;";
 
@@ -38,18 +35,13 @@ public CompilerPass getProcessor(Compiler compiler) {
 
   @Override
   public void setUp() throws Exception {
+    super.setUp();
     setAcceptedLanguage(LanguageMode.ECMASCRIPT6);
   }
 
   public void testCorrectCode() {
-    assertNoWarning("function foo(d) { (function() { d.foo(); }); d.bar(); } ");
-    assertNoWarning("function foo() { bar(); } function bar() { foo(); } ");
-    assertNoWarning("function f(d) { d = 3; }");
-    assertNoWarning(VARIABLE_RUN);
     assertNoWarning(LET_RUN);
-    assertNoWarning("function f() { " + VARIABLE_RUN + "}");
     assertNoWarning("function f() { " + LET_RUN + "}");
-    assertNoWarning("if (a) { var x; }");
     assertNoWarning("try { let e; } catch (e) { let x; }");
   }
 
@@ -104,14 +96,15 @@ public void testDuplicateLetConst() {
     assertRedeclareError("const x = 0, x = 0;");
   }
 
-  public void testIllegalLetConstEarlyReference() {
+  public void testIllegalBlockScopedEarlyReference() {
     assertEarlyReferenceError("let x = x");
     assertEarlyReferenceError("const x = x");
     assertEarlyReferenceError("let x = x || 0");
     assertEarlyReferenceError("const x = x || 0");
     // In the following cases, "x" might not be reachable but we warn anyways
     assertEarlyReferenceError("let x = expr || x");
     assertEarlyReferenceError("const x = expr || x");
+    assertEarlyReferenceError("X; class X {};");
   }
 
   public void testCorrectEarlyReference() {
@@ -156,20 +149,27 @@ public void testVarShadowing() {
   public void testParameterShadowing() {
     assertParameterShadowed("function f(x) { let x; }");
     assertParameterShadowed("function f(x) { const x = 3; }");
-    assertRedeclare("function f(x) { function x() {} }");
     assertParameterShadowed("function f(X) { class X {} }");
+
+    assertRedeclare("function f(x) { function x() {} }");
     assertRedeclare("function f(x) { var x; }");
-    assertParameterShadowed("function f(x=3) { var x; }");
-    assertParameterShadowed("function f(...x) { var x; }");
-    assertParameterShadowed("function f(x=3) { function x() {} }");
-    assertParameterShadowed("function f(...x) { function x() {} }");
+    assertRedeclare("function f(x=3) { var x; }");
+    assertRedeclare("function f(...x) { var x; }");
+    assertRedeclare("function f(...x) { function x() {} }");
+    assertRedeclare("function f(x=3) { function x() {} }");
     assertNoWarning("function f(x) { if (true) { let x; } }");
-    assertNoWarning(
-        LINE_JOINER.join(
-            "function outer(x) {", "  function inner() {", "    let x = 1;", "  }", "}"));
-    assertNoWarning(
-        LINE_JOINER.join(
-            "function outer(x) {", "  function inner() {", "    var x = 1;", "  }", "}"));
+    assertNoWarning(LINE_JOINER.join(
+        "function outer(x) {",
+        "  function inner() {",
+        "    let x = 1;",
+        "  }",
+        "}"));
+    assertNoWarning(LINE_JOINER.join(
+        "function outer(x) {",
+        "  function inner() {",
+        "    var x = 1;",
+        "  }",
+        "}"));
   }
 
   public void testReassignedConst() {