Skip to content

Commit

Permalink
Overhaul interface property inheritance checking.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@rluble @tjgq
rluble authored and tjgq committed Aug 3, 2019
1 parent 532072c commit 86acbd5
Show file tree
Hide file tree
Showing 3 changed files with 176 additions and 189 deletions.
145 changes: 41 additions & 104 deletions src/com/google/javascript/jscomp/TypeCheck.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down
118 changes: 74 additions & 44 deletions src/com/google/javascript/jscomp/TypeValidator.java
View file
Expand Up @@ -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(
Expand Down Expand Up @@ -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);
}
}
}
Expand All @@ -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(
Expand All @@ -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);
}

/**
Expand Down

0 comments on commit 86acbd5

Please sign in to comment.