Clean up CheckSuper to handle non-constructor usages

The main change is to loosen the requirement that 'super' may only be used in classes that extend other classes.  This is not actually required by the spec.  This change also makes the error messages slightly more precise, rather than just assuming super is only ever called.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=203161904

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;
     }
   }
 

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);

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!
 }