Diagnose impl method without matching virtual function in base class (#5817)

Also update `RequestVtableIfVirtual` to strip `impl` from functions in
classes without a base class - this avoids a duplicate diagnostic where
they're diagnosed as being in a class without a base class, then
diagnosed again because they don't have a matching function in a base
class.

toolchain/check/class.cpp

@@ -169,6 +169,7 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id,
   };
 
   llvm::SmallVector<SemIR::InstId> vtable;
+  Set<SemIR::FunctionId> implemented_impls;
   if (base_vtable_id.has_value()) {
     auto base_vtable_inst_block = context.inst_blocks().Get(
         context.vtables().Get(base_vtable_id).virtual_functions_id);
@@ -190,8 +191,10 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id,
                    override_fn.name_id == fn.name_id;
           });
       if (i != vtable_contents.end()) {
-        auto& override_fn = context.functions().Get(
-            context.insts().GetAs<SemIR::FunctionDecl>(*i).function_id);
+        auto override_fn_id =
+            context.insts().GetAs<SemIR::FunctionDecl>(*i).function_id;
+        implemented_impls.Insert(override_fn_id);
+        auto& override_fn = context.functions().Get(override_fn_id);
         CheckFunctionTypeMatches(context, override_fn, fn, specific_id,
                                  /*check_syntax=*/false,
                                  /*check_self=*/false);
@@ -209,6 +212,10 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id,
     if (fn.virtual_modifier != SemIR::FunctionFields::VirtualModifier::Impl) {
       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);
     }
   }
 

toolchain/check/handle_function.cpp

@@ -327,7 +327,7 @@ static auto IsGenericFunction(Context& context,
 // Requests a vtable be created when processing a virtual function.
 static auto RequestVtableIfVirtual(
     Context& context, Parse::AnyFunctionDeclId node_id,
-    SemIR::Function::VirtualModifier virtual_modifier,
+    SemIR::Function::VirtualModifier& virtual_modifier,
     const std::optional<SemIR::Inst>& parent_scope_inst, SemIR::InstId decl_id,
     SemIR::GenericId generic_id) -> void {
   // In order to request a vtable, the function must be virtual, and in a class
@@ -346,11 +346,14 @@ static auto RequestVtableIfVirtual(
       !class_info.base_id.has_value()) {
     CARBON_DIAGNOSTIC(ImplWithoutBase, Error, "impl without base class");
     context.emitter().Emit(node_id, ImplWithoutBase);
+    virtual_modifier = SemIR::Function::VirtualModifier::None;
+    return;
   }
 
   if (IsGenericFunction(context, generic_id, class_info.generic_id)) {
     CARBON_DIAGNOSTIC(GenericVirtual, Error, "generic virtual function");
     context.emitter().Emit(node_id, GenericVirtual);
+    virtual_modifier = SemIR::Function::VirtualModifier::None;
     return;
   }
 
@@ -528,8 +531,8 @@ static auto BuildFunctionDecl(Context& context,
     // TODO: Validate that the redeclaration doesn't set an access modifier.
   }
 
-  RequestVtableIfVirtual(context, node_id, virtual_modifier, parent_scope_inst,
-                         decl_id, function_info.generic_id);
+  RequestVtableIfVirtual(context, node_id, function_info.virtual_modifier,
+                         parent_scope_inst, decl_id, function_info.generic_id);
 
   // Write the function ID into the FunctionDecl.
   ReplaceInstBeforeConstantUse(context, decl_id, function_decl);

toolchain/check/testdata/class/virtual_modifiers.carbon

@@ -130,7 +130,7 @@ fn F() {
   b1.m2 = 4;
 }
 
-// --- todo_fail_impl_without_base_declaration.carbon
+// --- fail_impl_without_base_declaration.carbon
 
 library "[[@TEST_NAME]]";
 
@@ -139,6 +139,10 @@ base class Base {
 
 class Derived {
   extend base: Base;
+  // CHECK:STDERR: fail_impl_without_base_declaration.carbon:[[@LINE+4]]:3: error: impl without compatible virtual in base class [ImplWithoutVirtualInBase]
+  // CHECK:STDERR:   impl fn F[self: Self]();
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
   impl fn F[self: Self]();
 }
 
@@ -1252,10 +1256,8 @@ var v: Base(T1) = {};
 // CHECK:STDOUT:   %pattern_type: type = pattern_type %C [concrete]
 // CHECK:STDOUT:   %F.type: type = fn_type @F [concrete]
 // CHECK:STDOUT:   %F: %F.type = struct_value () [concrete]
-// CHECK:STDOUT:   %ptr: type = ptr_type <vtable> [concrete]
-// CHECK:STDOUT:   %C.vtable_ptr: ref %ptr = vtable_ptr @C.vtable [concrete]
-// CHECK:STDOUT:   %struct_type.vptr: type = struct_type {.<vptr>: %ptr} [concrete]
-// CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %struct_type.vptr [concrete]
+// CHECK:STDOUT:   %empty_struct_type: type = struct_type {} [concrete]
+// CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %empty_struct_type [concrete]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: imports {
@@ -1283,19 +1285,15 @@ var v: Base(T1) = {};
 // CHECK:STDOUT:     %Self.ref: type = name_ref Self, constants.%C [concrete = constants.%C]
 // CHECK:STDOUT:     %self: %C = bind_name self, %self.param
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %vtable_ptr: ref %ptr = vtable_ptr @C.vtable [concrete = constants.%C.vtable_ptr]
-// CHECK:STDOUT:   %struct_type.vptr: type = struct_type {.<vptr>: %ptr} [concrete = constants.%struct_type.vptr]
-// CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %struct_type.vptr [concrete = constants.%complete_type]
+// CHECK:STDOUT:   %empty_struct_type: type = struct_type {} [concrete = constants.%empty_struct_type]
+// CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %empty_struct_type [concrete = constants.%complete_type]
 // CHECK:STDOUT:   complete_type_witness = %complete_type
-// CHECK:STDOUT:   vtable_ptr = %vtable_ptr
 // CHECK:STDOUT:
 // CHECK:STDOUT: !members:
 // CHECK:STDOUT:   .Self = constants.%C
 // CHECK:STDOUT:   .F = %F.decl
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: vtable @C.vtable {}
-// CHECK:STDOUT:
 // CHECK:STDOUT: impl fn @F(%self.param: %C);
 // CHECK:STDOUT:
 // CHECK:STDOUT: --- init_members.carbon
@@ -1518,7 +1516,7 @@ var v: Base(T1) = {};
 // CHECK:STDOUT:   return
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: --- todo_fail_impl_without_base_declaration.carbon
+// CHECK:STDOUT: --- fail_impl_without_base_declaration.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT:   %Base: type = class_type @Base [concrete]

toolchain/diagnostics/diagnostic_kind.def

@@ -269,6 +269,7 @@ CARBON_DIAGNOSTIC_KIND(ClassSpecificDeclPrevious)
 CARBON_DIAGNOSTIC_KIND(ClassIncompleteWithinDefinition)
 CARBON_DIAGNOSTIC_KIND(GenericVirtual)
 CARBON_DIAGNOSTIC_KIND(ImplWithoutBase)
+CARBON_DIAGNOSTIC_KIND(ImplWithoutVirtualInBase)
 CARBON_DIAGNOSTIC_KIND(VirtualWithoutSelf)
 
 // Deduction.