Skip to content

Commit

Permalink
Roll forward of "Clean up disambiguation invalidation reporting."
Browse files Browse the repository at this point in the history 
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.

NEW: Fixed IllegalStateException caused by incorrect use of String#split.  Uses a Splitter now instead.

Automated g4 rollback of changelist 166654473.

*** Reason for rollback ***

Roll forward

*** Original change description ***

Automated g4 rollback of changelist 166550770.

*** 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=166773065
  • Loading branch information
@shicks @blickly
shicks authored and blickly committed Aug 29, 2017
1 parent d5aa638 commit f648367
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 53 deletions.
27 changes: 19 additions & 8 deletions src/com/google/javascript/jscomp/DisambiguateProperties.java
View file
Expand Up @@ -19,6 +19,9 @@


import com.google.common.annotations.VisibleForTesting; import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner; 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.HashMultimap;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
import com.google.common.collect.LinkedHashMultimap; import com.google.common.collect.LinkedHashMultimap;
Expand Down Expand Up @@ -81,6 +84,9 @@
* *
*/ */
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_INVALIDATION_WARNINGS_PER_PROPERTY = 10;


private static final Logger logger = Logger.getLogger( private static final Logger logger = Logger.getLogger(
DisambiguateProperties.class.getName()); DisambiguateProperties.class.getName());
Expand Down Expand Up @@ -115,7 +121,7 @@ static class Warnings {
* Map of a type to all the related errors that invalidated the type * Map of a type to all the related errors that invalidated the type
* for disambiguation. * for disambiguation.
*/ */
private final Multimap<TypeI, JSError> invalidationMap; private final Multimap<TypeI, Supplier<JSError>> invalidationMap;


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


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


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


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


Expand Down
49 changes: 16 additions & 33 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 static com.google.common.base.Preconditions.checkState;


import com.google.common.base.Supplier;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Multimap; import com.google.common.collect.Multimap;
import com.google.javascript.rhino.ObjectTypeI; import com.google.javascript.rhino.ObjectTypeI;
import com.google.javascript.rhino.TypeI; import com.google.javascript.rhino.TypeI;
import com.google.javascript.rhino.TypeIRegistry; import com.google.javascript.rhino.TypeIRegistry;
import com.google.javascript.rhino.jstype.JSTypeNative; import com.google.javascript.rhino.jstype.JSTypeNative;
import java.util.Collection;
import javax.annotation.Nullable; import javax.annotation.Nullable;


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


Builder(TypeIRegistry registry) { Builder(TypeIRegistry registry) {
this.registry = registry; this.registry = registry;
Expand All @@ -97,6 +93,11 @@ InvalidatingTypes build() {
// TODO(sdh): Investigate whether this can be consolidated between all three passes. // TODO(sdh): Investigate whether this can be consolidated between all three passes.
// In particular, mutation testing suggests allowEnums=true should work everywhere. // In particular, mutation testing suggests allowEnums=true should work everywhere.
// We should revisit what breaks when we disallow scalars 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() { Builder allowEnumsAndScalars() {
// Ambiguate and Inline do not allow enums or scalars. // Ambiguate and Inline do not allow enums or scalars.
this.allowEnums = this.allowScalars = true; this.allowEnums = this.allowScalars = true;
Expand All @@ -112,14 +113,9 @@ Builder disallowGlobalThis() {
} }


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

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


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


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


private void recordInvalidation( private void recordInvalidation(TypeI t, TypeMismatch mis) {
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()) { if (!t.isObjectType()) {
return; return;
} }
if (invalidationMap != null) { if (invalidationMap != null) {
Collection<JSError> errors = invalidationMap.get(t); invalidationMap.put(t, mis.error);
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
52 changes: 40 additions & 12 deletions src/com/google/javascript/jscomp/TypeMismatch.java
View file
Expand Up @@ -15,6 +15,12 @@
*/ */
package com.google.javascript.jscomp; package com.google.javascript.jscomp;


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

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


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


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


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


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


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


static void recordImplicitInterfaceUses( static void recordImplicitInterfaceUses(
List<TypeMismatch> implicitInterfaceUses, Node src, TypeI sourceType, TypeI targetType) { List<TypeMismatch> implicitInterfaceUses, Node node, TypeI sourceType, TypeI targetType) {
sourceType = removeNullUndefinedAndTemplates(sourceType); sourceType = removeNullUndefinedAndTemplates(sourceType);
targetType = removeNullUndefinedAndTemplates(targetType); targetType = removeNullUndefinedAndTemplates(targetType);
if (targetType.isUnknownType()) { if (targetType.isUnknownType()) {
Expand All @@ -117,9 +124,7 @@ static void recordImplicitInterfaceUses(
if (strictMismatch || mismatch) { if (strictMismatch || mismatch) {
// We don't report a type error, but we still need to construct a JSError, // We don't report a type error, but we still need to construct a JSError,
// for people who enable the invalidation diagnostics in DisambiguateProperties. // for people who enable the invalidation diagnostics in DisambiguateProperties.
// Use the empty string as the error string. Creating an actual error message can be slow LazyError err = LazyError.of("Implicit use of type %s as %s", node, sourceType, targetType);
// 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)); implicitInterfaceUses.add(new TypeMismatch(sourceType, targetType, err));
} }
} }
Expand Down Expand Up @@ -149,4 +154,27 @@ private static TypeI removeNullUndefinedAndTemplates(TypeI t) {
@Override public String toString() { @Override public String toString() {
return "(" + typeA + ", " + typeB + ")"; 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.
List<String> parts = Splitter.on("%s").splitToList(message());
checkState(parts.size() == 3);
return JSError.make(
node(),
TypeValidator.TYPE_MISMATCH_WARNING,
parts.get(0) + sourceType() + parts.get(1) + targetType() + parts.get(2));
}
}
} }
11 changes: 11 additions & 0 deletions test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java
View file
Expand Up @@ -1742,6 +1742,17 @@ public void testStructuralTypingWithDisambiguatePropertyRenaming1_7() throws Exc
testSets(js, js, "{}"); testSets(js, js, "{}");
} }


public void testReportImplicitUseOfStructuralInterfaceInvalidingProperty() {
test(
srcs(lines(
"/** @record */ function I() {}",
"/** @type {number} */ I.prototype.foobar;",
"/** @param {I} arg */ function f(arg) {}",
"/** @constructor */ function C() { this.foobar = 42; }",
"f(new C());")),
error(DisambiguateProperties.Warnings.INVALIDATION).withMessageContaining("foobar"));
}

public void testDisambiguatePropertiesClassCastedToUnrelatedInterface() { public void testDisambiguatePropertiesClassCastedToUnrelatedInterface() {
String js = LINE_JOINER.join( String js = LINE_JOINER.join(
"/** @interface */", "/** @interface */",
Expand Down

0 comments on commit f648367

Please sign in to comment.