Skip to content

Commit

Permalink
Adds typing support for this and super in static methods.
Browse files Browse the repository at this point in the history
The main changes are in `TypedScope` (to generate the type for `super`) and `TypedScopeCreator` (to determine the type of `this` for static methods).

This changes applies to all functions declared as properties on constructors and interfaces. That includes ES5 constructors. However, this does not mean any kind of support for class side inheritance on ES5 constructors.

The majority of the changes are test updates to accommodate the new type information.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=245573045
  • Loading branch information
nreid260 authored and blickly committed Apr 29, 2019
1 parent 12f092b commit 5b1e531
Show file tree
Hide file tree
Showing 12 changed files with 175 additions and 128 deletions.
67 changes: 24 additions & 43 deletions src/com/google/javascript/jscomp/TypeInference.java
Expand Up @@ -764,51 +764,32 @@ private void traverseSuper(Node superNode) {
superNode.setJSType(unknownType); superNode.setJSType(unknownType);
return; return;
} }
Node root = scope.getRootNode();
JSType jsType = root.getJSType(); FunctionType enclosingFunctionType =
FunctionType functionType = jsType != null ? jsType.toMaybeFunctionType() : null; JSType.toMaybeFunctionType(scope.getRootNode().getJSType());
ObjectType superNodeType = unknownType; ObjectType superNodeType = null;
Node context = superNode.getParent();
// NOTE: we currently transpile subclass constructors to use "super.apply", which is not switch (superNode.getParent().getToken()) {
// actually valid ES6. For now, provide a special case to support this, but it should be case CALL:
// removed once class transpilation is after type checking. // Inside a constructor, `super` may have two different types. Calls to `super()` use the
if (context.isCall()) { // super-ctor type, while property accesses use the super-instance type. `Scopes` are only
// Call the superclass constructor. // aware of the latter case.
if (functionType != null && functionType.isConstructor()) { if (enclosingFunctionType != null && enclosingFunctionType.isConstructor()) {
FunctionType superCtor = functionType.getSuperClassConstructor(); superNodeType = enclosingFunctionType.getSuperClassConstructor();
if (superCtor != null) {
superNodeType = superCtor;
}
}
} else if (context.isGetProp() || context.isGetElem()) {
// TODO(sdh): once getTypeOfThis supports statics, we can get rid of this branch, as well as
// the vanilla function search at the top and just return functionScope.getVar("super").
if (root.getParent().isStaticMember()) {
// Since the root is a static member, we're guaranteed that the parent scope is a class.
Node classNode = scope.getParent().getRootNode();
checkState(classNode.isClass());
FunctionType thisCtor = JSType.toMaybeFunctionType(classNode.getJSType());
if (thisCtor != null) {
FunctionType superCtor = thisCtor.getSuperClassConstructor();
if (superCtor != null) {
superNodeType = superCtor;
}
}
} else if (functionType != null) {
// Refer to a superclass instance property.
ObjectType thisInstance = ObjectType.cast(functionType.getTypeOfThis());
if (thisInstance != null) {
FunctionType superCtor = thisInstance.getSuperClassConstructor();
if (superCtor != null) {
ObjectType superInstance = superCtor.getInstanceType();
if (superInstance != null) {
superNodeType = superInstance;
}
}
} }
} break;

case GETELEM:
case GETPROP:
superNodeType = ObjectType.cast(functionScope.getSlot("super").getType());
break;

default:
throw new IllegalStateException(
"Unexpected parent of SUPER: " + superNode.getParent().toStringTree());
} }
superNode.setJSType(superNodeType);
superNode.setJSType(superNodeType != null ? superNodeType : unknownType);
} }


private void traverseNewTarget(Node newTargetNode) { private void traverseNewTarget(Node newTargetNode) {
Expand Down
36 changes: 27 additions & 9 deletions src/com/google/javascript/jscomp/TypedScope.java
Expand Up @@ -21,6 +21,7 @@
import com.google.common.collect.Iterables; import com.google.common.collect.Iterables;
import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.jstype.FunctionType;
import com.google.javascript.rhino.jstype.JSType; import com.google.javascript.rhino.jstype.JSType;
import com.google.javascript.rhino.jstype.ObjectType; import com.google.javascript.rhino.jstype.ObjectType;
import com.google.javascript.rhino.jstype.StaticTypedScope; import com.google.javascript.rhino.jstype.StaticTypedScope;
Expand Down Expand Up @@ -146,16 +147,33 @@ TypedVar makeImplicitVar(ImplicitVar var) {
} }


private JSType getImplicitVarType(ImplicitVar var) { private JSType getImplicitVarType(ImplicitVar var) {
if (var == ImplicitVar.ARGUMENTS) { switch (var) {
// Look for an extern named "arguments" and use its type if available. case ARGUMENTS:
// TODO(sdh): consider looking for "Arguments" ctor rather than "arguments" var: this could // Look for an extern named "arguments" and use its type if available.
// allow deleting the variable, which doesn't really belong in externs in the first place. // TODO(sdh): consider looking for "Arguments" ctor rather than "arguments" var: this could
TypedVar globalArgs = getGlobalScope().getVar(Var.ARGUMENTS); // allow deleting the variable, which doesn't really belong in externs in the first place.
return globalArgs != null && globalArgs.isExtern() TypedVar globalArgs = getGlobalScope().getVar(Var.ARGUMENTS);
? globalArgs.getType() return globalArgs != null && globalArgs.isExtern() ? globalArgs.getType() : null;
: null;
case THIS:
return getTypeOfThis();

case SUPER:
// Inside a constructor, `super` may have two different types. Calls to `super()` use the
// super-ctor type, while property accesses use the super-instance type. This logic always
// returns the latter case.
ObjectType receiverType = ObjectType.cast(getTypeOfThis());
if (receiverType == null) {
return null;
} else if (receiverType.isInstanceType()) {
FunctionType superclassCtor = receiverType.getSuperClassConstructor();
return superclassCtor == null ? null : superclassCtor.getInstanceType();
} else {
return receiverType.getImplicitPrototype();
}
} }
return getTypeOfThis();
throw new AssertionError();
} }


/** /**
Expand Down
23 changes: 17 additions & 6 deletions src/com/google/javascript/jscomp/TypedScopeCreator.java
Expand Up @@ -1685,18 +1685,29 @@ private FunctionType createFunctionTypeFromNodes(
} }


// Infer the context type. // Infer the context type.
JSType fallbackReceiverType = null;
if (ownerType != null if (ownerType != null
&& ownerType.isFunctionPrototypeType() && ownerType.isFunctionPrototypeType()
&& ownerType.getOwnerFunction().hasInstanceType()) { && ownerType.getOwnerFunction().hasInstanceType()) {
builder.inferThisType(info, ownerType.getOwnerFunction().getInstanceType()); fallbackReceiverType = ownerType.getOwnerFunction().getInstanceType();
} else if (ownerType != null
&& ownerType.isFunctionType()
&& ownerType.toMaybeFunctionType().hasInstanceType()
&& lvalueNode != null
&& lvalueNode.isStaticMember()) {
// Limit this case to members of ctors and interfaces decalared using `static`. Most
// namespaces, like object literals, are assumed to declare free functions, so we exclude
// them. Additionally, methods *assigned* to a ctor, especially an ES5 ctor, were never
// designed with static polymorphism in mind, so excluding them preserves their assumptions.
fallbackReceiverType = ownerType;
} else if (ownerNode != null && ownerNode.isThis()) { } else if (ownerNode != null && ownerNode.isThis()) {
// If we have a 'this' node, use the scope type. fallbackReceiverType = currentScope.getTypeOfThis();
builder.inferThisType(info, currentScope.getTypeOfThis());
} else {
builder.inferThisType(info);
} }


return builder.inferParameterTypes(parametersNode, info).buildAndRegister(); return builder
.inferThisType(info, fallbackReceiverType)
.inferParameterTypes(parametersNode, info)
.buildAndRegister();
} }


/** /**
Expand Down
5 changes: 3 additions & 2 deletions test/com/google/javascript/jscomp/AstFactoryTest.java
Expand Up @@ -248,7 +248,7 @@ public void createSuperForFunction() {
"class A {}", "class A {}",
"class B extends A {", // "class B extends A {", //
" method() {", " method() {",
" super;", // created `super` should match this one " super.method();", // created `super` should match this one
" }", " }",
"}", "}",
"")); ""));
Expand All @@ -268,7 +268,8 @@ public void createSuperForFunction() {
methodFunction methodFunction
.getLastChild() // method function body .getLastChild() // method function body
.getFirstChild() // expr_result .getFirstChild() // expr_result
.getOnlyChild(); // super .getOnlyChild() // call "method"
.getFirstFirstChild(); // super


Node newSuper = astFactory.createSuperForFunction(methodFunction); Node newSuper = astFactory.createSuperForFunction(methodFunction);
assertNode(newSuper).isEqualTo(existingSuper); assertNode(newSuper).isEqualTo(existingSuper);
Expand Down
8 changes: 3 additions & 5 deletions test/com/google/javascript/jscomp/CheckConformanceTest.java
Expand Up @@ -929,15 +929,13 @@ public void testRestrictedCall2() {
"/** @param {*} a */\n" + "/** @param {*} a */\n" +
"C.m = function(a){}\n"; "C.m = function(a){}\n";


testNoWarning( testNoWarning(code + "C.m(1);");
code + "C.m(1);");


testWarning(code + "C.m('str');", CheckConformance.CONFORMANCE_VIOLATION); testWarning(code + "C.m('str');", CheckConformance.CONFORMANCE_VIOLATION);


testNoWarning( testNoWarning(code + "C.m.call(C, 1);");
code + "C.m.call(this, 1);");


testWarning(code + "C.m.call(this, 'str');", CheckConformance.CONFORMANCE_VIOLATION); testWarning(code + "C.m.call(C, 'str');", CheckConformance.CONFORMANCE_VIOLATION);
} }


@Test @Test
Expand Down
29 changes: 24 additions & 5 deletions test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java
Expand Up @@ -3014,26 +3014,45 @@ public void testEs6ClassSideInheritedMethods_areDisambiguated() {


@Test @Test
public void testEs6ClassSideInheritedMethods_referencedWithThis_areDisambiguated() { public void testEs6ClassSideInheritedMethods_referencedWithThis_areDisambiguated() {
// TODO(b/117437011): compiler should disambiguate `method` but can't because `this` is unknown test(
// in static methods
testSame(
lines( lines(
"class Foo {", "class Foo {",
" static method() {}", " static method() {}",
" static useMethod() {", " static useMethod() {",
" this.method();", // this could refer to either Foo.method or SubFoo.method " this.method();",
" }", " }",
"}", "}",
"class SubFoo extends Foo {", "class SubFoo extends Foo {",
" static method() {}", " static method() {}",
"}", "}",
"",
"class Bar {", "class Bar {",
" static method() {}", " static method() {}",
"}", "}",
"",
"SubFoo.useMethod();", "SubFoo.useMethod();",
"Foo.method();", "Foo.method();",
"SubFoo.method();", "SubFoo.method();",
"Bar.method();")); "Bar.method();"),
lines(
"class Foo {",
" static _typeof_Foo_$method() {}",
" static useMethod() {",
" this._typeof_Foo_$method();",
" }",
"}",
"class SubFoo extends Foo {",
" static _typeof_Foo_$method() {}",
"}",
"",
"class Bar {",
" static _typeof_Bar_$method() {}",
"}",
"",
"SubFoo.useMethod();",
"Foo._typeof_Foo_$method();",
"SubFoo._typeof_Foo_$method();",
"Bar._typeof_Bar_$method();"));
} }


@Test @Test
Expand Down
Expand Up @@ -118,6 +118,7 @@ public void exportClassAsSymbolAndMethodAsProperty() {
"};", "};",
"/**", "/**",
" * @return {undefined}", " * @return {undefined}",
" * @this {(typeof a.b.c)}",
" */", " */",
"foobar.exportedStaticMethod = function() {", "foobar.exportedStaticMethod = function() {",
"};", "};",
Expand Down
18 changes: 10 additions & 8 deletions test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java
Expand Up @@ -1590,14 +1590,16 @@ public void testAmbiguousDefinitionsAllPropagationTypes() {


@Test @Test
public void testAmbiguousDefinitionsCallWithThis() { public void testAmbiguousDefinitionsCallWithThis() {
String source = CompilerTypeTestCase.CLOSURE_DEFS + lines( String source =
"var globalVar = 1;", CompilerTypeTestCase.CLOSURE_DEFS
"A.modifiesThis = function() { this.x = 5; };", + lines(
"/**@constructor*/function Constructor() { Constructor.modifiesThis.call(this); };", "A.modifiesThis = function() { this.x = 5; };",
"Constructor.modifiesThis = function() {};", "B.modifiesThis = function() {};",
"new Constructor();", "",
"A.modifiesThis();" "/** @constructor */ function Constructor() { B.modifiesThis.call(this); };",
); "",
"new Constructor();",
"A.modifiesThis();");


// Can't tell which modifiesThis is being called so it assumes both. // Can't tell which modifiesThis is being called so it assumes both.
assertPureCallsMarked(source, ImmutableList.of("Constructor")); assertPureCallsMarked(source, ImmutableList.of("Constructor"));
Expand Down
Expand Up @@ -581,8 +581,7 @@ public void testInnerSuperCallStaticEs2015Out() {
Node fakeSuperDotM = wrapperArrowFunction.getLastChild(); Node fakeSuperDotM = wrapperArrowFunction.getLastChild();
assertNode(fakeSuperDotM).hasJSTypeThat().isEqualTo(classAPropertyMType); assertNode(fakeSuperDotM).hasJSTypeThat().isEqualTo(classAPropertyMType);
Node fakeSuperNode = fakeSuperDotM.getFirstChild(); Node fakeSuperNode = fakeSuperDotM.getFirstChild();
// TODO(b/118174876): We currently type `super` as unknown in static methods. assertNode(fakeSuperNode).isCall().hasJSTypeThat().toStringIsEqualTo("(typeof A)");
assertNode(fakeSuperNode).isCall().hasJSTypeThat().toStringIsEqualTo("?");
assertNode(fakeSuperNode.getFirstChild()).matchesQualifiedName("Object.getPrototypeOf"); assertNode(fakeSuperNode.getFirstChild()).matchesQualifiedName("Object.getPrototypeOf");
} }


Expand Down
25 changes: 12 additions & 13 deletions test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java
Expand Up @@ -3486,8 +3486,8 @@ public void testStaticMethod_overriddenInBody_withSupertype_isBad() {
lines( lines(
"mismatch of the method property type and the type of the property it overrides " "mismatch of the method property type and the type of the property it overrides "
+ "from supertype (typeof Base)", + "from supertype (typeof Base)",
"original: function((number|string)): undefined", "original: function(this:(typeof Base), (number|string)): undefined",
"override: function(string): undefined")); "override: function(this:(typeof Sub), string): undefined"));
} }


@Test @Test
Expand All @@ -3504,8 +3504,8 @@ public void testStaticMethod_overriddenInBody_withSupertype_fromInline_isBad() {
lines( lines(
"mismatch of the method property type and the type of the property it overrides " "mismatch of the method property type and the type of the property it overrides "
+ "from supertype (typeof Base)", + "from supertype (typeof Base)",
"original: function((number|string)): undefined", "original: function(this:(typeof Base), (number|string)): undefined",
"override: function(string): undefined")); "override: function(this:(typeof Sub), string): undefined"));
} }


@Test @Test
Expand Down Expand Up @@ -3544,7 +3544,7 @@ public void testStaticMethod_overriddenOutsideBody_withSupertype_isBad() {
lines( lines(
"mismatch of the method property type and the type of the property it overrides " "mismatch of the method property type and the type of the property it overrides "
+ "from supertype (typeof Base)", + "from supertype (typeof Base)",
"original: function((number|string)): undefined", "original: function(this:(typeof Base), (number|string)): undefined",
"override: function(string): undefined")); "override: function(string): undefined"));
} }


Expand Down Expand Up @@ -3637,7 +3637,7 @@ public void testStaticMethod_onNamespacedType_overridden_withNonSubtype_isBad()
lines( lines(
"mismatch of the method property type and the type of the property it overrides " "mismatch of the method property type and the type of the property it overrides "
+ "from supertype (typeof ns.Base)", + "from supertype (typeof ns.Base)",
"original: function(string): undefined", "original: function(this:(typeof ns.Base), string): undefined",
"override: function(number): undefined")); "override: function(number): undefined"));
} }


Expand Down Expand Up @@ -4064,16 +4064,15 @@ public void testClassTypeOfThisInMethod() {
public void testClassTypeOfThisInStaticMethod() { public void testClassTypeOfThisInStaticMethod() {
testTypes( testTypes(
lines( lines(
"class Foo {", "class Foo {", //
" static foo() {", " static foo() {",
" var /** null */ foo = this;", " var /** null */ foo = this;",
" }", " }",
"}")); "}"),
// TODO(sdh): Should be an error, but wait on it since it's a new warning. lines(
// lines( "initializing variable", //
// "initializing variable", "found : (typeof Foo)",
// "found : function(new:Foo): undefined", "required: null"));
// "required: null"));
} }


@Test @Test
Expand Down
18 changes: 10 additions & 8 deletions test/com/google/javascript/jscomp/TypeCheckTest.java
Expand Up @@ -3855,10 +3855,11 @@ public void testStubFunctionDeclaration2() {
} }


@Test @Test
public void testStubFunctionDeclaration3() { public void testStubFunctionDeclaration_static_onEs5Class() {
testFunctionType( testFunctionType(
"/** @constructor */ function f() {};" + lines(
"/** @return {undefined} */ f.foo;", "/** @constructor */ function f() {};", //
"/** @return {undefined} */ f.foo;"),
"f.foo", "f.foo",
"function(): undefined"); "function(): undefined");
} }
Expand Down Expand Up @@ -8453,11 +8454,12 @@ public void testThis8() {
} }


@Test @Test
public void testThis9() { public void testTypeOfThis_inStaticMethod_onEs5Ctor_isUnknown() {
// In A.bar, the type of {@code this} is unknown. testTypes(
testTypes("/** @constructor */function A(){};" + lines(
"A.prototype.foo = 3;" + "/** @constructor */function A(){};",
"/** @return {string} */ A.bar = function() { return this.foo; };"); "A.foo = 3;",
"/** @return {string} */ A.bar = function() { return this.foo; };"));
} }


@Test @Test
Expand Down

0 comments on commit 5b1e531

Please sign in to comment.