Skip to content

Commit

Permalink
Improves typechecking to consider instance properties of interfaces w…
Browse files Browse the repository at this point in the history
…hen verifying that a type correctly implements its interfaces.

Example:
```
/** @interface */
class Iface {
  constructor() {
    /** @type {!X} */
    this.x;
  }
}

/** @implements {Iface} */
class Impl {
  // Must have either instance or prototype field matching "x".
}
```

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=246214880
  • Loading branch information
nreid260 authored and brad4d committed May 2, 2019
1 parent 9c5e149 commit 43b142c
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 43 deletions.
30 changes: 14 additions & 16 deletions src/com/google/javascript/jscomp/TypeCheck.java
Expand Up @@ -1332,15 +1332,17 @@ private void checkPropertyInheritanceOnClassMember(


private void checkPropertyInheritance( private void checkPropertyInheritance(
Node key, String propertyName, FunctionType ctorType, ObjectType type) { Node key, String propertyName, FunctionType ctorType, ObjectType type) {
if (ctorType != null && (ctorType.isConstructor() || ctorType.isInterface())) { if (ctorType == null || !ctorType.hasInstanceType()) {
checkDeclaredPropertyAgainstNominalInheritance( return;
key.getFirstChild(),
ctorType,
propertyName,
key.getJSDocInfo(),
type.getPropertyType(propertyName));
checkAbstractMethodInConcreteClass(key, ctorType, key.getJSDocInfo());
} }

checkDeclaredPropertyAgainstNominalInheritance(
key.getFirstChild(),
ctorType,
propertyName,
key.getJSDocInfo(),
type.getPropertyType(propertyName));
checkAbstractMethodInConcreteClass(key, ctorType, key.getJSDocInfo());
} }


/** /**
Expand Down Expand Up @@ -1547,17 +1549,13 @@ private void checkDeclaredPropertyAgainstNominalInheritance(


boolean foundInterfaceProperty = false; boolean foundInterfaceProperty = false;
if (ctorType.isConstructor()) { if (ctorType.isConstructor()) {
for (JSType implementedInterface : for (ObjectType implementedInterface : ctorType.getAllImplementedInterfaces()) {
ctorType.getAllImplementedInterfaces()) {
if (implementedInterface.isUnknownType() || implementedInterface.isEmptyType()) { if (implementedInterface.isUnknownType() || implementedInterface.isEmptyType()) {
continue; continue;
} }
FunctionType interfaceType = checkState(implementedInterface.isInstanceType(), implementedInterface);
implementedInterface.toObjectType().getConstructor();
checkNotNull(interfaceType);


boolean interfaceHasProperty = boolean interfaceHasProperty = implementedInterface.hasProperty(propertyName);
interfaceType.getPrototype().hasProperty(propertyName);
foundInterfaceProperty = foundInterfaceProperty || interfaceHasProperty; foundInterfaceProperty = foundInterfaceProperty || interfaceHasProperty;
if (!declaredOverride if (!declaredOverride
&& interfaceHasProperty && interfaceHasProperty
Expand All @@ -1569,7 +1567,7 @@ private void checkDeclaredPropertyAgainstNominalInheritance(
n, n,
HIDDEN_INTERFACE_PROPERTY, HIDDEN_INTERFACE_PROPERTY,
propertyName, propertyName,
interfaceType.getTopMostDefiningType(propertyName).toString())); implementedInterface.getTopDefiningInterface(propertyName).toString()));
} }
} }
} }
Expand Down
24 changes: 9 additions & 15 deletions src/com/google/javascript/jscomp/TypeValidator.java
Expand Up @@ -19,7 +19,6 @@
import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Strings.nullToEmpty;
import static com.google.javascript.rhino.jstype.JSTypeNative.ARRAY_TYPE; import static com.google.javascript.rhino.jstype.JSTypeNative.ARRAY_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.ASYNC_GENERATOR_TYPE; import static com.google.javascript.rhino.jstype.JSTypeNative.ASYNC_GENERATOR_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.BOOLEAN_TYPE; import static com.google.javascript.rhino.jstype.JSTypeNative.BOOLEAN_TYPE;
Expand Down Expand Up @@ -877,9 +876,13 @@ TypedVar expectUndeclaredVariable(String sourceName, CompilerInput input,
void expectAllInterfaceProperties(Node n, FunctionType type) { void expectAllInterfaceProperties(Node n, FunctionType type) {
ObjectType instance = type.getInstanceType(); ObjectType instance = type.getInstanceType();
for (ObjectType implemented : type.getAllImplementedInterfaces()) { 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) { if (implemented.getImplicitPrototype() != null) {
for (String prop : // Case: `/** @interface */ class Foo { prop() { } }`
implemented.getImplicitPrototype().getOwnPropertyNames()) { for (String prop : implemented.getImplicitPrototype().getOwnPropertyNames()) {
expectInterfaceProperty(n, instance, implemented, prop); expectInterfaceProperty(n, instance, implemented, prop);
} }
} }
Expand All @@ -894,9 +897,6 @@ private void expectInterfaceProperty(
Node n, ObjectType instance, ObjectType implementedInterface, String prop) { Node n, ObjectType instance, ObjectType implementedInterface, String prop) {
StaticTypedSlot propSlot = instance.getSlot(prop); StaticTypedSlot propSlot = instance.getSlot(prop);
if (propSlot == null) { if (propSlot == null) {
// Not implemented
String sourceName = n.getSourceFileName();
sourceName = nullToEmpty(sourceName);
registerMismatch( registerMismatch(
instance, instance,
implementedInterface, implementedInterface,
Expand All @@ -918,8 +918,7 @@ private void expectInterfaceProperty(
JSType found = propSlot.getType(); JSType found = propSlot.getType();
found = found.restrictByNotNullOrUndefined(); found = found.restrictByNotNullOrUndefined();


JSType required JSType required = implementedInterface.getPropertyType(prop);
= implementedInterface.getImplicitPrototype().getPropertyType(prop);
TemplateTypeMap typeMap = implementedInterface.getTemplateTypeMap(); TemplateTypeMap typeMap = implementedInterface.getTemplateTypeMap();
if (!typeMap.isEmpty()) { if (!typeMap.isEmpty()) {
TemplateTypeMapReplacer replacer = new TemplateTypeMapReplacer( TemplateTypeMapReplacer replacer = new TemplateTypeMapReplacer(
Expand All @@ -930,15 +929,13 @@ private void expectInterfaceProperty(


if (!found.isSubtype(required, this.subtypingMode)) { if (!found.isSubtype(required, this.subtypingMode)) {
// Implemented, but not correctly typed // Implemented, but not correctly typed
FunctionType constructor =
implementedInterface.toObjectType().getConstructor();
JSError err = JSError err =
JSError.make( JSError.make(
propNode, propNode,
HIDDEN_INTERFACE_PROPERTY_MISMATCH, HIDDEN_INTERFACE_PROPERTY_MISMATCH,
prop, prop,
instance.toString(), instance.toString(),
constructor.getTopMostDefiningType(prop).toString(), implementedInterface.getTopDefiningInterface(prop).toString(),
required.toString(), required.toString(),
found.toString()); found.toString());
registerMismatch(found, required, err); registerMismatch(found, required, err);
Expand All @@ -962,8 +959,7 @@ void expectAbstractMethodsImplemented(Node n, FunctionType ctorType) {


while (currSuperCtor != null && currSuperCtor.isAbstract()) { while (currSuperCtor != null && currSuperCtor.isAbstract()) {
ObjectType superType = currSuperCtor.getInstanceType(); ObjectType superType = currSuperCtor.getInstanceType();
for (String prop : for (String prop : currSuperCtor.getPrototype().getOwnPropertyNames()) {
currSuperCtor.getInstanceType().getImplicitPrototype().getOwnPropertyNames()) {
FunctionType maybeAbstractMethod = superType.findPropertyType(prop).toMaybeFunctionType(); FunctionType maybeAbstractMethod = superType.findPropertyType(prop).toMaybeFunctionType();
if (maybeAbstractMethod != null if (maybeAbstractMethod != null
&& maybeAbstractMethod.isAbstract() && maybeAbstractMethod.isAbstract()
Expand All @@ -980,8 +976,6 @@ void expectAbstractMethodsImplemented(Node n, FunctionType ctorType) {
ObjectType superType = entry.getValue(); ObjectType superType = entry.getValue();
FunctionType abstractMethod = instance.findPropertyType(method).toMaybeFunctionType(); FunctionType abstractMethod = instance.findPropertyType(method).toMaybeFunctionType();
if (abstractMethod == null || abstractMethod.isAbstract()) { if (abstractMethod == null || abstractMethod.isAbstract()) {
String sourceName = n.getSourceFileName();
sourceName = nullToEmpty(sourceName);
registerMismatch( registerMismatch(
instance, instance,
superType, superType,
Expand Down
89 changes: 85 additions & 4 deletions test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java
Expand Up @@ -3001,6 +3001,28 @@ public void testClassMissingOverrideAnnotationForInterfaceMethod() {
"property foo already defined on interface Foo; use @override to override it"); "property foo already defined on interface Foo; use @override to override it");
} }


@Test
public void testClassMissingOverrideAnnotationForInterfaceInstanceProperty() {
testTypes(
lines(
"/** @record */", // `@interface` would also trigger this.
"class Foo {",
" constructor() {",
" /** @type {number} */",
" this.bar;",
" }",
"}",
"",
"/** @implements {Foo} */",
"class MyFoo { }",
// No `@override`.
// For some reason we only check this when assigning to prototype properties, not to
// instance properties.
"/** @type {number} */",
"MyFoo.prototype.bar = 0;"),
"property bar already defined on interface Foo; use @override to override it");
}

@Test @Test
public void testClassIncompatibleInterfaceMethodImplementation() { public void testClassIncompatibleInterfaceMethodImplementation() {
testTypes( testTypes(
Expand Down Expand Up @@ -3034,6 +3056,68 @@ public void testClassMissingTransitiveInterfaceMethod() {
"property foo on interface Foo is not implemented by type Baz"); "property foo on interface Foo is not implemented by type Baz");
} }


@Test
public void testClassMissingInterfaceInstanceProperty() {
testTypes(
lines(
"/** @record */", // `@interface` would also trigger this.
"class Foo {",
" constructor() {",
" /** @type {number} */",
" this.bar;",
" }",
"}",
"",
"/** @implements {Foo} */",
"class MyFoo { }"),
"property bar on interface Foo is not implemented by type MyFoo");
}

@Test
public void testClassInvalidOverrideOfInterfaceInstanceProperty() {
testTypes(
lines(
"/** @record */", // `@interface` would also trigger this.
"class Foo {",
" constructor() {",
" /** @type {number} */",
" this.bar;",
" }",
"}",
"",
"/** @implements {Foo} */",
"class MyFoo {",
" constructor() {",
" /** @type {string} */",
" this.bar;",
" }",
"}"),
lines(
"mismatch of the bar property on type MyFoo and the type "
+ "of the property it overrides from interface Foo",
"original: number",
"override: string"));
}

@Test
public void testClassPrototypeOverrideOfInterfaceInstanceProperty() {
testTypes(
lines(
"/** @record */", // `@interface` would also trigger this.
"class Foo {",
" constructor() {",
" /** @type {number} */",
" this.bar;",
" }",
"}",
"",
"/** @implements {Foo} */",
"class MyFoo { }",
// It's legal to fulfill the interface using either instance or prototype properties.
"/** @override */",
"MyFoo.prototype.bar;"));
}

@Test @Test
public void testClassInheritedInterfaceMethod() { public void testClassInheritedInterfaceMethod() {
testTypes( testTypes(
Expand Down Expand Up @@ -5575,8 +5659,6 @@ public void testGetterOverridesPrototypePropertyFromInterface() {


@Test @Test
public void testGetterOverridesInstancePropertyFromInterface() { public void testGetterOverridesInstancePropertyFromInterface() {
// We treat the interface fields in the constructor as different from prototype properties,
// so trying to override the `num` field with a getter doesn't work.
testTypes( testTypes(
lines( lines(
"/** @interface */", "/** @interface */",
Expand All @@ -5591,8 +5673,7 @@ public void testGetterOverridesInstancePropertyFromInterface() {
" /** @override */", " /** @override */",
" get num() { return 3; }", " get num() { return 3; }",
"}", "}",
"var /** string */ x = (new Baz).num;"), "var /** string */ x = (new Baz).num;"));
"property num not defined on any superclass of Baz");
} }


@Test @Test
Expand Down
19 changes: 11 additions & 8 deletions test/com/google/javascript/jscomp/TypeCheckTest.java
Expand Up @@ -13242,14 +13242,17 @@ public void testInterfacePropertyNotImplemented2() {
@Test @Test
public void testInterfacePropertyNotImplemented3() { public void testInterfacePropertyNotImplemented3() {
testTypes( testTypes(
"/** @interface\n @template T */function Int() {};" lines(
+ "/** @return {T} */Int.prototype.foo = function() {};" "/** @interface @template T */ function Int() {};",
+ "/** @constructor\n @implements {Int<string>} */function Foo() {};" "/** @return {T} */ Int.prototype.foo = function() {};",
+ "/** @return {number}\n @override */Foo.prototype.foo = function() {};", "",
"mismatch of the foo property on type Foo and the type of the property it " "/** @constructor @implements {Int<string>} */ function Foo() {};",
+ "overrides from interface Int\n" "/** @return {number} @override */ Foo.prototype.foo = function() {};"),
+ "original: function(this:Int): string\n" lines(
+ "override: function(this:Foo): number"); "mismatch of the foo property on type Foo and the type of the property it "
+ "overrides from interface Int<string>",
"original: function(this:Int): string",
"override: function(this:Foo): number"));
} }


@Test @Test
Expand Down

0 comments on commit 43b142c

Please sign in to comment.