Replace impl fn with override fn (#6008)

This proposal renames the syntax used to mark an overriding definition
of a virtual method from `impl fn` to `override fn` to avoid ambiguity:
besides indicating an overriding virtual function, it can be parsed as
an "impl" declaration when the construct following "impl" begins with a
lambda introduced by "fn".

Closes #5711

---------

Co-authored-by: Richard Smith <[email protected]>

Author: elkalt

docs/design/classes.md

@@ -1237,21 +1237,20 @@ There are three virtual modifier keywords:
     virtual" but is called
     ["pure virtual" in C++](https://en.wikipedia.org/wiki/Virtual_function#Abstract_classes_and_pure_virtual_functions).
     Only abstract classes may have unimplemented abstract methods.
--   `impl` - This marks a method that overrides a method marked `virtual` or
+-   `override` - This marks a method that overrides a method marked `virtual` or
     `abstract` in the base class with an implementation specific to -- and
     defined within -- this class. The method is still virtual and may be
     overridden again in subsequent derived classes if this is a base class. See
     [method overriding in Wikipedia](https://en.wikipedia.org/wiki/Method_overriding).
     Requiring a keyword when overriding allows the compiler to diagnose when the
     derived class accidentally uses the wrong signature or spelling and so
-    doesn't match the base class. We intentionally use the same keyword here as
-    for implementing interfaces, to emphasize that they are similar operations.
+    doesn't match the base class.
 
-| Keyword on<br />method in `C` | Allowed in<br />`abstract class C` | Allowed in<br />`base class C` | Allowed in<br />final `class C` | in `B` where<br />`C` extends `B`                        | in `D` where<br />`D` extends `C`                                                |
-| ----------------------------- | ---------------------------------- | ------------------------------ | ------------------------------- | -------------------------------------------------------- | -------------------------------------------------------------------------------- |
-| `virtual`                     | ✅                                 | ✅                             | ❌                              | _not present_                                            | `abstract`<br />`impl`<br />_not mentioned_                                      |
-| `abstract`                    | ✅                                 | ❌                             | ❌                              | _not present_<br />`virtual`<br />`abstract`<br />`impl` | `abstract`<br />`impl`<br />_may not be<br />mentioned if<br />`D` is not final_ |
-| `impl`                        | ✅                                 | ✅                             | ✅                              | `virtual`<br />`abstract`<br />`impl`                    | `abstract`<br />`impl`                                                           |
+| Keyword on<br />method in `C` | Allowed in<br />`abstract class C` | Allowed in<br />`base class C` | Allowed in<br />final `class C` | in `B` where<br />`C` extends `B`                            | in `D` where<br />`D` extends `C`                                                    |
+| ----------------------------- | ---------------------------------- | ------------------------------ | ------------------------------- | ------------------------------------------------------------ | ------------------------------------------------------------------------------------ |
+| `virtual`                     | ✅                                 | ✅                             | ❌                              | _not present_                                                | `abstract`<br />`override`<br />_not mentioned_                                      |
+| `abstract`                    | ✅                                 | ❌                             | ❌                              | _not present_<br />`virtual`<br />`abstract`<br />`override` | `abstract`<br />`override`<br />_may not be<br />mentioned if<br />`D` is not final_ |
+| `override`                    | ✅                                 | ✅                             | ✅                              | `virtual`<br />`abstract`<br />`override`                    | `abstract`<br />`override`                                                           |
 
 Since validating a method with a virtual modifier keyword involves looking for
 methods with the same name in the base class, virtual methods must be declared
@@ -1315,15 +1314,15 @@ base class B1 {
 class D1 {
   extend base: B1;
   // ❌ Illegal:
-  //   impl fn F[self: Self](x: Self) -> Self;
+  //   override fn F[self: Self](x: Self) -> Self;
   // since that would mean the same thing as:
-  //   impl fn F[self: Self](x: D1) -> D1;
+  //   override fn F[self: Self](x: D1) -> D1;
   // and `D1` is a different type than `B1`.
 
   // ✅ Allowed: Parameter and return types
   //  of `F` match declaration in `B1`.
-  impl fn F[self: Self](x: B1) -> B1;
-  // Or: impl fn F[self: D1](x: B1) -> B1;
+  override fn F[self: Self](x: B1) -> B1;
+  // Or: override fn F[self: D1](x: B1) -> B1;
 }
 ```
 
@@ -1341,9 +1340,9 @@ base class B2 {
 class D2 {
   extend base: B2;
   // ✅ Allowed
-  impl fn Clone[self: Self]() -> Self*;
+  override fn Clone[self: Self]() -> Self*;
   // Means the same thing as:
-  //   impl fn Clone[self: D2]() -> D2*;
+  //   override fn Clone[self: D2]() -> D2*;
   // which is allowed since `D2*` is a
   // subtype of `B2*`.
 }
@@ -1615,7 +1614,7 @@ of its base classes unless it has a
 [virtual destructor](https://en.wikipedia.org/wiki/Virtual_function#Virtual_destructors).
 An abstract or base class' destructor may be declared virtual using the
 `virtual` introducer, in which case any derived class destructor declaration
-must be `impl`:
+must be `override`:
 
 ```carbon
 base class MyBaseClass {
@@ -1624,7 +1623,7 @@ base class MyBaseClass {
 
 class MyDerivedClass {
   extend base: MyBaseClass;
-  impl fn destroy[addr self: Self*]() { ... }
+  override fn destroy[addr self: Self*]() { ... }
 }
 ```
 
@@ -2298,6 +2297,8 @@ the type of `U.x`."
     -   [Destructor syntax options](/proposals/p5017.md#destructor-syntax-options)
     -   [Destructor name options](/proposals/p5017.md#destructor-name-options)
 
+-   [#6008: Replace `impl fn` with `override fn`](https://github.com/carbon-language/carbon-lang/pull/6008)
+
 ## References
 
 -   [#257: Initialization of memory and variables](https://github.com/carbon-language/carbon-lang/pull/257)

proposals/p6008.md

@@ -0,0 +1,57 @@
+# Replace `impl fn` with `override fn`
+
+<!--
+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
+-->
+
+[Pull request](https://github.com/carbon-language/carbon-lang/pull/6008)
+
+<!-- toc -->
+
+## Table of contents
+
+-   [Abstract](#abstract)
+-   [Problem](#problem)
+-   [Background](#background)
+-   [Proposal](#proposal)
+-   [Rationale](#rationale)
+
+<!-- tocstop -->
+
+## Abstract
+
+This proposal renames the syntax used to mark an overriding definition of a
+virtual method from `impl fn` to `override fn` to avoid ambiguity.
+
+## Problem
+
+The phrase `impl fn` was introduced to mark overriding virtual functions,
+however, it is now ambiguous: besides indicating an overriding virtual function,
+it can be parsed as an "impl" declaration when the construct following "impl"
+begins with a lambda introduced by "fn".
+
+## Background
+
+The original syntax was adopted in
+[proposal #777](https://github.com/carbon-language/carbon-lang/pull/777) to
+emphasize that implementing interfaces and overriding virtual functions are
+similar operations.
+
+## Proposal
+
+This proposal is to replace the `impl fn` syntax with `override fn` for method
+overriding in class inheritance.
+
+Note that `impl` is still a modifier keyword for library and package
+declarations (`impl library ...` and `impl package ...` respectively). The
+former is unambiguous and the latter is easy to disambiguate, so no change is
+needed for these two cases.
+
+## Rationale
+
+This proposal is focused on the
+[That code is easy to read, understand, and write](/docs/project/goals.md#code-that-is-easy-to-read-understand-and-write)
+Carbon goal. Changing the keyword from `impl` to `override` makes the intent
+clearer, resolves syntax ambiguity, and adheres to existing C++ conventions.

toolchain/check/class.cpp

@@ -199,7 +199,7 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id,
                     .GetAs<SemIR::FunctionDecl>(override_fn_decl_id)
                     .function_id);
             return override_fn.virtual_modifier ==
-                       SemIR::FunctionFields::VirtualModifier::Impl &&
+                       SemIR::FunctionFields::VirtualModifier::Override &&
                    override_fn.name_id == fn.name_id;
           });
       if (i != vtable_contents.end()) {
@@ -237,13 +237,15 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id,
   for (auto inst_id : vtable_contents) {
     auto fn_decl = context.insts().GetAs<SemIR::FunctionDecl>(inst_id);
     auto& fn = context.functions().Get(fn_decl.function_id);
-    if (fn.virtual_modifier != SemIR::FunctionFields::VirtualModifier::Impl) {
+    if (fn.virtual_modifier !=
+        SemIR::FunctionFields::VirtualModifier::Override) {
       fn.virtual_index = vtable.size();
       vtable.push_back(build_specific_function(inst_id));
     } else if (!implemented_impls.Lookup(fn_decl.function_id)) {
-      CARBON_DIAGNOSTIC(ImplWithoutVirtualInBase, Error,
-                        "impl without compatible virtual in base class");
-      context.emitter().Emit(SemIR::LocId(inst_id), ImplWithoutVirtualInBase);
+      CARBON_DIAGNOSTIC(OverrideWithoutVirtualInBase, Error,
+                        "override without compatible virtual in base class");
+      context.emitter().Emit(SemIR::LocId(inst_id),
+                             OverrideWithoutVirtualInBase);
     }
   }
 

toolchain/check/handle_function.cpp

@@ -128,7 +128,8 @@ static auto GetVirtualModifier(const KeywordModifierSet& modifier_set)
             SemIR::Function::VirtualModifier::Virtual)
       .Case(KeywordModifierSet::Abstract,
             SemIR::Function::VirtualModifier::Abstract)
-      .Case(KeywordModifierSet::Impl, SemIR::Function::VirtualModifier::Impl)
+      .Case(KeywordModifierSet::Override,
+            SemIR::Function::VirtualModifier::Override)
       .Default(SemIR::Function::VirtualModifier::None);
 }
 
@@ -342,10 +343,11 @@ static auto RequestVtableIfVirtual(
   }
 
   auto& class_info = context.classes().Get(class_decl->class_id);
-  if (virtual_modifier == SemIR::Function::VirtualModifier::Impl &&
+  if (virtual_modifier == SemIR::Function::VirtualModifier::Override &&
       !class_info.base_id.has_value()) {
-    CARBON_DIAGNOSTIC(ImplWithoutBase, Error, "impl without base class");
-    context.emitter().Emit(node_id, ImplWithoutBase);
+    CARBON_DIAGNOSTIC(OverrideWithoutBase, Error,
+                      "override without base class");
+    context.emitter().Emit(node_id, OverrideWithoutBase);
     virtual_modifier = SemIR::Function::VirtualModifier::None;
     return;
   }

toolchain/check/keyword_modifier_set.h

@@ -38,6 +38,7 @@ enum class ModifierOrder : int8_t { Access, Extern, Extend, Decl, Last = Decl };
   X(Export)                                                                  \
   X(Final)                                                                   \
   X(Impl)                                                                    \
+  X(Override)                                                                \
   X(Returned)                                                                \
   X(Virtual)
 
@@ -113,11 +114,11 @@ CARBON_KEYWORD_MODIFIER_SET(CARBON_KEYWORD_MODIFIER_SET_WITH_TYPE)
 
 constexpr KeywordModifierSet KeywordModifierSet::Access(Private | Protected);
 constexpr KeywordModifierSet KeywordModifierSet::Class(Abstract | Base);
-constexpr KeywordModifierSet KeywordModifierSet::Method(Abstract | Impl |
+constexpr KeywordModifierSet KeywordModifierSet::Method(Abstract | Override |
                                                         Virtual);
 constexpr KeywordModifierSet KeywordModifierSet::ImplDecl(Extend | Final);
 constexpr KeywordModifierSet KeywordModifierSet::Interface(Default | Final);
-constexpr KeywordModifierSet KeywordModifierSet::Decl(Class | Method |
+constexpr KeywordModifierSet KeywordModifierSet::Decl(Class | Method | Impl |
                                                       Interface | Export |
                                                       Returned);