Clean up format_provider uses (#4417)

Building on #4411, replace format_provider uses (other than `TokenKind`, which is more on the okay side of things) Also does some edits to `ClassMemberDefinition` to try to better match diagnostic style
carbon-language · Oct 17, 2024 · 5bdeb01 · 5bdeb01
1 parent a02dfe0
commit 5bdeb01
Show file tree

Hide file tree

Showing 8 changed files with 39 additions and 76 deletions.
diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp
@@ -21,6 +21,7 @@
 #include "toolchain/check/inst_block_stack.h"
 #include "toolchain/check/merge.h"
 #include "toolchain/diagnostics/diagnostic_emitter.h"
+#include "toolchain/diagnostics/format_providers.h"
 #include "toolchain/lex/tokenized_buffer.h"
 #include "toolchain/parse/node_ids.h"
 #include "toolchain/parse/node_kind.h"
@@ -362,13 +363,6 @@ static auto DiagnoseInvalidQualifiedNameAccess(Context& context, SemIRLoc loc,
   // TODO: Support scoped entities other than just classes.
   auto class_info = context.classes().Get(class_type->class_id);
 
-  CARBON_DIAGNOSTIC(ClassInvalidMemberAccess, Error,
-                    "cannot access {0} member `{1}` of type {2}",
-                    SemIR::AccessKind, SemIR::NameId, SemIR::TypeId);
-  CARBON_DIAGNOSTIC(ClassMemberDefinition, Note,
-                    "the {0} member `{1}` is defined here", SemIR::AccessKind,
-                    SemIR::NameId);
-
   auto parent_type_id = class_info.self_type_id;
 
   if (access_kind == SemIR::AccessKind::Private && is_parent_access) {
@@ -384,10 +378,15 @@ static auto DiagnoseInvalidQualifiedNameAccess(Context& context, SemIRLoc loc,
     }
   }
 
+  CARBON_DIAGNOSTIC(
+      ClassInvalidMemberAccess, Error,
+      "cannot access {0:private|protected} member `{1}` of type {2}",
+      BoolAsSelect, SemIR::NameId, SemIR::TypeId);
+  CARBON_DIAGNOSTIC(ClassMemberDeclaration, Note, "declared here");
   context.emitter()
-      .Build(loc, ClassInvalidMemberAccess, access_kind, name_id,
-             parent_type_id)
-      .Note(scope_result_id, ClassMemberDefinition, access_kind, name_id)
+      .Build(loc, ClassInvalidMemberAccess,
+             access_kind == SemIR::AccessKind::Private, name_id, parent_type_id)
+      .Note(scope_result_id, ClassMemberDeclaration)
       .Emit();
 }
 

diff --git a/toolchain/check/testdata/class/access_modifers.carbon b/toolchain/check/testdata/class/access_modifers.carbon
@@ -30,23 +30,23 @@ fn Run() {
   // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE+7]]:21: error: cannot access private member `radius` of type `Circle`
   // CHECK:STDERR:   let radius: i32 = circle.radius;
   // CHECK:STDERR:                     ^~~~~~~~~~~~~
-  // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE-17]]:15: note: the private member `radius` is defined here
+  // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE-17]]:15: note: declared here
   // CHECK:STDERR:   private var radius: i32;
   // CHECK:STDERR:               ^~~~~~~~~~~
   // CHECK:STDERR:
   let radius: i32 = circle.radius;
   // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE+7]]:3: error: cannot access private member `radius` of type `Circle`
   // CHECK:STDERR:   circle.radius = 5;
   // CHECK:STDERR:   ^~~~~~~~~~~~~
-  // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE-25]]:15: note: the private member `radius` is defined here
+  // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE-25]]:15: note: declared here
   // CHECK:STDERR:   private var radius: i32;
   // CHECK:STDERR:               ^~~~~~~~~~~
   // CHECK:STDERR:
   circle.radius = 5;
   // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE+7]]:3: error: cannot access private member `SOME_INTERNAL_CONSTANT` of type `Circle`
   // CHECK:STDERR:   circle.SOME_INTERNAL_CONSTANT;
   // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-  // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE-32]]:15: note: the private member `SOME_INTERNAL_CONSTANT` is defined here
+  // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE-32]]:15: note: declared here
   // CHECK:STDERR:   private let SOME_INTERNAL_CONSTANT: i32 = 5;
   // CHECK:STDERR:               ^~~~~~~~~~~~~~~~~~~~~~
   // CHECK:STDERR:
@@ -55,7 +55,7 @@ fn Run() {
   // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE+7]]:3: error: cannot access private member `SomeInternalFunction` of type `Circle`
   // CHECK:STDERR:   circle.SomeInternalFunction();
   // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
-  // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE-39]]:3: note: the private member `SomeInternalFunction` is defined here
+  // CHECK:STDERR: fail_private_field_access.carbon:[[@LINE-39]]:3: note: declared here
   // CHECK:STDERR:   private fn SomeInternalFunction() -> i32 {
   // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   // CHECK:STDERR:
@@ -74,7 +74,7 @@ fn Run() {
   // CHECK:STDERR: fail_protected_field_access.carbon:[[@LINE+7]]:16: error: cannot access protected member `x` of type `A`
   // CHECK:STDERR:   let x: i32 = A.x;
   // CHECK:STDERR:                ^~~
-  // CHECK:STDERR: fail_protected_field_access.carbon:[[@LINE-7]]:17: note: the protected member `x` is defined here
+  // CHECK:STDERR: fail_protected_field_access.carbon:[[@LINE-7]]:17: note: declared here
   // CHECK:STDERR:   protected var x: i32;
   // CHECK:STDERR:                 ^~~~~~
   // CHECK:STDERR:
@@ -123,15 +123,15 @@ class A {
 // CHECK:STDERR: fail_global_access.carbon:[[@LINE+7]]:14: error: cannot access protected member `x` of type `A`
 // CHECK:STDERR: let x: i32 = A.x;
 // CHECK:STDERR:              ^~~
-// CHECK:STDERR: fail_global_access.carbon:[[@LINE-7]]:17: note: the protected member `x` is defined here
+// CHECK:STDERR: fail_global_access.carbon:[[@LINE-7]]:17: note: declared here
 // CHECK:STDERR:   protected let x: i32 = 5;
 // CHECK:STDERR:                 ^
 // CHECK:STDERR:
 let x: i32 = A.x;
 // CHECK:STDERR: fail_global_access.carbon:[[@LINE+6]]:14: error: cannot access private member `y` of type `A`
 // CHECK:STDERR: let y: i32 = A.y;
 // CHECK:STDERR:              ^~~
-// CHECK:STDERR: fail_global_access.carbon:[[@LINE-14]]:15: note: the private member `y` is defined here
+// CHECK:STDERR: fail_global_access.carbon:[[@LINE-14]]:15: note: declared here
 // CHECK:STDERR:   private let y: i32 = 5;
 // CHECK:STDERR:               ^
 let y: i32 = A.y;

diff --git a/toolchain/check/testdata/class/inheritance_access.carbon b/toolchain/check/testdata/class/inheritance_access.carbon
@@ -82,7 +82,7 @@ class Square {
     // CHECK:STDERR: fail_inherited_private_field_access.carbon:[[@LINE+7]]:12: error: cannot access private member `y` of type `Shape`
     // CHECK:STDERR:     return self.y;
     // CHECK:STDERR:            ^~~~~~
-    // CHECK:STDERR: fail_inherited_private_field_access.carbon:[[@LINE-10]]:15: note: the private member `y` is defined here
+    // CHECK:STDERR: fail_inherited_private_field_access.carbon:[[@LINE-10]]:15: note: declared here
     // CHECK:STDERR:   private var y: i32;
     // CHECK:STDERR:               ^~~~~~
     // CHECK:STDERR:
@@ -121,7 +121,7 @@ class C {
   // CHECK:STDERR: fail_noninstance_private_on_parent.carbon:[[@LINE+7]]:12: error: cannot access private member `F` of type `B`
   // CHECK:STDERR:   fn G() { Self.F(); }
   // CHECK:STDERR:            ^~~~~~
-  // CHECK:STDERR: fail_noninstance_private_on_parent.carbon:[[@LINE-8]]:3: note: the private member `F` is defined here
+  // CHECK:STDERR: fail_noninstance_private_on_parent.carbon:[[@LINE-8]]:3: note: declared here
   // CHECK:STDERR:   private fn F() {}
   // CHECK:STDERR:   ^~~~~~~~~~~~~~~~
   // CHECK:STDERR:
@@ -161,7 +161,7 @@ class B {
     // CHECK:STDERR: fail_non_inherited_access.carbon:[[@LINE+7]]:5: error: cannot access private member `SOME_PRIVATE_CONSTANT` of type `A`
     // CHECK:STDERR:     A.SOME_PRIVATE_CONSTANT;
     // CHECK:STDERR:     ^~~~~~~~~~~~~~~~~~~~~~~
-    // CHECK:STDERR: fail_non_inherited_access.carbon:[[@LINE-14]]:15: note: the private member `SOME_PRIVATE_CONSTANT` is defined here
+    // CHECK:STDERR: fail_non_inherited_access.carbon:[[@LINE-14]]:15: note: declared here
     // CHECK:STDERR:   private let SOME_PRIVATE_CONSTANT: i32 = 5;
     // CHECK:STDERR:               ^~~~~~~~~~~~~~~~~~~~~
     // CHECK:STDERR:
@@ -170,7 +170,7 @@ class B {
     // CHECK:STDERR: fail_non_inherited_access.carbon:[[@LINE+7]]:12: error: cannot access protected member `SOME_PROTECTED_CONSTANT` of type `A`
     // CHECK:STDERR:     return A.SOME_PROTECTED_CONSTANT;
     // CHECK:STDERR:            ^~~~~~~~~~~~~~~~~~~~~~~~~
-    // CHECK:STDERR: fail_non_inherited_access.carbon:[[@LINE-24]]:17: note: the protected member `SOME_PROTECTED_CONSTANT` is defined here
+    // CHECK:STDERR: fail_non_inherited_access.carbon:[[@LINE-24]]:17: note: declared here
     // CHECK:STDERR:   protected let SOME_PROTECTED_CONSTANT: i32 = 5;
     // CHECK:STDERR:                 ^~~~~~~~~~~~~~~~~~~~~~~
     // CHECK:STDERR:
@@ -181,7 +181,7 @@ class B {
     // CHECK:STDERR: fail_non_inherited_access.carbon:[[@LINE+7]]:12: error: cannot access protected member `INTERNAL_CONSTANT` of type `Internal`
     // CHECK:STDERR:     return self.internal.INTERNAL_CONSTANT;
     // CHECK:STDERR:            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-    // CHECK:STDERR: fail_non_inherited_access.carbon:[[@LINE-30]]:17: note: the protected member `INTERNAL_CONSTANT` is defined here
+    // CHECK:STDERR: fail_non_inherited_access.carbon:[[@LINE-30]]:17: note: declared here
     // CHECK:STDERR:   protected let INTERNAL_CONSTANT: i32 = 5;
     // CHECK:STDERR:                 ^~~~~~~~~~~~~~~~~
     // CHECK:STDERR:
@@ -203,7 +203,7 @@ class B {
     // CHECK:STDERR: fail_compound_member_access.carbon:[[@LINE+6]]:11: error: cannot access private member `x` of type `A`
     // CHECK:STDERR:     self.(A.x);
     // CHECK:STDERR:           ^~~
-    // CHECK:STDERR: fail_compound_member_access.carbon:[[@LINE-9]]:15: note: the private member `x` is defined here
+    // CHECK:STDERR: fail_compound_member_access.carbon:[[@LINE-9]]:15: note: declared here
     // CHECK:STDERR:   private var x: i32;
     // CHECK:STDERR:               ^~~~~~
     self.(A.x);

diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def
@@ -367,7 +367,7 @@ CARBON_DIAGNOSTIC_KIND(ExternLibraryOnDefinition)
 CARBON_DIAGNOSTIC_KIND(ExternLibraryIsCurrentLibrary)
 
 // Access modifiers.
-CARBON_DIAGNOSTIC_KIND(ClassMemberDefinition)
+CARBON_DIAGNOSTIC_KIND(ClassMemberDeclaration)
 CARBON_DIAGNOSTIC_KIND(ClassInvalidMemberAccess)
 
 // Alias diagnostics.

diff --git a/toolchain/docs/diagnostics.md b/toolchain/docs/diagnostics.md
@@ -171,10 +171,14 @@ methods for formatting arguments:
             allocations are required.
         -   `llvm::StringRef` is disallowed due to lifetime issues.
 -   `llvm::format_provider<...>` specializations.
-    -   This can be used when formatting the parameter doesn't require
-        additional context.
-    -   For example, `Lex::TokenKind` and `Parse::RelativeLoc` provide
-        diagnostic formatting this way.
+    -   `BoolAsSelect` and `IntAsSelect` from
+        [format_providers.h](/toolchain/diagnostics/format_providers.h) are
+        recommended for many cases, because they allow putting the output string
+        in the format.
+        -   `IntAsSelect` can also be used to support pluralization.
+    -   Custom providers can also be added for non-translated values. For
+        example, `Lex::TokenKind` refers to syntax elements, and so can safely
+        have its own format provider.
 -   `DiagnosticConverter::ConvertArg` overrides.
     -   This can provide additional context to a formatter.
     -   For example, formatting `SemIR::NameId` accesses the IR's name list.

diff --git a/toolchain/lex/BUILD b/toolchain/lex/BUILD
@@ -74,6 +74,7 @@ cc_library(
         ":helpers",
         "//common:check",
         "//toolchain/diagnostics:diagnostic_emitter",
+        "//toolchain/diagnostics:format_providers",
         "@llvm-project//llvm:Support",
     ],
 )

diff --git a/toolchain/lex/numeric_literal.cpp b/toolchain/lex/numeric_literal.cpp
@@ -9,30 +9,10 @@
 #include "common/check.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/FormatVariadicDetails.h"
+#include "toolchain/diagnostics/format_providers.h"
 #include "toolchain/lex/character_set.h"
 #include "toolchain/lex/helpers.h"
 
-// We use formatv primarily for diagnostics. In these cases, it's expected that
-// the spelling in source code should be used.
-template <>
-struct llvm::format_provider<Carbon::Lex::NumericLiteral::Radix> {
-  using Radix = Carbon::Lex::NumericLiteral::Radix;
-  static void format(const Radix& radix, raw_ostream& out,
-                     StringRef /*style*/) {
-    switch (radix) {
-      case Radix::Binary:
-        out << "binary";
-        break;
-      case Radix::Decimal:
-        out << "decimal";
-        break;
-      case Radix::Hexadecimal:
-        out << "hexadecimal";
-        break;
-    }
-  }
-};
-
 namespace Carbon::Lex {
 
 auto NumericLiteral::Lex(llvm::StringRef source_text)
@@ -308,10 +288,12 @@ auto NumericLiteral::Parser::CheckDigitSequence(llvm::StringRef text,
       continue;
     }
 
-    CARBON_DIAGNOSTIC(InvalidDigit, Error,
-                      "invalid digit '{0}' in {1} numeric literal", char,
-                      NumericLiteral::Radix);
-    emitter_.Emit(text.begin() + i, InvalidDigit, c, radix);
+    CARBON_DIAGNOSTIC(
+        InvalidDigit, Error,
+        "invalid digit '{0}' in {1:=2:binary|=10:decimal|=16:hexadecimal} "
+        "numeric literal",
+        char, IntAsSelect);
+    emitter_.Emit(text.begin() + i, InvalidDigit, c, static_cast<int>(radix));
     return {.ok = false};
   }
 

diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h
@@ -18,29 +18,6 @@ enum class AccessKind : int8_t {
   Private,
 };
 
-}  // namespace Carbon::SemIR
-
-template <>
-struct llvm::format_provider<Carbon::SemIR::AccessKind> {
-  using AccessKind = Carbon::SemIR::AccessKind;
-  static void format(const AccessKind& loc, raw_ostream& out,
-                     StringRef /*style*/) {
-    switch (loc) {
-      case AccessKind::Private:
-        out << "private";
-        break;
-      case AccessKind::Protected:
-        out << "protected";
-        break;
-      case AccessKind::Public:
-        out << "public";
-        break;
-    }
-  }
-};
-
-namespace Carbon::SemIR {
-
 struct NameScope : Printable<NameScope> {
   struct Entry {
     NameId name_id;