From ed1af100c4c7470c6ad5d365c25a720d11e5b776 Mon Sep 17 00:00:00 2001 From: sdh Date: Sun, 27 Aug 2017 19:32:12 -0700 Subject: [PATCH] Automated g4 rollback of changelist 166550770. *** Reason for rollback *** Breaks tests. *** Original change description *** Clean up disambiguation invalidation reporting. TypeMismatch now stores a Supplier 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 --- .../jscomp/DisambiguateProperties.java | 27 +++------- .../javascript/jscomp/InvalidatingTypes.java | 49 ++++++++++++------ .../javascript/jscomp/TypeMismatch.java | 51 +++++-------------- 3 files changed, 53 insertions(+), 74 deletions(-) diff --git a/src/com/google/javascript/jscomp/DisambiguateProperties.java b/src/com/google/javascript/jscomp/DisambiguateProperties.java index e2456045a71..adb44537065 100644 --- a/src/com/google/javascript/jscomp/DisambiguateProperties.java +++ b/src/com/google/javascript/jscomp/DisambiguateProperties.java @@ -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; @@ -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()); @@ -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> invalidationMap; + private final Multimap invalidationMap; /** * In practice any large code base will have thousands and thousands of @@ -334,15 +328,12 @@ boolean scheduleRenaming(Node node, TypeI type) { this.propertiesToErrorFor = propertiesToErrorFor; this.invalidationMap = - propertiesToErrorFor.isEmpty() - ? null - : LinkedHashMultimap.>create(); + propertiesToErrorFor.isEmpty() ? null : LinkedHashMultimap.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(); } @@ -593,12 +584,10 @@ private void printErrorLocations(List errors, TypeI t) { return; } - Iterable invalidations = - FluentIterable.from(invalidationMap.get(t)) - .transform(Suppliers.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); + } } } diff --git a/src/com/google/javascript/jscomp/InvalidatingTypes.java b/src/com/google/javascript/jscomp/InvalidatingTypes.java index feb00c56131..032c362d22f 100644 --- a/src/com/google/javascript/jscomp/InvalidatingTypes.java +++ b/src/com/google/javascript/jscomp/InvalidatingTypes.java @@ -17,7 +17,6 @@ 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; @@ -25,6 +24,7 @@ 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; /** @@ -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 types; private final boolean allowEnums; private final boolean allowScalars; @@ -80,7 +85,6 @@ static final class Builder { private final TypeIRegistry registry; private boolean allowEnums = false; private boolean allowScalars = false; - @Nullable private Multimap> invalidationMap; Builder(TypeIRegistry registry) { this.registry = registry; @@ -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> invalidationMap) { - this.invalidationMap = invalidationMap; - return this; - } - Builder allowEnumsAndScalars() { // Ambiguate and Inline do not allow enums or scalars. this.allowEnums = this.allowScalars = true; @@ -113,9 +112,14 @@ Builder disallowGlobalThis() { } Builder addAllTypeMismatches(Iterable mismatches) { + return addAllTypeMismatches(mismatches, null); + } + + Builder addAllTypeMismatches( + Iterable mismatches, @Nullable Multimap 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; } @@ -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 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()); @@ -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 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 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); + } } } } diff --git a/src/com/google/javascript/jscomp/TypeMismatch.java b/src/com/google/javascript/jscomp/TypeMismatch.java index 43bbd4c8491..64c4360817e 100644 --- a/src/com/google/javascript/jscomp/TypeMismatch.java +++ b/src/com/google/javascript/jscomp/TypeMismatch.java @@ -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; @@ -39,17 +34,17 @@ class TypeMismatch implements Serializable { final TypeI typeA; final TypeI typeB; - final Supplier 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 error) { + TypeMismatch(TypeI a, TypeI b, JSError src) { this.typeA = a; this.typeB = b; - this.error = error; + this.src = src; } static void registerIfMismatch( @@ -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(); @@ -95,22 +90,21 @@ static void registerMismatch( } static void recordImplicitUseOfNativeObject( - List mismatches, Node node, TypeI sourceType, TypeI targetType) { + List 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 implicitInterfaceUses, Node node, TypeI sourceType, TypeI targetType) { + List implicitInterfaceUses, Node src, TypeI sourceType, TypeI targetType) { sourceType = removeNullUndefinedAndTemplates(sourceType); targetType = removeNullUndefinedAndTemplates(targetType); if (targetType.isUnknownType()) { @@ -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)); } } @@ -153,27 +149,4 @@ private static TypeI removeNullUndefinedAndTemplates(TypeI t) { @Override public String toString() { return "(" + typeA + ", " + typeB + ")"; } - - @AutoValue - abstract static class LazyError implements Supplier, 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]); - } - } }