From 7a5ae3b712592260b53600a8a9aa2d3e32c20c90 Mon Sep 17 00:00:00 2001 From: jonmeow Date: Wed, 16 Oct 2024 16:12:43 -0700 Subject: [PATCH] Clean up format_provider uses --- toolchain/check/context.cpp | 19 ++++++----- .../testdata/class/access_modifers.carbon | 14 ++++---- .../testdata/class/inheritance_access.carbon | 12 +++---- toolchain/diagnostics/diagnostic_kind.def | 2 +- toolchain/docs/diagnostics.md | 12 ++++--- toolchain/lex/BUILD | 1 + toolchain/lex/numeric_literal.cpp | 32 ++++--------------- toolchain/sem_ir/name_scope.h | 23 ------------- 8 files changed, 39 insertions(+), 76 deletions(-) diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 42b44cd40079b..bb9b227f34016 100644 --- 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 index 3ee4c644c03b7..20ba43da38723 100644 --- a/toolchain/check/testdata/class/access_modifers.carbon +++ b/toolchain/check/testdata/class/access_modifers.carbon @@ -30,7 +30,7 @@ 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: @@ -38,7 +38,7 @@ fn Run() { // 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: @@ -46,7 +46,7 @@ fn Run() { // 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,7 +123,7 @@ 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: @@ -131,7 +131,7 @@ 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 index 794f58e87918f..244fcaa063600 100644 --- 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 index 1bc2e2dad5603..194b85eb55fea 100644 --- 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 index 33fd8ba014e41..603b553614e6e 100644 --- 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 index e09e6a53baca6..faa255e9d240c 100644 --- 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 index f528a679eeb33..d75de7e8881aa 100644 --- 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 { - 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(radix)); return {.ok = false}; } diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index 18308d3f21f7d..d94d157d9042b 100644 --- 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 { - 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 { struct Entry { NameId name_id;