Skip to content

Commit

Permalink
Support inferring getter/setter types from overriden superclass prope…
Browse files Browse the repository at this point in the history 
…rties

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=209058830
  • Loading branch information
@lauraharker
lauraharker committed Aug 17, 2018
1 parent 60162c6 commit 6e935d7
Show file tree
Hide file tree
Showing 2 changed files with 231 additions and 13 deletions.
53 changes: 40 additions & 13 deletions src/com/google/javascript/jscomp/TypedScopeCreator.java
View file
Expand Up @@ -1249,10 +1249,37 @@ private FunctionType createFunctionTypeFromNodes(
} }


FunctionType overriddenType = null; FunctionType overriddenType = null;
// the type of the property this overrides, not necessarily a function.

if (ownerType != null && propName != null) { if (ownerType != null && propName != null) {
overriddenType = findOverriddenFunction(ownerType, propName, prototypeOwnerTypeMap); JSType overriddenPropType =
findOverriddenProperty(ownerType, propName, prototypeOwnerTypeMap);
if (overriddenPropType != null) {
// Overridden getters and setters need special handling because we declare
// getters/setters as simple properties with their respective return/parameter type. This
// causes a split during inference where left and right sides of a getter/setter
// declaration will be inferred to have different types; if the left side has type `T`,
// the right side will be some function type involving `T`.
if (lvalueNode.isGetterDef()) {
// Convert `number` to `function(): number`
overriddenType = typeRegistry.createFunctionType(overriddenPropType);
} else if (lvalueNode.isSetterDef()) {
// Convert `number` to `function(number): undefined`
overriddenType =
typeRegistry.createFunctionType(getNativeType(VOID_TYPE), overriddenPropType);
} else if (overriddenPropType.isFunctionType()) {
// for cases where we override a non-method (e.g. a number) with a method, don't put the
// non-method type (e.g. number) on the function.
// Instead do some basic inference to create a function type.
// we will warn during typechecking for an invalid override, but we don't want to put a
// non-function type on this function because that will interfere with type inference
// inside the function.
overriddenType = overriddenPropType.toMaybeFunctionType();
}
}
} }



AstFunctionContents contents = fnRoot != null ? new AstFunctionContents(fnRoot) : null; AstFunctionContents contents = fnRoot != null ? new AstFunctionContents(fnRoot) : null;
if (functionsWithNonEmptyReturns.contains(fnRoot)) { if (functionsWithNonEmptyReturns.contains(fnRoot)) {
contents.recordNonEmptyReturn(); contents.recordNonEmptyReturn();
Expand Down Expand Up @@ -1331,34 +1358,34 @@ private ObjectType getPrototypeOwnerType(ObjectType ownerType) {
} }


/** /**
* Find the function that's being overridden on this type, if any. * Find the property that's being overridden on this type, if any.
*
* <p>Said property could be a method, field, getter, or setter. We don't distinguish between
* these when looking up a property type.
*/ */
private FunctionType findOverriddenFunction( private JSType findOverriddenProperty(
ObjectType ownerType, String propName, TemplateTypeMap typeMap) { ObjectType ownerType, String propName, TemplateTypeMap typeMap) {
FunctionType result = null; JSType result = null;


// First, check to see if the property is implemented // First, check to see if the property is implemented
// on a superclass. // on a superclass.
JSType propType = ownerType.getPropertyType(propName); JSType propType = ownerType.getPropertyType(propName);
if (propType != null && propType.isFunctionType()) { if (propType != null && !propType.isUnknownType()) {
result = propType.toMaybeFunctionType(); result = propType;
} else { } else {
// If it's not, then check to see if it's implemented // If it's not, then check to see if it's implemented
// on an implemented interface. // on an implemented interface.
for (ObjectType iface : for (ObjectType iface : ownerType.getCtorImplementedInterfaces()) {
ownerType.getCtorImplementedInterfaces()) {
propType = iface.getPropertyType(propName); propType = iface.getPropertyType(propName);
if (propType != null && propType.isFunctionType()) { if (propType != null && !propType.isUnknownType()) {
result = propType.toMaybeFunctionType(); result = propType;
break; break;
} }
} }
} }


if (result != null && typeMap != null && !typeMap.isEmpty()) { if (result != null && typeMap != null && !typeMap.isEmpty()) {
result = result.visit( result = result.visit(new TemplateTypeMapReplacer(typeRegistry, typeMap));
new TemplateTypeMapReplacer(typeRegistry, typeMap))
.toMaybeFunctionType();
} }


return result; return result;
Expand Down
191 changes: 191 additions & 0 deletions test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java
View file
Expand Up @@ -4138,4 +4138,195 @@ public void testArrayPatternAssignWithIllegalPropCreationInStruct() {
public void testDictClass1() { public void testDictClass1() {
testTypes("/** @dict */ var C = class { constructor() {} 'x'(){} };"); testTypes("/** @dict */ var C = class { constructor() {} 'x'(){} };");
} }

public void testTypeCheckingOverriddenGetterFromSuperclass() {
testTypes(
lines(
"/** @abstract */",
"class Bar {",
" /**",
" * @abstract",
" * @return {number} ",
" */",
" get num() { return 1; }",
"}",
"/** @extends {Bar} */",
"class Baz extends Bar {",
" /** @override */",
" get num() { return 3; }",
"}",
"var /** string */ x = (new Baz).num;"),
lines(
"initializing variable", //
"found : number",
"required: string"));
}

public void testTypeCheckingOverriddenGetterFromSuperclassWithBadReturnType() {
testTypes(
lines(
"/** @abstract */",
"class Bar {",
" /**",
" * @abstract",
" * @return {number} ",
" */",
" get num() { return 1; }",
"}",
"/** @extends {Bar} */",
"class Baz extends Bar {",
" /** @override */",
" get num() { return 'foo'; }",
"}"),
lines(
"inconsistent return type", //
"found : string",
"required: number"));
}

public void testGetterOverridesPrototypePropertyFromInterface() {
testTypes(
lines(
"/** @interface */",
"class Bar {}",
"/** @type {number} */",
"Bar.prototype.num;",
"",
"/** @implements {Bar} */",
"class Baz {",
" /** @override */",
" get num() { return 3; }",
"}",
"var /** string */ x = (new Baz).num;"),
lines(
"initializing variable", //
"found : number",
"required: string"));
}

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(
lines(
"/** @interface */",
"class Bar {",
" constructor() {",
" /** @type {number} */",
" this.num;",
" }",
"}",
"/** @implements {Bar} */",
"class Baz {",
" /** @override */",
" get num() { return 3; }",
"}",
"var /** string */ x = (new Baz).num;"),
"property num not defined on any superclass of Baz");
}

public void testOverriddenSetterFromSuperclass() {
testTypes(
lines(
"/** @abstract */",
"class Bar {",
" /**",
" * @abstract",
" * @param {number} x",
" */",
" set num(x) {}",
"}",
"/** @extends {Bar} */",
"class Baz extends Bar {",
" /** @override */",
" set num(x) {}",
"}",
"(new Baz).num = 'foo';"),
lines(
"assignment to property num of Baz", //
"found : string",
"required: number"));
}

public void testGetterOverridesMethod() {
// If a getter overrides a method, we infer the getter to be for a function type
testTypes(
lines(
"class Bar {",
" /** @return {number} */",
" num() { return 1; }",
"}",
"/** @extends {Bar} */",
"class Baz extends Bar {",
" /** @override */",
" get num() { return 1; }",
"}"),
lines(
"inconsistent return type", //
"found : number",
"required: function(this:Bar): number"));
}

public void testMisplacedOverrideOnGetter() {
testTypes(
lines(
"/** @abstract */",
"class Bar {}",
"/** @extends {Bar} */",
"class Baz extends Bar {",
" /** @override */",
" get num() { return 3; }",
"}",
"var /** string */ x = (new Baz).num;"),
"property num not defined on any superclass of Baz");
}

public void testOverridingNonMethodWithMethodDoesntBlockTypeCheckingInsideMethod() {
// verify that we still type Bar.prototype.bar with function(this:Bar, number) even though it
// overrides a property from Foo
// thus we get both a "mismatch of ... and the property it overrides" warning
// and a warning for "initializing variable ..." inside bar()
testTypes(
lines(
"class Foo {}",
"/** @type {number} */",
"Foo.prototype.bar = 3;",
"",
"class Bar extends Foo {",
" /** @override */",
" bar(/** number */ n) {",
" var /** string */ str = n;",
" }",
"}"),
new String[] {
lines(
"mismatch of the bar property type "
+ "and the type of the property it overrides from superclass Foo",
"original: number",
"override: function(this:Bar, number): undefined"),
lines(
"initializing variable", //
"found : number",
"required: string")
});
}

public void testGetterWithTemplateTypeReturnIsTypeChecked() {
testTypes(
lines(
"/** @interface @template T */",
"class C {",
" /** @return {T} */",
" get t() {}",
"}",
"/** @implements {C<string>} */",
"class CString {",
" /** @override */",
" get t() { return 3; }", // inconsistent return type
"}"),
lines(
"inconsistent return type", //
"found : number",
"required: string"));
}
} }

0 comments on commit 6e935d7

Please sign in to comment.