diff --git a/src/com/google/javascript/jscomp/CheckSuper.java b/src/com/google/javascript/jscomp/CheckSuper.java index c6f20dc47f3..4f33da636f0 100644 --- a/src/com/google/javascript/jscomp/CheckSuper.java +++ b/src/com/google/javascript/jscomp/CheckSuper.java @@ -32,6 +32,14 @@ final class CheckSuper implements HotSwapCompilerPass, Callback { "JSC_INVALID_SUPER_CALL", "super() not allowed except in the constructor of a subclass"); + static final DiagnosticType INVALID_SUPER_USAGE = + DiagnosticType.error( + "JSC_INVALID_SUPER_USAGE", "''super'' may only be used in a call or property access"); + + static final DiagnosticType INVALID_SUPER_ACCESS = + DiagnosticType.error( + "JSC_INVALID_SUPER_ACCESS", "''super'' may only be accessed within a class method"); + static final DiagnosticType INVALID_SUPER_CALL_WITH_SUGGESTION = DiagnosticType.error( "JSC_INVALID_SUPER_CALL_WITH_SUGGESTION", "super() not allowed here. Did you mean super.{0}?"); @@ -93,29 +101,45 @@ private boolean visitClass(NodeTraversal t, Node n) { } private void visitSuper(NodeTraversal t, Node n, Node parent) { + if (parent.isCall()) { + visitSuperCall(t, n, parent); + } else if (parent.isGetProp() || parent.isGetElem()) { + visitSuperAccess(t, n); + } else { + t.report(n, INVALID_SUPER_USAGE); + } + } + + private void visitSuperCall(NodeTraversal t, Node n, Node parent) { Node classNode = NodeUtil.getEnclosingClass(n); if (classNode == null || classNode.getSecondChild().isEmpty()) { t.report(n, INVALID_SUPER_CALL); return; } - if (parent.isCall()) { - Node fn = NodeUtil.getEnclosingFunction(parent); - if (fn == null) { - t.report(n, INVALID_SUPER_CALL); - return; - } + Node fn = NodeUtil.getEnclosingFunction(parent); + if (fn == null) { + t.report(n, INVALID_SUPER_CALL); + return; + } - Node memberDef = fn.getParent(); - if (memberDef.isMemberFunctionDef()) { - if (memberDef.matchesQualifiedName("constructor")) { - // No error. - } else { - t.report(n, INVALID_SUPER_CALL_WITH_SUGGESTION, memberDef.getString()); - } + Node memberDef = fn.getParent(); + if (memberDef.isMemberFunctionDef()) { + if (memberDef.matchesQualifiedName("constructor")) { + // No error. } else { - t.report(n, INVALID_SUPER_CALL); + t.report(n, INVALID_SUPER_CALL_WITH_SUGGESTION, memberDef.getString()); } + } else { + t.report(n, INVALID_SUPER_CALL); + } + } + + private void visitSuperAccess(NodeTraversal t, Node n) { + Node classNode = NodeUtil.getEnclosingClass(n); + if (classNode == null) { + t.report(n, INVALID_SUPER_ACCESS); + return; } } diff --git a/src/com/google/javascript/jscomp/Es6ConvertSuperConstructorCalls.java b/src/com/google/javascript/jscomp/Es6ConvertSuperConstructorCalls.java index 6d0d5e9fa07..17ef0866687 100644 --- a/src/com/google/javascript/jscomp/Es6ConvertSuperConstructorCalls.java +++ b/src/com/google/javascript/jscomp/Es6ConvertSuperConstructorCalls.java @@ -64,7 +64,19 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { // TODO(bradfordcsmith): Avoid creating data for non-constructor functions. constructorDataStack.push(new ConstructorData(n)); } else if (n.isSuper()) { + // NOTE(sdh): Es6RewriteRestAndSpread rewrites super(...args) to super.apply(this, args), + // so we need to go up a level if that happened. This extra getParent() could be removed + // if we could flip the order of these transpilation passes. Node superCall = parent.isCall() ? parent : parent.getParent(); + if (!superCall.isCall() && parent.isGetProp()) { + // This is currently broken because whatever earlier pass is responsible for transpiling + // away super inside GETPROP is not handling it. Unfortunately there's not really a good + // way to handle it without additional runtime support, and it will be a lot easier to deal + // with after classes are typechecked natively. So for now, we just don't support it. This + // is not a problem, since any such usages were already broken, just with a different error. + t.report(n, Es6ToEs3Util.CANNOT_CONVERT_YET, "super access with no extends clause"); + return false; + } checkState(superCall.isCall(), superCall); ConstructorData constructorData = checkNotNull(constructorDataStack.peek()); constructorData.superCalls.add(superCall); diff --git a/test/com/google/javascript/jscomp/CheckSuperTest.java b/test/com/google/javascript/jscomp/CheckSuperTest.java index da9cd1cd504..5da819f8a95 100644 --- a/test/com/google/javascript/jscomp/CheckSuperTest.java +++ b/test/com/google/javascript/jscomp/CheckSuperTest.java @@ -15,8 +15,10 @@ */ package com.google.javascript.jscomp; +import static com.google.javascript.jscomp.CheckSuper.INVALID_SUPER_ACCESS; import static com.google.javascript.jscomp.CheckSuper.INVALID_SUPER_CALL; import static com.google.javascript.jscomp.CheckSuper.INVALID_SUPER_CALL_WITH_SUGGESTION; +import static com.google.javascript.jscomp.CheckSuper.INVALID_SUPER_USAGE; import static com.google.javascript.jscomp.CheckSuper.MISSING_CALL_TO_SUPER; import static com.google.javascript.jscomp.CheckSuper.THIS_BEFORE_SUPER; @@ -31,6 +33,14 @@ protected void setUp() throws Exception { super.setUp(); } + public void testInvalidUsage() { + // 'super' may only appear directly within a CALL, a GETPROP, or a GETELEM. + testError("class Foo extends Bar { constructor() { super(); super; } }", INVALID_SUPER_USAGE); + testError("class Foo extends Bar { foo() { super; } }", INVALID_SUPER_USAGE); + testError("function f() { super; }", INVALID_SUPER_USAGE); + testError("super;", INVALID_SUPER_USAGE); + } + public void testMissingSuper() { testError("class C extends D { constructor() {} }", MISSING_CALL_TO_SUPER); testError("class C extends D { constructor() { super.foo(); } }", MISSING_CALL_TO_SUPER); @@ -72,27 +82,34 @@ public void testNoWarning_thisWithinFunction() { testSame("class C extends D { constructor() { const c = function() { this; }; super(); } }"); } - public void testInConstructorNoBaseClass() { + public void testOutsideClass() { testError("var i = super();", INVALID_SUPER_CALL); + testError("var i = super.i;", INVALID_SUPER_ACCESS); + testError("var i = super[x];", INVALID_SUPER_ACCESS); + } + + public void testInOrdinaryFunction() { + testError("function f() { super(); }", INVALID_SUPER_CALL); + testError("function f() { var i = super.i; }", INVALID_SUPER_ACCESS); + testError("function f() { var i = super[x]; }", INVALID_SUPER_ACCESS); } - public void testNoBaseClass() { + public void testCallWithNoBaseClass() { testError("class C { constructor() { super(); }}", INVALID_SUPER_CALL); testError("class C { constructor() { super(1); }}", INVALID_SUPER_CALL); - testError("class C { static foo() { super(); }}", INVALID_SUPER_CALL); } - public void testInConstructor() { + public void testCallInConstructor() { testSame("class C extends D { constructor() { super(); }}"); testSame("class C extends D { constructor() { super(1); }}"); } - public void testNestedInConstructor() { + public void testNestedCallInConstructor() { testError( "class C extends D { constructor() { (()=>{ super(); })(); }}", MISSING_CALL_TO_SUPER); } - public void testInNonConstructor() { + public void testCallInMethod() { test( srcs("class C extends D { foo() { super(); }}"), error(INVALID_SUPER_CALL_WITH_SUGGESTION) @@ -101,17 +118,17 @@ public void testInNonConstructor() { srcs("class C extends D { foo() { super(1); }}"), error(INVALID_SUPER_CALL_WITH_SUGGESTION) .withMessage("super() not allowed here. Did you mean super.foo?")); - } - - public void testNestedInNonConstructor() { + testError("class C { static foo() { super(); }}", INVALID_SUPER_CALL); testError("class C extends D { foo() { (()=>{ super(); })(); }}", INVALID_SUPER_CALL); } - public void testDotMethodInNonConstructor() { + public void testPropertyInMethod() { testSame("class C extends D { foo() { super.foo(); }}"); testSame("class C extends D { foo() { super.foo(1); }}"); // TODO(tbreisacher): Consider warning for this. It's valid but likely indicates a mistake. + testSame("class C extends D { foo() { var x = super.bar; }}"); + testSame("class C extends D { foo() { var x = super[y]; }}"); testSame("class C extends D { foo() { super.bar(); }}"); } @@ -135,4 +152,27 @@ public void testWarning_withInvalidReturnInConstructor() { public void testInvalidThisReference_withExplicitReturnInConstructor() { testError("class C extends D { constructor() { this.x = 3; return {}; } }", THIS_BEFORE_SUPER); } + + public void testPropertyInConstructor() { + // TODO(sdh): See note in testPropertyInMethod - these are valid but questionable. + testSame("class C extends D { constructor() { super(); super.foo(); }}"); + testSame("class C extends D { constructor() { super(); super.foo(1); }}"); + testSame("class C extends D { constructor() { super(); var x = super.bar; }}"); + } + + public void testNestedProperty() { + testSame("class C extends D { constructor() { super(); (()=>{ super.foo(); })(); }}"); + testSame("class C extends D { foo() { (()=>{ super.foo(); })(); }}"); + } + + public void testPropertyNoBaseClass() { + // TODO(sdh): See note in testPropertyInMethod - these are valid but questionable. + testSame("class C { constructor() { super.foo(); }}"); + testSame("class C { foo() { super.foo(1); }}"); + testSame("class C { foo() { super.bar(); }}"); + testSame("class C { foo() { super.baz; }}"); + testSame("class C { foo() { super[x](); }}"); + } + + // TODO(sdh): test super.prop access from static methods - this will need runtime tests, too! } diff --git a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java index ec22c6b29c1..927cd288c56 100644 --- a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java @@ -1966,7 +1966,7 @@ public void testClassDeclarationInlineConstructorParameters() { public void testClassDeclarationConstructorParametersMismatch() { testTypes( lines( - "class Foo {", + "class Foo {", // " constructor(/** number */ arg) {}", "}", "new Foo('xyz');"), @@ -2142,15 +2142,33 @@ public void testClassSyntaxInterfaceExtendsInterfaceMismatch() { } public void testClassSyntaxRecord() { - // TODO(sdh): Add a matching property. testTypes( lines( "/** @record */", // - "class Rec {}", - "var /** !Rec */ rec = {};")); + "class Rec {", + " constructor() { /** @type {string} */ this.bar; }", + " foo(/** number */ arg) {}", + "}", + "var /** !Rec */ rec = {bar: 'x', foo() {}};")); + } + + public void testClassSyntaxRecordWithMethodMismatch() { + testTypes( + lines( + "/** @record */", // + "class Rec {", + " foo(/** number */ arg) {}", + "}", + "var /** !Rec */ rec = {foo(/** string */ arg) {}};"), + lines( + "initializing variable", + "found : {foo: function(string): undefined}", + "required: Rec", + "missing : []", + "mismatch: [foo]")); } - public void testClassSyntaxRecordMismatch() { + public void testClassSyntaxRecordWithPropertyMismatch() { testTypes( lines( "/** @record */", // @@ -2310,6 +2328,17 @@ public void testClassMissingTransitiveInterfaceMethod() { "property foo on interface Foo is not implemented by type Baz"); } + public void testClassInheritedInterfaceMethod() { + testTypes( + lines( + "/** @interface */", + "class Foo { foo() {} bar() {} }", + "/** @abstract */", + "class Bar { foo() {} }", + "/** @implements {Foo} */", + "class Baz extends Bar { /** @override */ bar() {} }")); + } + public void testClassMixinAllowsNonOverriddenInterfaceMethods() { testTypes( lines( @@ -2327,18 +2356,6 @@ public void testClassMixinAllowsNonOverriddenInterfaceMethods() { "property foo on interface Foo is not implemented by type Baz"); } - public void testClassMissingSuperCall() { - // TODO(sdh): Should be an error to access 'this' before super (but maybe not in TypeCheck). - testTypes( - lines( - "class Bar {}", // - "class Foo extends Bar {", - " constructor() {", - " this.x = 42;", - " }", - "}")); - } - public void testClassDeclarationWithExtendsOnlyInJSDoc() { // TODO(sdh): Should be an error, but we may need to clean up the codebase first. testTypes( @@ -2669,7 +2686,7 @@ public void testClassStaticMethodOverriddenWithIncompatibleType() { " static method(arg) {}", "}", "class Sub extends Base {", - " /** @override */", + " /** @override @param {string} arg */", " static method(arg) {}", "}")); // TODO(sdh): This should actually check the override. @@ -2680,6 +2697,24 @@ public void testClassStaticMethodOverriddenWithIncompatibleType() { // "override: function(string): undefined")); } + public void testClassStaticMethodOverriddenWithIncompatibleInlineType() { + testTypes( + lines( + "class Base {", + " static method(/** string|number */ arg) {}", + "}", + "class Sub extends Base {", + " /** @override */", + " static method(/** string */ arg) {}", + "}")); + // TODO(sdh): This should actually check the override. + // lines( + // "mismatch of the method property type and the type of the property it overrides " + // + "from superclass Base", + // "original: function((number|string)): undefined", + // "override: function(string): undefined")); + } + public void testClassTreatedAsStruct() { testTypes( lines( @@ -2796,18 +2831,14 @@ public void testClassSuperMethodFromDifferentMethod() { " foo() {}", "}", "class Bar extends Foo {", - " /** @override */", " bar() {", " var /** null */ x = super.foo();", " }", "}"), - // TODO(sdh): This should allow the call to super.foo but cause a type error. - new String[] { - "property bar not defined on any superclass of Bar", - lines( - "initializing variable", - "found : string", - "required: null")}); + lines( + "initializing variable", + "found : string", + "required: null")); } public void testClassSuperMethodNotWidenedWhenOverrideWidens() { @@ -2830,6 +2861,74 @@ public void testClassSuperMethodNotWidenedWhenOverrideWidens() { "required: string")); } + public void testClassStaticSuperParameterMismatch() { + testTypes( + lines( + "class Foo {", + " static foo(/** number */ arg) {}", + "}", + "class Bar extends Foo {", + " /** @override */", + " static foo() {", + " super.foo('x');", + " }", + "}")); + // TODO(sdh): Should produce an error. + // lines( + // "actual parameter 1 of Foo.foo does not match formal parameter", + // "found : string", + // "required: number")); + } + + public void testClassStaticSuperParameterCountMismatch() { + testTypes( + lines( + "class Foo {", + " static foo() {}", + "}", + "class Bar extends Foo {", + " /** @override */", + " static foo() {", + " super.foo(1);", + " }", + "}")); + // TODO(sdh): Should produce an error. + // "Function super.foo: called with 1 argument(s). " + // + "Function requires at least 0 argument(s) and no more than 0 argument(s)."); + } + + public void testClassStaticSuperNotPresent() { + testTypes( + lines( + "class Foo {}", + "class Bar extends Foo {", + " static foo() {", + " super.foo;", + " }", + "}")); + // TODO(sdh): Should produce an error. + // "Property foo never defined on Foo"); + } + + public void testClassStaticSuperCallsDifferentMethod() { + testTypes( + lines( + "class Foo {", + " /** @param {string} arg */", + " static foo(arg) {}", + "}", + "class Bar extends Foo {", + " /** @override */", + " static foo(/** string|number */ arg) {}", + " static bar() { super.foo(42); }", + "}")); + // TODO(sdh): Should produce an error. + // lines( + // "actual parameter 1 of Foo.foo does not match formal parameter", + // "found : number", + // "required: string")); + } + // TODO - class methods (parameter types, return types, overrides inherit param/returns, // errors for wrong-variant overrides, class-side inheritance override errors, calling // infers correctly, templates, async, super in ctor, super.foo in method) diff --git a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java index 755b5895c34..4ee90634ed6 100644 --- a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java @@ -1527,6 +1527,25 @@ public void testClassDeclarationWithOverriddenStaticMethod() { assertType(foo).withTypeOfProp("method").isEqualTo(method); } + public void testClassDeclarationWithAsyncMethod() { + testSame( + lines( + "class Foo {", + " /** @param {string} arg */", + " async method(arg) {", + " METHOD:;", + " }", + "}")); + TypedScope methodBlockScope = getLabeledStatement("METHOD").enclosingScope; + TypedScope methodScope = methodBlockScope.getParentScope(); + assertScope(methodScope).declares("arg").directly().withTypeThat().isString(); + + FunctionType method = (FunctionType) methodScope.getRootNode().getJSType(); + assertType(method).toStringIsEqualTo("function(this:Foo, string): Promise"); + FunctionType foo = (FunctionType) (findNameType("Foo", globalScope)); + assertType(foo.getInstanceType()).withTypeOfProp("method").isEqualTo(method); + } + public void testClassDeclarationWithGeneratorMethod() { testSame( lines( diff --git a/test/com/google/javascript/jscomp/runtime_tests/class_test.js b/test/com/google/javascript/jscomp/runtime_tests/class_test.js index 17f623eab47..cb6072d79bb 100644 --- a/test/com/google/javascript/jscomp/runtime_tests/class_test.js +++ b/test/com/google/javascript/jscomp/runtime_tests/class_test.js @@ -218,3 +218,24 @@ function testExtendsObject() { const withConstructor = new WithConstructor('message'); assertEquals(withConstructor, withConstructor.superResult); } + +function testSuperInStaticMethod() { + class C1 { + static foo() { return 42; } + } + class D extends C1 { + static foo() { return super.foo() + 1; } + } + assertEquals(43, D.foo()); +} + +function testSuperInDifferentStaticMethod() { + class C2 { + static foo() { return 12; } + } + class D extends C2 { + static foo() { throw new Error('unexpected'); } + static bar() { return super.foo() + 2; } + } + assertEquals(14, D.bar()); +}