Skip to content

Commit

Permalink
Be lazy about building union types in "addPropertyToType" with the as…
Browse files Browse the repository at this point in the history
…sumption that we don't add a lot of properties unnecessarily.

This reduces type scope creation time by 75% in example builds.  Because this change avoids building a union for loose missing property checks, it improves disambiguation of various function types that are collapsed by the union builder resulting in stricter missing property warnings.

These warnings are a subset of the warnings produced by "strictMissingProperties" so effectively promote these from "strictMissingProperties" to "missingProperties".

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=229772717
  • Loading branch information
concavelenz authored and lauraharker committed Jan 17, 2019
1 parent e46d074 commit 970f4cd
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 16 deletions.
42 changes: 26 additions & 16 deletions src/com/google/javascript/rhino/jstype/JSTypeRegistry.java
Expand Up @@ -56,6 +56,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.Multimap;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.Table;
import com.google.javascript.rhino.ErrorReporter;
import com.google.javascript.rhino.HamtPMap;
Expand Down Expand Up @@ -176,7 +177,8 @@ public class JSTypeRegistry implements Serializable {
private final transient Set<String> forwardDeclaredTypes;

// A map of properties to the types on which those properties have been declared.
private final Map<String, UnionTypeBuilder> typesIndexedByProperty = new HashMap<>();
private transient Multimap<String, JSType> typesIndexedByProperty =
MultimapBuilder.hashKeys().linkedHashSetValues().build();

private JSType sentinelObjectLiteral;

Expand Down Expand Up @@ -898,15 +900,16 @@ void registerDroppedPropertiesInUnion(RecordType subtype, RecordType supertype)
* show up in the type registry").
*/
public void registerPropertyOnType(String propertyName, JSType type) {
UnionTypeBuilder typeSet =
typesIndexedByProperty.computeIfAbsent(
propertyName, k -> UnionTypeBuilder.createForPropertyChecking(this));

if (isObjectLiteralThatCanBeSkipped(type)) {
type = getSentinelObjectLiteral();
}

typeSet.addAlternate(type);
if (type.isUnionType()) {
typesIndexedByProperty.putAll(propertyName, type.toMaybeUnionType().getAlternates());
} else {
typesIndexedByProperty.put(propertyName, type);
}

addReferenceTypeIndexedByProperty(propertyName, type);

// Clear cached values that depend on typesIndexedByProperty.
Expand Down Expand Up @@ -948,18 +951,24 @@ public void unregisterPropertyOnType(String propertyName, JSType type) {
}

/**
* Gets the greatest subtype of the {@code type} that has a property
* {@code propertyName} defined on it.
* Gets the greatest subtype of the {@code type} that has a property {@code propertyName} defined
* on it.
*
* <p>NOTE: Building the returned union here is an n^2 operation of relatively expensive subtype
* checks: for common properties named such as those on some generated classes this can be
* extremely expensive (programs with thousands of protos isn't uncommon resulting in millions of
* subtype relationship checks for each common property name). Currently, this is only used by
* "disambiguate properties" and there is should be removed.
*/
public JSType getGreatestSubtypeWithProperty(
JSType type, String propertyName) {
public JSType getGreatestSubtypeWithProperty(JSType type, String propertyName) {
JSType withProperty = greatestSubtypeByProperty.get(propertyName);
if (withProperty != null) {
return withProperty.getGreatestSubtype(type);
}
UnionTypeBuilder typesWithProp = typesIndexedByProperty.get(propertyName);
if (typesWithProp != null) {
JSType built = typesWithProp.build();
if (typesIndexedByProperty.containsKey(propertyName)) {
Collection<JSType> typesWithProp = typesIndexedByProperty.get(propertyName);
JSType built =
UnionTypeBuilder.createForPropertyChecking(this).addAlternates(typesWithProp).build();
greatestSubtypeByProperty.put(propertyName, built);
return built.getGreatestSubtype(type);
}
Expand Down Expand Up @@ -1007,9 +1016,8 @@ public PropDefinitionKind canPropertyBeDefined(JSType type, String propertyName)
}

if (typesIndexedByProperty.containsKey(propertyName)) {
for (JSType alt :
typesIndexedByProperty.get(propertyName).getAlternates()) {
JSType greatestSubtype = alt.getGreatestSubtype(type);
for (JSType alternative : typesIndexedByProperty.get(propertyName)) {
JSType greatestSubtype = alternative.getGreatestSubtype(type);
if (!greatestSubtype.isEmptyType()) {
// We've found a type with this property. Now we just have to make
// sure it's not a type used for internal bookkeeping.
Expand Down Expand Up @@ -2307,6 +2315,7 @@ TemplateType getTemplateType(String name) {
public void saveContents(ObjectOutputStream out) throws IOException {
out.writeObject(eachRefTypeIndexedByProperty);
out.writeObject(interfaceToImplementors);
out.writeObject(typesIndexedByProperty);
}

/**
Expand All @@ -2320,6 +2329,7 @@ public void saveContents(ObjectOutputStream out) throws IOException {
public void restoreContents(ObjectInputStream in) throws IOException, ClassNotFoundException {
eachRefTypeIndexedByProperty = (Map<String, Map<String, ObjectType>>) in.readObject();
interfaceToImplementors = (Multimap<String, FunctionType>) in.readObject();
typesIndexedByProperty = (Multimap<String, JSType>) in.readObject();
}

private FunctionBuilder nativeConstructorBuilder(String name) {
Expand Down
8 changes: 8 additions & 0 deletions src/com/google/javascript/rhino/jstype/UnionTypeBuilder.java
Expand Up @@ -51,6 +51,7 @@
import com.google.javascript.rhino.jstype.JSType.SubtypingMode;
import java.io.Serializable;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Iterator;
import java.util.List;

Expand Down Expand Up @@ -167,6 +168,13 @@ private boolean isSubtype(JSType rightType, JSType leftType) {
}
}

public UnionTypeBuilder addAlternates(Collection<JSType> c) {
for (JSType type : c) {
addAlternate(type);
}
return this;
}

/**
* Adds an alternate to the union type under construction.
*
Expand Down

0 comments on commit 970f4cd

Please sign in to comment.