Skip to content

Commit

Permalink
Consolidate DisambiguateProperties to use TypeInvalidator.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@shicks
shicks authored and Tyler Breisacher committed Jul 12, 2017
1 parent a43fc6d commit 8530f37
Show file tree
Hide file tree
Showing 6 changed files with 140 additions and 144 deletions.
9 changes: 6 additions & 3 deletions src/com/google/javascript/jscomp/AmbiguateProperties.java
View file
Expand Up @@ -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())
Expand Down Expand Up @@ -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()));
Expand Down
131 changes: 17 additions & 114 deletions src/com/google/javascript/jscomp/DisambiguateProperties.java
View file
Expand Up @@ -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());
Expand All @@ -107,7 +104,8 @@ static class Warnings {
}

private final AbstractCompiler compiler;
private final Set<TypeI> 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.
Expand Down Expand Up @@ -204,7 +202,7 @@ UnionFind<TypeI> 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;
}
Expand Down Expand Up @@ -309,7 +307,7 @@ boolean invalidate() {
*/
boolean scheduleRenaming(Node node, TypeI type) {
if (!skipRenaming) {
if (isInvalidatingType(type)) {
if (invalidatingTypes.isInvalidating(type)) {
invalidate();
return false;
}
Expand All @@ -327,40 +325,24 @@ 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.<TypeI, JSError>create();

this.invalidatingTypes = new InvalidatingTypes.Builder(registry)
.addTypesInvalidForPropertyRenaming()
.addAllTypeMismatches(compiler.getTypeMismatches(), this.invalidationMap)
.addAllTypeMismatches(compiler.getImplicitInterfaceUses(), this.invalidationMap)
.allowEnumsAndScalars()
.build();
}

@Override
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
Expand All @@ -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<JSError> 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)) {
Expand All @@ -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);
Expand Down Expand Up @@ -664,7 +600,7 @@ private void printErrorLocations(List<String> 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;
}

Expand All @@ -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);
Expand Down Expand Up @@ -796,46 +732,13 @@ Multimap<String, Collection<TypeI>> 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);
}
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.
Expand Down
3 changes: 3 additions & 0 deletions src/com/google/javascript/jscomp/InlineProperties.java
View file
Expand Up @@ -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
Expand Down

0 comments on commit 8530f37

Please sign in to comment.