From f562c69dab8dd5ee249b625e8d7a9a0468d69553 Mon Sep 17 00:00:00 2001 From: tbreisacher Date: Wed, 6 Sep 2017 15:59:21 -0700 Subject: [PATCH] Slight tweak in how scope variables are handled when a function and one of its parameters have the same name. This will allow us to simplify MakeDeclaredNamesUnique. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=167786746 --- .../jscomp/Es6SyntacticScopeCreator.java | 19 ++++++++++--------- .../jscomp/Es6SyntacticScopeCreatorTest.java | 17 ++++++++--------- .../javascript/jscomp/RenameVarsTest.java | 10 +++++----- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/com/google/javascript/jscomp/Es6SyntacticScopeCreator.java b/src/com/google/javascript/jscomp/Es6SyntacticScopeCreator.java index 265b446768b..2f8fa4d2790 100644 --- a/src/com/google/javascript/jscomp/Es6SyntacticScopeCreator.java +++ b/src/com/google/javascript/jscomp/Es6SyntacticScopeCreator.java @@ -131,16 +131,17 @@ void populate() { final Node fnNameNode = n.getFirstChild(); final Node args = fnNameNode.getNext(); - // Bleed the function name into the scope, if it hasn't - // been declared in the outer scope. + // Args: Declare function variables + checkState(args.isParamList()); + declareLHS(scope, args); + + // Bleed the function name into the scope, if it hasn't been declared in the outer scope + // and the name isn't already in the scope via the param list. String fnName = fnNameNode.getString(); if (!fnName.isEmpty() && NodeUtil.isFunctionExpression(n)) { declareVar(scope, fnNameNode); } - // Args: Declare function variables - checkState(args.isParamList()); - declareLHS(scope, args); // Since we create a separate scope for body, stop scanning here return; } @@ -321,7 +322,7 @@ private void declareVar(Scope s, Node n) { CompilerInput input = compiler.getInput(inputId); if (v != null - || isShadowingDisallowed(name, s) + || !isShadowingAllowed(name, s) || ((s.isFunctionScope() || s.isFunctionBlockScope()) && name.equals(ARGUMENTS))) { redeclarationHandler.onRedeclaration(s, name, n, input); @@ -332,12 +333,12 @@ private void declareVar(Scope s, Node n) { // Function body declarations are not allowed to shadow // function parameters. - private static boolean isShadowingDisallowed(String name, Scope s) { + private static boolean isShadowingAllowed(String name, Scope s) { if (s.isFunctionBlockScope()) { Var maybeParam = s.getParent().getOwnSlot(name); - return maybeParam != null && maybeParam.isParam(); + return maybeParam == null || !maybeParam.isParam(); } - return false; + return true; } } diff --git a/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java b/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java index d1ebc36b36c..ddacf221ff5 100644 --- a/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java @@ -209,9 +209,8 @@ public void testParamAndVarShadowFunctionName() { assertThat(Iterables.transform(functionScope.getVarIterable(), Var::getName)) .containsExactly("g"); - // TODO(tbreisacher): "var g" doesn't declare a new var, so the body scope should have no - // variables in it. - assertThat(Iterables.transform(bodyScope.getVarIterable(), Var::getName)).containsExactly("g"); + // "var g" doesn't declare a new var, so there is no 'g' variable in the function body scope. + assertThat(bodyScope.getVarIterable()).isEmpty(); } public void testVarRedeclaration1_inES6Module() { @@ -1007,9 +1006,9 @@ public void testFunctionNameMatchesParamName1() { assertFalse(fScope.isDeclared("f", false)); assertTrue(fScope.isDeclared("foo", false)); - // The Es6SyntacticScopeCreator considers the function name to be the declaration of 'foo', - // even though a reference to 'foo' inside the function would refer to the parameter foo. - assertNode(fScope.getVar("foo").getNode().getParent()).hasType(Token.FUNCTION); + // The parameter 'foo', not the function name, is the declaration of the variable 'foo' in this + // scope. + assertNode(fScope.getVar("foo").getNode().getParent()).hasType(Token.PARAM_LIST); } public void testFunctionNameMatchesParamName2() { @@ -1024,9 +1023,9 @@ public void testFunctionNameMatchesParamName2() { assertFalse(fScope.isDeclared("f", false)); assertTrue(fScope.isDeclared("foo", false)); - // The Es6SyntacticScopeCreator considers the function name to be the declaration of 'foo', - // even though a reference to 'foo' inside the function would refer to the parameter foo. - assertNode(fScope.getVar("foo").getNode().getParent()).hasType(Token.FUNCTION); + // The parameter 'foo', not the function name, is the declaration of the variable 'foo' in this + // scope. + assertNode(fScope.getVar("foo").getNode().getParent()).hasType(Token.PARAM_LIST); } public void testClassName() { diff --git a/test/com/google/javascript/jscomp/RenameVarsTest.java b/test/com/google/javascript/jscomp/RenameVarsTest.java index 1324e00c731..e676902e8d3 100644 --- a/test/com/google/javascript/jscomp/RenameVarsTest.java +++ b/test/com/google/javascript/jscomp/RenameVarsTest.java @@ -237,8 +237,8 @@ public void testBleedingRecursiveFunctions2() { "}"), LINE_JOINER.join( "function d() {", - " var e = function b(a) { return a ? 1 : b(1); };", - " var f = function a(c) { return c ? 2 : a(2); };", + " var e = function a(b) { return b ? 1 : a(1); };", + " var f = function c(a) { return a ? 2 : c(2); };", "}")); } @@ -252,9 +252,9 @@ public void testBleedingRecursiveFunctions3() { "}"), LINE_JOINER.join( "function f() {", - " var g = function c(a) { return a ? 1 : c(1); };", - " var d = function a(b) { return b ? 2 : a(2); };", - " var h = function b(e) { return e ? d : b(2); };", + " var g = function a(c) { return c ? 1 : a(1); };", + " var d = function b(a) { return a ? 2 : b(2); };", + " var h = function e(b) { return b ? d : e(2); };", "}")); }