From 74711c5a7b74b84f9a65ae184d637f0f8951b265 Mon Sep 17 00:00:00 2001 From: simranarora Date: Tue, 8 Aug 2017 16:30:52 -0700 Subject: [PATCH] Add default parameter compatibility to remove unused vars pass ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=164661798 --- .../google/javascript/jscomp/NodeUtil.java | 10 +- .../javascript/jscomp/RemoveUnusedVars.java | 100 ++++++++++++++++-- .../jscomp/RemoveUnusedVarsTest.java | 93 ++++++++++++++++ 3 files changed, 188 insertions(+), 15 deletions(-) diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 454f41ca253..f65b7422f1b 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -3784,15 +3784,15 @@ private static void getLhsNodesHelper(Node n, List lhsNodes) { } } - /** - * Retrieves lhs nodes declared in the current declaration. - */ - static Iterable getLhsNodesOfDeclaration(Node declNode) { + /** Retrieves lhs nodes declared in the current declaration. */ + static List getLhsNodesOfDeclaration(Node declNode) { checkArgument( isNameDeclaration(declNode) || declNode.isParamList() || declNode.isCatch() - || declNode.isDestructuringLhs(), declNode); + || declNode.isDestructuringLhs() + || declNode.isDefaultValue(), + declNode); ArrayList lhsNodes = new ArrayList<>(); getLhsNodesHelper(declNode, lhsNodes); return lhsNodes; diff --git a/src/com/google/javascript/jscomp/RemoveUnusedVars.java b/src/com/google/javascript/jscomp/RemoveUnusedVars.java index c98c90e04f3..3487b8c53ba 100644 --- a/src/com/google/javascript/jscomp/RemoveUnusedVars.java +++ b/src/com/google/javascript/jscomp/RemoveUnusedVars.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkState; import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.Lists; import com.google.common.collect.Multimap; import com.google.javascript.jscomp.CodingConvention.SubclassRelationship; import com.google.javascript.jscomp.DefinitionsRemover.Definition; @@ -496,19 +497,91 @@ private void removeUnreferencedFunctionArgs(Scope fparamScope) { boolean modifyCallers = modifyCallSites && callSiteOptimizer.canModifyCallers(function); if (!modifyCallers) { - // Strip unreferenced args off the end of the function declaration. - Node lastArg; - while ((lastArg = argList.getLastChild()) != null) { - Var var = fparamScope.getVar(lastArg.getString()); - if (!referenced.contains(var)) { - compiler.reportChangeToEnclosingScope(lastArg); - argList.removeChild(lastArg); + + // Remove any unused names from destructuring patterns as long as there are no side effects + removeUnusedDestructuringNames(argList, fparamScope); + + // Strip as many unreferenced args off the end of the function declaration as possible. + maybeRemoveUnusedTrailingParameters(argList, fparamScope); + } else { + callSiteOptimizer.optimize(fparamScope, referenced); + } + } + + /** + * Iterate through the parameters of the function and if they are destructuring parameters, remove + * any unreferenced variables from inside the destructuring pattern. + */ + private void removeUnusedDestructuringNames(Node argList, Scope fparamScope) { + List destructuringDeclarations = NodeUtil.getLhsNodesOfDeclaration(argList); + for (Node patternElt : Lists.reverse(destructuringDeclarations)) { + Node toRemove = patternElt; + if (patternElt.getParent().isDefaultValue()) { + Node defaultValueRhs = patternElt.getNext(); + if (NodeUtil.mayHaveSideEffects(defaultValueRhs)) { + // Protects in the case where function f({a:b = alert('bar')} = alert('foo')){}; + continue; + } + toRemove = patternElt.getParent(); + } + + if (toRemove.getParent().isParamList()) { + continue; + } + + // Go through all elements of the object pattern and determine whether they should be + // removed + Var var = fparamScope.getVar(patternElt.getString()); + if (!referenced.contains(var)) { + if (toRemove.getParent().isStringKey()) { + toRemove = toRemove.getParent(); + } + NodeUtil.deleteNode(toRemove, compiler); + } + } + } + + /** + * Strip as many unreferenced args off the end of the function declaration as possible. We start + * from the end of the function declaration because removing parameters from the middle of the + * param list could mess up the interpretation of parameters being sent over by any function + * calls. + * + * @param argList list of function's arguments + * @param fparamScope + */ + private void maybeRemoveUnusedTrailingParameters(Node argList, Scope fparamScope) { + Node lastArg; + while ((lastArg = argList.getLastChild()) != null) { + Node toRemove = lastArg; + if (lastArg.isDefaultValue()) { + toRemove = lastArg.getFirstChild(); + Node defaultValueSecondChild = toRemove.getNext(); + if (NodeUtil.mayHaveSideEffects(defaultValueSecondChild)) { + break; + } + } + + if (toRemove.isDestructuringPattern()) { + if (!toRemove.hasChildren()) { + // Remove empty destructuring patterns and their associated object literal assignment + // if it exists and if the right hand side does not have side effects. Note, a + // destructuring pattern with a "leftover" property key as in {a:{}} is not considered + // empty in this case! + NodeUtil.deleteNode(lastArg, compiler); + continue; } else { break; } } - } else { - callSiteOptimizer.optimize(fparamScope, referenced); + + // Remove unreferenced parameters + Var var = fparamScope.getVar(toRemove.getString()); + if (!referenced.contains(var)) { + NodeUtil.deleteNode(lastArg, compiler); + } else { + break; + } } } @@ -932,7 +1005,11 @@ private void removeUnreferencedVars() { || NodeUtil.isDestructuringDeclaration(grandParent) || toRemove.isArrayPattern() // Array Pattern || parent.isObjectPattern() // Object Pattern - || toRemove.isClass(), + || toRemove.isClass() + || ((toRemove.isDefaultValue() + && NodeUtil.getEnclosingScopeRoot(toRemove).isFunction()) + || ((parent.isDefaultValue()) + && NodeUtil.getEnclosingScopeRoot(parent).isFunction())), "We should only declare Vars and functions and function args and classes"); if ((toRemove.isParamList() && parent.isFunction()) @@ -948,6 +1025,9 @@ private void removeUnreferencedVars() { fnNameNode.setString(""); } // Don't remove bleeding functions. + } else if (toRemove.isArrayPattern() && grandParent.isParamList()) { + compiler.reportChangeToEnclosingScope(toRemove); + NodeUtil.removeChild(toRemove, nameNode); } else if (parent.isForIn()) { // foreach iterations have 3 children. Leave them alone. } else if (parent.isDestructuringPattern()) { diff --git a/test/com/google/javascript/jscomp/RemoveUnusedVarsTest.java b/test/com/google/javascript/jscomp/RemoveUnusedVarsTest.java index eb33e106a3f..1ea0f2d3a55 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedVarsTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedVarsTest.java @@ -219,6 +219,99 @@ public void testComputedPropertyDestructuring() { testSame("var a = {['foo' + 1]:1, ['foo' + 2]:2}; alert(a.foo1);"); } + public void testFunctionArgRemoval_defaultValue1() { + test( + "function f(unusedParam = undefined) {}; f();", + "function f( ) {}; f();"); + } + + public void testFunctionArgRemoval_defaultValue2() { + test("function f(unusedParam = 0) {}; f();", "function f( ) {}; f();"); + } + + public void testFunctionArgRemoval_defaultValue3() { + // Parameters already encountered can be used by later parameters + test("function f(x, y = x + 0) {}; f();", "function f( ) {}; f();"); + + testSame("function f(x, y = x + 0) { y; }; f();"); + } + + public void testFunctionArgRemoval_defaultValue4() { + // Default value inside arrow function param list + test("var f = (unusedParam = 0) => {}; f();", "var f = ( ) => {}; f();"); + + testSame("var f = (usedParam = 0) => { usedParam; }; f();"); + + test("var f = (usedParam = 0) => {usedParam;};", ""); + } + + public void testDestructuringParams() { + // Default value not used + test( + "function f({a:{b:b}} = {a:{}}) { /** b is unused */ }; f();", + "function f({a:{}} = {a:{}}) {/** b is unused */}; f();"); + + // Default value with nested value used in default value assignment + test( + "function f({a:{b:b}} = {a:{b:1}}) { /** b is unused */ }; f();", + "function f({a:{}} = {a:{b:1}}) {/** b is unused */}; f();"); + + // Default value with nested value used in function body + testSame("function f({a:{b:b}} = {a:{b:1}}) { b; }; f();"); + + test( + "function f({a:{b:b}} = {a:{b:1}}) { a.b; }; f();", + "function f({a:{}} = {a:{b:1}}) { a.b; }; f();"); + + test("function f({a:{b:b}} = {a:{}}) { a; }; f();", "function f({a:{}} = {a:{}}) { a; }; f();"); + + // Destructuring pattern not default and parameter not used + test( + "function f({a:{b:b}}) { /** b is unused */ }; f({c:{d:d}});", + "function f({a:{}}) { /** b is unused */ }; f({c:{d:d}});"); + + // Destructuring pattern not default and parameter used + testSame("function f({a:{b:b}}) { b; }; f({c:{d:d}});"); + } + + public void testMixedParamTypes() { + // Traditional and destructuring pattern + test("function f({a:{b:b}}, c, d) { c; }; f();", "function f({a:{}}, c) { c; }; f();"); + + test("function f({a:{b:b = 5}}, c, d) { c; }; f();", "function f({a:{}}, c) { c; }; f()"); + + testSame("function f({}, c, {d:{e:e}}) { c; e; }; f();"); + + // Parent is the parameter list + test("function f({a}, b, {c}) {use(a)}; f({}, {});", "function f({a}) {use(a)}; f({}, {});"); + + // Default and traditional + testSame("function f(unusedParam = undefined, z) { z; }; f();"); + } + + public void testDefaultParamsWithSideEffects() { + testSame("function f(a = alert('foo')) {}; f();"); + + testSame("function f({} = alert('foo')){}; f()"); + + test("function f({a:b} = alert('foo')){}; f();", "function f({} = alert('foo')){}; f();"); + + testSame("function f({a:b = alert('bar')} = alert('foo')){}; f();"); + } + + public void testArrayDestructuringParams() { + // Default values in array pattern unused + test("function f([x,y] = [1,2]) {}; f();", "function f([,] = [1,2]) {}; f();"); + + test("function f([x,y] = [1,2], z) { z; }; f();", "function f([,] = [1,2], z) { z; }; f();"); + + // Default values in array pattern used + test("function f([x,y,z] =[1,2,3]) { y; }; f();", "function f([,y] = [1,2,3]) { y; }; f();"); + + // Side effects + test("function f([x] = [foo()]) {}; f();", "function f([] = [foo()]) {}; f();"); + } + public void testFunctionsDeadButEscaped() { testSame("function b(a) { a = 1; print(arguments[0]) }; b(6)"); testSame("function b(a) { var c = 2; a = c; print(arguments[0]) }; b(6)");