Allow @abstract on classes and warn when creating an instance of an a…

…bstract class. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=126432647
google · Jul 7, 2016 · 2c5d023 · 2c5d023
1 parent 7cc6f9a
commit 2c5d023
Show file tree

Hide file tree

Showing 7 changed files with 53 additions and 30 deletions.
diff --git a/src/com/google/javascript/jscomp/CheckJSDoc.java b/src/com/google/javascript/jscomp/CheckJSDoc.java
@@ -169,6 +169,10 @@ private void validateAbstractJsDoc(Node n, JSDocInfo info) {
     if (info == null || !info.isAbstract()) {
       return;
     }
+    if (n.isClass()) {
+      return;
+    }
+
     Node functionNode = getFunctionDecl(n);
 
     if (functionNode == null) {
@@ -190,7 +194,9 @@ private void validateAbstractJsDoc(Node n, JSDocInfo info) {
       return;
     }
 
-    if (!n.isMemberFunctionDef() && !NodeUtil.isPrototypeMethod(functionNode)) {
+    if (!info.isConstructor()
+        && !n.isMemberFunctionDef()
+        && !NodeUtil.isPrototypeMethod(functionNode)) {
       // @abstract annotation on a non-method (or static method) in ES5
       report(n, MISPLACED_ANNOTATION, "@abstract", "only functions or methods can be abstract");
       return;

diff --git a/src/com/google/javascript/jscomp/TypeCheck.java b/src/com/google/javascript/jscomp/TypeCheck.java
@@ -116,6 +116,11 @@ public final class TypeCheck implements NodeTraversal.Callback, CompilerPass {
           "JSC_NOT_A_CONSTRUCTOR",
           "cannot instantiate non-constructor");
 
+  static final DiagnosticType INSTANTIATE_ABSTRACT_CLASS =
+      DiagnosticType.warning(
+          "JSC_INSTANTIATE_ABSTRACT_CLASS",
+          "cannot instantiate abstract class");
+
   static final DiagnosticType BIT_OPERATION =
       DiagnosticType.warning(
           "JSC_BAD_TYPE_FOR_BIT_OPERATION",
@@ -265,6 +270,7 @@ public final class TypeCheck implements NodeTraversal.Callback, CompilerPass {
       POSSIBLE_INEXISTENT_PROPERTY,
       INEXISTENT_PROPERTY_WITH_SUGGESTION,
       NOT_A_CONSTRUCTOR,
+      INSTANTIATE_ABSTRACT_CLASS,
       BIT_OPERATION,
       NOT_CALLABLE,
       CONSTRUCTOR_NOT_CALLABLE,
@@ -1562,18 +1568,28 @@ private void visitVar(NodeTraversal t, Node n) {
   private void visitNew(NodeTraversal t, Node n) {
     Node constructor = n.getFirstChild();
     JSType type = getJSType(constructor).restrictByNotNullOrUndefined();
-    if (type.isConstructor() || type.isEmptyType() || type.isUnknownType()) {
-      FunctionType fnType = type.toMaybeFunctionType();
-      if (fnType != null && fnType.hasInstanceType()) {
-        visitParameterList(t, n, fnType);
-        ensureTyped(t, n, fnType.getInstanceType());
-      } else {
-        ensureTyped(t, n);
-      }
-    } else {
+    if (!couldBeAConstructor(type)) {
       report(t, n, NOT_A_CONSTRUCTOR);
       ensureTyped(t, n);
+      return;
     }
+
+    Var var = t.getScope().getVar(constructor.getQualifiedName());
+    if (var != null && var.getJSDocInfo() != null && var.getJSDocInfo().isAbstract()) {
+      report(t, n, INSTANTIATE_ABSTRACT_CLASS);
+    }
+
+    FunctionType fnType = type.toMaybeFunctionType();
+    if (fnType != null && fnType.hasInstanceType()) {
+      visitParameterList(t, n, fnType);
+      ensureTyped(t, n, fnType.getInstanceType());
+    } else {
+      ensureTyped(t, n);
+    }
+  }
+
+  private boolean couldBeAConstructor(JSType type) {
+    return type.isConstructor() || type.isEmptyType() || type.isUnknownType();
   }
 
   /**

diff --git a/src/com/google/javascript/rhino/JSDocInfoBuilder.java b/src/com/google/javascript/rhino/JSDocInfoBuilder.java
@@ -866,8 +866,7 @@ public boolean recordNoCollapse() {
    */
   public boolean recordConstructor() {
     if (!hasAnySingletonTypeTags()
-        && !currentInfo.isConstructorOrInterface()
-        && !currentInfo.isAbstract()) {
+        && !currentInfo.isConstructorOrInterface()) {
       currentInfo.setConstructor(true);
       populated = true;
       return true;
@@ -939,7 +938,6 @@ public boolean isUnrestrictedRecorded() {
   public boolean recordAbstract() {
     if (!hasAnySingletonTypeTags()
         && !currentInfo.isInterface()
-        && !currentInfo.isConstructor()
         && !currentInfo.isAbstract()) {
       currentInfo.setAbstract();
       populated = true;

diff --git a/test/com/google/javascript/jscomp/CheckJsDocTest.java b/test/com/google/javascript/jscomp/CheckJsDocTest.java
@@ -117,17 +117,15 @@ public void testAbstract_staticMethod() {
   }
 
   public void testAbstract_class() {
-    testWarningEs6(
-        "/** @abstract */class Foo { constructor() {}}",
-        MISPLACED_ANNOTATION);
-    // Adding @abstract to an ES5 class/constructor is a parse error.
+    testSameEs6("/** @abstract */class Foo { constructor() {}}");
+    testSame("/** @abstract @constructor */ var Foo = function() {};");
   }
 
   public void testAbstract_constructor() {
     testWarningEs6(
         "class Foo { /** @abstract */ constructor() {}}",
         MISPLACED_ANNOTATION);
-    // Adding @abstract to an ES5 class/constructor is a parse error.
+    // ES5 constructors are treated as class definitions and tested above.
   }
 
   public void testAbstract_field() {

diff --git a/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java b/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java
@@ -19,6 +19,7 @@
 import static com.google.javascript.jscomp.Es6ToEs3Converter.CANNOT_CONVERT;
 import static com.google.javascript.jscomp.Es6ToEs3Converter.CANNOT_CONVERT_YET;
 import static com.google.javascript.jscomp.Es6ToEs3Converter.CONFLICTING_GETTER_SETTER_TYPE;
+import static com.google.javascript.jscomp.TypeCheck.INSTANTIATE_ABSTRACT_CLASS;
 
 import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
 
@@ -502,6 +503,15 @@ public void testClassExpression() {
             "(condition ? obj1 : obj2).prop = testcode$classdecl$var0;"));
   }
 
+  public void testAbstractClass() {
+    enableTypeCheck();
+    test(
+        "/** @abstract */ class Foo {} var x = new Foo();",
+        "/** @abstract @constructor @struct */ var Foo = function() {}; var x = new Foo();",
+        null,
+        INSTANTIATE_ABSTRACT_CLASS);
+  }
+
   /**
    * We don't bother transpiling this case because the transpiled code will be very difficult to
    * typecheck.

diff --git a/test/com/google/javascript/jscomp/TypeCheckTest.java b/test/com/google/javascript/jscomp/TypeCheckTest.java
@@ -17,6 +17,7 @@
 package com.google.javascript.jscomp;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.javascript.jscomp.TypeCheck.INSTANTIATE_ABSTRACT_CLASS;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
@@ -7884,6 +7885,12 @@ public void testNew18() {
               "/** @constructor */ goog.G = goog.F;");
   }
 
+  public void testNew19() {
+    testTypes(
+        "/** @constructor @abstract */ var Foo = function() {}; var foo = new Foo();",
+        INSTANTIATE_ABSTRACT_CLASS);
+  }
+
   public void testName1() {
     assertTypeEquals(VOID_TYPE, testNameNode("undefined"));
   }

diff --git a/test/com/google/javascript/jscomp/parsing/JsDocInfoParserTest.java b/test/com/google/javascript/jscomp/parsing/JsDocInfoParserTest.java
@@ -1349,12 +1349,6 @@ public void testParseAbstract_typeBeforeAbstract() throws Exception {
         "Bad type annotation. type annotation incompatible with other annotations");
   }
 
-  public void testParseAbstract_constructorBeforeAbstract() throws Exception {
-    // TODO(michaelthomas): Remove when @abstract is supported on constructors.
-    parse("@constructor \n * @abstract */",
-        "Bad type annotation. type annotation incompatible with other annotations");
-  }
-
   public void testParseAbstract_interfaceBeforeAbstract() throws Exception {
     parse("* @interface \n * @abstract */",
         "Bad type annotation. type annotation incompatible with other annotations");
@@ -1375,12 +1369,6 @@ public void testParseAbstract_abstractBeforeInterface() throws Exception {
         "Bad type annotation. type annotation incompatible with other annotations");
   }
 
-  public void testParseAbstract_abstractBeforeConstructor() throws Exception {
-    // TODO(michaelthomas): Remove when @abstract is supported on constructors.
-    parse("* @abstract \n * @constructor */",
-        "Bad type annotation. type annotation incompatible with other annotations");
-  }
-
   public void testStackedAnnotation() throws Exception {
     JSDocInfo info = parse("@const @type {string}*/");
     assertThat(info.isConstant()).isTrue();