diff --git a/src/com/google/javascript/jscomp/newtypes/FunctionType.java b/src/com/google/javascript/jscomp/newtypes/FunctionType.java index dc6a3475564..0ca132124e8 100644 --- a/src/com/google/javascript/jscomp/newtypes/FunctionType.java +++ b/src/com/google/javascript/jscomp/newtypes/FunctionType.java @@ -619,7 +619,7 @@ private static FunctionType looseMerge(FunctionType f1, FunctionType f2) { public boolean isValidOverride(FunctionType other) { // Note: SubtypeCache.create() is cheap, since the data structure is persistent. // 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 @@ -629,7 +629,7 @@ public boolean isValidOverride(FunctionType other) { /** Returns true if this function is a subtype of {@code other}. */ boolean isSubtypeOf(FunctionType other, SubtypeCache subSuperMap) { - return isSubtypeOfHelper(other, true, subSuperMap, null); + return isSubtypeOfHelper(other, false, subSuperMap, null); } /** @@ -639,7 +639,7 @@ boolean isSubtypeOf(FunctionType other, SubtypeCache subSuperMap) { static void whyNotSubtypeOf(FunctionType f1, FunctionType f2, SubtypeCache subSuperMap, MismatchInfo[] boxedInfo) { checkArgument(boxedInfo.length == 1); - f1.isSubtypeOfHelper(f2, true, subSuperMap, boxedInfo); + f1.isSubtypeOfHelper(f2, false, subSuperMap, boxedInfo); } /** @@ -659,11 +659,11 @@ private boolean acceptsAnyArguments() { * Recursively checks that this is a subtype of other: this's parameter * types are supertypes of other's corresponding parameters (contravariant), * 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 - * must be covariant. A cache is used to resolve cycles, and details - * about some mismatched types are written to boxedInfo[0]. + * Additionally, any 'new' type must be covariant. + * A cache is used to resolve cycles, and details about some mismatched types + * are written to boxedInfo[0]. */ - private boolean isSubtypeOfHelper(FunctionType other, boolean checkThisType, + private boolean isSubtypeOfHelper(FunctionType other, boolean isMethodOverrideCheck, SubtypeCache subSuperMap, MismatchInfo[] boxedInfo) { if (other.isTopFunction() || other.isQmarkFunction() || this.isQmarkFunction()) { @@ -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. // See unit tests in NewTypeInferenceTest#testGenericsSubtyping return instantiateGenericsWithUnknown() - .isSubtypeOfHelper(other, checkThisType, subSuperMap, boxedInfo); + .isSubtypeOfHelper(other, isMethodOverrideCheck, subSuperMap, boxedInfo); } if (!other.acceptsAnyArguments()) { @@ -734,17 +734,28 @@ private boolean isSubtypeOfHelper(FunctionType other, boolean checkThisType, return false; } - if (checkThisType) { - // A function without @this can be a subtype of a function with @this. - if (!this.commonTypes.allowMethodsAsFunctions - && this.receiverType != null && other.receiverType == null) { - return false; - } - if (this.receiverType != null && other.receiverType != null - // Contravariance for the receiver type - && !other.receiverType.isSubtypeOf(this.receiverType, subSuperMap)) { - return false; - } + // A function without @this can be a subtype of a function with @this. + if (!this.commonTypes.allowMethodsAsFunctions + // For method overrides, we allow a function with @this to be a subtype of a function + // without @this, but we don't allow it for general function subtyping. + && !isMethodOverrideCheck + && this.receiverType != null + && other.receiverType == null) { + 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 diff --git a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java index c96c7bdfd13..0613cba338f 100644 --- a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java +++ b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java @@ -2187,7 +2187,9 @@ public void testPropInParentInterface3() { } // 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. // Just documenting the behavior here. public void testUnsafeTypingOfThis() { @@ -2231,16 +2233,7 @@ public void testUnsafeTypingOfThis() { "}", "myArrayPrototypeMap(Foo.prototype.method, new Bar);"); - this.mode = TypeInferenceMode.OTI_ONLY; 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( "/** @constructor */", @@ -2274,13 +2267,7 @@ public void testUnsafeTypingOfThis() { "}", "f(Foo.prototype.method);"); - testSets("", js, output, - "{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")); + testSets("", js, output, "{method=[[Foo.prototype]], myprop=[[Bar], [Foo]]}"); } public void testErrorOnProtectedProperty() { diff --git a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java index ec8f7ed2c93..d0b8fc9bcc5 100644 --- a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java +++ b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java @@ -134,8 +134,7 @@ public void testThisInAtTypeFunction() { " var fun = (1 < 2) ? low : high;", " var /** function(this:High) */ f2 = fun;", " var /** function(this:Low) */ f3 = fun;", - "}"), - NewTypeInference.MISTYPED_ASSIGN_RHS); + "}")); typeCheck(LINE_JOINER.join( "/** @constructor */", @@ -147,8 +146,7 @@ public void testThisInAtTypeFunction() { " var fun = (1 < 2) ? low : high;", " var /** function(function(this:High)) */ f2 = fun;", " var /** function(function(this:Low)) */ f3 = fun;", - "}"), - NewTypeInference.MISTYPED_ASSIGN_RHS); + "}")); typeCheck(LINE_JOINER.join( "/**", @@ -4485,6 +4483,52 @@ public void testMethodPropertyOverride() { "function Baz() {}", "/** @param {number} 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() { diff --git a/test/com/google/javascript/jscomp/NewTypeInferenceWithTranspilationTest.java b/test/com/google/javascript/jscomp/NewTypeInferenceWithTranspilationTest.java index 9524ca53177..edd50436967 100644 --- a/test/com/google/javascript/jscomp/NewTypeInferenceWithTranspilationTest.java +++ b/test/com/google/javascript/jscomp/NewTypeInferenceWithTranspilationTest.java @@ -106,8 +106,7 @@ public void testFunctionSubtypingForReceiverType() { "}", "/** @param {{always: function(this:Controller)}} spec */", "function vsyncMethod(spec) {}", - "vsyncMethod({always: (new SubController).method});"), - NewTypeInference.INVALID_ARGUMENT_TYPE); + "vsyncMethod({always: (new SubController).method});")); } public void testDetectPropertyDefinitionOnNullableObject() {