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 <richard@metafoo.co.uk>

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.

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,8 +77,9 @@ 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);
     }
   }
@@ -59,19 +87,40 @@ auto ForbidModifiersOnDecl(Context& context, DeclIntroducerState& introducer,
   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

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:

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:

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;

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

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:

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:

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]();

toolchain/check/testdata/function/declaration/no_prelude/fail_modifiers.carbon

@@ -8,7 +8,7 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/function/declaration/no_prelude/fail_modifiers.carbon
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `default` not allowed on `fn` declaration outside of an interface
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `default` not allowed; requires interface scope
 // CHECK:STDERR: default protected fn WrongOrder();
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
@@ -21,7 +21,7 @@
 // CHECK:STDERR:
 default protected fn WrongOrder();
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `virtual` not allowed on `fn` declaration outside of a class
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `virtual` not allowed; requires class scope
 // CHECK:STDERR: virtual virtual fn DuplicateVirtual() {}
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
@@ -43,7 +43,7 @@ virtual virtual fn DuplicateVirtual() {}
 // CHECK:STDERR:
 private protected fn TwoAccess();
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `abstract` not allowed on `fn` declaration outside of a class
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `abstract` not allowed; requires class scope
 // CHECK:STDERR: abstract virtual fn ModifiersConflict() {}
 // CHECK:STDERR: ^~~~~~~~
 // CHECK:STDERR:
@@ -62,7 +62,7 @@ abstract virtual fn ModifiersConflict() {}
 // CHECK:STDERR:
 base fn InvalidModifier();
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+18]]:1: error: `default` not allowed on `fn` declaration outside of an interface
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+18]]:1: error: `default` not allowed; requires interface scope
 // CHECK:STDERR: default final virtual fn ModifiersConflict2() {}
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:

toolchain/check/testdata/interface/no_prelude/fail_modifiers.carbon

@@ -28,7 +28,7 @@ default interface Default;
 virtual interface Virtual {
 }
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:1: error: `protected` not allowed on `interface` declaration at file scope, `protected` is only allowed on class members
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:1: error: `protected` not allowed; requires class scope
 // CHECK:STDERR: protected interface Protected;
 // CHECK:STDERR: ^~~~~~~~~
 protected interface Protected;

toolchain/check/testdata/let/fail_modifiers.carbon

@@ -8,19 +8,19 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/let/fail_modifiers.carbon
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `protected` not allowed on `let` declaration at file scope, `protected` is only allowed on class members
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `protected` not allowed; requires class scope
 // CHECK:STDERR: protected let b: i32 = 1;
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:
 protected let b: i32 = 1;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `default` not allowed on `let` declaration outside of an interface
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `default` not allowed; requires interface scope
 // CHECK:STDERR: default let c: i32 = 1;
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
 default let c: i32 = 1;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `final` not allowed on `let` declaration outside of an interface
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `final` not allowed; requires interface scope
 // CHECK:STDERR: final let d: i32 = 1;
 // CHECK:STDERR: ^~~~~
 // CHECK:STDERR:
@@ -32,7 +32,7 @@ final let d: i32 = 1;
 // CHECK:STDERR:
 virtual let e: i32 = 1;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `default` not allowed on `let` declaration outside of an interface
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `default` not allowed; requires interface scope
 // CHECK:STDERR: default final let f: i32 = 1;
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
@@ -45,7 +45,7 @@ virtual let e: i32 = 1;
 // CHECK:STDERR:
 default final let f: i32 = 1;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `default` not allowed on `let` declaration outside of an interface
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `default` not allowed; requires interface scope
 // CHECK:STDERR: default default let g: i32 = 1;
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
@@ -58,7 +58,7 @@ default final let f: i32 = 1;
 // CHECK:STDERR:
 default default let g: i32 = 1;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `protected` not allowed on `let` declaration at file scope, `protected` is only allowed on class members
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `protected` not allowed; requires class scope
 // CHECK:STDERR: protected private let h: i32 = 1;
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:
@@ -71,7 +71,7 @@ default default let g: i32 = 1;
 // CHECK:STDERR:
 protected private let h: i32 = 1;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+10]]:1: error: `protected` not allowed on `let` declaration at file scope, `protected` is only allowed on class members
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+10]]:1: error: `protected` not allowed; requires class scope
 // CHECK:STDERR: protected protected let i: i32 = 1;
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:

toolchain/check/testdata/var/no_prelude/fail_modifiers.carbon

@@ -8,7 +8,7 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/var/no_prelude/fail_modifiers.carbon
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `protected` not allowed on `var` declaration at file scope, `protected` is only allowed on class members
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `protected` not allowed; requires class scope
 // CHECK:STDERR: protected var b: ();
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:
@@ -23,7 +23,7 @@ protected var b: ();
 // CHECK:STDERR:
 private protected var c: ();
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `protected` not allowed on `var` declaration at file scope, `protected` is only allowed on class members
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `protected` not allowed; requires class scope
 // CHECK:STDERR: protected protected var d: ();
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:

toolchain/diagnostics/diagnostic.h

@@ -42,7 +42,7 @@ enum class DiagnosticLevel : int8_t {
 // See `DiagnosticEmitter::Emit` for comments about argument lifetimes.
 #define CARBON_DIAGNOSTIC(DiagnosticName, Level, Format, ...) \
   static constexpr auto DiagnosticName =                      \
-      ::Carbon::Internal::DiagnosticBase<__VA_ARGS__>(        \
+      ::Carbon::DiagnosticBase<__VA_ARGS__>(                  \
           ::Carbon::DiagnosticKind::DiagnosticName,           \
           ::Carbon::DiagnosticLevel::Level, Format)
 
@@ -110,8 +110,6 @@ struct Diagnostic {
   llvm::SmallVector<DiagnosticMessage> messages;
 };
 
-namespace Internal {
-
 // Use the DIAGNOSTIC macro to instantiate this.
 // This stores static information about a diagnostic category.
 template <typename... Args>
@@ -132,8 +130,6 @@ struct DiagnosticBase {
   llvm::StringLiteral Format;
 };
 
-}  // namespace Internal
-
 }  // namespace Carbon
 
 #endif  // CARBON_TOOLCHAIN_DIAGNOSTICS_DIAGNOSTIC_H_

toolchain/diagnostics/diagnostic_converter.h

@@ -18,8 +18,8 @@ class DiagnosticConverter {
   // Callback type used to report context messages from ConvertLoc.
   // Note that the first parameter type is DiagnosticLoc rather than
   // LocT, because ConvertLoc must not recurse.
-  using ContextFnT = llvm::function_ref<void(
-      DiagnosticLoc, const Internal::DiagnosticBase<>&)>;
+  using ContextFnT =
+      llvm::function_ref<void(DiagnosticLoc, const DiagnosticBase<>&)>;
 
   virtual ~DiagnosticConverter() = default;
 

toolchain/diagnostics/diagnostic_emitter.h

@@ -60,8 +60,7 @@ class DiagnosticEmitter {
     // The API mirrors the main emission API: `DiagnosticEmitter::Emit`.
     // For the expected usage see the builder API: `DiagnosticEmitter::Build`.
     template <typename... Args>
-    auto Note(LocT loc,
-              const Internal::DiagnosticBase<Args...>& diagnostic_base,
+    auto Note(LocT loc, const DiagnosticBase<Args...>& diagnostic_base,
               Internal::NoTypeDeduction<Args>... args) -> DiagnosticBuilder& {
       if (!emitter_) {
         return *this;
@@ -95,10 +94,9 @@ class DiagnosticEmitter {
     friend class DiagnosticEmitter<LocT>;
 
     template <typename... Args>
-    explicit DiagnosticBuilder(
-        DiagnosticEmitter<LocT>* emitter, LocT loc,
-        const Internal::DiagnosticBase<Args...>& diagnostic_base,
-        llvm::SmallVector<llvm::Any> args)
+    explicit DiagnosticBuilder(DiagnosticEmitter<LocT>* emitter, LocT loc,
+                               const DiagnosticBase<Args...>& diagnostic_base,
+                               llvm::SmallVector<llvm::Any> args)
         : emitter_(emitter), diagnostic_({.level = diagnostic_base.Level}) {
       AddMessage(loc, diagnostic_base, std::move(args));
       CARBON_CHECK(diagnostic_base.Level != DiagnosticLevel::Note);
@@ -111,8 +109,7 @@ class DiagnosticEmitter {
     // Adds a message to the diagnostic, handling conversion of the location and
     // arguments.
     template <typename... Args>
-    auto AddMessage(LocT loc,
-                    const Internal::DiagnosticBase<Args...>& diagnostic_base,
+    auto AddMessage(LocT loc, const DiagnosticBase<Args...>& diagnostic_base,
                     llvm::SmallVector<llvm::Any> args) -> void {
       if (!emitter_) {
         return;
@@ -121,7 +118,7 @@ class DiagnosticEmitter {
           emitter_->converter_->ConvertLoc(
               loc,
               [&](DiagnosticLoc context_loc,
-                  const Internal::DiagnosticBase<>& context_diagnostic_base) {
+                  const DiagnosticBase<>& context_diagnostic_base) {
                 AddMessageWithDiagnosticLoc(context_loc,
                                             context_diagnostic_base, {});
               }),
@@ -133,8 +130,7 @@ class DiagnosticEmitter {
     // avoid potential recursion.
     template <typename... Args>
     auto AddMessageWithDiagnosticLoc(
-        DiagnosticLoc loc,
-        const Internal::DiagnosticBase<Args...>& diagnostic_base,
+        DiagnosticLoc loc, const DiagnosticBase<Args...>& diagnostic_base,
         llvm::SmallVector<llvm::Any> args) -> void {
       if (!emitter_) {
         return;
@@ -185,7 +181,7 @@ class DiagnosticEmitter {
   // When passing arguments, they may be buffered. As a consequence, lifetimes
   // may outlive the `Emit` call.
   template <typename... Args>
-  auto Emit(LocT loc, const Internal::DiagnosticBase<Args...>& diagnostic_base,
+  auto Emit(LocT loc, const DiagnosticBase<Args...>& diagnostic_base,
             Internal::NoTypeDeduction<Args>... args) -> void {
     DiagnosticBuilder(this, loc, diagnostic_base, {MakeAny<Args>(args)...})
         .Emit();
@@ -198,7 +194,7 @@ class DiagnosticEmitter {
   //     .Note(loc2, MyDiagnosticNote)
   //     .Emit();
   template <typename... Args>
-  auto Build(LocT loc, const Internal::DiagnosticBase<Args...>& diagnostic_base,
+  auto Build(LocT loc, const DiagnosticBase<Args...>& diagnostic_base,
              Internal::NoTypeDeduction<Args>... args) -> DiagnosticBuilder {
     return DiagnosticBuilder(this, loc, diagnostic_base,
                              {MakeAny<Args>(args)...});

toolchain/diagnostics/diagnostic_kind.def

@@ -356,7 +356,15 @@ CARBON_DIAGNOSTIC_KIND(QualifiedExprNameNotFound)
 CARBON_DIAGNOSTIC_KIND(UseOfNonExprAsValue)
 
 // Modifier checking.
-CARBON_DIAGNOSTIC_KIND(ModifierNotAllowedOn)
+CARBON_DIAGNOSTIC_KIND(ModifierNotAllowedOnDeclaration)
+CARBON_DIAGNOSTIC_KIND(ModifierOnlyAllowedOnDefinition)
+CARBON_DIAGNOSTIC_KIND(ModifierPrivateNotAllowed)
+CARBON_DIAGNOSTIC_KIND(ModifierProtectedNotAllowed)
+CARBON_DIAGNOSTIC_KIND(ModifierVirtualNotAllowed)
+CARBON_DIAGNOSTIC_KIND(ModifierAbstractNotAllowed)
+CARBON_DIAGNOSTIC_KIND(ModifierRequiresClass)
+CARBON_DIAGNOSTIC_KIND(ModifierRequiresInterface)
+CARBON_DIAGNOSTIC_KIND(ModifierExternNotAllowed)
 CARBON_DIAGNOSTIC_KIND(ModifierNotInContext)
 CARBON_DIAGNOSTIC_KIND(ModifierRepeated)
 CARBON_DIAGNOSTIC_KIND(ModifierNotAllowedWith)

toolchain/lower/file_context.cpp

@@ -568,9 +568,8 @@ auto FileContext::BuildGlobalVariableDecl(SemIR::VarStorage var_storage)
 
 auto FileContext::GetLocForDI(SemIR::InstId inst_id) -> LocForDI {
   auto diag_loc = converter_.ConvertLoc(
-      inst_id,
-      [&](DiagnosticLoc /*context_loc*/,
-          const Internal::DiagnosticBase<>& /*context_diagnostic_base*/) {});
+      inst_id, [&](DiagnosticLoc /*context_loc*/,
+                   const DiagnosticBase<>& /*context_diagnostic_base*/) {});
   return {.filename = diag_loc.filename,
           .line_number = diag_loc.line_number == -1 ? 0 : diag_loc.line_number,
           .column_number =