Skip to content

Commit

Permalink
[NTI] Remove the failed-to-unify warning.
Browse files Browse the repository at this point in the history
As we have gradually improved unification, this warning happens less and less. After the recent improvement to enum unification, the warning should only happen due to argument contravariance when unifying function types. That's a limitation of our algorithm and not something the user can act on.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=156337842
  • Loading branch information
dimvar authored and brad4d committed May 18, 2017
1 parent 1e098cb commit 28d343c
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 64 deletions.
29 changes: 2 additions & 27 deletions src/com/google/javascript/jscomp/NewTypeInference.java
Expand Up @@ -118,11 +118,6 @@ final class NewTypeInference implements CompilerPass {
+ " Found {0} types for type variable {1}: {2},\n"
+ " when instantiating type: {3}");

static final DiagnosticType FAILED_TO_UNIFY =
DiagnosticType.warning(
"JSC_NTI_FAILED_TO_UNIFY",
"Could not instantiate type {0} with {1}.");

static final DiagnosticType INVALID_INDEX_TYPE =
DiagnosticType.warning(
"JSC_NTI_INVALID_INDEX_TYPE",
Expand Down Expand Up @@ -325,7 +320,6 @@ final class NewTypeInference implements CompilerPass {
CONST_REASSIGNED,
REFLECT_CONSTRUCTOR_EXPECTED,
CONSTRUCTOR_NOT_CALLABLE,
FAILED_TO_UNIFY,
FORIN_EXPECTS_STRING_KEY,
GLOBAL_THIS,
GOOG_BIND_EXPECTS_FUNCTION,
Expand Down Expand Up @@ -2622,15 +2616,13 @@ private ImmutableMap<String, JSType> calcTypeInstantiation(
recvType = pair.type;
typeEnv = pair.env;
}
unifyWithSubtypeWarnIfFail(
funRecvType, recvType, typeParameters, typeMultimap, receiver, isFwd);
funRecvType.unifyWith(recvType, typeParameters, typeMultimap);
}
Node arg = firstArg;
int i = 0;
while (arg != null) {
EnvTypePair pair = isFwd ? analyzeExprFwd(arg, typeEnv) : analyzeExprBwd(arg, typeEnv);
unifyWithSubtypeWarnIfFail(funType.getFormalType(i), pair.type,
typeParameters, typeMultimap, arg, isFwd);
funType.getFormalType(i).unifyWith(pair.type, typeParameters, typeMultimap);
arg = arg.getNext();
typeEnv = pair.env;
i++;
Expand Down Expand Up @@ -2667,23 +2659,6 @@ private ImmutableMap<String, JSType> calcTypeInstantiation(
return builder.build();
}

private void unifyWithSubtypeWarnIfFail(JSType genericType, JSType concreteType,
List<String> typeParameters, Multimap<String, JSType> typeMultimap,
Node toWarnOn, boolean isFwd) {
if (!genericType.unifyWith(concreteType, typeParameters, typeMultimap) && isFwd) {
// Unification may fail b/c of types irrelevant to generics, eg,
// number vs string.
// In this case, don't warn here; we'll show invalid-arg-type later.
JSType afterInstantiation = genericType.substituteGenericsWithUnknown();
if (!concreteType.isLoose()
&& !genericType.equals(afterInstantiation)
&& concreteType.isSubtypeOf(afterInstantiation)) {
warnings.add(JSError.make(toWarnOn, FAILED_TO_UNIFY,
genericType.toString(), concreteType.toString()));
}
}
}

private EnvTypePair analyzeNonStrictComparisonFwd(
Node expr, TypeEnv inEnv, JSType specializedType) {
Token tokenType = expr.getToken();
Expand Down
13 changes: 5 additions & 8 deletions src/com/google/javascript/jscomp/newtypes/FunctionType.java
Expand Up @@ -1067,13 +1067,11 @@ boolean unifyWithSubtype(FunctionType other, List<String> typeParameters,
// contravariance here.
// But it's probably an overkill to do, so instead we just do a subtype
// check if unification fails. Same for restFormals and receiverType.
// Altenatively, maybe the unifyWith function could handle both subtype
// and supertype, and we'd catch type errors as invalid-argument-type
// after unification. (Not sure this is correct, I'd have to try it.)
if (otherFormal != null
&& !thisFormal.unifyWithSubtype(
otherFormal, typeParameters, typeMultimap, subSuperMap)
&& !thisFormal.isSubtypeOf(otherFormal, SubtypeCache.create())) {
&& !thisFormal.substituteGenericsWithUnknown().isSubtypeOf(
otherFormal, SubtypeCache.create())) {
return false;
}
}
Expand All @@ -1082,7 +1080,7 @@ boolean unifyWithSubtype(FunctionType other, List<String> typeParameters,
if (otherRestFormals != null
&& !this.restFormals.unifyWithSubtype(
otherRestFormals, typeParameters, typeMultimap, subSuperMap)
&& !this.restFormals.isSubtypeOf(
&& !this.restFormals.substituteGenericsWithUnknown().isSubtypeOf(
otherRestFormals, SubtypeCache.create())) {
return false;
}
Expand All @@ -1099,12 +1097,11 @@ boolean unifyWithSubtype(FunctionType other, List<String> typeParameters,
return false;
}

// If one of the two functions doesn't use THIS in the body, we can still
// unify.
// If one of the two functions doesn't use THIS in the body, we can still unify.
if (this.receiverType != null && other.receiverType != null
&& !this.receiverType.unifyWithSubtype(
other.receiverType, typeParameters, typeMultimap, subSuperMap)
&& !this.receiverType.isSubtypeOf(
&& !this.receiverType.substituteGenericsWithUnknown().isSubtypeOf(
other.receiverType, SubtypeCache.create())) {
return false;
}
Expand Down
5 changes: 2 additions & 3 deletions src/com/google/javascript/jscomp/newtypes/JSType.java
Expand Up @@ -782,7 +782,6 @@ static JSType unifyUnknowns(JSType t1, JSType t2) {
* Unify {@code this}, which may contain free type variables,
* with {@code other}, a concrete subtype, modifying the supplied
* {@code typeMultimap} to add any new template variable type bindings.
* @return Whether unification succeeded
*
* This method should only be called outside the newtypes package;
* classes inside the package should use unifyWithSubtype.
Expand Down Expand Up @@ -811,9 +810,9 @@ static JSType unifyUnknowns(JSType t1, JSType t2) {
* E.g., (T|Foo|Bar(U)) unifies with (number|string|Foo|Bar(Baz))
* SubC is (number|string), T is mapped to (number|string), and U is mapped to Baz.
*/
public final boolean unifyWith(JSType other, List<String> typeParameters,
public final void unifyWith(JSType other, List<String> typeParameters,
Multimap<String, JSType> typeMultimap) {
return unifyWithSubtype(other, typeParameters, typeMultimap, SubtypeCache.create());
unifyWithSubtype(other, typeParameters, typeMultimap, SubtypeCache.create());
}

/**
Expand Down
4 changes: 4 additions & 0 deletions src/com/google/javascript/rhino/ObjectTypeI.java
Expand Up @@ -92,6 +92,10 @@ public interface ObjectTypeI extends TypeI {
*/
ImmutableList<? extends TypeI> getTemplateTypes();

/**
* When this type represents an instance of a generic class/interface Foo, return an instance
* of Foo with type parameters mapped to the unknown type.
*/
ObjectTypeI instantiateGenericsWithUnknown();

boolean hasProperty(String propertyName);
Expand Down
74 changes: 48 additions & 26 deletions test/com/google/javascript/jscomp/NewTypeInferenceTest.java
Expand Up @@ -8414,30 +8414,30 @@ public void testDifficultClassGenericsInstantiation() {
"(new Foo(123)).method('asdf') - 5;"),
NewTypeInference.INVALID_OPERAND_TYPE);

// typeCheck(LINE_JOINER.join(
// "/**",
// " * @template T",
// " * @constructor",
// " */",
// "function Foo() {}",
// "/** @param {T} x */",
// "Foo.prototype.method = function(x) {};",
// "",
// "/**",
// " * @template T",
// " * @constructor",
// " * @extends {Foo<T>}",
// " * @param {T} x",
// " */",
// "function Bar(x) {}",
// // Invalid instantiation here, must be T, o/w bugs like the call to f
// "/** @param {number} x */",
// "Bar.prototype.method = function(x) {};",
// "",
// "/** @param {!Foo<string>} x */",
// "function f(x) { x.method('sadf'); };",
// "f(new Bar('asdf'));"),
// NewTypeInference.FAILED_TO_UNIFY);
typeCheck(LINE_JOINER.join(
"/**",
" * @template T",
" * @constructor",
" */",
"function Foo() {}",
"/** @param {T} x */",
"Foo.prototype.method = function(x) {};",
"",
"/**",
" * @template T",
" * @constructor",
" * @extends {Foo<T>}",
" * @param {T} x",
" */",
"function Bar(x) {}",
// Invalid instantiation here, must be T, o/w bugs like the call to f
"/** @param {number} x */",
"Bar.prototype.method = function(x) {};",
"",
"/** @param {!Foo<string>} x */",
"function f(x) { x.method('sadf'); };",
"f(new Bar('asdf'));"),
GlobalTypeInfo.INVALID_PROP_OVERRIDE);

typeCheck(LINE_JOINER.join(
"/**",
Expand Down Expand Up @@ -15749,8 +15749,30 @@ public void testFunctionUnificationWithSubtyping() {
"function g(x) {}",
"/** @type {function(!Parent<number>)} */",
"function h(x) {}",
"g(h);"),
NewTypeInference.FAILED_TO_UNIFY);
"g(h);"));

// Missed warning because we don't have unifyWithSuperType.
typeCheck(LINE_JOINER.join(
"/**",
" * @constructor",
" * @template T",
" */",
"function Parent() {}",
"/**",
" * @constructor",
" * @template T",
" * @extends {Parent<T>}",
" */",
"function Child() {}",
"/**",
" * @template T",
" * @param {function(!Child<T>)} x",
" * @return {T}",
" */",
"function g(x) { return /** @type {?} */ (''); }",
"/** @type {function(!Parent<number>)} */",
"function h(x) {}",
"var /** string */ s = g(h);"));
}

public void testNamespaceDefinitionInExternsWithoutConst() {
Expand Down

0 comments on commit 28d343c

Please sign in to comment.