From 8530f3701d13eef2acc64a114c4aded93b6af404 Mon Sep 17 00:00:00 2001 From: sdh Date: Tue, 11 Jul 2017 18:24:34 -0700 Subject: [PATCH] Consolidate DisambiguateProperties to use TypeInvalidator. There were a number of changes that needed to happen to TypeInvalidator for this to work, since disambiguation has rather significantly different criteria compared to ambiguation and inlining. Additionally, the disambiguate pass has some warning rate-limiting logic shoe-horned into it, which needed to instead be added to InvalidatingTypes. I hope to simplify some of this logic (and, in particular, the way nodes and errors are stored in TypeMismatch objects) in a future change. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=161612705 --- .../jscomp/AmbiguateProperties.java | 9 +- .../jscomp/DisambiguateProperties.java | 131 +++--------------- .../javascript/jscomp/InlineProperties.java | 3 + .../javascript/jscomp/InvalidatingTypes.java | 127 ++++++++++++++--- .../jscomp/AmbiguatePropertiesTest.java | 9 ++ .../jscomp/DisambiguatePropertiesTest.java | 5 - 6 files changed, 140 insertions(+), 144 deletions(-) diff --git a/src/com/google/javascript/jscomp/AmbiguateProperties.java b/src/com/google/javascript/jscomp/AmbiguateProperties.java index d2c99a288b4..20a19600469 100644 --- a/src/com/google/javascript/jscomp/AmbiguateProperties.java +++ b/src/com/google/javascript/jscomp/AmbiguateProperties.java @@ -137,6 +137,9 @@ public int compare(Property p1, Property p2) { this.reservedNonFirstCharacters = reservedNonFirstCharacters; this.invalidatingTypes = new InvalidatingTypes.Builder(compiler.getTypeIRegistry()) + // TODO(sdh): consider allowing ambiguation on properties of global this + // (we already reserve extern'd names, so this should be safe). + .disallowGlobalThis() .addTypesInvalidForPropertyRenaming() .addAllTypeMismatches(compiler.getTypeMismatches()) .addAllTypeMismatches(compiler.getImplicitInterfaceUses()) @@ -313,9 +316,9 @@ private void computeRelatedTypes(TypeI type) { * its related types to the given bit set. */ private void addRelatedInstance(FunctionTypeI constructor, JSTypeBitSet related) { - // TODO(user): A constructor which doesn't have an instance type - // (e.g. it's missing the @constructor annotation) should be an invalidating - // type which doesn't reach this code path. + // TODO(user): Make a constructor which doesn't have an instance type + // (e.g. it's missing the @constructor annotation) an invalidating type which + // doesn't reach this code path. if (constructor.hasInstanceType()) { ObjectTypeI instanceType = constructor.getInstanceType(); related.set(getIntForType(instanceType.getPrototypeObject())); diff --git a/src/com/google/javascript/jscomp/DisambiguateProperties.java b/src/com/google/javascript/jscomp/DisambiguateProperties.java index 4e64319af90..adb44537065 100644 --- a/src/com/google/javascript/jscomp/DisambiguateProperties.java +++ b/src/com/google/javascript/jscomp/DisambiguateProperties.java @@ -81,9 +81,6 @@ * */ class DisambiguateProperties implements CompilerPass { - // To prevent the logs from filling up, we cap the number of warnings - // that we tell the user to fix per-property. - private static final int MAX_INVALIDATION_WARNINGS_PER_PROPERTY = 10; private static final Logger logger = Logger.getLogger( DisambiguateProperties.class.getName()); @@ -107,7 +104,8 @@ static class Warnings { } private final AbstractCompiler compiler; - private final Set invalidatingTypes; + + private final InvalidatingTypes invalidatingTypes; private final TypeIRegistry registry; // Used as a substitute for null in gtwpCache. The method gtwpCacheGet returns // null to indicate that an element wasn't present. @@ -204,7 +202,7 @@ UnionFind getTypes() { void addType(TypeI type, TypeI relatedType) { checkState(!skipRenaming, "Attempt to record skipped property: %s", name); TypeI top = getTypeWithProperty(this.name, type); - if (isInvalidatingType(top)) { + if (invalidatingTypes.isInvalidating(top)) { invalidate(); return; } @@ -309,7 +307,7 @@ boolean invalidate() { */ boolean scheduleRenaming(Node node, TypeI type) { if (!skipRenaming) { - if (isInvalidatingType(type)) { + if (invalidatingTypes.isInvalidating(type)) { invalidate(); return false; } @@ -327,21 +325,17 @@ boolean scheduleRenaming(Node node, TypeI type) { this.registry = compiler.getTypeIRegistry(); this.BOTTOM_OBJECT = this.registry.getNativeType(JSTypeNative.NO_OBJECT_TYPE).toMaybeObjectType(); - this.invalidatingTypes = new HashSet<>(ImmutableSet.of( - registry.getNativeType(JSTypeNative.ALL_TYPE), - registry.getNativeType(JSTypeNative.NO_OBJECT_TYPE), - registry.getNativeType(JSTypeNative.NO_TYPE), - registry.getNativeType(JSTypeNative.FUNCTION_PROTOTYPE), - registry.getNativeType(JSTypeNative.FUNCTION_INSTANCE_TYPE), - registry.getNativeType(JSTypeNative.OBJECT_PROTOTYPE), - registry.getNativeType(JSTypeNative.TOP_LEVEL_PROTOTYPE), - registry.getNativeType(JSTypeNative.UNKNOWN_TYPE))); + this.propertiesToErrorFor = propertiesToErrorFor; - if (!this.propertiesToErrorFor.isEmpty()) { - this.invalidationMap = LinkedHashMultimap.create(); - } else { - this.invalidationMap = null; - } + this.invalidationMap = + propertiesToErrorFor.isEmpty() ? null : LinkedHashMultimap.create(); + + this.invalidatingTypes = new InvalidatingTypes.Builder(registry) + .addTypesInvalidForPropertyRenaming() + .addAllTypeMismatches(compiler.getTypeMismatches(), this.invalidationMap) + .addAllTypeMismatches(compiler.getImplicitInterfaceUses(), this.invalidationMap) + .allowEnumsAndScalars() + .build(); } @Override @@ -349,18 +343,6 @@ public void process(Node externs, Node root) { checkState(compiler.getLifeCycleStage() == LifeCycleStage.NORMALIZED); this.ancestorInterfaces = new HashMap<>(); this.gtwpCache = new HashMap<>(); - // TypeValidator records places where a type A is used in a context that - // expects a type B. - // For each pair (A, B), here we mark both A and B as types whose properties - // cannot be renamed. - for (TypeMismatch mis : compiler.getTypeMismatches()) { - recordInvalidatingType(mis.typeA, mis); - recordInvalidatingType(mis.typeB, mis); - } - for (TypeMismatch mis : compiler.getImplicitInterfaceUses()) { - recordInvalidatingType(mis.typeA, mis); - recordInvalidatingType(mis.typeB, mis); - } // Gather names of properties in externs; these properties can't be renamed. NodeTraversal.traverseEs6(compiler, externs, new FindExternProperties()); // Look at each unquoted property access and decide if that property will @@ -370,52 +352,6 @@ public void process(Node externs, Node root) { renameProperties(); } - private void recordInvalidationError(TypeI t, TypeMismatch mis) { - if (!t.isObjectType()) { - return; - } - if (invalidationMap != null) { - Collection errors = this.invalidationMap.get(t); - if (errors.size() < MAX_INVALIDATION_WARNINGS_PER_PROPERTY) { - JSError error = mis.src; - if (error.getType().equals(TypeValidator.TYPE_MISMATCH_WARNING) - && error.description.isEmpty()) { - String msg = "Implicit use of type " + mis.typeA + " as " + mis.typeB; - error = JSError.make(error.node, TypeValidator.TYPE_MISMATCH_WARNING, msg); - } - errors.add(error); - } - } - } - - /** - * Invalidates the given type, so that no properties on it will be renamed. - */ - private void recordInvalidatingType(TypeI type, TypeMismatch mis) { - type = type.restrictByNotNullOrUndefined(); - if (type.isUnionType()) { - for (TypeI alt : type.getUnionMembers()) { - recordInvalidatingType(alt, mis); - } - } else if (type.isEnumElement()) { - recordInvalidatingType(type.getEnumeratedTypeOfEnumElement(), mis); - } else { - addInvalidatingType(type); - recordInvalidationError(type, mis); - ObjectTypeI objType = type == null ? null : type.toMaybeObjectType(); - ObjectTypeI proto = objType == null ? null : objType.getPrototypeObject(); - if (objType != null && proto != null) { - addInvalidatingType(proto); - recordInvalidationError(proto, mis); - } - if (objType != null - && objType.isConstructor() && objType.isFunctionType()) { - addInvalidatingType(objType.toMaybeFunctionType().getInstanceType()); - } - } - } - - /** Returns the property for the given name, creating it if necessary. */ protected Property getProperty(String name) { if (!properties.containsKey(name)) { @@ -438,7 +374,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { Property prop = getProperty(n.getLastChild().getString()); // TODO(dimvar): invalidating here when isStructuralInterfacePrototype is true is // kind of arbitrary. We should only do it when the @record is implicitly implemented. - if (isInvalidatingType(recvType) || isStructuralInterfacePrototype(recv)) { + if (invalidatingTypes.isInvalidating(recvType) || isStructuralInterfacePrototype(recv)) { prop.invalidate(); } else if (!prop.skipRenaming) { prop.addTypeToSkip(recvType); @@ -664,7 +600,7 @@ private void printErrorLocations(List errors, TypeI t) { */ private TypeI processProperty(NodeTraversal t, Property prop, TypeI type, TypeI relatedType) { type = type.restrictByNotNullOrUndefined(); - if (prop.skipRenaming || isInvalidatingType(type)) { + if (prop.skipRenaming || invalidatingTypes.isInvalidating(type)) { return null; } @@ -680,7 +616,7 @@ private TypeI processProperty(NodeTraversal t, Property prop, TypeI type, TypeI return firstType; } else { TypeI topType = getTypeWithProperty(prop.name, type); - if (isInvalidatingType(topType)) { + if (invalidatingTypes.isInvalidating(topType)) { return null; } prop.addType(type, relatedType); @@ -796,11 +732,6 @@ Multimap> getRenamedTypesForTesting() { return ret; } - private void addInvalidatingType(TypeI type) { - checkState(!type.isUnionType()); - invalidatingTypes.add(type); - } - private TypeI getType(Node node) { if (node == null || node.getTypeI() == null) { return registry.getNativeType(JSTypeNative.UNKNOWN_TYPE); @@ -808,34 +739,6 @@ private TypeI getType(Node node) { return node.getTypeI(); } - /** - * Returns true if a field reference on this type will invalidate all - * references to that field as candidates for renaming. This is true if the - * type is unknown or all-inclusive, as variables with such a type could be - * references to any object. - */ - private boolean isInvalidatingType(TypeI type) { - if (type == null - || invalidatingTypes.contains(type) - || type.isUnknownType() /* unresolved types */) { - return true; - } - ObjectTypeI objType = type.toMaybeObjectType(); - if (objType != null) { - FunctionTypeI ft = objType.toMaybeFunctionType(); - // TODO(dimvar): types of most object literals are considered anonymous objects, and as such, - // a property of an object literal prevents all properties with the same name from being - // disambiguated. In OTI, this might be hard to change, but in NTI, it is easy to change - // isUnknownObject to return false for object literals. I deliberately followed the behavior - // of OTI to help with the migration, but can revisit in the future to improve - // disambiguation. - return objType.isAmbiguousObject() - // Invalidate constructors of already-invalidated types - || (ft != null && ft.isConstructor() && isInvalidatingType(ft.getInstanceType())); - } - return false; - } - /** * Returns a set of types that should be skipped given the given type. This is * necessary for interfaces, as all super interfaces must also be skipped. diff --git a/src/com/google/javascript/jscomp/InlineProperties.java b/src/com/google/javascript/jscomp/InlineProperties.java index ee4fd07df63..3197f338932 100644 --- a/src/com/google/javascript/jscomp/InlineProperties.java +++ b/src/com/google/javascript/jscomp/InlineProperties.java @@ -63,6 +63,9 @@ private static class PropertyInfo { InlineProperties(AbstractCompiler compiler) { this.compiler = compiler; this.invalidatingTypes = new InvalidatingTypes.Builder(compiler.getTypeIRegistry()) + // TODO(sdh): consider allowing inlining properties of global this + // (we already reserve extern'd names, so this should be safe). + .disallowGlobalThis() .addTypesInvalidForPropertyRenaming() // NOTE: Mismatches are less important to this pass than to (dis)ambiguate properties. // This pass doesn't remove values (it only inlines them when the type is known), so diff --git a/src/com/google/javascript/jscomp/InvalidatingTypes.java b/src/com/google/javascript/jscomp/InvalidatingTypes.java index cc58e7b2a31..032c362d22f 100644 --- a/src/com/google/javascript/jscomp/InvalidatingTypes.java +++ b/src/com/google/javascript/jscomp/InvalidatingTypes.java @@ -15,27 +15,46 @@ */ package com.google.javascript.jscomp; +import static com.google.common.base.Preconditions.checkState; + import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Multimap; import com.google.javascript.rhino.ObjectTypeI; import com.google.javascript.rhino.TypeI; import com.google.javascript.rhino.TypeIRegistry; import com.google.javascript.rhino.jstype.JSTypeNative; +import java.util.Collection; +import javax.annotation.Nullable; /** * Keeps track of "invalidating types" that force type-based - * optimizations to back off, specifically for {@link InlineProperties} - * and {@link AmbiguateProperties}. + * optimizations to back off, specifically for {@link InlineProperties}, + * {@link AmbiguateProperties}, and {@link DisambiguateProperties}. + * Note that disambiguation has slightly different behavior from the + * other two, as pointed out in implementation comments. */ final class InvalidatingTypes { + // To prevent the logs from filling up, we cap the number of warnings + // that we tell the user to fix per-property. + // TODO(sdh): this shouldn't matter once we no longer construct JSErrors + private static final int MAX_INVALIDATION_WARNINGS_PER_PROPERTY = 10; + private final ImmutableSet types; + private final boolean allowEnums; + private final boolean allowScalars; - private InvalidatingTypes(ImmutableSet types) { - this.types = types; + private InvalidatingTypes(Builder builder) { + this.types = builder.types.build(); + this.allowEnums = builder.allowEnums; + this.allowScalars = builder.allowScalars; } boolean isInvalidating(TypeI type) { + if (type == null) { + return true; + } if (type.isUnionType()) { type = type.restrictByNotNullOrUndefined(); if (type.isUnionType()) { @@ -47,32 +66,60 @@ boolean isInvalidating(TypeI type) { return false; } } + ObjectTypeI objType = type.toMaybeObjectType(); - return objType == null - || types.contains(objType) + + if (objType == null) { + return !allowScalars; + } + return types.contains(objType) || objType.isAmbiguousObject() - || objType.isUnknownType() - || objType.isBottom() - || objType.isEnumObject() - || objType.isBoxableScalar(); + || objType.isUnknownType() // TODO(sdh): remove when OTI gone (always false in NTI) + || objType.isBottom() // TODO(sdh): remove when OTI gone (always false in NTI) + || (!allowEnums && objType.isEnumObject()) + || (!allowScalars && objType.isBoxableScalar()); } static final class Builder { private final ImmutableSet.Builder types = ImmutableSet.builder(); private final TypeIRegistry registry; + private boolean allowEnums = false; + private boolean allowScalars = false; Builder(TypeIRegistry registry) { this.registry = registry; } InvalidatingTypes build() { - return new InvalidatingTypes(types.build()); + return new InvalidatingTypes(this); + } + + // TODO(sdh): Investigate whether this can be consolidated between all three passes. + // In particular, mutation testing suggests allowEnums=true should work everywhere. + // We should revisit what breaks when we disallow scalars everywhere. + Builder allowEnumsAndScalars() { + // Ambiguate and Inline do not allow enums or scalars. + this.allowEnums = this.allowScalars = true; + return this; + } + + Builder disallowGlobalThis() { + // Disambiguate does not invalidate global this because it + // sets skipping explicitly for extern properties only on + // the extern types. + types.add(registry.getNativeType(JSTypeNative.GLOBAL_THIS)); + return this; } Builder addAllTypeMismatches(Iterable mismatches) { + return addAllTypeMismatches(mismatches, null); + } + + Builder addAllTypeMismatches( + Iterable mismatches, @Nullable Multimap invalidationMap) { for (TypeMismatch mis : mismatches) { - addType(mis.typeA); - addType(mis.typeB); + addType(mis.typeA, mis, invalidationMap); + addType(mis.typeB, mis, invalidationMap); } return this; } @@ -83,7 +130,6 @@ Builder addTypesInvalidForPropertyRenaming() { registry.getNativeType(JSTypeNative.FUNCTION_FUNCTION_TYPE), registry.getNativeType(JSTypeNative.FUNCTION_INSTANCE_TYPE), registry.getNativeType(JSTypeNative.FUNCTION_PROTOTYPE), - registry.getNativeType(JSTypeNative.GLOBAL_THIS), registry.getNativeType(JSTypeNative.OBJECT_TYPE), registry.getNativeType(JSTypeNative.OBJECT_PROTOTYPE), registry.getNativeType(JSTypeNative.OBJECT_FUNCTION_TYPE), @@ -91,20 +137,57 @@ Builder addTypesInvalidForPropertyRenaming() { return this; } - /** Invalidates the given type, so that no properties on it will be inlined. */ - Builder addType(TypeI type) { + /** Invalidates the given type, so that no properties on it will be inlined or renamed. */ + private Builder addType( + TypeI type, TypeMismatch mismatch, @Nullable Multimap invalidationMap) { type = type.restrictByNotNullOrUndefined(); if (type.isUnionType()) { for (TypeI alt : type.getUnionMembers()) { - addType(alt); + addType(alt, mismatch, invalidationMap); + } + } else if (type.isEnumElement()) { // only in disamb + addType(type.getEnumeratedTypeOfEnumElement(), mismatch, invalidationMap); + } else { // amb and inl both do this without the else + checkState(!type.isUnionType()); + types.add(type); + recordInvalidation(type, mismatch, invalidationMap); + + ObjectTypeI objType = type.toMaybeObjectType(); + if (objType != null) { + ObjectTypeI proto = objType.getPrototypeObject(); + if (proto != null) { + types.add(proto); + recordInvalidation(proto, mismatch, invalidationMap); + } + if (objType.isConstructor()) { + types.add(objType.toMaybeFunctionType().getInstanceType()); + } else if (objType.isInstanceType()) { + types.add(objType.getConstructor()); + } } - } - types.add(type); - ObjectTypeI objType = type.toMaybeObjectType(); - if (objType != null && objType.isInstanceType()) { - types.add(objType.getPrototypeObject()); } return this; } + + private void recordInvalidation( + TypeI t, TypeMismatch mis, @Nullable Multimap invalidationMap) { + // TODO(sdh): Replace TypeMismatch#src with a (nullable?) Node, + // then generate the relevant errors later. + if (!t.isObjectType()) { + return; + } + if (invalidationMap != null) { + Collection errors = invalidationMap.get(t); + if (errors.size() < MAX_INVALIDATION_WARNINGS_PER_PROPERTY) { + JSError error = mis.src; + if (error.getType().equals(TypeValidator.TYPE_MISMATCH_WARNING) + && error.description.isEmpty()) { + String msg = "Implicit use of type " + mis.typeA + " as " + mis.typeB; + error = JSError.make(error.node, TypeValidator.TYPE_MISMATCH_WARNING, msg); + } + errors.add(error); + } + } + } } } diff --git a/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java b/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java index 3905f53417c..0730138ce26 100644 --- a/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java +++ b/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java @@ -1024,4 +1024,13 @@ public void testObjectLitOverlappingOriginalAndGeneratedNames() { "var bar = new Bar();", "bar.a();")); } + + public void testEnum() { + // TODO(sdh): Consider removing this test if we decide that enum objects are + // okay to ambiguate (in which case, this would rename to Foo.a). + testSame( + LINE_JOINER.join( + "/** @enum {string} */ var Foo = {X: 'y'};", + "var x = Foo.X")); + } } diff --git a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java index eff4212c14e..a81888a1b51 100644 --- a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java +++ b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java @@ -453,11 +453,6 @@ public void testIgnoreUnknownType1() { "/** @return {Object} */", "var U = function() { return {} };", "U().blah();"); - - this.mode = TypeInferenceMode.OTI_ONLY; - testSets(js, "{blah=[[Foo.prototype]]}"); - - this.mode = TypeInferenceMode.NTI_ONLY; testSets(js, "{}"); }