diff --git a/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java b/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java index f95d02aa83d..d389e5a6ca2 100644 --- a/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java +++ b/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java @@ -20,6 +20,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Iterators; import com.google.common.collect.PeekingIterator; +import com.google.common.collect.Sets; import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.jscomp.NodeTraversal.Callback; import com.google.javascript.rhino.Node; @@ -48,6 +49,7 @@ *
  • Any reference to a property getter/setter is treated like a call that escapes all props.
  • *
  • If there's an Object.definePropert{y,ies} call where the object or property name is aliased * then the optimization does not run at all.
  • + *
  • Properties names defined in externs will not be pruned.
  • * */ public class DeadPropertyAssignmentElimination implements CompilerPass { @@ -65,6 +67,12 @@ public class DeadPropertyAssignmentElimination implements CompilerPass { @Override public void process(Node externs, Node root) { + // GatherExternProperties must be enabled for this pass to safely know what property writes are + // eligible for removal. + if (compiler.getExternProperties() == null) { + return; + } + GetterSetterCollector getterSetterCollector = new GetterSetterCollector(); NodeTraversal.traverseEs6(compiler, root, getterSetterCollector); @@ -73,8 +81,9 @@ public void process(Node externs, Node root) { return; } - NodeTraversal.traverseEs6(compiler, root, - new FunctionVisitor(compiler, getterSetterCollector.propNames)); + Set blacklistedPropNames = Sets.union( + getterSetterCollector.propNames, compiler.getExternProperties()); + NodeTraversal.traverseEs6(compiler, root, new FunctionVisitor(compiler, blacklistedPropNames)); } private static class FunctionVisitor extends AbstractPostOrderCallback { @@ -82,14 +91,13 @@ private static class FunctionVisitor extends AbstractPostOrderCallback { private final AbstractCompiler compiler; /** - * A set of properties names that are known to be assigned to getter/setters. This is important - * since any reference to these properties needs to be treated as if it were a call. + * A set of properties names that are potentially unsafe to remove duplicate writes to. */ - private final Set getterSetterNames; + private final Set blacklistedPropNames; - FunctionVisitor(AbstractCompiler compiler, Set getterSetterNames) { + FunctionVisitor(AbstractCompiler compiler, Set blacklistedPropNames) { this.compiler = compiler; - this.getterSetterNames = getterSetterNames; + this.blacklistedPropNames = blacklistedPropNames; } @Override @@ -104,7 +112,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { } FindCandidateAssignmentTraversal traversal = - new FindCandidateAssignmentTraversal(getterSetterNames, NodeUtil.isConstructor(n)); + new FindCandidateAssignmentTraversal(blacklistedPropNames, NodeUtil.isConstructor(n)); NodeTraversal.traverseEs6(compiler, body, traversal); // Any candidate property assignment can have a write removed if that write is never read @@ -221,18 +229,17 @@ private static class FindCandidateAssignmentTraversal implements Callback { Map propertyMap = new HashMap<>(); /** - * A set of properties names that are known to be assigned to getter/setters. This is important - * since any reference to these properties needs to be treated as if it were a call. + * A set of properties names that are potentially unsafe to remove duplicate writes to. */ - private final Set getterSetterNames; + private final Set blacklistedPropNames; /** * Whether or not the function being analyzed is a constructor. */ private final boolean isConstructor; - FindCandidateAssignmentTraversal(Set getterSetterNames, boolean isConstructor) { - this.getterSetterNames = getterSetterNames; + FindCandidateAssignmentTraversal(Set blacklistedPropNames, boolean isConstructor) { + this.blacklistedPropNames = blacklistedPropNames; this.isConstructor = isConstructor; } @@ -361,7 +368,7 @@ private boolean visitNode(Node n, Node parent) { // Handle potential getters/setters. if (n.isGetProp() && n.getLastChild().isString() - && getterSetterNames.contains(n.getLastChild().getString())) { + && blacklistedPropNames.contains(n.getLastChild().getString())) { // We treat getters/setters as if they were a call, thus we mark all properties as read. markAllPropsRead(); return true; diff --git a/test/com/google/javascript/jscomp/DeadPropertyAssignmentEliminationTest.java b/test/com/google/javascript/jscomp/DeadPropertyAssignmentEliminationTest.java index 6fe2c51dc91..5f547ef6d48 100644 --- a/test/com/google/javascript/jscomp/DeadPropertyAssignmentEliminationTest.java +++ b/test/com/google/javascript/jscomp/DeadPropertyAssignmentEliminationTest.java @@ -22,6 +22,11 @@ public class DeadPropertyAssignmentEliminationTest extends CompilerTestCase { + @Override + public void setUp() throws Exception { + enableGatherExternProperties(); + } + public void testBasic() { testSame(LINE_JOINER.join( "var foo = function() {", @@ -988,6 +993,43 @@ public void testObjectDefineProperties_aliasedVars() { "}")); } + public void testPropertyDefinedInExterns() { + String externs = LINE_JOINER.join( + "var window = {};", + "/** @type {number} */ window.innerWidth", + "/** @constructor */", + "var Image = function() {};", + "/** @type {string} */ Image.prototype.src;" + ); + + testSame( + externs, + LINE_JOINER.join( + "function z() {", + " window.innerWidth = 10;", + " window.innerWidth = 20;", + "}"), + null); + + testSame( + externs, + LINE_JOINER.join( + "function z() {", + " var img = new Image();", + " img.src = '';", + " img.src = 'foo.bar';", + "}"), + null); + + testSame( + externs, + LINE_JOINER.join( + "function z(x) {", + " x.src = '';", + " x.src = 'foo.bar';", + "}"), + null); + } @Override protected CompilerPass getProcessor(Compiler compiler) {