Skip to content

Commit

Permalink
[NTI] Change the subtyping rule for the receiver type of functions.
Browse files Browse the repository at this point in the history 
For method overrides, make it stricter (to catch the same warnings as OTI). We were ignoring @this completely before and now we don't.

For other function subtyping, make it looser. The motivation here is to avoid spurious warnings when dubious overrides (such as toString) need to be checked during structural subtyping. See the new test with IObject.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=173298821
  • Loading branch information
@dimvar @blickly
dimvar authored and blickly committed Oct 24, 2017
1 parent d9afefb commit 05ad006
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 42 deletions.
49 changes: 30 additions & 19 deletions src/com/google/javascript/jscomp/newtypes/FunctionType.java
View file
Expand Up @@ -619,7 +619,7 @@ private static FunctionType looseMerge(FunctionType f1, FunctionType f2) {
public boolean isValidOverride(FunctionType other) { public boolean isValidOverride(FunctionType other) {
// Note: SubtypeCache.create() is cheap, since the data structure is persistent. // Note: SubtypeCache.create() is cheap, since the data structure is persistent.
// The cache is used to handle cycles in types' dependencies. // The cache is used to handle cycles in types' dependencies.
return isSubtypeOfHelper(other, false, SubtypeCache.create(), null); return isSubtypeOfHelper(other, true, SubtypeCache.create(), null);
} }


// We want to warn about argument mismatch, so we don't consider a function // We want to warn about argument mismatch, so we don't consider a function
Expand All @@ -629,7 +629,7 @@ public boolean isValidOverride(FunctionType other) {


/** Returns true if this function is a subtype of {@code other}. */ /** Returns true if this function is a subtype of {@code other}. */
boolean isSubtypeOf(FunctionType other, SubtypeCache subSuperMap) { boolean isSubtypeOf(FunctionType other, SubtypeCache subSuperMap) {
return isSubtypeOfHelper(other, true, subSuperMap, null); return isSubtypeOfHelper(other, false, subSuperMap, null);
} }


/** /**
Expand All @@ -639,7 +639,7 @@ boolean isSubtypeOf(FunctionType other, SubtypeCache subSuperMap) {
static void whyNotSubtypeOf(FunctionType f1, FunctionType f2, static void whyNotSubtypeOf(FunctionType f1, FunctionType f2,
SubtypeCache subSuperMap, MismatchInfo[] boxedInfo) { SubtypeCache subSuperMap, MismatchInfo[] boxedInfo) {
checkArgument(boxedInfo.length == 1); checkArgument(boxedInfo.length == 1);
f1.isSubtypeOfHelper(f2, true, subSuperMap, boxedInfo); f1.isSubtypeOfHelper(f2, false, subSuperMap, boxedInfo);
} }


/** /**
Expand All @@ -659,11 +659,11 @@ private boolean acceptsAnyArguments() {
* Recursively checks that this is a subtype of other: this's parameter * Recursively checks that this is a subtype of other: this's parameter
* types are supertypes of other's corresponding parameters (contravariant), * types are supertypes of other's corresponding parameters (contravariant),
* and this's return type is a subtype of other's return type (covariant). * and this's return type is a subtype of other's return type (covariant).
* Additionally, any 'this' type must be contravariant and any 'new' type * Additionally, any 'new' type must be covariant.
* must be covariant. A cache is used to resolve cycles, and details * A cache is used to resolve cycles, and details about some mismatched types
* about some mismatched types are written to boxedInfo[0]. * are written to boxedInfo[0].
*/ */
private boolean isSubtypeOfHelper(FunctionType other, boolean checkThisType, private boolean isSubtypeOfHelper(FunctionType other, boolean isMethodOverrideCheck,
SubtypeCache subSuperMap, MismatchInfo[] boxedInfo) { SubtypeCache subSuperMap, MismatchInfo[] boxedInfo) {
if (other.isTopFunction() || if (other.isTopFunction() ||
other.isQmarkFunction() || this.isQmarkFunction()) { other.isQmarkFunction() || this.isQmarkFunction()) {
Expand All @@ -684,7 +684,7 @@ private boolean isSubtypeOfHelper(FunctionType other, boolean checkThisType,
// and the fix is not trivial, so for now we decided to not fix. // and the fix is not trivial, so for now we decided to not fix.
// See unit tests in NewTypeInferenceTest#testGenericsSubtyping // See unit tests in NewTypeInferenceTest#testGenericsSubtyping
return instantiateGenericsWithUnknown() return instantiateGenericsWithUnknown()
.isSubtypeOfHelper(other, checkThisType, subSuperMap, boxedInfo); .isSubtypeOfHelper(other, isMethodOverrideCheck, subSuperMap, boxedInfo);
} }


if (!other.acceptsAnyArguments()) { if (!other.acceptsAnyArguments()) {
Expand Down Expand Up @@ -734,17 +734,28 @@ private boolean isSubtypeOfHelper(FunctionType other, boolean checkThisType,
return false; return false;
} }


if (checkThisType) { // A function without @this can be a subtype of a function with @this.
// A function without @this can be a subtype of a function with @this. if (!this.commonTypes.allowMethodsAsFunctions
if (!this.commonTypes.allowMethodsAsFunctions // For method overrides, we allow a function with @this to be a subtype of a function
&& this.receiverType != null && other.receiverType == null) { // without @this, but we don't allow it for general function subtyping.
return false; && !isMethodOverrideCheck
} && this.receiverType != null
if (this.receiverType != null && other.receiverType != null && other.receiverType == null) {
// Contravariance for the receiver type return false;
&& !other.receiverType.isSubtypeOf(this.receiverType, subSuperMap)) { }
return false; if (this.receiverType != null && other.receiverType != null
} // Checking of @this is unfortunately loose, because it covers two different cases.
// 1) When a method overrides another method from a supertype, we only require
// that the new @this meets with the @this from the supertype, e.g., see the typing
// of toString in the externs. This is unsafe, but that's how we have typed it.
// 2) When checking for subtyping of two arbitrary function types, the correct semantics
// would be to check @this in a contravariant way, but we allow looseness in order to
// handle cases like using a Foo that overrides toString as an IObject<?,?>.
// It would be possible to add special-case code here and in ObjectType#isSubtypeOf
// to allow us to be loose for #1 but stricter for #2, but it's awkward and doesn't seem
// worth doing.
&& !JSType.haveCommonSubtype(this.receiverType, other.receiverType)) {
return false;
} }


// covariance in the return type // covariance in the return type
Expand Down
21 changes: 4 additions & 17 deletions test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java
View file
Expand Up @@ -2187,7 +2187,9 @@ public void testPropInParentInterface3() {
} }


// In function subtyping, the type of THIS should be contravariant, like the argument types. // In function subtyping, the type of THIS should be contravariant, like the argument types.
// Because it's not, we get wrong disambiguation. // But when overriding a method, it's covariant, and on top of that, we allow methods redefining
// it with @this.
// So we check THIS loosely for functions, and as a result, we get wrong disambiguation.
// On top of that, this can happen in OTI when types are joined during generics instantiation. // On top of that, this can happen in OTI when types are joined during generics instantiation.
// Just documenting the behavior here. // Just documenting the behavior here.
public void testUnsafeTypingOfThis() { public void testUnsafeTypingOfThis() {
Expand Down Expand Up @@ -2231,16 +2233,7 @@ public void testUnsafeTypingOfThis() {
"}", "}",
"myArrayPrototypeMap(Foo.prototype.method, new Bar);"); "myArrayPrototypeMap(Foo.prototype.method, new Bar);");


this.mode = TypeInferenceMode.OTI_ONLY;
testSets(js, output, "{method=[[Foo.prototype]], myprop=[[Bar], [Foo]]}"); testSets(js, output, "{method=[[Foo.prototype]], myprop=[[Bar], [Foo]]}");
this.mode = TypeInferenceMode.NTI_ONLY;
testSets("", js, output,
"{method=[[Foo.prototype]], myprop=[[Bar], [Foo]]}",
NewTypeInference.INVALID_ARGUMENT_TYPE,
LINE_JOINER.join(
"Invalid type for parameter 1 of function myArrayPrototypeMap.",
"Expected : function(this:(Bar|Foo)): ?",
"Found : function(this:Foo): ?\n"));


js = LINE_JOINER.join( js = LINE_JOINER.join(
"/** @constructor */", "/** @constructor */",
Expand Down Expand Up @@ -2274,13 +2267,7 @@ public void testUnsafeTypingOfThis() {
"}", "}",
"f(Foo.prototype.method);"); "f(Foo.prototype.method);");


testSets("", js, output, testSets("", js, output, "{method=[[Foo.prototype]], myprop=[[Bar], [Foo]]}");
"{method=[[Foo.prototype]], myprop=[[Bar], [Foo]]}",
NewTypeInference.INVALID_ARGUMENT_TYPE,
LINE_JOINER.join(
"Invalid type for parameter 1 of function f.",
"Expected : function(this:(Bar|Foo)): ?",
"Found : function(this:Foo): ?\n"));
} }


public void testErrorOnProtectedProperty() { public void testErrorOnProtectedProperty() {
Expand Down
52 changes: 48 additions & 4 deletions test/com/google/javascript/jscomp/NewTypeInferenceTest.java
View file
Expand Up @@ -134,8 +134,7 @@ public void testThisInAtTypeFunction() {
" var fun = (1 < 2) ? low : high;", " var fun = (1 < 2) ? low : high;",
" var /** function(this:High) */ f2 = fun;", " var /** function(this:High) */ f2 = fun;",
" var /** function(this:Low) */ f3 = fun;", " var /** function(this:Low) */ f3 = fun;",
"}"), "}"));
NewTypeInference.MISTYPED_ASSIGN_RHS);


typeCheck(LINE_JOINER.join( typeCheck(LINE_JOINER.join(
"/** @constructor */", "/** @constructor */",
Expand All @@ -147,8 +146,7 @@ public void testThisInAtTypeFunction() {
" var fun = (1 < 2) ? low : high;", " var fun = (1 < 2) ? low : high;",
" var /** function(function(this:High)) */ f2 = fun;", " var /** function(function(this:High)) */ f2 = fun;",
" var /** function(function(this:Low)) */ f3 = fun;", " var /** function(function(this:Low)) */ f3 = fun;",
"}"), "}"));
NewTypeInference.MISTYPED_ASSIGN_RHS);


typeCheck(LINE_JOINER.join( typeCheck(LINE_JOINER.join(
"/**", "/**",
Expand Down Expand Up @@ -4485,6 +4483,52 @@ public void testMethodPropertyOverride() {
"function Baz() {}", "function Baz() {}",
"/** @param {number} x */", "/** @param {number} x */",
"Baz.prototype.method = function(x) {};")); "Baz.prototype.method = function(x) {};"));

typeCheck(LINE_JOINER.join(
"/** @constructor */",
"function Foo() {}",
"Foo.prototype.f = function() {};",
"/** @constructor @extends {Foo} */",
"function Bar() {}",
"/** @this {number} */",
"Bar.prototype.f = function() {};"),
GlobalTypeInfoCollector.INVALID_PROP_OVERRIDE);

typeCheck(LINE_JOINER.join(
"/** @constructor */",
"function Foo() {}",
"Foo.prototype.f = function() {};",
"/** @constructor @extends {Foo} */",
"function Bar() {}",
"/** @this {!Object} */",
"Bar.prototype.f = function() {};"));

typeCheck(LINE_JOINER.join(
"/** @constructor */",
"function Foo() {}",
"Foo.prototype.f = function() {};",
"/** @constructor @extends {Foo} */",
"function Bar() {}",
"/** @this {!Bar|number} */",
"Bar.prototype.f = function() {};"));

typeCheck(LINE_JOINER.join(
"/**",
" * @this {*}",
" * @return {string}",
" */",
"Object.prototype.foobar = function() { return ''; };",
"/** @constructor */",
"function Bar() {}",
"/**",
" * @this {Bar}",
" * @return {string}",
" */",
"Bar.prototype.foobar = function() { return ''; };",
"/** @constructor @extends {Bar} */",
"function Baz() {}",
"var x = new Baz;",
"var /** !IObject<?,?> */ y = x;"));
} }


public void testMultipleObjects() { public void testMultipleObjects() {
Expand Down
3 changes: 1 addition & 2 deletions test/com/google/javascript/jscomp/NewTypeInferenceWithTranspilationTest.java
View file
Expand Up @@ -106,8 +106,7 @@ public void testFunctionSubtypingForReceiverType() {
"}", "}",
"/** @param {{always: function(this:Controller)}} spec */", "/** @param {{always: function(this:Controller)}} spec */",
"function vsyncMethod(spec) {}", "function vsyncMethod(spec) {}",
"vsyncMethod({always: (new SubController).method});"), "vsyncMethod({always: (new SubController).method});"));
NewTypeInference.INVALID_ARGUMENT_TYPE);
} }


public void testDetectPropertyDefinitionOnNullableObject() { public void testDetectPropertyDefinitionOnNullableObject() {
Expand Down

0 comments on commit 05ad006

Please sign in to comment.