From 1f820b521ea9d84a400a71b629d4743ccea0f332 Mon Sep 17 00:00:00 2001 From: sdh Date: Thu, 13 Dec 2018 19:22:44 -0800 Subject: [PATCH] Roll RemoveUnusedPolyfills into RemoveUnusedCode By taking advantage of RemoveUnusedCode's better logic, we are able to do significantly better removal than we could before (in particular, removing the Set polyfill no longer leaves the Map and WeakMap polyfills behind). This introduces a direct dependency on type information into RemoveUnusedCode where previously it relied entirely on disambiguation for type awareness. Without type information, polyfills may still be removed, but the accuracy is poorer (unused polyfills may not be removed, and used polyfills that are hidden on aliases may be incorrectly removed). This CL does not actually switch over to the new logic in DefaultPassConfig yet. We will do that in a follow-up CL to minimize the size of potentially breaking changes in case of rollbacks. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=225476066 --- .../javascript/jscomp/RemoveUnusedCode.java | 317 ++++++- .../jscomp/RemoveUnusedCodeTest.java | 825 +++++++++++++++++- .../javascript/jscomp/TestExternsBuilder.java | 8 + 3 files changed, 1130 insertions(+), 20 deletions(-) diff --git a/src/com/google/javascript/jscomp/RemoveUnusedCode.java b/src/com/google/javascript/jscomp/RemoveUnusedCode.java index 273085ec48f..4c9bf39b08d 100644 --- a/src/com/google/javascript/jscomp/RemoveUnusedCode.java +++ b/src/com/google/javascript/jscomp/RemoveUnusedCode.java @@ -16,6 +16,7 @@ package com.google.javascript.jscomp; +import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; @@ -28,11 +29,14 @@ import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; import com.google.javascript.rhino.jstype.JSType; +import com.google.javascript.rhino.jstype.JSTypeNative; +import com.google.javascript.rhino.jstype.ObjectType; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Deque; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -127,6 +131,14 @@ class RemoveUnusedCode implements CompilerPass { */ private final List allFunctionParamScopes = new ArrayList<>(); + /** + * Stores the names of all "leaf" properties that are polyfilled, to avoid unnecessary qualified + * name matching and searches for all the other properties. This includes global names such as + * "Promise" and "Map", static methods on global names such as "Array.from" and "Math.fround", and + * instance properties such as "String.prototype.repeat" and "Promise.prototype.finally". + */ + private final Multimap polyfills = HashMultimap.create(); + private final Es6SyntacticScopeCreator scopeCreator; private final boolean removeUnusedPrototypeProperties; @@ -134,6 +146,7 @@ class RemoveUnusedCode implements CompilerPass { private final boolean removeUnusedThisProperties; private final boolean removeUnusedStaticProperties; private final boolean removeUnusedObjectDefinePropertiesDefinitions; + private final boolean removeUnusedPolyfills; RemoveUnusedCode(Builder builder) { this.compiler = builder.compiler; @@ -147,6 +160,7 @@ class RemoveUnusedCode implements CompilerPass { this.removeUnusedStaticProperties = builder.removeUnusedStaticProperties; this.removeUnusedObjectDefinePropertiesDefinitions = builder.removeUnusedObjectDefinePropertiesDefinitions; + this.removeUnusedPolyfills = builder.removeUnusedPolyfills; this.scopeCreator = new Es6SyntacticScopeCreator(builder.compiler); // All Vars that are completely unremovable will share this VarInfo instance. @@ -165,6 +179,7 @@ public static class Builder { private boolean removeUnusedThisProperties = false; private boolean removeUnusedStaticProperties = false; private boolean removeUnusedObjectDefinePropertiesDefinitions = false; + private boolean removeUnusedPolyfills = false; Builder(AbstractCompiler compiler) { this.compiler = compiler; @@ -210,6 +225,11 @@ Builder removeUnusedObjectDefinePropertiesDefinitions(boolean value) { return this; } + Builder removeUnusedPolyfills(boolean value) { + this.removeUnusedPolyfills = value; + return this; + } + RemoveUnusedCode build() { return new RemoveUnusedCode(this); } @@ -249,7 +269,7 @@ private void traverseAndRemoveUnusedReferences(Node root) { continuation.apply(); } - removeUnreferencedVars(); + removeUnreferencedVarsAndPolyfills(); removeIndependentlyRemovableProperties(); for (Scope fparamScope : allFunctionParamScopes) { removeUnreferencedFunctionArgs(fparamScope); @@ -436,6 +456,12 @@ private void traverseGetProp(Node getProp, Scope scope) { Node propertyNameNode = objectNode.getNext(); String propertyName = propertyNameNode.getString(); + if (polyfills.containsKey(propertyName)) { + for (PolyfillInfo info : polyfills.get(propertyName)) { + info.considerPossibleReference(getProp); + } + } + if (NodeUtil.isExpressionResultUsed(getProp)) { // must record as reference to the property and continue traversal. markPropertyNameReferenced(propertyName); @@ -528,6 +554,12 @@ private void traverseCompoundAssign(Node compoundAssignNode, Scope scope) { } private VarInfo traverseNameNode(Node n, Scope scope) { + if (polyfills.containsKey(n.getString())) { + for (PolyfillInfo info : polyfills.get(n.getString())) { + info.considerPossibleReference(n); + } + } + return traverseVar(getVarForNameNode(n, scope)); } @@ -544,6 +576,13 @@ private void traverseCall(Node callNode, Scope scope) { } else if (NodeUtil.isObjectDefinePropertiesDefinition(callNode)) { // TODO(bradfordcsmith): Should also handle Object.create() and Object.defineProperty(). traverseObjectDefinePropertiesCall(callNode, scope); + } else if (removeUnusedPolyfills && isJscompPolyfill(callee)) { + Node firstArg = callee.getNext(); + String polyfillName = firstArg.getString(); + PolyfillInfo info = createPolyfillInfo(callNode, scope, polyfillName); + polyfills.put(info.key, info); + // Only traverse the callee (to mark it as used). The arguments may be traversed later. + traverseNode(callNode.getFirstChild(), scope); } else { Node parent = callNode.getParent(); String classVarName = null; @@ -589,6 +628,23 @@ private void traverseCall(Node callNode, Scope scope) { } } + /** Checks whether this is a recognizable call to $jscomp.polyfill. */ + private static boolean isJscompPolyfill(Node n) { + switch (n.getToken()) { + case NAME: + // Need to work correctly after CollapseProperties. + return n.getString().equals("$jscomp$polyfill") && n.getNext().isString(); + case GETPROP: + // Need to work correctly without CollapseProperties. + return n.getLastChild().getString().equals("polyfill") + && n.getFirstChild().isName() + && n.getFirstChild().getString().equals("$jscomp") + && n.getNext().isString(); + default: + return false; + } + } + /** Traverse `Object.defineProperties(someObject, propertyDefinitions);`. */ private void traverseObjectDefinePropertiesCall(Node callNode, Scope scope) { // First child is Object.defineProperties or some equivalent of it. @@ -1474,8 +1530,8 @@ private VarInfo getVarInfo(Var var) { * Removes any vars in the scope that were not referenced. Removes any assignments to those * variables as well. */ - private void removeUnreferencedVars() { - for (Entryentry : varInfoMap.entrySet()) { + private void removeUnreferencedVarsAndPolyfills() { + for (Entry entry : varInfoMap.entrySet()) { Var var = entry.getKey(); VarInfo varInfo = entry.getValue(); @@ -1512,6 +1568,15 @@ private void removeUnreferencedVars() { toRemove); } } + + Iterator iter = polyfills.values().iterator(); + while (iter.hasNext()) { + PolyfillInfo polyfill = iter.next(); + if (polyfill.isRemovable) { + polyfill.removable.remove(compiler); + iter.remove(); + } + } } /** @@ -1683,6 +1748,10 @@ DestructuringAssign buildDestructuringAssign(Node targetNode) { return new DestructuringAssign(this, targetNode); } + Polyfill buildPolyfill(Node polyfillNode) { + return new Polyfill(this, polyfillNode); + } + ClassDeclaration buildClassDeclaration(Node classNode) { return new ClassDeclaration(this, classNode); } @@ -1996,6 +2065,26 @@ && canRemoveParameters(targetParent)) { } + /** A call to $jscomp.polyfill that can be removed if it is no longer referenced. */ + private class Polyfill extends Removable { + final Node polyfillNode; + + Polyfill(RemovableBuilder builder, Node polyfillNode) { + super(builder); + this.polyfillNode = polyfillNode; + } + + @Override + public void removeInternal(AbstractCompiler compiler) { + NodeUtil.deleteNode(polyfillNode, compiler); + } + + @Override + public String toString() { + return "Polyfill:" + polyfillNode; + } + } + private class ClassDeclaration extends Removable { final Node classDeclarationNode; @@ -2494,24 +2583,220 @@ void removeAllRemovables() { } + /** + * Makes a new PolyfillInfo, including the correct Removable. Parses the name to determine whether + * this is a global, static, or prototype polyfill. + */ + private PolyfillInfo createPolyfillInfo(Node call, Scope scope, String name) { + checkState(scope.isGlobal()); + checkState(call.getParent().isExprResult()); + // Make the removable and polyfill info. Add continuations for all arguments. + RemovableBuilder builder = new RemovableBuilder(); + for (Node n = call.getFirstChild().getNext(); n != null; n = n.getNext()) { + builder.addContinuation(new Continuation(n, scope)); + } + Polyfill removable = builder.buildPolyfill(call.getParent()); + int lastDot = name.lastIndexOf("."); + if (lastDot < 0) { + return new GlobalPolyfillInfo(removable, name); + } + String owner = name.substring(0, lastDot); + String prop = name.substring(lastDot + 1); + boolean typed = call.getJSType() != null; + if (owner.endsWith(DOT_PROTOTYPE)) { + owner = owner.substring(0, owner.length() - DOT_PROTOTYPE.length()); + return new PrototypePropertyPolyfillInfo( + removable, prop, typed ? compiler.getTypeRegistry().getType(scope, owner) : null); + } + ObjectType ownerInstanceType = + typed ? ObjectType.cast(compiler.getTypeRegistry().getType(scope, owner)) : null; + JSType ownerCtorType = ownerInstanceType != null ? ownerInstanceType.getConstructor() : null; + return new StaticPropertyPolyfillInfo(removable, prop, ownerCtorType, owner); + } + + private static final String DOT_PROTOTYPE = ".prototype"; + + /** + * Stores information about definitions and usages of polyfills. + * + *

The polyfill removal strategy is as follows. First, look for all the polyfill definitions, + * whose names are stores as strings passed as the first argument to {@code $jscomp.polyfill}. + * Each definition falls into one of three categories: (1) global names, such as {@code Map} or + * {@code Promise}; (2) static properties, such as {@code Array.from} or {@code Reflect.get}, + * which must always have exactly two name components; or (3) prototype properties, such as {@code + * String.prototype.repeat} or {@code Promise.prorotype.finally}, which must always have exactly + * three name components. The definition can be removed once it is found that there is an + * unguarded reference to it. References guarded by existence or truthiness checks (such as + * {@code if (Promise) return Promise.resolve('ok');} do not prevent removal of the polyfill + * definition. + * + *

Determining whether a node is a reference (guarded or not) depends on the type of polyfill. + * When type information is available, the type of the expected owner (i.e. the global object for + * global polyfills, the namespace or class for static polyfills, or an instance of the owning + * class (or its implicit prototype) for prototype polyfills) is used exclusively to determine + * this with very good accuracy. Types are considered to match if a direct cast would be allowed + * without a warning (i.e. some element of the union is a direct subtype or supertype). + * + *

When type information is not available (or is too loose) then we fall back on a heuristic: + * + *

    + *
  • globals are referenced by any same-named NAME node or any GETPROP node whose last child + * has the same string (this allows matching {@code goog.global.Map}, but will also match + * {@code MyOuter.Map}). + *
  • static properties are referenced by any GETPROP node whose last child is the same as the + * polyfill's property name and whose owner references the polyfill owner per the above + * rule. + *
  • prototype properties are referenced by any GETPROP node whose last child is the same as + * the polyfill's property name, regardless of its owner. + *
+ * + *

Note that this results in both false positives and false negatives in untyped code: we may + * remove polyfills that are actually used (e.g. if {@code Array.from} is accessed via a subclass + * as {@code SubArray.from} or in a subclass' static method as {@code this.from}) and we may + * retain polyfills that are not used (e.g. if a user-defined nested class shares the same name as + * a global builtin, as in {@code Foo.Map}). For greater consistency we may shift this balance in + * the future to eliminate the possibility of incorrect removals, at the cost of more incorrect + * retentions. + */ + private abstract static class PolyfillInfo { + /** The {@link Polyfill} instance corresponding to the polyfill's definition. */ + final Polyfill removable; + /** The rightmost component of the polyfill's qualified name (does not contain a dot). */ + final String key; + /** Whether the polyfill is unreferenced and this can be removed safely. */ + boolean isRemovable = true; + + PolyfillInfo(Polyfill removable, String key) { + this.removable = removable; + this.key = key; + } + + /** + * Accepts a NAME or GETPROP node whose (property) string matches {@code key} and checks whether + * the node should be considered as a possible reference to this polyfill. If so, mark the + * polyfill as referenced and therefore not removable. + */ + void considerPossibleReference(Node n) { + if (isRemovable) { + considerPossibleReferenceInternal(n); + if (!isRemovable) { + removable.applyContinuations(); + } + } + } + + /** Template method to check the node. */ + abstract void considerPossibleReferenceInternal(Node n); + } + + private class GlobalPolyfillInfo extends PolyfillInfo { + + GlobalPolyfillInfo(Polyfill removable, String name) { + super(removable, name); + } + + @Override + void considerPossibleReferenceInternal(Node possiblyReferencingNode) { + if (possiblyReferencingNode.isName()) { + // A matching NAME node must be a reference (there's no need to check that the referenced + // Var is global, since local variables have all been renamed by normalization). + isRemovable = false; + } else if (possiblyReferencingNode.isGetProp()) { + // Does the owner have type information? If so then this is a reference only if the owner + // could be the global 'this' type. Absent type information, just always assume it might be. + JSType ownerType = possiblyReferencingNode.getFirstChild().getJSType(); + if (ownerType == null + || ownerType.canCastTo( + compiler.getTypeRegistry().getNativeType(JSTypeNative.GLOBAL_THIS))) { + isRemovable = false; + } + } + } + } + + private class StaticPropertyPolyfillInfo extends PolyfillInfo { + // Type of the owner, if available. + @Nullable final JSType polyfillOwnerType; + // Name of the owning type, used only when there's no type information. + final String polyfillOwnerName; + + StaticPropertyPolyfillInfo( + Polyfill removable, String key, @Nullable JSType owner, String ownerName) { + super(removable, key); + this.polyfillOwnerName = ownerName; + this.polyfillOwnerType = owner; + } + + @Override + void considerPossibleReferenceInternal(Node possiblyReferencingNode) { + if (!possiblyReferencingNode.isGetProp()) { + return; + } + Node nodeOwner = possiblyReferencingNode.getFirstChild(); + JSType nodeOwnerType = nodeOwner.getJSType(); + if (polyfillOwnerType != null && nodeOwnerType != null) { + if (typesAreRelated(polyfillOwnerType, nodeOwnerType)) { + isRemovable = false; + } + return; + } + // no type information: check polyfillOwnerName instead. + if ((nodeOwner.isGetProp() && nodeOwner.getLastChild().getString().equals(polyfillOwnerName)) + || (nodeOwner.isName() && nodeOwner.getString().equals(polyfillOwnerName))) { + isRemovable = false; + } + } + } + + private class PrototypePropertyPolyfillInfo extends PolyfillInfo { + @Nullable final JSType polyfillOwnerType; + + PrototypePropertyPolyfillInfo(Polyfill removable, String key, @Nullable JSType owner) { + super(removable, key); + this.polyfillOwnerType = owner; + } + + @Override + void considerPossibleReferenceInternal(Node possiblyReferencingNode) { + if (!possiblyReferencingNode.isGetProp()) { + return; + } + JSType nodeOwnerType = possiblyReferencingNode.getFirstChild().getJSType(); + if (polyfillOwnerType != null && nodeOwnerType != null) { + if (typesAreRelated(polyfillOwnerType, nodeOwnerType)) { + isRemovable = false; + } + return; + } + // Fallback for no type information: prototype properties are simply not removable. + isRemovable = false; + } + } + + private static boolean typesAreRelated(JSType expected, JSType actual) { + if (actual.isConstructor() && expected.isConstructor()) { + // All constructors can cast to one another, even if they're incompatible. + // If both types are constructors then compare the instance types instead. + actual = actual.toMaybeFunctionType().getInstanceType(); + expected = expected.toMaybeFunctionType().getInstanceType(); + } + actual = firstNonNull(actual.autobox(), actual); + + return actual.canCastTo(expected); + } + /** * Represents declarations in the standard for-loop initialization. * - * e.g. the `let i = 0` part of `for (let i = 0; i < 10; ++i) {...}`. - * These must be handled differently from declaration statements because: + *

e.g. the `let i = 0` part of `for (let i = 0; i < 10; ++i) {...}`. These must be handled + * differently from declaration statements because: * *

    - *
  1. - * For-loop declarations may declare more than one variable. - * The normalization doesn't break them up as it does for declaration statements. - *
    2. - *
  2. - * Removal must be handled differently. - *
    3. - *
  3. - * We don't currently preserve initializers with side effects here. - * Instead, we just consider such cases non-removable. - *
    4. + *
  4. For-loop declarations may declare more than one variable. The normalization doesn't break + * them up as it does for declaration statements. + *
  5. Removal must be handled differently. + *
  6. We don't currently preserve initializers with side effects here. Instead, we just + * consider such cases non-removable. *
*/ private class VanillaForNameDeclaration extends Removable { diff --git a/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java b/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java index e23b1a55635..de606ca3fbc 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java @@ -34,16 +34,32 @@ public RemoveUnusedCodeTest() { lines( "var undefined;", "var goog = {};", + "/** @const {!Global} */ goog.global;", "goog.reflect = {};", "goog.reflect.object = function(obj, propertiesObj) {};", "function goog$inherits(subClass, superClass) {}", - "function goog$mixin(dstPrototype, srcPrototype){}", + "function goog$mixin(dstPrototype, srcPrototype) {}", "function alert() {}", "function use() {}", "function externFunction() {}", "var externVar;", - "var window;")); - } + "var window;", + "var console = {};", + "console.log = function(var_args) {};", + "/** @constructor @return {!Array} */ function Array(/** ...* */ var_args) {}", + "/** @constructor @return {string} */ function String(/** *= */ opt_arg) {}", + "/** @constructor */ function Map() {}", + "/** @constructor */ function Set() {}", + "/** @constructor */ function WeakMap() {}", + "")); + } + + private static final String JSCOMP_POLYFILL = + lines( + "var $jscomp = {};", + "$jscomp.polyfill = function(", + " /** string */ name, /** Function */ func, /** string */ from, /** string */ to) {};", + ""); @Override protected int getNumRepetitions() { @@ -68,6 +84,7 @@ public void process(Node externs, Node root) { new RemoveUnusedCode.Builder(compiler) .removeLocalVars(true) .removeGlobals(removeGlobal) + .removeUnusedPolyfills(true) .preserveFunctionExpressionNames(preserveFunctionExpressionNames) .build() .process(externs, root); @@ -1906,5 +1923,805 @@ public void testRemoveUnusedVarsDeclaredInDestructuring1() { // Same as above case without the destructuring declaration test("var a, b = alert();", "alert();"); } -} + @Test + public void testRemoveUnusedPolyfills_global_untyped() { + Externs externs = + externs( + new TestExternsBuilder() + .addConsole() + .addExtra(JSCOMP_POLYFILL, "/** @constructor */ function Map() {}") + .build()); + + // Unused polyfill is removed. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", // + "console.log();")), + expected("console.log();")); + + // Used polyfill is not removed. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", // + "console.log(new Map());"))); + + // Local names shadowing global polyfills are not themselves polyfill references. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", + "console.log(function(Map) {", + " console.log(new Map());", + "});")), + expected( + lines( + "console.log(function(Map) {", // + " console.log(new Map());", + "});"))); + } + + @Test + public void testRemoveUnusedPolyfills_global_typed() { + Externs externs = + externs( + new TestExternsBuilder() + .addConsole() + .addExtra(JSCOMP_POLYFILL, "/** @constructor */ function Map() {}", "") + .build()); + enableTypeCheck(); + + // Unused polyfill is removed. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", // + "console.log();")), + expected("console.log();")); + + // Used polyfill is not removed. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", // + "console.log(new Map());"))); + + // Local names shadowing global polyfills are not themselves polyfill references. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", + "console.log(function(Map) {", + " console.log(new Map());", + "});")), + expected( + lines( + "console.log(function(Map) {", // + " console.log(new Map());", + "});"))); + } + + @Test + public void testRemoveUnusedPolyfills_propertyOfGlobalObject_untyped() { + Externs externs = + externs( + new TestExternsBuilder() + .addConsole() + .addExtra( + JSCOMP_POLYFILL, + "/** @constructor */ function Map() {}", + "/** @const */ var goog = {};") + .build()); + + // Global polyfills may be accessed as properties on the global object. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", // + "console.log(new goog.global.Map());"))); + + // NOTE: Without type information we don't see that x is not the global object. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", // + "console.log(new goog.structs.Map());"))); + + // Global polyfills may be accessed as properties on the global object. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", + "var x = {Map: /** @constructor */ function() {}};", + "console.log(new x.Map());"))); + } + + @Test + public void testRemoveUnusedPolyfills_propertyOfGlobalObject_typed() { + Externs externs = + externs( + new TestExternsBuilder() + .addConsole() + .addExtra( + JSCOMP_POLYFILL, + "/** @constructor */ function Map() {}", + "/** @type {!Global} */ var someGlobal;", + "/** @const */ var notGlobal = {};", + "/** @constructor */ notGlobal.Map = function() {};", + "") + .build()); + enableTypeCheck(); + + // Global polyfills may be accessed as properties on the global object. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", + "console.log(new someGlobal.Map());"))); + + // With proper type information we can tell that notGlobal.Map is not Map. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", + "console.log(new notGlobal.Map());")), + expected("console.log(new notGlobal.Map());")); + + // Global polyfills may be accessed as properties on the global object. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", + "var alsoNotGlobal = {Map: /** @constructor */ function() {}};", + "console.log(new alsoNotGlobal.Map());")), + expected( + lines( + "var alsoNotGlobal = {Map: /** @constructor */ function() {}};", // + "console.log(new alsoNotGlobal.Map());"))); + } + + @Test + public void testRemoveUnusedPolyfills_staticProperty_untyped() { + Externs externs = + externs( + new TestExternsBuilder() + .addConsole() + .addArray() + .addExtra( + JSCOMP_POLYFILL, + "/** @constructor */ function Map() {}", + "/** @const */ var goog = {};") + .build()); + + // Used polyfill is retained. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", // + "console.log(Array.from());"))); + + // Used polyfill is retained, even if accessed as a property of global. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "console.log(goog.global.Array.from());"))); + + // Unused polyfill is removed if there's sufficient type information. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "class NotArray { static from() {} }", + "console.log(NotArray.from());")), + expected( + lines( + "class NotArray { static from() {} }", // + "console.log(NotArray.from());"))); + + // Without type information, we can't correctly remove this polyfill. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "var x = {Array: {from: function() {}}};", + "console.log(x.Array.from());"))); + + // Without type information, we don't recognize the aliased call. + // https://github.com/google/closure-compiler/issues/3171 + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "/** @const */ var MyArray = Array;", + "console.log(MyArray.from());")), + expected( + lines( + "/** @const */ var MyArray = Array;", // + "console.log(MyArray.from());"))); + + // Without type information, we don't recognize the subclass call. + // https://github.com/google/closure-compiler/issues/3171 + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "class SubArray extends Array {}", + "console.log(SubArray.from());")), + expected( + lines( + "class SubArray extends Array {}", // + "console.log(SubArray.from());"))); + + // Heurisitic is still able to remove Set.from while retaining Array.from. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Set.from', function() {}, 'es6', 'es3');", + "console.log(Array.from());")), + expected( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", // + "console.log(Array.from());"))); + } + + @Test + public void testRemoveUnusedPolyfills_staticProperty_typed() { + Externs externs = + externs( + new TestExternsBuilder() + .addArray() + .addConsole() + .addExtra( + JSCOMP_POLYFILL, + "Array.from = function() {};", + "/** @constructor */ function Set() {}", + // NOTE: this is not a real API but it allows testing that we can tell it apart. + "Set.from = function() {};", + "") + .build()); + enableTypeCheck(); + + // Used polyfill is retained. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", // + "console.log(Array.from());"))); + + // Used polyfill is retained, even if accessed via a global object. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "console.log($jscomp.global.Array.from());"))); + + // Used polyfill is retained, even if accessed via a global object. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "class NotArray { static from() {} }", + "console.log(NotArray.from());")), + expected( + lines( + "class NotArray { static from() {} }", // + "console.log(NotArray.from());"))); + + // Unused polyfill is deleted since compiler knows `x` is not the global object. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "var x = {Array: {from: function() {}}};", + "console.log(x.Array.from());")), + expected( + lines( + "var x = {Array: {from: function() {}}};", // + "console.log(x.Array.from());"))); + + // Used polyfill via aliased owner: retains definition. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "/** @const */ var MyArray = Array;", + "console.log(MyArray.from());"))); + + // Used polyfill via subclass: retains definition. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "class SubArray extends Array {}", + "console.log(SubArray.from());"))); + + // Distinguish static polyfills on different types: remove the unused one. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Set.from', function() {}, 'es6', 'es3');", + "console.log(Array.from());")), + expected( + lines( + "$jscomp.polyfill('Array.from', function() {}, 'es6', 'es3');", // + "console.log(Array.from());"))); + } + + @Test + public void testRemoveUnusedPolyfills_prototypeProperty_untyped() { + Externs externs = + externs( + new TestExternsBuilder() + .addArray() + .addConsole() + .addString() + .addExtra(JSCOMP_POLYFILL, "function externFunction() {}", "function alert() {}") + .build()); + + // Unused polyfill is removed. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "console.log();")), + expected("console.log();")); + + // Used polyfill is retained. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "''.repeat();"))); + + // Used polyfill (directly via String.prototype) is retained. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "String.prototype.repeat();"))); + + // Used polyfill (directly String.prototype and Function.prototype.call) is retained. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "String.prototype.repeat.call('');"))); + + // Unknown type is not removed. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "var x = externFunction();", + "x.repeat();"))); + + // Without type information, cannot remove the polyfill. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "String.repeat();"))); + + // Without type information, cannot remove the polyfill. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "var /** number */ x = 42;", + "x.repeat();"))); + + // Multiple same-name methods + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.includes', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Array.prototype.includes', function() {}, 'es6', 'es3');", + "[].includes(5);"))); + } + + @Test + public void testRemoveUnusedPolyfills_prototypeProperty_typed() { + Externs externs = + externs( + new TestExternsBuilder() + .addFunction() + .addString() + .addArray() + .addConsole() + .addExtra( + JSCOMP_POLYFILL, + "/** @this {string} */", + "String.prototype.repeat = function() {}", + "String.prototype.includes = function(arg) {}", + "Array.prototype.includes = function(arg) {}", + "function externFunction() {}") + .build()); + enableTypeCheck(); + + // Unused polyfill is removed. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "console.log();")), + expected("console.log();")); + + // Used polyfill is retained. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "''.repeat();"))); + + // Used polyfill (via String.prototype) is retained. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "String.prototype.repeat();"))); + + // Used polyfill (via String.prototype and Function.prototype.call) is retained. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "String.prototype.repeat.call('');"))); + + // Unknown type is not removed. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "var x = externFunction();", + "x.repeat();"))); + + // Calling a prototype property like a static allows removing the prototype. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.repeat', function() {}, 'es6', 'es3');", + "String.repeat();")), + expected("String.repeat();"), + warning(TypeCheck.INEXISTENT_PROPERTY)); + + // Correctly discern between string and array. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.includes', function() {}, 'es6', 'es3');", + "var x = [];", + "x.includes(1);")), + expected( + lines( + "var x = [];", // + "x.includes(1);"))); + + // Union type prevents removal. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.includes', function() {}, 'es6', 'es3');", + "var /** string|Array */ x = [];", + "x.includes(1);"))); + + // Multiple same-name methods removes the right one. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.includes', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Array.prototype.includes', function() {}, 'es6', 'es3');", + "'x'.includes(5);")), + expected( + lines( + "$jscomp.polyfill('String.prototype.includes', function() {}, 'es6', 'es3');", + "'x'.includes(5);"))); + + // Multiple same-name methods removes the right one. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('String.prototype.includes', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Array.prototype.includes', function() {}, 'es6', 'es3');", + "Array.prototype.includes.call('x', 1);")), + expected( + lines( + "$jscomp.polyfill('Array.prototype.includes', function() {}, 'es6', 'es3');", + "Array.prototype.includes.call('x', 1);")), + warning(TypeValidator.TYPE_MISMATCH_WARNING)); + } + + @Test + public void testRemoveUnusedPolyfills_globalWithPrototypePolyfill_untyped() { + Externs externs = + externs( + new TestExternsBuilder().addPromise().addConsole().addExtra(JSCOMP_POLYFILL).build()); + + // Both the base polyfill and the extra method are removed when unused. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "console.log();")), + expected("console.log();")); + + // The extra method polyfill is removed if not used, even when the base is retained. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "console.log(Promise.resolve());")), + expected( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "console.log(Promise.resolve());"))); + + // Can't remove finally without type information + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "const p = {finally() {}};", + "p.finally();")), + expected( + lines( + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "const p = {finally() {}};", + "p.finally();"))); + + // Retain both the base and the extra method when both are used. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "console.log(Promise.resolve().finally(() => {}));"))); + + // The base polyfill is removed and the extra method retained if the constructor never shows up + // anywhere in the source code. NOTE: this is probably the wrong thing to do. This situation + // should only be possible if async function transpilation happens *after* RemoveUnusedCode + // (since we have an async function). The fact that we inserted the Promise polyfill in the + // first case indicates the output language is < ES6. When that later transpilation occurs, we + // will end up adding uses of the Promise constructor. We need to keep this in mind when moving + // transpilation after optimizations. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "async function f() {}", + "f().finally(() => {});")), + expected( + lines( + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "async function f() {}", + "f().finally(() => {});"))); + } + + @Test + public void testRemoveUnusedPolyfills_globalWithPrototypePolyfill_typed() { + Externs externs = + externs( + new TestExternsBuilder().addPromise().addConsole().addExtra(JSCOMP_POLYFILL).build()); + enableTypeCheck(); + + // Both the base polyfill and the extra method are removed when unused. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "console.log();")), + expected("console.log();")); + + // The extra method polyfill is removed if not used, even when the base is retained. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "console.log(Promise.resolve());")), + expected( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "console.log(Promise.resolve());"))); + + // Calls a different finally so both polyfills can be removed. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "const p = {finally() {}};", + "p.finally();")), + expected(lines("const p = {finally() {}};", "p.finally();"))); + + // Retain both the base and the extra method when both are used. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "console.log(Promise.resolve().finally(() => {}));"))); + + // The base polyfill is removed and the extra method retained if the constructor never shows up + // anywhere in the source code. NOTE: this is probably the wrong thing to do. This situation + // should only be possible if async function transpilation happens *after* RemoveUnusedCode + // (since we have an async function). The fact that we inserted the Promise polyfill in the + // first case indicates the output language is < ES6. When that later transpilation occurs, we + // will end up adding uses of the Promise constructor. We need to keep this in mind when moving + // transpilation after optimizations. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Promise', function() {}, 'es6', 'es3');", + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "async function f() {}", + "f().finally(() => {});")), + expected( + lines( + "$jscomp.polyfill('Promise.prototype.finally', function() {}, 'es8', 'es3');", + "async function f() {}", + "f().finally(() => {});"))); + } + + @Test + public void testRemoveUnusedPolyfills_chained() { + Externs externs = + externs( + new TestExternsBuilder() + .addConsole() + .addExtra( + JSCOMP_POLYFILL, + "/** @constructor */ function Map() {}", + "/** @constructor */ function Set() {}", + "/** @constructor */ function WeakMap() {}") + .build()); + + // Removes polyfills that are only referenced in other (removed) polyfills' definitions. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('WeakMap', function() { console.log(); }, 'es6', 'es3');", + "$jscomp.polyfill('Map', function() { new WeakMap(); }, 'es6', 'es3');", + "$jscomp.polyfill('Set', function() { new Map(); }, 'es6', 'es3');", + "function unused() { new Set(); }", + "console.log();")), + expected("console.log();")); + + // Chains can be partially removed if just an outer-most symbol is unreferenced. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('WeakMap', function() { console.log(); }, 'es6', 'es3');", + "$jscomp.polyfill('Map', function() { new WeakMap(); }, 'es6', 'es3');", + "$jscomp.polyfill('Set', function() { new Map(); }, 'es6', 'es3');", + "console.log(new Map());")), + expected( + lines( + "$jscomp.polyfill('WeakMap', function() { console.log(); }, 'es6', 'es3');", + "$jscomp.polyfill('Map', function() { new WeakMap(); }, 'es6', 'es3');", + "console.log(new Map());"))); + + // Only requires a single reference to the outermost symbol to retain the whole chain. + testSame( + externs, + srcs( + lines( + "$jscomp.polyfill('WeakMap', function() { console.log(); }, 'es6', 'es3');", + "$jscomp.polyfill('Map', function() { new WeakMap(); }, 'es6', 'es3');", + "$jscomp.polyfill('Set', function() { new Map(); }, 'es6', 'es3');", + "console.log(new Set())"))); + } + + @Test + public void testRemoveUnusedPolyfills_continued() { + Externs externs = + externs( + new TestExternsBuilder() + .addConsole() + .addExtra(JSCOMP_POLYFILL, "/** @constructor */ function Map() {}") + .build()); + + // Ensure that continuations occur so that retained polyfill definitions are still optimized. + test( + externs, + srcs( + lines( + "$jscomp.polyfill('Map', function() { var x; }, 'es6', 'es3');", + "console.log(new Map());")), + expected( + lines( + "$jscomp.polyfill('Map', function() {}, 'es6', 'es3');", // + "console.log(new Map());"))); + } + + @Test + public void testRemoveUnusedPolyfills_collapsedPolyfillFunction() { + Externs externs = + externs( + new TestExternsBuilder() + .addConsole() + .addExtra("function $jscomp$polyfill() {}", "/** @constructor */ function Map() {}") + .build()); + + // The pass should also work after CollapseProperties. + test( + externs, + srcs( + lines( + "$jscomp$polyfill('Map', function() {}, 'es6', 'es3');", // + "console.log();")), + expected("console.log();")); + + testSame( + externs, + srcs( + lines( + "$jscomp$polyfill('Map', function() {}, 'es6', 'es3');", // + "console.log(new Map());"))); + } +} diff --git a/test/com/google/javascript/jscomp/TestExternsBuilder.java b/test/com/google/javascript/jscomp/TestExternsBuilder.java index c2bb45e4047..31a3685d3cc 100644 --- a/test/com/google/javascript/jscomp/TestExternsBuilder.java +++ b/test/com/google/javascript/jscomp/TestExternsBuilder.java @@ -18,6 +18,7 @@ import com.google.common.base.Joiner; import java.util.ArrayList; +import java.util.Collections; import java.util.List; /** @@ -430,6 +431,7 @@ public class TestExternsBuilder { private boolean includeArgumentsExterns = false; private boolean includeConsoleExterns = false; private boolean includePromiseExterns = false; + private final List extraExterns = new ArrayList<>(); public TestExternsBuilder addIterable() { includeIterableExterns = true; @@ -478,6 +480,11 @@ public TestExternsBuilder addConsole() { return this; } + public TestExternsBuilder addExtra(String... lines) { + Collections.addAll(extraExterns, lines); + return this; + } + public String build() { List externSections = new ArrayList<>(); if (includeIterableExterns) { @@ -504,6 +511,7 @@ public String build() { if (includePromiseExterns) { externSections.add(PROMISE_EXTERNS); } + externSections.addAll(extraExterns); return LINE_JOINER.join(externSections); }