Refactor modifier formatting to remove string passing. (#4418)

I'm taking the approach of making DiagnosticBase an API so that we can pass similar diagnostics as parameters. An alternative would be to do the function_ref approach we've done elsewhere, but these felt more boilerplate to me. Note I'm also modifying messages here. Let me know if you'd like different changes and/or just keeping current formatting (keeping current formatting would also allow removing some of the templating I've added, but it felt helpful putting explicit tokens where possible). But also, things like "`protected` not allowed on `interface` declaration at file scope" were part of the phrasing issue, I think. --------- Co-authored-by: Richard Smith <[email protected]>
carbon-language · Oct 17, 2024 · b5a837a · b5a837a
1 parent 5a795db
commit b5a837a
Show file tree

Hide file tree

Showing 18 changed files with 156 additions and 107 deletions.
diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp
@@ -861,7 +861,7 @@ static auto ProcessNodeIds(Context& context, llvm::raw_ostream* vlog_stream,
   // On crash, report which token we were handling.
   PrettyStackTraceFunction node_dumper([&](llvm::raw_ostream& output) {
     auto loc = converter.ConvertLoc(
-        node_id, [](DiagnosticLoc, const Internal::DiagnosticBase<>&) {});
+        node_id, [](DiagnosticLoc, const DiagnosticBase<>&) {});
     loc.FormatLocation(output);
     output << ": checking " << context.parse_tree().node_kind(node_id) << "\n";
     // Crash output has a tab indent; try to indent slightly past that.

diff --git a/toolchain/check/modifiers.cpp b/toolchain/check/modifiers.cpp
@@ -8,16 +8,37 @@
 
 namespace Carbon::Check {
 
-static auto DiagnoseNotAllowed(Context& context, Parse::NodeId modifier_node,
-                               Lex::TokenKind decl_kind,
-                               llvm::StringRef context_string,
-                               SemIR::LocId context_loc_id) -> void {
-  CARBON_DIAGNOSTIC(ModifierNotAllowedOn, Error,
-                    "`{0}` not allowed on `{1}` declaration{2}", Lex::TokenKind,
-                    Lex::TokenKind, std::string);
-  auto diag = context.emitter().Build(modifier_node, ModifierNotAllowedOn,
-                                      context.token_kind(modifier_node),
-                                      decl_kind, context_string.str());
+// Builds the diagnostic for DiagnoseNotAllowed.
+template <typename... TokenKinds>
+static auto StartDiagnoseNotAllowed(
+    Context& context, const DiagnosticBase<TokenKinds...>& diagnostic_base,
+    Parse::NodeId modifier_node, Lex::TokenKind declaration_kind)
+    -> DiagnosticEmitter<SemIRLoc>::DiagnosticBuilder {
+  if constexpr (sizeof...(TokenKinds) == 0) {
+    return context.emitter().Build(modifier_node, diagnostic_base);
+  } else if constexpr (sizeof...(TokenKinds) == 1) {
+    return context.emitter().Build(modifier_node, diagnostic_base,
+                                   context.token_kind(modifier_node));
+  } else {
+    static_assert(sizeof...(TokenKinds) == 2);
+    return context.emitter().Build(modifier_node, diagnostic_base,
+                                   context.token_kind(modifier_node),
+                                   declaration_kind);
+  }
+}
+
+// Diagnoses that a modifier wasn't allowed. Handles adding context when
+// possible.
+//
+// The diagnostic can take up to two TokenKinds: the modifier kind, and the
+// declaration kind.
+template <typename... TokenKinds>
+static auto DiagnoseNotAllowed(
+    Context& context, const DiagnosticBase<TokenKinds...>& diagnostic_base,
+    Parse::NodeId modifier_node, Lex::TokenKind decl_kind,
+    SemIR::LocId context_loc_id) -> void {
+  auto diag = StartDiagnoseNotAllowed(context, diagnostic_base, modifier_node,
+                                      decl_kind);
   if (context_loc_id.is_valid()) {
     CARBON_DIAGNOSTIC(ModifierNotInContext, Note, "containing definition here");
     diag.Note(context_loc_id, ModifierNotInContext);
@@ -37,10 +58,16 @@ static auto ModifierOrderAsSet(ModifierOrder order) -> KeywordModifierSet {
   }
 }
 
-auto ForbidModifiersOnDecl(Context& context, DeclIntroducerState& introducer,
-                           KeywordModifierSet forbidden,
-                           llvm::StringRef context_string,
-                           SemIR::LocId context_loc_id) -> void {
+// Like `LimitModifiersOnDecl`, except says which modifiers are forbidden, and a
+// `context_string` (and optional `context_loc_id`) specifying the context in
+// which those modifiers are forbidden.
+//
+// See DiagnoseNotAllowed for details regarding diagnostic_base.
+template <typename DiagnosticBaseT>
+static auto ForbidModifiersOnDecl(
+    Context& context, const DiagnosticBaseT& diagnostic_base,
+    DeclIntroducerState& introducer, KeywordModifierSet forbidden,
+    SemIR::LocId context_loc_id = SemIR::LocId::Invalid) -> void {
   auto not_allowed = introducer.modifier_set & forbidden;
   if (not_allowed.empty()) {
     return;
@@ -50,28 +77,50 @@ auto ForbidModifiersOnDecl(Context& context, DeclIntroducerState& introducer,
        order_index <= static_cast<int8_t>(ModifierOrder::Last); ++order_index) {
     auto order = static_cast<ModifierOrder>(order_index);
     if (not_allowed.HasAnyOf(ModifierOrderAsSet(order))) {
-      DiagnoseNotAllowed(context, introducer.modifier_node_id(order),
-                         introducer.kind, context_string, context_loc_id);
+      DiagnoseNotAllowed(context, diagnostic_base,
+                         introducer.modifier_node_id(order), introducer.kind,
+                         context_loc_id);
       introducer.set_modifier_node_id(order, Parse::NodeId::Invalid);
     }
   }
 
   introducer.modifier_set.Remove(forbidden);
 }
 
+auto LimitModifiersOnDecl(Context& context, DeclIntroducerState& introducer,
+                          KeywordModifierSet allowed) -> void {
+  CARBON_DIAGNOSTIC(ModifierNotAllowedOnDeclaration, Error,
+                    "`{0}` not allowed on `{1}` declaration", Lex::TokenKind,
+                    Lex::TokenKind);
+  ForbidModifiersOnDecl(context, ModifierNotAllowedOnDeclaration, introducer,
+                        ~allowed);
+}
+
+auto LimitModifiersOnNotDefinition(Context& context,
+                                   DeclIntroducerState& introducer,
+                                   KeywordModifierSet allowed) -> void {
+  CARBON_DIAGNOSTIC(
+      ModifierOnlyAllowedOnDefinition, Error,
+      "`{0}` not allowed on `{1}` forward declaration, only definition",
+      Lex::TokenKind, Lex::TokenKind);
+  ForbidModifiersOnDecl(context, ModifierOnlyAllowedOnDefinition, introducer,
+                        ~allowed);
+}
+
 auto CheckAccessModifiersOnDecl(Context& context,
                                 DeclIntroducerState& introducer,
                                 std::optional<SemIR::Inst> parent_scope_inst)
     -> void {
+  CARBON_DIAGNOSTIC(ModifierProtectedNotAllowed, Error,
+                    "`protected` not allowed; requires class scope");
   if (parent_scope_inst) {
     if (parent_scope_inst->Is<SemIR::Namespace>()) {
       // TODO: This assumes that namespaces can only be declared at file scope.
       // If we add support for non-file-scope namespaces, we will need to check
       // the parents of the target scope to determine whether we're at file
       // scope.
-      ForbidModifiersOnDecl(
-          context, introducer, KeywordModifierSet::Protected,
-          " at file scope, `protected` is only allowed on class members");
+      ForbidModifiersOnDecl(context, ModifierProtectedNotAllowed, introducer,
+                            KeywordModifierSet::Protected);
       return;
     }
 
@@ -82,11 +131,13 @@ auto CheckAccessModifiersOnDecl(Context& context,
   }
 
   // Otherwise neither `private` nor `protected` allowed.
-  ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Protected,
-                        ", `protected` is only allowed on class members");
-  ForbidModifiersOnDecl(
-      context, introducer, KeywordModifierSet::Private,
-      ", `private` is only allowed on class members and at file scope");
+  ForbidModifiersOnDecl(context, ModifierProtectedNotAllowed, introducer,
+                        KeywordModifierSet::Protected);
+
+  CARBON_DIAGNOSTIC(ModifierPrivateNotAllowed, Error,
+                    "`private` not allowed; requires class or file scope");
+  ForbidModifiersOnDecl(context, ModifierPrivateNotAllowed, introducer,
+                        KeywordModifierSet::Private);
 }
 
 auto CheckMethodModifiersOnFunction(
@@ -98,21 +149,29 @@ auto CheckMethodModifiersOnFunction(
       auto inheritance_kind =
           context.classes().Get(class_decl->class_id).inheritance_kind;
       if (inheritance_kind == SemIR::Class::Final) {
-        ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Virtual,
-                              " in a non-abstract non-base `class` definition",
+        CARBON_DIAGNOSTIC(
+            ModifierVirtualNotAllowed, Error,
+            "`virtual` not allowed; requires `abstract` or `base` class scope");
+        ForbidModifiersOnDecl(context, ModifierVirtualNotAllowed, introducer,
+                              KeywordModifierSet::Virtual,
                               context.insts().GetLocId(parent_scope_inst_id));
       }
       if (inheritance_kind != SemIR::Class::Abstract) {
-        ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Abstract,
-                              " in a non-abstract `class` definition",
+        CARBON_DIAGNOSTIC(
+            ModifierAbstractNotAllowed, Error,
+            "`abstract` not allowed; requires `abstract` class scope");
+        ForbidModifiersOnDecl(context, ModifierAbstractNotAllowed, introducer,
+                              KeywordModifierSet::Abstract,
                               context.insts().GetLocId(parent_scope_inst_id));
       }
       return;
     }
   }
 
-  ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Method,
-                        " outside of a class");
+  CARBON_DIAGNOSTIC(ModifierRequiresClass, Error,
+                    "`{0}` not allowed; requires class scope", Lex::TokenKind);
+  ForbidModifiersOnDecl(context, ModifierRequiresClass, introducer,
+                        KeywordModifierSet::Method);
 }
 
 auto RestrictExternModifierOnDecl(Context& context,
@@ -124,8 +183,11 @@ auto RestrictExternModifierOnDecl(Context& context,
   }
 
   if (parent_scope_inst && !parent_scope_inst->Is<SemIR::Namespace>()) {
-    ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Extern,
-                          " that is a member");
+    CARBON_DIAGNOSTIC(ModifierExternNotAllowed, Error,
+                      "`{0}` not allowed; requires file or namespace scope",
+                      Lex::TokenKind);
+    ForbidModifiersOnDecl(context, ModifierExternNotAllowed, introducer,
+                          KeywordModifierSet::Extern);
     // Treat as unset.
     introducer.extern_library = SemIR::LibraryNameId::Invalid;
     return;
@@ -158,8 +220,11 @@ auto RequireDefaultFinalOnlyInInterfaces(
     // Both `default` and `final` allowed in an interface definition.
     return;
   }
-  ForbidModifiersOnDecl(context, introducer, KeywordModifierSet::Interface,
-                        " outside of an interface");
+  CARBON_DIAGNOSTIC(ModifierRequiresInterface, Error,
+                    "`{0}` not allowed; requires interface scope",
+                    Lex::TokenKind);
+  ForbidModifiersOnDecl(context, ModifierRequiresInterface, introducer,
+                        KeywordModifierSet::Interface);
 }
 
 }  // namespace Carbon::Check
diff --git a/toolchain/check/modifiers.h b/toolchain/check/modifiers.h
@@ -28,29 +28,14 @@ auto CheckMethodModifiersOnFunction(
     SemIR::InstId parent_scope_inst_id,
     std::optional<SemIR::Inst> parent_scope_inst) -> void;
 
-// Like `LimitModifiersOnDecl`, except says which modifiers are forbidden, and a
-// `context_string` (and optional `context_loc_id`) specifying the context in
-// which those modifiers are forbidden.
-// TODO: Take another look at diagnostic phrasing for callers.
-auto ForbidModifiersOnDecl(Context& context, DeclIntroducerState& introducer,
-                           KeywordModifierSet forbidden,
-                           llvm::StringRef context_string,
-                           SemIR::LocId context_loc_id = SemIR::LocId::Invalid)
-    -> void;
-
 // Reports a diagnostic (using `decl_kind`) if modifiers on this declaration are
 // not in `allowed`. Updates `introducer`.
-inline auto LimitModifiersOnDecl(Context& context,
-                                 DeclIntroducerState& introducer,
-                                 KeywordModifierSet allowed) -> void {
-  ForbidModifiersOnDecl(context, introducer, ~allowed, "");
-}
+auto LimitModifiersOnDecl(Context& context, DeclIntroducerState& introducer,
+                          KeywordModifierSet allowed) -> void;
 
-inline auto LimitModifiersOnNotDefinition(Context& context,
-                                          DeclIntroducerState& introducer,
-                                          KeywordModifierSet allowed) -> void {
-  ForbidModifiersOnDecl(context, introducer, ~allowed, ", only definition");
-}
+auto LimitModifiersOnNotDefinition(Context& context,
+                                   DeclIntroducerState& introducer,
+                                   KeywordModifierSet allowed) -> void;
 
 // Restricts the `extern` modifier to only be used on namespace-scoped
 // declarations. Diagnoses and cleans up:

diff --git a/toolchain/check/testdata/class/fail_base_bad_type.carbon b/toolchain/check/testdata/class/fail_base_bad_type.carbon
@@ -118,7 +118,7 @@ fn AccessMemberWithInvalidBaseStruct(p: DeriveFromStruct*) -> i32 { return (*p).
 
 // --- fail_derive_from_incomplete.carbon
 
-// CHECK:STDERR: fail_derive_from_incomplete.carbon:[[@LINE+4]]:1: error: `base` not allowed on `class` declaration, only definition
+// CHECK:STDERR: fail_derive_from_incomplete.carbon:[[@LINE+4]]:1: error: `base` not allowed on `class` forward declaration, only definition
 // CHECK:STDERR: base class Incomplete;
 // CHECK:STDERR: ^~~~
 // CHECK:STDERR:

diff --git a/toolchain/check/testdata/class/fail_field_modifiers.carbon b/toolchain/check/testdata/class/fail_field_modifiers.carbon
@@ -22,13 +22,13 @@ class Class {
   // CHECK:STDERR:
   final var k: i32;
 
-  // CHECK:STDERR: fail_field_modifiers.carbon:[[@LINE+4]]:3: error: `default` not allowed on `let` declaration outside of an interface
+  // CHECK:STDERR: fail_field_modifiers.carbon:[[@LINE+4]]:3: error: `default` not allowed; requires interface scope
   // CHECK:STDERR:   default let l: i32 = 0;
   // CHECK:STDERR:   ^~~~~~~
   // CHECK:STDERR:
   default let l: i32 = 0;
 
-  // CHECK:STDERR: fail_field_modifiers.carbon:[[@LINE+3]]:3: error: `final` not allowed on `let` declaration outside of an interface
+  // CHECK:STDERR: fail_field_modifiers.carbon:[[@LINE+3]]:3: error: `final` not allowed; requires interface scope
   // CHECK:STDERR:   final let m: i32 = 1;
   // CHECK:STDERR:   ^~~~~
   final let m: i32 = 1;

diff --git a/toolchain/check/testdata/class/fail_method_modifiers.carbon b/toolchain/check/testdata/class/fail_method_modifiers.carbon
@@ -10,7 +10,7 @@
 
 class FinalClass {
 
-  // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE+7]]:3: error: `abstract` not allowed on `fn` declaration in a non-abstract `class` definition
+  // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE+7]]:3: error: `abstract` not allowed; requires `abstract` class scope
   // CHECK:STDERR:   abstract fn Abstract[self: Self]();
   // CHECK:STDERR:   ^~~~~~~~
   // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE-5]]:1: note: containing definition here
@@ -19,7 +19,7 @@ class FinalClass {
   // CHECK:STDERR:
   abstract fn Abstract[self: Self]();
 
-  // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE+7]]:3: error: `virtual` not allowed on `fn` declaration in a non-abstract non-base `class` definition
+  // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE+7]]:3: error: `virtual` not allowed; requires `abstract` or `base` class scope
   // CHECK:STDERR:   virtual fn Virtual[self: Self]();
   // CHECK:STDERR:   ^~~~~~~
   // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE-14]]:1: note: containing definition here
@@ -31,13 +31,13 @@ class FinalClass {
 
 abstract class AbstractClass {
 
-  // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE+4]]:3: error: `default` not allowed on `fn` declaration outside of an interface
+  // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE+4]]:3: error: `default` not allowed; requires interface scope
   // CHECK:STDERR:   default fn Default[self: Self]();
   // CHECK:STDERR:   ^~~~~~~
   // CHECK:STDERR:
   default fn Default[self: Self]();
 
-  // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE+4]]:3: error: `final` not allowed on `fn` declaration outside of an interface
+  // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE+4]]:3: error: `final` not allowed; requires interface scope
   // CHECK:STDERR:   final fn Final[self: Self]();
   // CHECK:STDERR:   ^~~~~
   // CHECK:STDERR:
@@ -46,7 +46,7 @@ abstract class AbstractClass {
 
 base class BaseClass {
 
-  // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE+6]]:3: error: `abstract` not allowed on `fn` declaration in a non-abstract `class` definition
+  // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE+6]]:3: error: `abstract` not allowed; requires `abstract` class scope
   // CHECK:STDERR:   abstract fn Abstract[self: Self]();
   // CHECK:STDERR:   ^~~~~~~~
   // CHECK:STDERR: fail_method_modifiers.carbon:[[@LINE-5]]:1: note: containing definition here

diff --git a/toolchain/check/testdata/class/fail_modifiers.carbon b/toolchain/check/testdata/class/fail_modifiers.carbon
@@ -17,7 +17,7 @@
 // CHECK:STDERR:
 private private class DuplicatePrivate;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `abstract` not allowed on `class` declaration, only definition
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `abstract` not allowed on `class` forward declaration, only definition
 // CHECK:STDERR: abstract class AbstractDecl;
 // CHECK:STDERR: ^~~~~~~~
 // CHECK:STDERR:
@@ -32,7 +32,7 @@ abstract class AbstractDecl;
 // CHECK:STDERR:
 private protected class TwoAccess;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `base` not allowed on `class` declaration, only definition
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `base` not allowed on `class` forward declaration, only definition
 // CHECK:STDERR: base class BaseDecl;
 // CHECK:STDERR: ^~~~
 // CHECK:STDERR:
@@ -47,7 +47,7 @@ base class BaseDecl;
 // CHECK:STDERR:
 abstract abstract class TwoAbstract { }
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+15]]:1: error: `protected` not allowed on `class` declaration at file scope, `protected` is only allowed on class members
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+15]]:1: error: `protected` not allowed; requires class scope
 // CHECK:STDERR: protected virtual base class Virtual {}
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:

diff --git a/toolchain/check/testdata/class/no_prelude/extern.carbon b/toolchain/check/testdata/class/no_prelude/extern.carbon
@@ -93,7 +93,7 @@ class C;
 library "[[@TEST_NAME]]";
 
 class C {
-  // CHECK:STDERR: fail_extern_member_class.carbon:[[@LINE+4]]:3: error: `extern` not allowed on `class` declaration that is a member
+  // CHECK:STDERR: fail_extern_member_class.carbon:[[@LINE+4]]:3: error: `extern` not allowed; requires file or namespace scope
   // CHECK:STDERR:   extern class D;
   // CHECK:STDERR:   ^~~~~~
   // CHECK:STDERR:

diff --git a/toolchain/check/testdata/function/declaration/no_prelude/extern.carbon b/toolchain/check/testdata/function/declaration/no_prelude/extern.carbon
@@ -53,12 +53,12 @@ fn F();
 library "[[@TEST_NAME]]";
 
 class C {
-  // CHECK:STDERR: fail_member_extern.carbon:[[@LINE+4]]:3: error: `extern` not allowed on `fn` declaration that is a member
+  // CHECK:STDERR: fail_member_extern.carbon:[[@LINE+4]]:3: error: `extern` not allowed; requires file or namespace scope
   // CHECK:STDERR:   extern fn F();
   // CHECK:STDERR:   ^~~~~~
   // CHECK:STDERR:
   extern fn F();
-  // CHECK:STDERR: fail_member_extern.carbon:[[@LINE+3]]:3: error: `extern` not allowed on `fn` declaration that is a member
+  // CHECK:STDERR: fail_member_extern.carbon:[[@LINE+3]]:3: error: `extern` not allowed; requires file or namespace scope
   // CHECK:STDERR:   extern fn G[self: Self]();
   // CHECK:STDERR:   ^~~~~~
   extern fn G[self: Self]();