Add compiler error when instantiating abstract class

runtime/vm/parser.cc

Index: runtime/vm/parser.cc
===================================================================
--- runtime/vm/parser.cc	(revision 11538)
+++ runtime/vm/parser.cc	(working copy)
@@ -497,6 +497,7 @@
       : clazz_(cls),
         class_name_(cls_name),
         is_interface_(is_interface),
+        is_abstract_(false),
         token_pos_(token_pos),
         functions_(GrowableObjectArray::Handle(GrowableObjectArray::New())),
         fields_(GrowableObjectArray::Handle(GrowableObjectArray::New())) {
@@ -572,6 +573,14 @@
     return is_interface_;
   }
 
+  void MarkAbstract(bool flag) {

[regis, 2012/08/29 20:58:27]

This name is misleading, since MarkAbstract(false) will not do anything. How
about set_is_abstract() and only call if flag is true?

[hausner, 2012/08/29 21:56:12]

Done.

+    is_abstract_ = is_abstract_ || flag;
+  }
+
+  bool is_abstract() const {
+    return is_abstract_;
+  }
+
   bool has_constructor() const {
     Function& func = Function::Handle();
     for (int i = 0; i < functions_.Length(); i++) {
@@ -622,6 +631,7 @@
   const Class& clazz_;
   const String& class_name_;
   const bool is_interface_;
+  bool is_abstract_;
   intptr_t token_pos_;   // Token index of "class" keyword.
   GrowableObjectArray& functions_;
   GrowableObjectArray& fields_;
@@ -2475,29 +2485,42 @@
                "Constructor with redirection may not have a function body");
     }
     ParseNativeDeclaration();
-  } else if (CurrentToken() == Token::kSEMICOLON) {
-    if (members->is_interface() ||
-        method->has_abstract ||
-        method->has_external ||
-        (method->redirect_name != NULL) ||
-        method->IsConstructor()) {
-      ConsumeToken();
-    } else {
+  } else {
+    // We haven't found a method body. Issue error if one is required.
+    bool must_have_body =

[regis, 2012/08/29 20:58:27]

const bool

[hausner, 2012/08/29 21:56:12]

Done.

+        !members->is_interface() &&
+        method->has_static && !method->has_external;
+    if (must_have_body) {
       ErrorMsg(method->name_pos,
                "function body expected for method '%s'",
                method->name->ToCString());
     }
-  } else {
-    if (members->is_interface() ||
-        method->has_abstract ||
-        method->has_external ||
-        (method->redirect_name != NULL) ||
-        (method->IsConstructor() && method->has_const)) {
-      ExpectSemicolon();
+
+    if (CurrentToken() == Token::kSEMICOLON) {
+      ConsumeToken();
+      if (!members->is_interface() &&
+        !method->has_static &&

[regis, 2012/08/29 20:58:27]

indentation

[hausner, 2012/08/29 21:56:12]

Done.

+        !method->has_external &&
+        !method->IsConstructor()) {
+          // Methods, getters and setters without a body are
+          // implicitly abstract.
+          method->has_abstract = true;
+      }
     } else {
-      ErrorMsg(method->name_pos,
-               "function body expected for method '%s'",
-               method->name->ToCString());
+      // Signature is not followd by semicolon or body. Issue an

[regis, 2012/08/29 20:58:27]

followed

[hausner, 2012/08/29 21:56:12]

Done.

+      // appropriate error.
+      bool must_have_semicolon =

[regis, 2012/08/29 20:58:27]

const bool

[hausner, 2012/08/29 21:56:12]

Done.

+          members->is_interface() ||
+          (method->redirect_name != NULL) ||
+          (method->IsConstructor() && method->has_const) ||
+          method->has_external;
+      if (must_have_semicolon) {
+        ExpectSemicolon();
+      } else {
+        ErrorMsg(method->name_pos,
+                 "function body or semicolon expected for method '%s'",
+                 method->name->ToCString());
+      }
     }
   }
 
@@ -2527,6 +2550,7 @@
   ASSERT(is_top_level_);
   AddFormalParamsToFunction(&method->params, func);
   members->AddFunction(func);
+  members->MarkAbstract(method->has_abstract);

[regis, 2012/08/29 20:58:27]

if (method->has_abstract) members->set_is_abstract();

[hausner, 2012/08/29 21:56:12]

Done.

 }
 
 
@@ -2930,14 +2954,18 @@
 
 void Parser::ParseClassDefinition(const GrowableObjectArray& pending_classes) {
   TRACE_PARSER("ParseClassDefinition");
-  const intptr_t class_pos = TokenPos();
   bool is_patch = false;
+  bool is_abstract = false;
   if (is_patch_source() &&
       (CurrentToken() == Token::kIDENT) &&
       CurrentLiteral()->Equals("patch")) {
     ConsumeToken();
     is_patch = true;
+  } else if (CurrentToken() == Token::kABSTRACT) {
+    is_abstract = true;
+    ConsumeToken();
   }
+  const intptr_t class_pos = TokenPos();
   ExpectToken(Token::kCLASS);
   const intptr_t classname_pos = TokenPos();
   String& class_name = *ExpectClassIdentifier("class name expected");
@@ -3020,11 +3048,16 @@
 
   ExpectToken(Token::kLBRACE);
   ClassDesc members(cls, class_name, false, class_pos);
+  members.MarkAbstract(is_abstract);

[regis, 2012/08/29 20:58:27]

ditto

[hausner, 2012/08/29 21:56:12]

Done.

   while (CurrentToken() != Token::kRBRACE) {
     ParseClassMemberDefinition(&members);
   }
   ExpectToken(Token::kRBRACE);
 
+  if (members.is_abstract()) {
+    cls.set_is_abstract();
+  }
+
   // Add an implicit constructor if no explicit constructor is present. No
   // implicit constructors are needed for patch classes.
   if (!members.has_constructor() && !is_patch) {
@@ -4073,7 +4106,6 @@
       ParseInterfaceDefinition(pending_classes);
     } else if ((CurrentToken() == Token::kABSTRACT) &&
         (LookaheadToken(1) == Token::kCLASS)) {
-      ConsumeToken();  // Consume and ignore 'abstract'.
       ParseClassDefinition(pending_classes);
     } else if (is_patch_source() && IsLiteral("patch") &&
                (LookaheadToken(1) == Token::kCLASS)) {
@@ -8456,8 +8488,8 @@
   if (type.IsDynamicType()) {
     ErrorMsg(type_pos, "Dynamic cannot be instantiated");
   }
-  Class& type_class = Class::Handle(type.type_class());
-  String& type_class_name = String::Handle(type_class.Name());
+  const Class& type_class = Class::Handle(type.type_class());
+  const String& type_class_name = String::Handle(type_class.Name());
   AbstractTypeArguments& type_arguments =
       AbstractTypeArguments::ZoneHandle(type.arguments());
 
@@ -8562,6 +8594,14 @@
              String::Handle(constructor_class.Name()).ToCString(),
              external_constructor_name.ToCString());
   }
+
+  // It is ok to call a factory method of an abstract class, but it is
+  // an error to instantiate an abstract class.
+  if (type_class.is_abstract() && !constructor.IsFactory()) {

[regis, 2012/08/29 20:58:27]

Why don't you check constructor_class instead of type_class?
if (constructor_class.is_abstract()) {
  ErrorMsg(...);
}

[hausner, 2012/08/29 21:56:12]

Ok. In a short while, when we get rid of interfaces, the two will be the same I
think.

+    ErrorMsg(type_pos, "Cannot instantiate abstract class %s",
+             type_class_name.ToCString());
+  }
+
   String& error_message = String::Handle();
   if (!constructor.AreValidArguments(arguments_length,
                                      arguments->names(),