Reject abstract function definitions (carbon-language#4350)

Not sure about error recovery options - can/should we drop the definition as a means of recovery when building the SemIR? I guess probably not, so I guess this change is about right. Phrasing of the error message I'm certainly open to.
josh11b · Oct 3, 2024 · eab5dd6 · eab5dd6
1 parent 8f54736
commit eab5dd6
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 3 deletions.
diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp
@@ -299,6 +299,16 @@ static auto BuildFunctionDecl(Context& context,
   // Write the function ID into the FunctionDecl.
   context.ReplaceInstBeforeConstantUse(decl_id, function_decl);
 
+  // Diagnose 'definition of `abstract` function' using the canonical Function's
+  // modifiers.
+  if (is_definition &&
+      context.functions().Get(function_decl.function_id).virtual_modifier ==
+          SemIR::Function::VirtualModifier::Abstract) {
+    CARBON_DIAGNOSTIC(DefinedAbstractFunction, Error,
+                      "definition of `abstract` function");
+    context.emitter().Emit(TokenOnly(node_id), DefinedAbstractFunction);
+  }
+
   // Check if we need to add this to name lookup, now that the function decl is
   // done.
   if (!name_context.prev_inst_id().is_valid()) {

diff --git a/toolchain/check/testdata/class/fail_modifiers.carbon b/toolchain/check/testdata/class/fail_modifiers.carbon
@@ -61,14 +61,29 @@ protected virtual base class Virtual {}
 // CHECK:STDERR:
 abstract protected class WrongOrder;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+6]]:10: error: `base` not allowed on declaration with `abstract`
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:10: error: `base` not allowed on declaration with `abstract`
 // CHECK:STDERR: abstract base class AbstractAndBase {}
 // CHECK:STDERR:          ^~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:1: note: `abstract` previously appeared here
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: note: `abstract` previously appeared here
 // CHECK:STDERR: abstract base class AbstractAndBase {}
 // CHECK:STDERR: ^~~~~~~~
+// CHECK:STDERR:
 abstract base class AbstractAndBase {}
 
+abstract class AbstractWithDefinition {
+  // CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:19: error: definition of `abstract` function
+  // CHECK:STDERR:   abstract fn F() {}
+  // CHECK:STDERR:                   ^
+  // CHECK:STDERR:
+  abstract fn F() {}
+  abstract fn G();
+}
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:31: error: definition of `abstract` function
+// CHECK:STDERR: fn AbstractWithDefinition.G() {
+// CHECK:STDERR:                               ^
+fn AbstractWithDefinition.G() {
+}
+
 // CHECK:STDOUT: --- fail_modifiers.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
@@ -80,6 +95,12 @@ abstract base class AbstractAndBase {}
 // CHECK:STDOUT:   %Virtual: type = class_type @Virtual [template]
 // CHECK:STDOUT:   %WrongOrder: type = class_type @WrongOrder [template]
 // CHECK:STDOUT:   %AbstractAndBase: type = class_type @AbstractAndBase [template]
+// CHECK:STDOUT:   %AbstractWithDefinition: type = class_type @AbstractWithDefinition [template]
+// CHECK:STDOUT:   %F.type: type = fn_type @F [template]
+// CHECK:STDOUT:   %.3: type = tuple_type () [template]
+// CHECK:STDOUT:   %F: %F.type = struct_value () [template]
+// CHECK:STDOUT:   %G.type: type = fn_type @G [template]
+// CHECK:STDOUT:   %G: %G.type = struct_value () [template]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: imports {
@@ -104,6 +125,7 @@ abstract base class AbstractAndBase {}
 // CHECK:STDOUT:     .Virtual = %Virtual.decl
 // CHECK:STDOUT:     .WrongOrder = %WrongOrder.decl
 // CHECK:STDOUT:     .AbstractAndBase = %AbstractAndBase.decl
+// CHECK:STDOUT:     .AbstractWithDefinition = %AbstractWithDefinition.decl
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %Core.import = import Core
 // CHECK:STDOUT:   %DuplicatePrivate.decl: type = class_decl @DuplicatePrivate [template = constants.%DuplicatePrivate] {} {}
@@ -112,6 +134,8 @@ abstract base class AbstractAndBase {}
 // CHECK:STDOUT:   %Virtual.decl: type = class_decl @Virtual [template = constants.%Virtual] {} {}
 // CHECK:STDOUT:   %WrongOrder.decl: type = class_decl @WrongOrder [template = constants.%WrongOrder] {} {}
 // CHECK:STDOUT:   %AbstractAndBase.decl: type = class_decl @AbstractAndBase [template = constants.%AbstractAndBase] {} {}
+// CHECK:STDOUT:   %AbstractWithDefinition.decl: type = class_decl @AbstractWithDefinition [template = constants.%AbstractWithDefinition] {} {}
+// CHECK:STDOUT:   %G.decl: %G.type = fn_decl @G [template = constants.%G] {} {}
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: class @DuplicatePrivate;
@@ -135,9 +159,30 @@ abstract base class AbstractAndBase {}
 // CHECK:STDOUT: class @WrongOrder;
 // CHECK:STDOUT:
 // CHECK:STDOUT: class @AbstractAndBase {
-// CHECK:STDOUT:   %.loc70: <witness> = complete_type_witness %.1 [template = constants.%.2]
+// CHECK:STDOUT:   %.loc71: <witness> = complete_type_witness %.1 [template = constants.%.2]
 // CHECK:STDOUT:
 // CHECK:STDOUT: !members:
 // CHECK:STDOUT:   .Self = constants.%AbstractAndBase
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
+// CHECK:STDOUT: class @AbstractWithDefinition {
+// CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [template = constants.%F] {} {}
+// CHECK:STDOUT:   %G.decl: %G.type = fn_decl @G [template = constants.%G] {} {}
+// CHECK:STDOUT:   %.loc80: <witness> = complete_type_witness %.1 [template = constants.%.2]
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   .Self = constants.%AbstractWithDefinition
+// CHECK:STDOUT:   .F = %F.decl
+// CHECK:STDOUT:   .G = %G.decl
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: abstract fn @F() {
+// CHECK:STDOUT: !entry:
+// CHECK:STDOUT:   return
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: abstract fn @G() {
+// CHECK:STDOUT: !entry:
+// CHECK:STDOUT:   return
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def
@@ -194,6 +194,7 @@ CARBON_DIAGNOSTIC_KIND(MissingObjectInMethodCall)
 CARBON_DIAGNOSTIC_KIND(SelfParameterNotAllowed)
 
 // Function declaration checking.
+CARBON_DIAGNOSTIC_KIND(DefinedAbstractFunction)
 CARBON_DIAGNOSTIC_KIND(FunctionRedeclReturnTypeDiffers)
 CARBON_DIAGNOSTIC_KIND(FunctionRedeclReturnTypeDiffersNoReturn)
 CARBON_DIAGNOSTIC_KIND(FunctionRedeclReturnTypePrevious)