From 86acbd5e3ec78f08a1804df5f1efbea129d29c0e Mon Sep 17 00:00:00 2001 From: rluble Date: Fri, 2 Aug 2019 14:16:28 -0700 Subject: [PATCH] Overhaul interface property inheritance checking. The patch unifies interface property type checking to single place which fixes missing interface-to-interface property type checks. The patch also simplifies class property inheritance checking where it was difficult to ensure all cases were covered due to accumulated state over many independent state checks. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=261389758 --- .../google/javascript/jscomp/TypeCheck.java | 145 +++++------------- .../javascript/jscomp/TypeValidator.java | 118 ++++++++------ .../javascript/jscomp/TypeCheckTest.java | 102 +++++++----- 3 files changed, 176 insertions(+), 189 deletions(-) diff --git a/src/com/google/javascript/jscomp/TypeCheck.java b/src/com/google/javascript/jscomp/TypeCheck.java index d4ce335cac9..e3aba649365 100644 --- a/src/com/google/javascript/jscomp/TypeCheck.java +++ b/src/com/google/javascript/jscomp/TypeCheck.java @@ -55,8 +55,6 @@ import com.google.javascript.rhino.jstype.ObjectType; import com.google.javascript.rhino.jstype.Property; import com.google.javascript.rhino.jstype.Property.OwnedProperty; -import com.google.javascript.rhino.jstype.TemplateTypeMap; -import com.google.javascript.rhino.jstype.TemplateTypeReplacer; import com.google.javascript.rhino.jstype.TemplatizedType; import com.google.javascript.rhino.jstype.TernaryValue; import java.util.HashMap; @@ -230,14 +228,6 @@ public final class TypeCheck implements NodeTraversal.Callback, CompilerPass { "JSC_HIDDEN_INTERFACE_PROPERTY", "property {0} already defined on interface {1}; use @override to override it"); - static final DiagnosticType HIDDEN_SUPERCLASS_PROPERTY_MISMATCH = - DiagnosticType.warning( - "JSC_HIDDEN_SUPERCLASS_PROPERTY_MISMATCH", - "mismatch of the {0} property type and the type " - + "of the property it overrides from superclass {1}\n" - + "original: {2}\n" - + "override: {3}"); - static final DiagnosticType HIDDEN_PROTOTYPAL_SUPERTYPE_PROPERTY_MISMATCH = DiagnosticType.warning( "JSC_HIDDEN_PROTOTYPAL_SUPERTYPE_PROPERTY_MISMATCH", @@ -341,7 +331,7 @@ public final class TypeCheck implements NodeTraversal.Callback, CompilerPass { CONFLICTING_EXTENDED_TYPE, CONFLICTING_IMPLEMENTED_TYPE, BAD_IMPLEMENTED_TYPE, - HIDDEN_SUPERCLASS_PROPERTY_MISMATCH, + TypeValidator.HIDDEN_SUPERCLASS_PROPERTY_MISMATCH, HIDDEN_PROTOTYPAL_SUPERTYPE_PROPERTY_MISMATCH, UNKNOWN_OVERRIDE, UNKNOWN_PROTOTYPAL_OVERRIDE, @@ -1535,54 +1525,55 @@ private void checkDeclaredPropertyAgainstNominalInheritance( String propertyName, @Nullable JSDocInfo info, JSType propertyType) { + + // No need to check special properties; @override is not required for them, nor they are + // manually typed by the developers. + if ("__proto__".equals(propertyName) || "constructor".equals(propertyName)) { + return; + } + + // Interfaces are checked elsewhere. + if (ctorType.isInterface()) { + return; + } + // If the supertype doesn't resolve correctly, we've warned about this already. if (hasUnknownOrEmptySupertype(ctorType)) { return; } - FunctionType superCtor = ctorType.getSuperClassConstructor(); + boolean foundProperty = false; - ObjectType superClass = null; - boolean superClassHasProperty = false; - boolean superClassHasDeclaredProperty = false; + FunctionType superCtor = ctorType.getSuperClassConstructor(); if (superCtor != null) { OwnedProperty propSlot = superCtor.getInstanceType().findClosestDefinition(propertyName); - superClassHasProperty = propSlot != null && !propSlot.isOwnedByInterface(); - if (superClassHasProperty) { - superClass = propSlot.getOwnerInstanceType(); - superClassHasDeclaredProperty = !propSlot.getValue().isTypeInferred(); - } - } + boolean superClassHasProperty = propSlot != null && !propSlot.isOwnedByInterface(); + foundProperty |= superClassHasProperty; - // For interface - boolean superInterfaceHasProperty = false; - boolean superInterfaceHasDeclaredProperty = false; - if (ctorType.isInterface()) { - for (ObjectType interfaceType : ctorType.getExtendedInterfaces()) { - superInterfaceHasProperty = - superInterfaceHasProperty || interfaceType.hasProperty(propertyName); - superInterfaceHasDeclaredProperty = - superInterfaceHasDeclaredProperty || interfaceType.isPropertyTypeDeclared(propertyName); + if (superClassHasProperty) { + ObjectType superClass = propSlot.getOwnerInstanceType(); + boolean superClassHasDeclaredProperty = !propSlot.getValue().isTypeInferred(); + if (superClassHasDeclaredProperty) { + if (isDeclaredLocally(ctorType, propertyName) && !declaresOverride(info)) { + compiler.report( + JSError.make( + n, HIDDEN_SUPERCLASS_PROPERTY, propertyName, superClass.getReferenceName())); + } + validator.checkPropertyType( + n, ctorType.getTypeOfThis(), superClass, propertyName, propertyType); + } } } - boolean declaredOverride = declaresOverride(info); - - boolean foundInterfaceProperty = false; for (ObjectType implementedInterface : ctorType.getAllImplementedInterfaces()) { if (implementedInterface.isUnknownType() || implementedInterface.isEmptyType()) { continue; } - checkState(implementedInterface.isInstanceType(), implementedInterface); - OwnedProperty propSlot = implementedInterface.findClosestDefinition(propertyName); boolean interfaceHasProperty = propSlot != null; - foundInterfaceProperty = foundInterfaceProperty || interfaceHasProperty; - if (!declaredOverride - && interfaceHasProperty - && !"__proto__".equals(propertyName) - && !"constructor".equals(propertyName)) { - // @override not present, but the property does override an interface property + foundProperty |= interfaceHasProperty; + + if (interfaceHasProperty && !declaresOverride(info)) { compiler.report( JSError.make( n, @@ -1592,75 +1583,19 @@ private void checkDeclaredPropertyAgainstNominalInheritance( } } - if (!declaredOverride && !superClassHasProperty && !superInterfaceHasProperty) { - // nothing to do here, it's just a plain new property - return; - } - - boolean declaredLocally = - ctorType.isConstructor() - && (ctorType.getPrototype().hasOwnProperty(propertyName) - || ctorType.getInstanceType().hasOwnProperty(propertyName)); - if (!declaredOverride - && superClassHasDeclaredProperty - && declaredLocally - && !"__proto__".equals(propertyName) - // constructor always "overrides" the superclass "constructor" there is no - // value in marking it with "@override". - && !"constructor".equals(propertyName)) { - // @override not present, but the property does override a superclass - // property - compiler.report( - JSError.make(n, HIDDEN_SUPERCLASS_PROPERTY, propertyName, superClass.getReferenceName())); - } - - // @override is present and we have to check that it is ok - if (superClassHasDeclaredProperty) { - // there is a superclass implementation - JSType superClassPropType = superClass.getPropertyType(propertyName); - TemplateTypeMap ctorTypeMap = ctorType.getTypeOfThis().getTemplateTypeMap(); - if (!ctorTypeMap.isEmpty()) { - superClassPropType = - superClassPropType.visit( - TemplateTypeReplacer.forPartialReplacement(typeRegistry, ctorTypeMap)); - } - - if (!propertyType.isSubtype(superClassPropType, this.subtypingMode)) { - compiler.report( - JSError.make( - n, - HIDDEN_SUPERCLASS_PROPERTY_MISMATCH, - propertyName, - superClass.getReferenceName(), - superClassPropType.toString(), - propertyType.toString())); - } - } else if (superInterfaceHasDeclaredProperty) { - // there is an super interface property - for (ObjectType interfaceType : ctorType.getExtendedInterfaces()) { - OwnedProperty propSlot = interfaceType.findClosestDefinition(propertyName); - if (propSlot != null) { - JSType superPropertyType = propSlot.getValue().getType(); - if (!propertyType.isSubtype(superPropertyType, this.subtypingMode)) { - compiler.report( - JSError.make( - n, - HIDDEN_SUPERCLASS_PROPERTY_MISMATCH, - propertyName, - propSlot.getOwnerInstanceType().getReferenceName(), - superPropertyType.toString(), - propertyType.toString())); - } - } - } - } else if (!foundInterfaceProperty && !superClassHasProperty && !superInterfaceHasProperty) { - // there is no superclass nor interface implementation + if (!foundProperty && declaresOverride(info)) { compiler.report( JSError.make( n, UNKNOWN_OVERRIDE, propertyName, ctorType.getInstanceType().getReferenceName())); } } + private static boolean isDeclaredLocally(FunctionType ctorType, String propertyName) { + return ctorType.isConstructor() + && (ctorType.getPrototype().hasOwnProperty(propertyName) + || ctorType.getInstanceType().hasOwnProperty(propertyName)); + } + /** * Given a {@code receiverType}, check that the property ({@code propertyName}) conforms to * inheritance rules. @@ -2500,6 +2435,8 @@ private void checkInterface(Node n, FunctionType functionType) { compiler.report( JSError.make(n, INTERFACE_EXTENDS_LOOP, loopPath.get(0).getDisplayName(), strPath)); } + + validator.expectAllInterfaceProperties(n, functionType); } private String getBestFunctionName(Node n) { diff --git a/src/com/google/javascript/jscomp/TypeValidator.java b/src/com/google/javascript/jscomp/TypeValidator.java index 012cf2a6897..c6a6c0785d3 100644 --- a/src/com/google/javascript/jscomp/TypeValidator.java +++ b/src/com/google/javascript/jscomp/TypeValidator.java @@ -144,10 +144,20 @@ class TypeValidator implements Serializable { static final DiagnosticType HIDDEN_INTERFACE_PROPERTY_MISMATCH = DiagnosticType.warning( "JSC_HIDDEN_INTERFACE_PROPERTY_MISMATCH", - "mismatch of the {0} property on type {1} and the type " - + "of the property it overrides from interface {2}\n" - + "original: {3}\n" - + "override: {4}"); + "mismatch of the {0} property on type {4} and the type " + + "of the property it overrides from interface {1}\n" + + "original: {2}\n" + + "override: {3}"); + + // TODO(goktug): consider updating super class / interface property mismatch to follow the same + // pattern. + static final DiagnosticType HIDDEN_SUPERCLASS_PROPERTY_MISMATCH = + DiagnosticType.warning( + "JSC_HIDDEN_SUPERCLASS_PROPERTY_MISMATCH", + "mismatch of the {0} property type and the type " + + "of the property it overrides from superclass {1}\n" + + "original: {2}\n" + + "override: {3}"); static final DiagnosticType ABSTRACT_METHOD_NOT_IMPLEMENTED = DiagnosticType.warning( @@ -866,15 +876,23 @@ TypedVar expectUndeclaredVariable(String sourceName, CompilerInput input, void expectAllInterfaceProperties(Node n, FunctionType type) { ObjectType instance = type.getInstanceType(); for (ObjectType implemented : type.getAllImplementedInterfaces()) { - // Case: `/** @interface */ class Foo { constructor() { this.prop; } }` - for (String prop : implemented.getOwnPropertyNames()) { - expectInterfaceProperty(n, instance, implemented, prop); - } - if (implemented.getImplicitPrototype() != null) { - // Case: `/** @interface */ class Foo { prop() { } }` - for (String prop : implemented.getImplicitPrototype().getOwnPropertyNames()) { - expectInterfaceProperty(n, instance, implemented, prop); - } + expectInterfaceProperties(n, instance, implemented); + } + for (ObjectType extended : type.getExtendedInterfaces()) { + expectInterfaceProperties(n, instance, extended); + } + } + + private void expectInterfaceProperties( + Node n, ObjectType instance, ObjectType ancestorInterface) { + // Case: `/** @interface */ class Foo { constructor() { this.prop; } }` + for (String prop : ancestorInterface.getOwnPropertyNames()) { + expectInterfaceProperty(n, instance, ancestorInterface, prop); + } + if (ancestorInterface.getImplicitPrototype() != null) { + // Case: `/** @interface */ class Foo { prop() { } }` + for (String prop : ancestorInterface.getImplicitPrototype().getOwnPropertyNames()) { + expectInterfaceProperty(n, instance, ancestorInterface, prop); } } } @@ -886,9 +904,11 @@ void expectAllInterfaceProperties(Node n, FunctionType type) { private void expectInterfaceProperty( Node n, ObjectType instance, ObjectType implementedInterface, String propName) { OwnedProperty propSlot = instance.findClosestDefinition(propName); - if (propSlot == null || propSlot.isOwnedByInterface()) { - if (instance.getConstructor().isAbstract()) { - // Abstract classes are not required to implement interface properties. + if (propSlot == null + || (!instance.getConstructor().isInterface() && propSlot.isOwnedByInterface())) { + + if (instance.getConstructor().isAbstract() || instance.getConstructor().isInterface()) { + // Abstract classes and interfaces are not required to implement interface properties. return; } registerMismatch( @@ -902,40 +922,50 @@ private void expectInterfaceProperty( implementedInterface.getReferenceName(), instance.toString()))); } else { + boolean local = propSlot.getOwnerInstanceType().equals(instance); + if (!local && instance.getConstructor().isInterface()) { + // non-local interface mismatches already handled via interface property conflict checks. + return; + } + Property prop = propSlot.getValue(); Node propNode = prop.getDeclaration() == null ? null : prop.getDeclaration().getNode(); - - // Fall back on the constructor node if we can't find a node for the - // property. + // Fall back on the constructor node if we can't find a node for the property. propNode = propNode == null ? n : propNode; - JSType found = prop.getType(); - found = found.restrictByNotNullOrUndefined(); + checkPropertyType(propNode, instance, implementedInterface, propName, prop.getType()); + } + } - JSType required = implementedInterface.getPropertyType(propName); - TemplateTypeMap typeMap = implementedInterface.getTemplateTypeMap(); - if (!typeMap.isEmpty()) { - TemplateTypeReplacer replacer = - TemplateTypeReplacer.forPartialReplacement(typeRegistry, typeMap); - required = required.visit(replacer); - } - required = required.restrictByNotNullOrUndefined(); - - if (!found.isSubtype(required, this.subtypingMode)) { - // Implemented, but not correctly typed - JSError err = - JSError.make( - propNode, - HIDDEN_INTERFACE_PROPERTY_MISMATCH, - propName, - instance.toString(), - implementedInterface.getReferenceName(), - required.toString(), - found.toString()); - registerMismatch(found, required, err); - report(err); - } + /** + * Check the property is correctly typed (i.e. subtype of the parent property's type declaration). + */ + void checkPropertyType( + Node n, JSType instance, ObjectType parent, String propertyName, JSType found) { + JSType required = parent.getPropertyType(propertyName); + TemplateTypeMap typeMap = instance.getTemplateTypeMap(); + if (!typeMap.isEmpty() && required.hasAnyTemplateTypes()) { + required = required.visit(TemplateTypeReplacer.forPartialReplacement(typeRegistry, typeMap)); } + + if (found.isSubtype(required, this.subtypingMode)) { + return; + } + + // Implemented, but not correctly typed + JSError err = + JSError.make( + n, + parent.getConstructor().isInterface() + ? HIDDEN_INTERFACE_PROPERTY_MISMATCH + : HIDDEN_SUPERCLASS_PROPERTY_MISMATCH, + propertyName, + parent.getReferenceName(), + required.toString(), + found.toString(), + instance.toString()); + registerMismatch(found, required, err); + report(err); } /** diff --git a/test/com/google/javascript/jscomp/TypeCheckTest.java b/test/com/google/javascript/jscomp/TypeCheckTest.java index d1080415966..507a20b5996 100644 --- a/test/com/google/javascript/jscomp/TypeCheckTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckTest.java @@ -5455,24 +5455,25 @@ public void testBadExtends4() { @Test public void testBadExtends5() { - testTypes("" - + "/** @interface */\n" - + "var MyInterface = function() {};\n" - + "MyInterface.prototype = {\n" - + " /** @return {number} */\n" - + " method: function() {}\n" - + "}\n" - + "/** @extends {MyInterface}\n * @interface */\n" - + "var MyOtherInterface = function() {};\n" - + "MyOtherInterface.prototype = {\n" - + " /** @return {string} \n @override */\n" - + " method: function() {}\n" - + "}", - "" - + "mismatch of the method property type and the type of the property " - + "it overrides from superclass MyInterface\n" - + "original: function(this:MyInterface): number\n" - + "override: function(this:MyOtherInterface): string"); + testTypes( + lines( + "/** @interface */", + "var MyInterface = function() {};", + "MyInterface.prototype = {", + " /** @return {number} */", + " method: function() {}", + "}", + "/** @extends {MyInterface}\n * @interface */", + "var MyOtherInterface = function() {};", + "MyOtherInterface.prototype = {", + " /** @return {string} \n @override */", + " method: function() {}", + "}"), + lines( + "mismatch of the method property on type MyOtherInterface and the type of the property" + + " it overrides from interface MyInterface", + "original: function(this:MyInterface): number", + "override: function(this:MyOtherInterface): string")); } @Test @@ -8511,7 +8512,7 @@ public void testOverriddenPropertyWithUnknownInterfaceAncestor() { "mismatch of the data property on type SubFoo and the type " + "of the property it overrides from interface Foo", "original: string", - "override: (Object|string)") + "override: (Object|null|string)") }); } @@ -13185,6 +13186,23 @@ public void testInterfacePropertyOverride2() { "Sub.prototype.foo = function() {};"); } + @Test + public void testInterfacePropertyBadOverrideFails() { + testTypes( + lines( + "/** @interface */function Super() {};", + "/** @type {number} */", + "Super.prototype.foo;", + "/** @interface @extends {Super} */function Sub() {};", + "/** @type {string} */", + "Sub.prototype.foo;"), + lines( + "mismatch of the foo property on type Sub and the type of the property it " + + "overrides from interface Super", + "original: number", + "override: string")); + } + @Test public void testInterfaceInheritanceCheck1() { testTypes( @@ -16944,17 +16962,18 @@ public void testMultipleExtendsInterface5() { @Test public void testMultipleExtendsInterface6() { testTypes( - "/** @interface */function Super1() {};" + - "/** @interface */function Super2() {};" + - "/** @param {number} bar */Super2.prototype.foo = function(bar) {};" + - "/** @interface\n @extends {Super1}\n " + - "@extends {Super2} */function Sub() {};" + - "/** @override\n @param {string} bar */Sub.prototype.foo =\n" + - "function(bar) {};", - "mismatch of the foo property type and the type of the property it " + - "overrides from superclass Super2\n" + - "original: function(this:Super2, number): undefined\n" + - "override: function(this:Sub, string): undefined"); + lines( + "/** @interface */function Super1() {};", + "/** @interface */function Super2() {};", + "/** @param {number} bar */Super2.prototype.foo = function(bar) {};", + "/** @interface @extends {Super1} @extends {Super2} */function Sub() {};", + "/** @override @param {string} bar */Sub.prototype.foo =", + "function(bar) {};"), + lines( + "mismatch of the foo property on type Sub and the type of the property it " + + "overrides from interface Super2", + "original: function(this:Super2, number): undefined", + "override: function(this:Sub, string): undefined")); } @Test @@ -18194,17 +18213,18 @@ public void testBadSuperclassInheritance1() { @Test public void testBadSuperclassInheritance2() { - testTypes(lines( - "/** @constructor */", - "function Foo() {}", - "/** @type {number} */", - "Foo.prototype.myprop = 2;", - "", - "/** @constructor @extends {Foo} */", - "function Bar() {}", - "/** @override @type {string} */", - "Bar.prototype.myprop = 'qwer';"), - TypeCheck.HIDDEN_SUPERCLASS_PROPERTY_MISMATCH); + testTypes( + lines( + "/** @constructor */", + "function Foo() {}", + "/** @type {number} */", + "Foo.prototype.myprop = 2;", + "", + "/** @constructor @extends {Foo} */", + "function Bar() {}", + "/** @override @type {string} */", + "Bar.prototype.myprop = 'qwer';"), + TypeValidator.HIDDEN_SUPERCLASS_PROPERTY_MISMATCH); } // If the property has no initializer, the HIDDEN_SUPERCLASS_PROPERTY_MISMATCH warning is missed.