Basic support for `impl` virtual override keyword (#2493)

Features:
* Add basic support for `impl` virtual methods (override virtual method)
* Error on invalid declaration for `impl` and `virtual`, covering simple use cases
* Add `abstract` fn parser-only support

Changes:
* Modify parser to handle new function specifiers, resolve conflicts
    * Group `virtual_override` and `FN`, and group `impl_kind` and `IMPL` to avoid ambiguities with around `impl` token parsing
* Add new `VirtualOverride` enum for function declarations, and matching `virt_override() -> VirtualOverride` getter
* Update function declaration logic

Depends on #2462

explorer/ast/ast_test_matchers_test.cpp

@@ -103,7 +103,7 @@ TEST(MatchesFunctionDeclarationTest, BasicUsage) {
   Block body(DummyLoc, {});
   FunctionDeclaration decl(DummyLoc, "Foo", {}, std::nullopt, &params,
                            ReturnTerm::Omitted(DummyLoc), &body,
-                           /*is_virtual=*/false);
+                           VirtualOverride::None);
 
   EXPECT_THAT(decl, MatchesFunctionDeclaration());
   EXPECT_THAT(&decl, MatchesFunctionDeclaration());
@@ -117,7 +117,7 @@ TEST(MatchesFunctionDeclarationTest, BasicUsage) {
 
   FunctionDeclaration forward_decl(DummyLoc, "Foo", {}, std::nullopt, &params,
                                    ReturnTerm::Omitted(DummyLoc), std::nullopt,
-                                   /*is_virtual=*/false);
+                                   VirtualOverride::None);
   EXPECT_THAT(forward_decl, MatchesFunctionDeclaration().WithName("Foo"));
   EXPECT_THAT(forward_decl, Not(MatchesFunctionDeclaration().WithBody(_)));
 
@@ -151,7 +151,7 @@ TEST(ASTDeclarationsTest, BasicUsage) {
   Block body(DummyLoc, {});
   FunctionDeclaration decl(DummyLoc, "Foo", {}, std::nullopt, &params,
                            ReturnTerm::Omitted(DummyLoc), &body,
-                           /*is_virtual=*/false);
+                           VirtualOverride::None);
   AST ast = {.declarations = {&decl}};
 
   EXPECT_THAT(ast, ASTDeclarations(ElementsAre(MatchesFunctionDeclaration())));

explorer/ast/declaration.cpp

@@ -342,14 +342,15 @@ auto FunctionDeclaration::Create(Nonnull<Arena*> arena,
                                  Nonnull<TuplePattern*> param_pattern,
                                  ReturnTerm return_term,
                                  std::optional<Nonnull<Block*>> body,
-                                 bool is_virtual)
+                                 VirtualOverride virt_override)
     -> ErrorOr<Nonnull<FunctionDeclaration*>> {
   DeducedParameters split_params;
   CARBON_ASSIGN_OR_RETURN(split_params,
                           SplitDeducedParameters(source_loc, deduced_params));
   return arena->New<FunctionDeclaration>(
       source_loc, name, std::move(split_params.resolved_params),
-      split_params.self_pattern, param_pattern, return_term, body, is_virtual);
+      split_params.self_pattern, param_pattern, return_term, body,
+      virt_override);
 }
 
 void CallableDeclaration::PrintDepth(int depth, llvm::raw_ostream& out) const {

explorer/ast/declaration.h

@@ -125,6 +125,9 @@ class Declaration : public AstNode {
   bool is_type_checked_ = false;
 };
 
+// A function's virtual override keyword.
+enum class VirtualOverride { None, Abstract, Virtual, Impl };
+
 class CallableDeclaration : public Declaration {
  public:
   CallableDeclaration(AstNodeKind kind, SourceLocation loc, std::string name,
@@ -132,7 +135,8 @@ class CallableDeclaration : public Declaration {
                       std::optional<Nonnull<Pattern*>> self_pattern,
                       Nonnull<TuplePattern*> param_pattern,
                       ReturnTerm return_term,
-                      std::optional<Nonnull<Block*>> body, bool is_virtual)
+                      std::optional<Nonnull<Block*>> body,
+                      VirtualOverride virt_override)
       : Declaration(kind, loc),
         name_(std::move(name)),
         deduced_parameters_(std::move(deduced_params)),
@@ -140,7 +144,7 @@ class CallableDeclaration : public Declaration {
         param_pattern_(param_pattern),
         return_term_(return_term),
         body_(body),
-        is_virtual_(is_virtual) {}
+        virt_override_(virt_override) {}
 
   void PrintDepth(int depth, llvm::raw_ostream& out) const;
 
@@ -161,7 +165,7 @@ class CallableDeclaration : public Declaration {
   auto return_term() -> ReturnTerm& { return return_term_; }
   auto body() const -> std::optional<Nonnull<const Block*>> { return body_; }
   auto body() -> std::optional<Nonnull<Block*>> { return body_; }
-  auto is_virtual() const -> bool { return is_virtual_; }
+  auto virt_override() const -> VirtualOverride { return virt_override_; }
 
   auto value_category() const -> ValueCategory { return ValueCategory::Let; }
 
@@ -174,7 +178,7 @@ class CallableDeclaration : public Declaration {
   Nonnull<TuplePattern*> param_pattern_;
   ReturnTerm return_term_;
   std::optional<Nonnull<Block*>> body_;
-  bool is_virtual_;
+  VirtualOverride virt_override_;
 };
 
 class FunctionDeclaration : public CallableDeclaration {
@@ -186,7 +190,8 @@ class FunctionDeclaration : public CallableDeclaration {
                      std::vector<Nonnull<AstNode*>> deduced_params,
                      Nonnull<TuplePattern*> param_pattern,
                      ReturnTerm return_term,
-                     std::optional<Nonnull<Block*>> body, bool is_virtual)
+                     std::optional<Nonnull<Block*>> body,
+                     VirtualOverride virt_override)
       -> ErrorOr<Nonnull<FunctionDeclaration*>>;
 
   // Use `Create()` instead. This is public only so Arena::New() can call it.
@@ -195,11 +200,12 @@ class FunctionDeclaration : public CallableDeclaration {
                       std::optional<Nonnull<Pattern*>> self_pattern,
                       Nonnull<TuplePattern*> param_pattern,
                       ReturnTerm return_term,
-                      std::optional<Nonnull<Block*>> body, bool is_virtual)
+                      std::optional<Nonnull<Block*>> body,
+                      VirtualOverride virt_override)
       : CallableDeclaration(AstNodeKind::FunctionDeclaration, source_loc,
                             std::move(name), std::move(deduced_params),
                             self_pattern, param_pattern, return_term, body,
-                            is_virtual) {}
+                            virt_override) {}
 
   static auto classof(const AstNode* node) -> bool {
     return InheritsFromFunctionDeclaration(node->kind());
@@ -227,8 +233,8 @@ class DestructorDeclaration : public CallableDeclaration {
       : CallableDeclaration(AstNodeKind::DestructorDeclaration, source_loc,
                             "destructor", std::move(deduced_params),
                             self_pattern, param_pattern, return_term, body,
-                            // TODO: Add virtual destructors.
-                            /*is_virtual=*/false) {}
+                            // TODO: Add virtual destructors
+                            VirtualOverride::None) {}
 
   static auto classof(const AstNode* node) -> bool {
     return InheritsFromDestructorDeclaration(node->kind());

explorer/interpreter/type_checker.cpp

@@ -4511,16 +4511,48 @@ auto TypeChecker::DeclareClassDeclaration(Nonnull<ClassDeclaration*> class_decl,
   const int class_level = base_class ? (*base_class)->hierarchy_level() + 1 : 0;
   for (const auto* m : class_decl->members()) {
     const auto* fun = dyn_cast<FunctionDeclaration>(m);
-    if (!fun || !fun->is_virtual()) {
+    if (!fun) {
       continue;
     }
-    // TODO: Implement complete declaration logic from
-    // /docs/design/classes.md#virtual-methods.
-    if (!fun->is_method()) {
+    if (fun->virt_override() != VirtualOverride::None && !fun->is_method()) {
       return ProgramError(fun->source_loc())
              << "Error declaring `" << fun->name() << "`"
              << ": class functions cannot be virtual.";
     }
+    bool has_vtable_entry =
+        class_vtable.find(fun->name()) != class_vtable.end();
+    // TODO: Implement complete declaration logic from
+    // `/docs/design/classes.md#virtual-methods`.
+    switch (fun->virt_override()) {
+      case VirtualOverride::Abstract:
+        // Not supported yet.
+        return ProgramError(fun->source_loc())
+               << "Error declaring `" << fun->name() << "`"
+               << ": `abstract` methods are not yet supported.";
+      case VirtualOverride::None:
+      case VirtualOverride::Virtual:
+        if (has_vtable_entry) {
+          return ProgramError(fun->source_loc())
+                 << "Error declaring `" << fun->name() << "`"
+                 << ": method is declared virtual in base class, use `impl` "
+                    "to override it.";
+        }
+        // TODO: Error if declaring virtual method shadowing non-virtual method.
+        // See https://github.com/carbon-language/carbon-lang/issues/2355.
+        if (fun->virt_override() == VirtualOverride::None) {
+          // Not added to the vtable.
+          continue;
+        }
+        break;
+      case VirtualOverride::Impl:
+        if (!has_vtable_entry) {
+          return ProgramError(fun->source_loc())
+                 << "Error declaring `" << fun->name() << "`"
+                 << ": cannot override a method that is not declared "
+                    "`abstract` or `virtual` in base class.";
+        }
+        break;
+    }
     class_vtable[fun->name()] = {fun, class_level};
   }
 

explorer/interpreter/value.cpp

@@ -161,7 +161,7 @@ static auto GetNamedElement(Nonnull<Arena*> arena, Nonnull<const Value*> v,
         } else if ((*func)->declaration().is_method()) {
           // Found a method. Turn it into a bound method.
           const auto& m = cast<FunctionValue>(**func);
-          if (!m.declaration().is_virtual()) {
+          if (m.declaration().virt_override() == VirtualOverride::None) {
             return arena->New<BoundMethodValue>(&m.declaration(), me_value,
                                                 &class_type.bindings());
           }

explorer/syntax/parser.ypp

@@ -97,14 +97,14 @@
 %token <std::string> sized_type_literal
 %token <std::string> string_literal
 %type <std::string> designator
-%type <ImplKind> impl_kind
+%type <ImplKind> impl_kind_intro
 %type <Nonnull<Expression*>> impl_type
 %type <std::pair<LibraryName, bool>> package_directive
 %type <LibraryName> import_directive
 %type <std::vector<LibraryName>> import_directives
 %type <std::string> optional_library_path
 %type <bool> api_or_impl
-%type <bool> fn_virtual_override_intro
+%type <VirtualOverride> fn_virtual_override_intro
 %type <ClassExtensibility> class_declaration_extensibility
 %type <std::optional<Nonnull<Expression*>>> class_declaration_extends
 %type <Nonnull<Declaration*>> declaration
@@ -1067,9 +1067,13 @@ impl_deduced_params:
 // This includes the FN keyword to work around a shift-reduce conflict between virtual function's `IMPL FN` and interfaces `IMPL`.
 fn_virtual_override_intro:
   FN
-    { $$ = false; }
+    { $$ = VirtualOverride::None; }
+| ABSTRACT FN
+    { $$ = VirtualOverride::Abstract; }
 | VIRTUAL FN
-    { $$ = true; }
+    { $$ = VirtualOverride::Virtual; }
+| IMPL FN
+    { $$ = VirtualOverride::Impl; }
 ;
 function_declaration:
   fn_virtual_override_intro identifier deduced_params maybe_empty_tuple_pattern return_term block
@@ -1210,10 +1214,10 @@ declaration:
       $$ = arena->New<ConstraintDeclaration>(arena, context.source_loc(), $2,
                                              $3, $5);
     }
-| impl_kind IMPL impl_deduced_params impl_type AS type_or_where_expression LEFT_CURLY_BRACE impl_body RIGHT_CURLY_BRACE
+| impl_kind_intro impl_deduced_params impl_type AS type_or_where_expression LEFT_CURLY_BRACE impl_body RIGHT_CURLY_BRACE
     {
       ErrorOr<ImplDeclaration*> impl = ImplDeclaration::Create(
-          arena, context.source_loc(), $1, $4, $6, $3, $8);
+          arena, context.source_loc(), $1, $3, $5, $2, $7);
       if (impl.ok()) {
         $$ = *impl;
       } else {
@@ -1224,10 +1228,10 @@ declaration:
 | alias_declaration
     { $$ = $1; }
 ;
-impl_kind:
-  // Internal
+impl_kind_intro:
+  IMPL // Internal
     { $$ = Carbon::ImplKind::InternalImpl; }
-| EXTERNAL
+| EXTERNAL IMPL
     { $$ = Carbon::ImplKind::ExternalImpl; }
 ;
 impl_type:

explorer/testdata/class/fail_abstract_method_not_supported.carbon

@@ -0,0 +1,24 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// AUTOUPDATE
+// RUN: %{not} %{explorer-run}
+// RUN: %{not} %{explorer-run-trace}
+
+package ExplorerTest api;
+
+base class C {
+}
+
+class D extends C {
+  abstract fn Foo[self:Self]() -> i32 {
+    return 1;
+  // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_abstract_method_not_supported.carbon:[[@LINE+1]]: Error declaring `Foo`: `abstract` methods are not yet supported.
+  }
+}
+
+fn Main() -> i32 {
+  let d: D = {};
+  return 0;
+}

explorer/testdata/class/fail_impl_method_not_existing.carbon

@@ -0,0 +1,24 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// AUTOUPDATE
+// RUN: %{not} %{explorer-run}
+// RUN: %{not} %{explorer-run-trace}
+
+package ExplorerTest api;
+
+base class C {
+}
+
+class D extends C {
+  impl fn Foo[self:Self]() -> i32 {
+    return 1;
+  // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_impl_method_not_existing.carbon:[[@LINE+1]]: Error declaring `Foo`: cannot override a method that is not declared `abstract` or `virtual` in base class.
+  }
+}
+
+fn Main() -> i32 {
+  let d: D = {};
+  return 0;
+}

explorer/testdata/class/fail_impl_method_not_virtual.carbon

@@ -0,0 +1,27 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// AUTOUPDATE
+// RUN: %{not} %{explorer-run}
+// RUN: %{not} %{explorer-run-trace}
+
+package ExplorerTest api;
+
+base class C {
+  fn Foo[self:Self]() -> i32 {
+    return 1;
+  }
+}
+
+class D extends C {
+  impl fn Foo[self:Self]() -> i32 {
+    return 1;
+  // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_impl_method_not_virtual.carbon:[[@LINE+1]]: Error declaring `Foo`: cannot override a method that is not declared `abstract` or `virtual` in base class.
+  }
+}
+
+fn Main() -> i32 {
+  let c: C = {};
+  return 0;
+}

explorer/testdata/class/fail_override_virtual_with_non_virtual.carbon

@@ -0,0 +1,22 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// AUTOUPDATE
+// RUN: %{not} %{explorer-run}
+// RUN: %{not} %{explorer-run-trace}
+
+package ExplorerTest api;
+
+base class A {
+  virtual fn Foo[self: Self]() {}
+}
+
+class B extends A {
+  // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_override_virtual_with_non_virtual.carbon:[[@LINE+1]]: Error declaring `Foo`: method is declared virtual in base class, use `impl` to override it.
+  fn Foo[self: Self]() {}
+}
+
+fn Main() -> i32 {
+  return 0;
+}

explorer/testdata/class/fail_redeclare_virtual_method.carbon

@@ -0,0 +1,23 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// AUTOUPDATE
+// RUN: %{not} %{explorer-run}
+// RUN: %{not} %{explorer-run-trace}
+
+package ExplorerTest api;
+
+base class C {
+  virtual fn Foo[self:Self]() {}
+}
+
+class D extends C {
+  // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_redeclare_virtual_method.carbon:[[@LINE+1]]: Error declaring `Foo`: method is declared virtual in base class, use `impl` to override it.
+  virtual fn Foo[self:Self]() {}
+}
+
+fn Main() -> i32 {
+  let c: C = {};
+  return 0;
+}

explorer/testdata/class/virtual_method.carbon

@@ -25,8 +25,7 @@ base class C {
 }
 
 class D extends C {
-  // TODO: This should instead use `impl` when supported
-  virtual fn Foo[self: Self]() -> i32 {
+  impl fn Foo[self: Self]() -> i32 {
     return 3;
   }
   fn Bar[self: Self]() -> i32 {

explorer/testdata/class/virtual_method_self.carbon

@@ -23,16 +23,14 @@ base class C {
 
 base class D extends C {
   var value_d: i32;
-  // TODO: This should instead use `impl` when supported
-  virtual fn Foo[self: Self]() -> i32 {
+  impl fn Foo[self: Self]() -> i32 {
     return self.value_d;
   }
 }
 
 class E extends D {
   var value_e: i32;
-  // TODO: This should instead use `impl` when supported
-  virtual fn Foo[self: Self]() -> i32 {
+  impl fn Foo[self: Self]() -> i32 {
     return self.value_e;
   }
 }

explorer/testdata/class/virtual_method_shadowed_attr.carbon

@@ -21,8 +21,7 @@ base class C {
 
 base class D extends C {
   var value: i32;
-  // TODO: This should instead use `impl` when supported
-  virtual fn Foo[self: Self]() -> i32 {
+  impl fn Foo[self: Self]() -> i32 {
     return self.value;
   }
 }

explorer/testdata/impl/fail_bad_member_kind.carbon

@@ -14,7 +14,7 @@ interface A {
 }
 
 external impl i32 as A {
-  // CHECK:STDERR: SYNTAX ERROR: {{.*}}/explorer/testdata/impl/fail_bad_member_kind.carbon:[[@LINE+1]]: syntax error, unexpected CLASS, expecting ALIAS or FN or RIGHT_CURLY_BRACE or VIRTUAL
+  // CHECK:STDERR: SYNTAX ERROR: {{.*}}/explorer/testdata/impl/fail_bad_member_kind.carbon:[[@LINE+1]]: syntax error, unexpected CLASS
   class T {}
 }
 

explorer/testdata/mixin/fail_mix_in_impl.carbon

@@ -19,7 +19,7 @@ interface A {
 }
 
 external impl i32 as A {
-  // CHECK:STDERR: SYNTAX ERROR: {{.*}}/explorer/testdata/mixin/fail_mix_in_impl.carbon:[[@LINE+1]]: syntax error, unexpected MIX, expecting ALIAS or FN or RIGHT_CURLY_BRACE or VIRTUAL
+  // CHECK:STDERR: SYNTAX ERROR: {{.*}}/explorer/testdata/mixin/fail_mix_in_impl.carbon:[[@LINE+1]]: syntax error, unexpected MIX
   __mix Operations;
   fn F() {}
 }