From 407cb25bcf5c4afd23737ebc1414c04f84a31829 Mon Sep 17 00:00:00 2001
From: sdh <sdh@google.com>
Date: Fri, 6 Jul 2018 14:14:01 -0700
Subject: [PATCH] Validate dynamic extends clauses during TypeCheck

Allows extending things other than qualified names, provided `@extends` is specified in the JSDoc.  Emits a warning if no such annotation is given, or else checks that the extended type is in fact compatible with any declared supertype.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=203523284
---
 .../google/javascript/jscomp/TypeCheck.java   |  4 +-
 .../javascript/jscomp/TypeValidator.java      | 38 +++++++-
 .../javascript/jscomp/TypedScopeCreator.java  | 24 +++--
 .../jscomp/TypeCheckNoTranspileTest.java      | 90 ++++++++++++++-----
 4 files changed, 123 insertions(+), 33 deletions(-)

diff --git a/src/com/google/javascript/jscomp/TypeCheck.java b/src/com/google/javascript/jscomp/TypeCheck.java
index 1148af2db8b..8e9e6df766c 100644
--- a/src/com/google/javascript/jscomp/TypeCheck.java
+++ b/src/com/google/javascript/jscomp/TypeCheck.java
@@ -2001,7 +2001,9 @@ private void visitClass(NodeTraversal t, Node n) {
       // Ensure that the `extends` clause is actually a constructor or interface.  If it is, but
       // it's the wrong one then checkConstructor or checkInterface will warn.
       JSType superType = extendsClause.getJSType();
-      if (!(superType.isConstructor() || superType.isInterface())) {
+      if (superType.isConstructor() || superType.isInterface()) {
+        validator.expectExtends(n, functionType, superType.toMaybeFunctionType());
+      } else {
         compiler.report(
             t.makeError(
                 n,
diff --git a/src/com/google/javascript/jscomp/TypeValidator.java b/src/com/google/javascript/jscomp/TypeValidator.java
index 3f977fcff56..178a8ebf596 100644
--- a/src/com/google/javascript/jscomp/TypeValidator.java
+++ b/src/com/google/javascript/jscomp/TypeValidator.java
@@ -678,8 +678,7 @@ void expectSuperType(NodeTraversal t, Node n, ObjectType superObject,
             superObject, declaredSuper,
             report(t.makeError(n, MISSING_EXTENDS_TAG_WARNING, subObject.toString())));
       } else {
-        mismatch(n, "mismatch in declaration of superclass type",
-            superObject, declaredSuper);
+        mismatch(n, "mismatch in declaration of superclass type", superObject, declaredSuper);
       }
 
       // Correct the super type.
@@ -689,6 +688,41 @@ void expectSuperType(NodeTraversal t, Node n, ObjectType superObject,
     }
   }
 
+  /**
+   * Expect that an ES6 class's extends clause is actually a supertype of the given class.
+   *
+   * @param n The node where warnings should point to.
+   * @param subCtor The sub constructor type.
+   * @param superCtor The expected super constructor.
+   */
+  void expectExtends(Node n, FunctionType subCtor, FunctionType superCtor) {
+    if (superCtor == null || (!superCtor.isConstructor() && !superCtor.isInterface())) {
+      // toMaybeFunctionType failed, or we've got a loose type.  Let it go for now.
+      return;
+    }
+    if (superCtor.isConstructor() != subCtor.isConstructor()) {
+      // Don't bother looking if one is a constructor and the other is an interface.
+      // We'll report an error elsewhere.
+      return;
+    }
+    ObjectType superInstance = superCtor.getInstanceType();
+    if (subCtor.isConstructor()) {
+      // There should be exactly one superclass, and it needs to have this constructor.
+      ObjectType declaredSuper = subCtor.getSuperClassConstructor().getInstanceType();
+      if (!superInstance.isEquivalentTo(declaredSuper)) {
+        mismatch(n, "mismatch in declaration of superclass type", superInstance, declaredSuper);
+      }
+    } else if (subCtor.isInterface()) {
+      // Find an equivalent constructor in the superinterfaces.  There may have been multiple
+      // super-interfaces marked, but we can't know which was intended so just give the error
+      // on the first one.
+      if (!subCtor.explicitlyImplOrExtInterface(superCtor)) {
+        ObjectType extended = subCtor.getExtendedInterfaces().get(0);
+        mismatch(n, "mismatch in declaration of superclass type", superInstance, extended);
+      }
+    }
+  }
+
   /**
    * Expect that it's valid to assign something to a given type's prototype.
    *
diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java
index dca2804dbf3..481bf00353f 100644
--- a/src/com/google/javascript/jscomp/TypedScopeCreator.java
+++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java
@@ -155,6 +155,12 @@ final class TypedScopeCreator implements ScopeCreator, StaticSymbolTable<TypedVa
           "JSC_INCOMPATIBLE_ALIAS_ANNOTATION",
           "Annotation {0} on {1} incompatible with aliased type.");
 
+  static final DiagnosticType DYNAMIC_EXTENDS_WITHOUT_JSDOC =
+      DiagnosticType.warning(
+          "JSC_DYNAMIC_EXTENDS_WITHOUT_JSDOC",
+          "The right-hand side of an extends clause must be a qualified name, or else @extends must"
+              + " be specified in JSDoc");
+
   static final DiagnosticGroup ALL_DIAGNOSTICS = new DiagnosticGroup(
       DELEGATE_PROXY_SUFFIX,
       MALFORMED_TYPEDEF,
@@ -164,7 +170,8 @@ final class TypedScopeCreator implements ScopeCreator, StaticSymbolTable<TypedVa
       CONSTRUCTOR_EXPECTED,
       UNKNOWN_LENDS,
       LENDS_ON_NON_OBJECT,
-      INCOMPATIBLE_ALIAS_ANNOTATION);
+      INCOMPATIBLE_ALIAS_ANNOTATION,
+      DYNAMIC_EXTENDS_WITHOUT_JSDOC);
 
   private final AbstractCompiler compiler;
   private final ErrorReporter typeParsingErrorReporter;
@@ -966,7 +973,7 @@ private FunctionType createClassTypeFromNodes(
 
       // Look at the extends clause and/or JSDoc info to find a super class.  Use generics from the
       // JSDoc to supplement the extends type when available.
-      ObjectType baseType = findSuperClassFromNodes(extendsClause);
+      ObjectType baseType = findSuperClassFromNodes(extendsClause, info);
       builder.inferInheritance(info, baseType);
 
       // Look for an explicit constructor.
@@ -1012,7 +1019,7 @@ private FunctionType createClassTypeFromNodes(
      * be determined.
      */
     @Nullable
-    private ObjectType findSuperClassFromNodes(Node extendsNode) {
+    private ObjectType findSuperClassFromNodes(Node extendsNode, @Nullable JSDocInfo info) {
       if (extendsNode.isEmpty()) {
         // No extends clause: return null.
         return null;
@@ -1025,10 +1032,13 @@ private ObjectType findSuperClassFromNodes(Node extendsNode) {
           if (var != null) {
             ctorType = var.getType();
           }
-        } else if (extendsNode.isCall()) {
-          // TODO(sdh): Do some limited type inference to get the return type for a mixin?
-          // The difficulty is that mixin functions are likely to be generic, so we need to be at
-          // least a little sophisticated here.
+        } else {
+          // Anything TypedScopeCreator can infer has already been read off the AST.  This is likely
+          // a CALL or GETELEM, which are unknown until TypeInference.  Instead, ignore it for now,
+          // require an @extends tag in the JSDoc, and verify correctness in TypeCheck.
+          if (info == null || !info.hasBaseType()) {
+            report(JSError.make(extendsNode, DYNAMIC_EXTENDS_WITHOUT_JSDOC));
+          }
         }
       }
       if (ctorType != null && (ctorType.isConstructor() || ctorType.isInterface())) {
diff --git a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java
index b5554618324..5b2e603dae9 100644
--- a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java
+++ b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java
@@ -2211,13 +2211,16 @@ public void testClassSyntaxRecordWithPropertyMismatch() {
   }
 
   public void testClassJSDocExtendsInconsistentWithExtendsClause() {
-    // TODO(sdh): Should be an error.
     testTypes(
         lines(
             "class Bar {}", //
             "class Baz {}",
             "/** @extends {Bar} */",
-            "class Foo extends Baz {}"));
+            "class Foo extends Baz {}"),
+        lines(
+            "mismatch in declaration of superclass type",
+            "found   : Baz",
+            "required: Bar"));
   }
 
   public void testClassJSDocExtendsWithMissingExtendsClause() {
@@ -2237,11 +2240,16 @@ public void testClassExtendsGetElem() {
             "var obj = {};",
             "class Bar extends obj['abc'] {}",
             "var /** !Foo */ foo = new Bar();"),
-        // TODO(sdh): It would be good to recognize that Bar actually *is* a Foo.
-        lines(
-            "initializing variable",
-            "found   : Bar",
-            "required: Foo"));
+        new String[] {
+          "The right-hand side of an extends clause must be a qualified name, or else @extends must"
+              + " be specified in JSDoc",
+          // TODO(sdh): This is a little confusing, but there doesn't seem to be a way to suppress
+          // this additional error.
+          lines(
+              "initializing variable",
+              "found   : Bar",
+              "required: Foo"),
+        });
   }
 
   public void testClassExtendsFunctionCall() {
@@ -2252,27 +2260,61 @@ public void testClassExtendsFunctionCall() {
             "function mixin() {}",
             "class Bar extends mixin() {}",
             "var /** !Foo */ foo = new Bar();"),
-        // TODO(sdh): It would be good to recognize that Bar actually *is* a Foo.
+        new String[] {
+          "The right-hand side of an extends clause must be a qualified name, or else @extends must"
+              + " be specified in JSDoc",
+          // TODO(sdh): This is a little confusing, but there doesn't seem to be a way to suppress
+          // this additional error.
+          lines(
+              "initializing variable",
+              "found   : Bar",
+              "required: Foo"),
+        });
+  }
+
+  public void testClassInterfaceExtendsFunctionCall() {
+    testTypes(
         lines(
-            "initializing variable",
-            "found   : Bar",
-            "required: Foo"));
+            "/** @interface */",
+            "class Foo {}",
+            "/** @return {function(new:Foo)} */",
+            "function mixin() {}",
+            "/** @interface */",
+            "class Bar extends mixin() {}"),
+        "The right-hand side of an extends clause must be a qualified name, or else @extends must"
+            + " be specified in JSDoc");
   }
 
-  public void testClassExtendsFunctionCallOverridesMethod() {
+  public void testClassExtendsFunctionCallWithJSDoc() {
     testTypes(
         lines(
             "class Foo {",
-            "  /** @return {number} */ foo() {}",
+            "  constructor() { /** @type {number} */ this.foo; }",
             "}",
             "/** @return {function(new:Foo)} */",
             "function mixin() {}",
-            "class Bar extends mixin() {",
-            "  /** @override */",
-            "  foo() {}",
-            "}"),
-        // TODO(sdh): We should respect the type of the function return if possible.
-        "property foo not defined on any superclass of Bar");
+            "/** @extends {Foo} */",
+            "class Bar extends mixin() {}",
+            "var /** null */ x = new Bar().foo;"),
+        lines(
+            "initializing variable", //
+            "found   : number",
+            "required: null"));
+  }
+
+  public void testClassExtendsFunctionCallWithIncompatibleJSDoc() {
+    testTypes(
+        lines(
+            "class Foo {}",
+            "class Baz {}",
+            "/** @return {function(new:Foo)} */",
+            "function mixin() {}",
+            "/** @extends {Baz} */",
+            "class Bar extends mixin() {}"),
+        lines(
+            "mismatch in declaration of superclass type", //
+            "found   : Foo",
+            "required: Baz"));
   }
 
   public void testClassImplementsInterface() {
@@ -2297,7 +2339,7 @@ public void testClassMissingInterfaceMethod() {
         "property foo on interface Foo is not implemented by type Bar");
   }
 
-  public void testClassAbstractClassNeedNonExplicitlyOverrideUnimplementedInterfaceMethods() {
+  public void testClassAbstractClassNeedNotExplicitlyOverrideUnimplementedInterfaceMethods() {
     testTypes(
         lines(
             "/** @interface */",
@@ -2363,6 +2405,7 @@ public void testClassInheritedInterfaceMethod() {
   }
 
   public void testClassMixinAllowsNonOverriddenInterfaceMethods() {
+    // See cl/188076790 and b/74120976
     testTypes(
         lines(
             "/** @interface */",
@@ -2373,7 +2416,7 @@ public void testClassMixinAllowsNonOverriddenInterfaceMethods() {
             // TODO(sdh): Intersection types would allow annotating this correctly.
             "/** @return {function(new:Bar)} */",
             "function mixin() {}",
-            "/** @implements {Foo} */",
+            "/** @extends {Bar} @implements {Foo} */",
             "class Baz extends mixin() {}"),
         // TODO(sdh): This is supposed to be allowed.
         "property foo on interface Foo is not implemented by type Baz");
@@ -2644,7 +2687,7 @@ public void testClassStaticMethodCalledOnInstance() {
             "}",
             "new C().m();"),
         // TODO(sdh): This error message should be different from the converse case.
-        // Probably should say "Instance property m never defined on C".
+        // Probably should say "Property m never defined on instances of C".
         "Property m never defined on C");
   }
 
@@ -2656,7 +2699,8 @@ public void testClassInstanceMethodCalledOnClass() {
             "}",
             "C.m();"),
         // TODO(sdh): This error message should be different from the converse case.
-        // Probably should say "Static property m never defined on C".
+        // Maybe should say "Property m never defined on namespace C". (but we need to think
+        // about union types, etc)
         "Property m never defined on C");
   }