Skip to content

Commit

Permalink
Automated g4 rollback of changelist 166550770.
Browse files Browse the repository at this point in the history 
*** Reason for rollback ***

Breaks tests.

*** Original change description ***

Clean up disambiguation invalidation reporting.

TypeMismatch now stores a Supplier<JSError> rather than a JSError.  This allows simplifying the logic whereby non-reported error messages were delayed, and removing the disambiguation-specific message limiting from InvalidatingTypes.

***

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=166654473
  • Loading branch information
@shicks @blickly
shicks authored and blickly committed Aug 28, 2017
1 parent 2e69b00 commit ed1af10
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 74 deletions.
27 changes: 8 additions & 19 deletions src/com/google/javascript/jscomp/DisambiguateProperties.java
View file
Expand Up @@ -19,9 +19,6 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.LinkedHashMultimap;
Expand Down Expand Up @@ -84,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 Down Expand Up @@ -121,7 +115,7 @@ static class Warnings {
* Map of a type to all the related errors that invalidated the type
* for disambiguation.
*/
private final Multimap<TypeI, Supplier<JSError>> invalidationMap;
private final Multimap<TypeI, JSError> invalidationMap;

/**
* In practice any large code base will have thousands and thousands of
Expand Down Expand Up @@ -334,15 +328,12 @@ boolean scheduleRenaming(Node node, TypeI type) {

this.propertiesToErrorFor = propertiesToErrorFor;
this.invalidationMap =
propertiesToErrorFor.isEmpty()
? null
: LinkedHashMultimap.<TypeI, Supplier<JSError>>create();
propertiesToErrorFor.isEmpty() ? null : LinkedHashMultimap.<TypeI, JSError>create();

this.invalidatingTypes = new InvalidatingTypes.Builder(registry)
.recordInvalidations(this.invalidationMap)
.addTypesInvalidForPropertyRenaming()
.addAllTypeMismatches(compiler.getTypeMismatches())
.addAllTypeMismatches(compiler.getImplicitInterfaceUses())
.addAllTypeMismatches(compiler.getTypeMismatches(), this.invalidationMap)
.addAllTypeMismatches(compiler.getImplicitInterfaceUses(), this.invalidationMap)
.allowEnumsAndScalars()
.build();
}
Expand Down Expand Up @@ -593,12 +584,10 @@ private void printErrorLocations(List<String> errors, TypeI t) {
return;
}

Iterable<JSError> invalidations =
FluentIterable.from(invalidationMap.get(t))
.transform(Suppliers.<JSError>supplierFunction())
.limit(MAX_INVALIDATION_WARNINGS_PER_PROPERTY);
for (JSError error : invalidations) {
errors.add(t + " at " + error.sourceName + ":" + error.lineNumber);
for (JSError error : invalidationMap.get(t)) {
if (error != null) {
errors.add(t + " at " + error.sourceName + ":" + error.lineNumber);
}
}
}

Expand Down
49 changes: 33 additions & 16 deletions src/com/google/javascript/jscomp/InvalidatingTypes.java
View file
Expand Up @@ -17,14 +17,14 @@

import static com.google.common.base.Preconditions.checkState;

import com.google.common.base.Supplier;
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;

/**
Expand All @@ -36,6 +36,11 @@
*/
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<TypeI> types;
private final boolean allowEnums;
private final boolean allowScalars;
Expand Down Expand Up @@ -80,7 +85,6 @@ static final class Builder {
private final TypeIRegistry registry;
private boolean allowEnums = false;
private boolean allowScalars = false;
@Nullable private Multimap<TypeI, Supplier<JSError>> invalidationMap;

Builder(TypeIRegistry registry) {
this.registry = registry;
Expand All @@ -93,11 +97,6 @@ InvalidatingTypes build() {
// 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 recordInvalidations(@Nullable Multimap<TypeI, Supplier<JSError>> invalidationMap) {
this.invalidationMap = invalidationMap;
return this;
}

Builder allowEnumsAndScalars() {
// Ambiguate and Inline do not allow enums or scalars.
this.allowEnums = this.allowScalars = true;
Expand All @@ -113,9 +112,14 @@ Builder disallowGlobalThis() {
}

Builder addAllTypeMismatches(Iterable<TypeMismatch> mismatches) {
return addAllTypeMismatches(mismatches, null);
}

Builder addAllTypeMismatches(
Iterable<TypeMismatch> mismatches, @Nullable Multimap<TypeI, JSError> invalidationMap) {
for (TypeMismatch mis : mismatches) {
addType(mis.typeA, mis);
addType(mis.typeB, mis);
addType(mis.typeA, mis, invalidationMap);
addType(mis.typeB, mis, invalidationMap);
}
return this;
}
Expand All @@ -134,25 +138,26 @@ Builder addTypesInvalidForPropertyRenaming() {
}

/** Invalidates the given type, so that no properties on it will be inlined or renamed. */
private Builder addType(TypeI type, TypeMismatch mismatch) {
private Builder addType(
TypeI type, TypeMismatch mismatch, @Nullable Multimap<TypeI, JSError> invalidationMap) {
type = type.restrictByNotNullOrUndefined();
if (type.isUnionType()) {
for (TypeI alt : type.getUnionMembers()) {
addType(alt, mismatch);
addType(alt, mismatch, invalidationMap);
}
} else if (type.isEnumElement()) { // only in disamb
addType(type.getEnumeratedTypeOfEnumElement(), mismatch);
addType(type.getEnumeratedTypeOfEnumElement(), mismatch, invalidationMap);
} else { // amb and inl both do this without the else
checkState(!type.isUnionType());
types.add(type);
recordInvalidation(type, mismatch);
recordInvalidation(type, mismatch, invalidationMap);

ObjectTypeI objType = type.toMaybeObjectType();
if (objType != null) {
ObjectTypeI proto = objType.getPrototypeObject();
if (proto != null) {
types.add(proto);
recordInvalidation(proto, mismatch);
recordInvalidation(proto, mismatch, invalidationMap);
}
if (objType.isConstructor()) {
types.add(objType.toMaybeFunctionType().getInstanceType());
Expand All @@ -164,12 +169,24 @@ private Builder addType(TypeI type, TypeMismatch mismatch) {
return this;
}

private void recordInvalidation(TypeI t, TypeMismatch mis) {
private void recordInvalidation(
TypeI t, TypeMismatch mis, @Nullable Multimap<TypeI, JSError> invalidationMap) {
// TODO(sdh): Replace TypeMismatch#src with a (nullable?) Node,
// then generate the relevant errors later.
if (!t.isObjectType()) {
return;
}
if (invalidationMap != null) {
invalidationMap.put(t, mis.error);
Collection<JSError> 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);
}
}
}
}
Expand Down
51 changes: 12 additions & 39 deletions src/com/google/javascript/jscomp/TypeMismatch.java
View file
Expand Up @@ -15,11 +15,6 @@
*/
package com.google.javascript.jscomp;

import static com.google.common.base.Preconditions.checkState;

import com.google.auto.value.AutoValue;
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
import com.google.javascript.rhino.FunctionTypeI;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.ObjectTypeI;
Expand All @@ -39,17 +34,17 @@
class TypeMismatch implements Serializable {
final TypeI typeA;
final TypeI typeB;
final Supplier<JSError> error;
final JSError src;

/**
* It's the responsibility of the class that creates the
* {@code TypeMismatch} to ensure that {@code a} and {@code b} are
* non-matching types.
*/
TypeMismatch(TypeI a, TypeI b, Supplier<JSError> error) {
TypeMismatch(TypeI a, TypeI b, JSError src) {
this.typeA = a;
this.typeB = b;
this.error = error;
this.src = src;
}

static void registerIfMismatch(
Expand All @@ -72,12 +67,12 @@ static void registerMismatch(
!found.isSubtypeWithoutStructuralTyping(required)
&& !required.isSubtypeWithoutStructuralTyping(found);
if (strictMismatch) {
implicitInterfaceUses.add(new TypeMismatch(found, required, Suppliers.ofInstance(error)));
implicitInterfaceUses.add(new TypeMismatch(found, required, error));
}
return;
}

mismatches.add(new TypeMismatch(found, required, Suppliers.ofInstance(error)));
mismatches.add(new TypeMismatch(found, required, error));

if (found.isFunctionType() && required.isFunctionType()) {
FunctionTypeI fnTypeA = found.toMaybeFunctionType();
Expand All @@ -95,22 +90,21 @@ static void registerMismatch(
}

static void recordImplicitUseOfNativeObject(
List<TypeMismatch> mismatches, Node node, TypeI sourceType, TypeI targetType) {
List<TypeMismatch> mismatches, Node src, TypeI sourceType, TypeI targetType) {
sourceType = sourceType.restrictByNotNullOrUndefined();
targetType = targetType.restrictByNotNullOrUndefined();
if (sourceType.isInstanceofObject()
&& !targetType.isInstanceofObject() && !targetType.isUnknownType()) {
// We don't report a type error, but we still need to construct a JSError,
// for people who enable the invalidation diagnostics in DisambiguateProperties.
LazyError err =
LazyError.of(
"Implicit use of Object type: %s as type: %s", node, sourceType, targetType);
String msg = "Implicit use of Object type: " + sourceType + " as type: " + targetType;
JSError err = JSError.make(src, TypeValidator.TYPE_MISMATCH_WARNING, msg);
mismatches.add(new TypeMismatch(sourceType, targetType, err));
}
}

static void recordImplicitInterfaceUses(
List<TypeMismatch> implicitInterfaceUses, Node node, TypeI sourceType, TypeI targetType) {
List<TypeMismatch> implicitInterfaceUses, Node src, TypeI sourceType, TypeI targetType) {
sourceType = removeNullUndefinedAndTemplates(sourceType);
targetType = removeNullUndefinedAndTemplates(targetType);
if (targetType.isUnknownType()) {
Expand All @@ -123,7 +117,9 @@ static void recordImplicitInterfaceUses(
if (strictMismatch || mismatch) {
// We don't report a type error, but we still need to construct a JSError,
// for people who enable the invalidation diagnostics in DisambiguateProperties.
LazyError err = LazyError.of("Implicit use of type %s as %s", node, sourceType, targetType);
// Use the empty string as the error string. Creating an actual error message can be slow
// for large types; we create an error string lazily in DisambiguateProperties.
JSError err = JSError.make(src, TypeValidator.TYPE_MISMATCH_WARNING, "");
implicitInterfaceUses.add(new TypeMismatch(sourceType, targetType, err));
}
}
Expand Down Expand Up @@ -153,27 +149,4 @@ private static TypeI removeNullUndefinedAndTemplates(TypeI t) {
@Override public String toString() {
return "(" + typeA + ", " + typeB + ")";
}

@AutoValue
abstract static class LazyError implements Supplier<JSError>, Serializable {
abstract String message();
abstract Node node();
abstract TypeI sourceType();
abstract TypeI targetType();

private static LazyError of(String message, Node node, TypeI sourceType, TypeI targetType) {
return new AutoValue_TypeMismatch_LazyError(message, node, sourceType, targetType);
}

@Override
public JSError get() {
// NOTE: GWT does not support String.format, so we work around it with a quick hack.
String[] parts = message().split("%s");
checkState(parts.length == 3);
return JSError.make(
node(),
TypeValidator.TYPE_MISMATCH_WARNING,
parts[0] + sourceType() + parts[1] + targetType() + parts[2]);
}
}
}

0 comments on commit ed1af10

Please sign in to comment.