diff --git a/src/com/google/javascript/jscomp/DisambiguateProperties.java b/src/com/google/javascript/jscomp/DisambiguateProperties.java index 5d4ee056464..960dcb2541f 100644 --- a/src/com/google/javascript/jscomp/DisambiguateProperties.java +++ b/src/com/google/javascript/jscomp/DisambiguateProperties.java @@ -72,7 +72,7 @@ * * */ -class DisambiguateProperties implements CompilerPass { +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_INVALDIATION_WARNINGS_PER_PROPERTY = 10; @@ -95,7 +95,7 @@ static class Warnings { } private final AbstractCompiler compiler; - private final TypeSystem typeSystem; + private final TypeSystem typeSystem; /** * Map of a type to all the related errors that invalidated the type @@ -119,13 +119,13 @@ private class Property { final String name; /** All types on which the field exists, grouped together if related. */ - private UnionFind types; + private UnionFind types; /** * A set of types for which renaming this field should be skipped. This * list is first filled by fields defined in the externs file. */ - Set typesToSkip = new HashSet<>(); + Set typesToSkip = new HashSet<>(); /** * If true, do not rename any instance of this field, as it has been @@ -141,20 +141,20 @@ private class Property { * field for that node. In the case of a union, the type is the highest type * of one of the types in the union. */ - final Map rootTypes = new HashMap<>(); + final Map rootTypes = new HashMap<>(); /** * For every property p and type t, we only need to run recordInterfaces * once. Use this cache to avoid needless calls. */ - private final Set recordInterfacesCache = new HashSet<>(); + private final Set recordInterfacesCache = new HashSet<>(); Property(String name) { this.name = name; } /** Returns the types on which this field is referenced. */ - UnionFind getTypes() { + UnionFind getTypes() { if (types == null) { types = new StandardUnionFind<>(); } @@ -166,9 +166,9 @@ UnionFind getTypes() { * @return true if the type was recorded for this property, else false, * which would happen if the type was invalidating. */ - boolean addType(T type, T relatedType) { + boolean addType(JSType type, JSType relatedType) { checkState(!skipRenaming, "Attempt to record skipped property: %s", name); - T top = typeSystem.getTypeWithProperty(this.name, type); + JSType top = typeSystem.getTypeWithProperty(this.name, type); if (typeSystem.isInvalidatingType(top)) { invalidate(); return false; @@ -189,8 +189,8 @@ boolean addType(T type, T relatedType) { } /** Records the given type as one to skip for this property. */ - void addTypeToSkip(T type) { - for (T skipType : typeSystem.getTypesToSkipForType(type)) { + void addTypeToSkip(JSType type) { + for (JSType skipType : typeSystem.getTypesToSkipForType(type)) { typesToSkip.add(skipType); getTypes().union(skipType, type); } @@ -208,23 +208,23 @@ void expandTypesToSkip() { // Make sure that the representative type for each type to skip is // marked as being skipped. - Set rootTypesToSkip = new HashSet<>(); - for (T subType : typesToSkip) { + Set rootTypesToSkip = new HashSet<>(); + for (JSType subType : typesToSkip) { rootTypesToSkip.add(types.find(subType)); } typesToSkip.addAll(rootTypesToSkip); - Set newTypesToSkip = new HashSet<>(); - Set allTypes = types.elements(); + Set newTypesToSkip = new HashSet<>(); + Set allTypes = types.elements(); int originalTypesSize = allTypes.size(); - for (T subType : allTypes) { + for (JSType subType : allTypes) { if (!typesToSkip.contains(subType) && typesToSkip.contains(types.find(subType))) { newTypesToSkip.add(subType); } } - for (T newType : newTypesToSkip) { + for (JSType newType : newTypesToSkip) { addTypeToSkip(newType); } @@ -247,7 +247,7 @@ boolean shouldRename() { * expandTypesToSkip() should be called before this, if anything has been * added to the typesToSkip list. */ - boolean shouldRename(T type) { + boolean shouldRename(JSType type) { return !skipRenaming && !typesToSkip.contains(type); } @@ -271,7 +271,7 @@ boolean invalidate() { * was already invalidated. False if this property was invalidated this * time. */ - boolean scheduleRenaming(Node node, T type) { + boolean scheduleRenaming(Node node, JSType type) { if (!skipRenaming) { if (typeSystem.isInvalidatingType(type)) { invalidate(); @@ -286,11 +286,11 @@ boolean scheduleRenaming(Node node, T type) { private Map properties = new HashMap<>(); - static DisambiguateProperties forJSTypeSystem( + static DisambiguateProperties forJSTypeSystem( AbstractCompiler compiler, Map propertiesToErrorFor) { - return new DisambiguateProperties<>( - compiler, new JSTypeSystem(compiler), propertiesToErrorFor); + return new DisambiguateProperties( + compiler, new TypeSystem(compiler), propertiesToErrorFor); } /** @@ -298,7 +298,7 @@ static DisambiguateProperties forJSTypeSystem( * above for either the JSType system, or the concrete type system. */ private DisambiguateProperties(AbstractCompiler compiler, - TypeSystem typeSystem, Map propertiesToErrorFor) { + TypeSystem typeSystem, Map propertiesToErrorFor) { this.compiler = compiler; this.typeSystem = typeSystem; this.propertiesToErrorFor = propertiesToErrorFor; @@ -363,8 +363,10 @@ private void addInvalidatingType(JSType type, JSError error) { typeSystem.addInvalidatingType(objType.getImplicitPrototype()); recordInvalidationError(objType.getImplicitPrototype(), error); } - if (objType != null && objType.isConstructor() && objType.isFunctionType()) { - typeSystem.addInvalidatingType(objType.toMaybeFunctionType().getInstanceType()); + if (objType != null + && objType.isConstructor() && objType.isFunctionType()) { + typeSystem.addInvalidatingType( + objType.toMaybeFunctionType().getInstanceType()); } } } @@ -379,13 +381,13 @@ protected Property getProperty(String name) { } /** Public for testing. */ - T getTypeWithProperty(String field, T type) { + JSType getTypeWithProperty(String field, JSType type) { return typeSystem.getTypeWithProperty(field, type); } /** Tracks the current type system scope while traversing. */ private abstract class AbstractScopingCallback implements ScopedCallback { - protected final Stack> scopes = + protected final Stack> scopes = new Stack<>(); @Override @@ -408,7 +410,7 @@ public void exitScope(NodeTraversal t) { } /** Returns the current scope at this point in the file. */ - protected StaticTypedScope getScope() { + protected StaticTypedScope getScope() { return scopes.peek(); } } @@ -422,7 +424,7 @@ private class FindExternProperties extends AbstractScopingCallback { // TODO(johnlenz): Support object-literal property definitions. if (n.isGetProp()) { String field = n.getLastChild().getString(); - T type = typeSystem.getType(getScope(), n.getFirstChild(), field); + JSType type = typeSystem.getType(getScope(), n.getFirstChild(), field); Property prop = getProperty(field); if (typeSystem.isInvalidatingType(type)) { prop.invalidate(); @@ -457,10 +459,12 @@ public void visit(NodeTraversal t, Node n, Node parent) { private void handleGetProp(NodeTraversal t, Node n) { String name = n.getLastChild().getString(); - T type = typeSystem.getType(getScope(), n.getFirstChild(), name); + JSType type = typeSystem.getType(getScope(), n.getFirstChild(), name); Property prop = getProperty(name); - if (!prop.scheduleRenaming(n.getLastChild(), processProperty(t, prop, type, null)) + if (!prop.scheduleRenaming( + n.getLastChild(), + processProperty(t, prop, type, null)) && propertiesToErrorFor.containsKey(name)) { String suggestion = ""; if (type instanceof JSType) { @@ -482,8 +486,9 @@ private void handleGetProp(NodeTraversal t, Node n) { } } } - compiler.report(JSError.make(n, propertiesToErrorFor.get(name), Warnings.INVALIDATION, name, - (String.valueOf(type)), n.toString(), suggestion)); + compiler.report(JSError.make(n, propertiesToErrorFor.get(name), + Warnings.INVALIDATION, name, String.valueOf(type), n.toString(), + suggestion)); } } @@ -498,7 +503,7 @@ private void handleObjectLit(NodeTraversal t, Node n) { // We should never see a mix of numbers and strings. String name = child.getString(); - T type = typeSystem.getType(getScope(), n, name); + JSType type = typeSystem.getType(getScope(), n, name); Property prop = getProperty(name); if (!prop.scheduleRenaming(child, @@ -507,7 +512,7 @@ private void handleObjectLit(NodeTraversal t, Node n) { // case right now. if (propertiesToErrorFor.containsKey(name)) { compiler.report(JSError.make(child, propertiesToErrorFor.get(name), - Warnings.INVALIDATION, name, (String.valueOf(type)), n.toString(), "")); + Warnings.INVALIDATION, name, String.valueOf(type), n.toString(), "")); } } } @@ -541,25 +546,25 @@ private void printErrorLocations(List errors, JSType t) { * case of a union type, it will be the highest type on the prototype * chain of one of the members of the union. */ - private T processProperty( - NodeTraversal t, Property prop, T type, T relatedType) { + private JSType processProperty( + NodeTraversal t, Property prop, JSType type, JSType relatedType) { type = typeSystem.restrictByNotNullOrUndefined(type); if (prop.skipRenaming || typeSystem.isInvalidatingType(type)) { return null; } - Iterable alternatives = typeSystem.getTypeAlternatives(type); + Iterable alternatives = typeSystem.getTypeAlternatives(type); if (alternatives != null) { - T firstType = relatedType; - for (T subType : alternatives) { - T lastType = processProperty(t, prop, subType, firstType); + JSType firstType = relatedType; + for (JSType subType : alternatives) { + JSType lastType = processProperty(t, prop, subType, firstType); if (lastType != null) { firstType = firstType == null ? lastType : firstType; } } return firstType; } else { - T topType = typeSystem.getTypeWithProperty(prop.name, type); + JSType topType = typeSystem.getTypeWithProperty(prop.name, type); if (typeSystem.isInvalidatingType(topType)) { return null; } @@ -577,7 +582,7 @@ void renameProperties() { Set reported = new HashSet<>(); for (Property prop : properties.values()) { if (prop.shouldRename()) { - Map propNames = buildPropNames(prop.getTypes(), prop.name); + Map propNames = buildPropNames(prop.getTypes(), prop.name); ++propsRenamed; prop.expandTypesToSkip(); @@ -585,7 +590,7 @@ void renameProperties() { // we iterate over all accesses of a property, which can be in very // different places in the code. for (Node node : prop.renameNodes) { - T rootType = prop.rootTypes.get(node); + JSType rootType = prop.rootTypes.get(node); if (prop.shouldRename(rootType)) { String newName = propNames.get(rootType); node.setString(newName); @@ -627,13 +632,13 @@ void renameProperties() { * Chooses a name to use for renaming in each equivalence class and maps * each type in that class to it. */ - private Map buildPropNames(UnionFind types, String name) { - Map names = new HashMap<>(); - for (Set set : types.allEquivalenceClasses()) { + private Map buildPropNames(UnionFind types, String name) { + Map names = new HashMap<>(); + for (Set set : types.allEquivalenceClasses()) { checkState(!set.isEmpty()); String typeName = null; - for (T type : set) { + for (JSType type : set) { if (typeName == null || type.toString().compareTo(typeName) < 0) { typeName = type.toString(); } @@ -647,7 +652,7 @@ private Map buildPropNames(UnionFind types, String name) { + name; } - for (T type : set) { + for (JSType type : set) { names.put(type, newName); } } @@ -655,12 +660,12 @@ private Map buildPropNames(UnionFind types, String name) { } /** Returns a map from field name to types for which it will be renamed. */ - Multimap> getRenamedTypesForTesting() { - Multimap> ret = HashMultimap.create(); + Multimap> getRenamedTypesForTesting() { + Multimap> ret = HashMultimap.create(); for (Map.Entry entry : properties.entrySet()) { Property prop = entry.getValue(); if (!prop.skipRenaming) { - for (Collection c : prop.getTypes().allEquivalenceClasses()) { + for (Collection c : prop.getTypes().allEquivalenceClasses()) { if (!c.isEmpty() && !prop.typesToSkip.contains(c.iterator().next())) { ret.put(entry.getKey(), c); } @@ -670,93 +675,14 @@ Multimap> getRenamedTypesForTesting() { return ret; } - /** Interface for providing the type information needed by this pass. */ - private interface TypeSystem { - // TODO(user): add a getUniqueName(T type) method that is guaranteed - // to be unique, performant and human-readable. - - /** Returns the top-most scope used by the type system (if any). */ - StaticTypedScope getRootScope(); - - /** Returns the new scope started at the given function node. */ - StaticTypedScope getFunctionScope(Node node); - - /** - * Returns the type of the given node. - * @param prop Only types with this property need to be returned. In general - * with type tightening, this will require no special processing, but in - * the case of an unknown JSType, we might need to add in the native - * types since we don't track them, but only if they have the given - * property. - */ - T getType(StaticTypedScope scope, Node node, String prop); - - /** - * 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. - */ - boolean isInvalidatingType(T type); - - /** - * Informs the given type system that a type is invalidating due to a type - * mismatch found during type checking. - */ - void addInvalidatingType(JSType type); - - /** - * Returns a set of types that should be skipped given the given type. - * This is necessary for interfaces when using JSTypes, as all super - * interfaces must also be skipped. - */ - ImmutableSet getTypesToSkipForType(T type); - - /** - * Determines whether the given type is one whose properties should not be - * considered for renaming. - */ - boolean isTypeToSkip(T type); - - /** Remove null and undefined from the options in the given type. */ - T restrictByNotNullOrUndefined(T type); - - /** - * Returns the alternatives if this is a type that represents multiple - * types, and null if not. Union and interface types can correspond to - * multiple other types. - */ - Iterable getTypeAlternatives(T type); - - /** - * Returns the type in the chain from the given type that contains the given - * field or null if it is not found anywhere. - */ - T getTypeWithProperty(String field, T type); - - /** - * Returns the type of the instance of which this is the prototype or null - * if this is not a function prototype. - */ - T getInstanceFromPrototype(T type); - - /** - * Records that this property could be referenced from any interface that - * this type, or any type in its superclass chain, implements. - */ - void recordInterfaces(T type, T relatedType, - DisambiguateProperties.Property p); - } - - /** Implementation of TypeSystem using JSTypes. */ - private static class JSTypeSystem implements TypeSystem { + private static class TypeSystem { private final Set invalidatingTypes; // FunctionType#getImplementedInterfaces() is slow, so we use this cache // in recordInterfaces to call it just once per constructor. private final Map> implementedInterfaces; private JSTypeRegistry registry; - public JSTypeSystem(AbstractCompiler compiler) { + public TypeSystem(AbstractCompiler compiler) { registry = compiler.getTypeRegistry(); implementedInterfaces = new HashMap<>(); invalidatingTypes = new HashSet<>(ImmutableSet.of( @@ -770,18 +696,20 @@ public JSTypeSystem(AbstractCompiler compiler) { registry.getNativeType(JSTypeNative.UNKNOWN_TYPE))); } - @Override public void addInvalidatingType(JSType type) { + public void addInvalidatingType(JSType type) { checkState(!type.isUnionType()); invalidatingTypes.add(type); } - @Override public StaticTypedScope getRootScope() { return null; } + public StaticTypedScope getRootScope() { + return null; + } - @Override public StaticTypedScope getFunctionScope(Node node) { + public StaticTypedScope getFunctionScope(Node node) { return null; } - @Override public JSType getType( + public JSType getType( StaticTypedScope scope, Node node, String prop) { if (node.getJSType() == null) { return registry.getNativeType(JSTypeNative.UNKNOWN_TYPE); @@ -789,7 +717,7 @@ public JSTypeSystem(AbstractCompiler compiler) { return node.getJSType(); } - @Override public boolean isInvalidatingType(JSType type) { + public boolean isInvalidatingType(JSType type) { if (type == null || invalidatingTypes.contains(type) || type.isUnknownType() /* unresolved types */) { return true; @@ -799,7 +727,7 @@ public JSTypeSystem(AbstractCompiler compiler) { return objType != null && !objType.hasReferenceName(); } - @Override public ImmutableSet getTypesToSkipForType(JSType type) { + public ImmutableSet getTypesToSkipForType(JSType type) { type = type.restrictByNotNullOrUndefined(); if (type.isUnionType()) { ImmutableSet.Builder types = ImmutableSet.builder(); @@ -831,15 +759,15 @@ private static Set getTypesToSkipForTypeNonUnion(JSType type) { return types; } - @Override public boolean isTypeToSkip(JSType type) { + public boolean isTypeToSkip(JSType type) { return type.isEnumType() || (type.autoboxesTo() != null); } - @Override public JSType restrictByNotNullOrUndefined(JSType type) { + public JSType restrictByNotNullOrUndefined(JSType type) { return type.restrictByNotNullOrUndefined(); } - @Override public Iterable getTypeAlternatives(JSType type) { + public Iterable getTypeAlternatives(JSType type) { if (type.isUnionType()) { return type.toMaybeUnionType().getAlternatesWithoutStructuralTyping(); } else { @@ -859,7 +787,7 @@ private static Set getTypesToSkipForTypeNonUnion(JSType type) { } } - @Override public ObjectType getTypeWithProperty(String field, JSType type) { + public ObjectType getTypeWithProperty(String field, JSType type) { if (type == null) { return null; } @@ -931,7 +859,7 @@ private static Set getTypesToSkipForTypeNonUnion(JSType type) { return foundType; } - @Override public JSType getInstanceFromPrototype(JSType type) { + public JSType getInstanceFromPrototype(JSType type) { if (type.isFunctionPrototypeType()) { ObjectType prototype = (ObjectType) type; FunctionType owner = prototype.getOwnerFunction(); @@ -942,9 +870,8 @@ private static Set getTypesToSkipForTypeNonUnion(JSType type) { return null; } - @Override - public void recordInterfaces(JSType type, JSType relatedType, - DisambiguateProperties.Property p) { + public void recordInterfaces( + JSType type, JSType relatedType, DisambiguateProperties.Property p) { ObjectType objType = ObjectType.cast(type); if (objType == null) { return; diff --git a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java index 663626aeeb0..ed9e68980b1 100644 --- a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java +++ b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java @@ -37,7 +37,7 @@ */ public final class DisambiguatePropertiesTest extends CompilerTestCase { - private DisambiguateProperties lastPass; + private DisambiguateProperties lastPass; public DisambiguatePropertiesTest() { parseTypeInfo = true;