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, "{}"); }