Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 28 additions & 13 deletions toolchain/check/call.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@

namespace Carbon::Check {

namespace {
// Entity kinds, for diagnostics. Converted to an int for a select.
enum class EntityKind : uint8_t {
Function = 0,
GenericClass = 1,
GenericInterface = 2,
};
} // namespace

// Resolves the callee expression in a call to a specific callee, or diagnoses
// if no specific callee can be identified. This verifies the arity of the
// callee and determines any compile-time arguments, but doesn't check that the
Expand All @@ -31,7 +40,7 @@ namespace Carbon::Check {
// callee is not generic, or `nullopt` if an error has been diagnosed.
static auto ResolveCalleeInCall(Context& context, SemIR::LocId loc_id,
const SemIR::EntityWithParamsBase& entity,
llvm::StringLiteral entity_kind_for_diagnostic,
EntityKind entity_kind_for_diagnostic,
SemIR::GenericId entity_generic_id,
SemIR::SpecificId enclosing_specific_id,
SemIR::InstId self_id,
Expand All @@ -43,16 +52,20 @@ static auto ResolveCalleeInCall(Context& context, SemIR::LocId loc_id,
auto params = context.inst_blocks().GetOrEmpty(callee_info.param_refs_id);
if (arg_ids.size() != params.size()) {
CARBON_DIAGNOSTIC(CallArgCountMismatch, Error,
"{0} argument{0:s} passed to {1} expecting "
"{2} argument{2:s}",
IntAsSelect, llvm::StringLiteral, IntAsSelect);
CARBON_DIAGNOSTIC(InCallToEntity, Note, "calling {0} declared here",
llvm::StringLiteral);
"{0} argument{0:s} passed to "
"{1:=0:function|=1:generic class|=2:generic interface}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I'm not sure I agree with the thesis that the string "should probably be written in the format string instead of separately". This change makes it possible to spot bugs in the diagnostic's phrasing by local inspection, but the tradeoff is that it makes it noticeably harder for me to read the format string as a whole, and it introduces a new point of failure: the int -> kind mapping in the format string might not match the kind -> int mapping in the enum (which can't be spotted by local inspection).

If you want to make sure phrasing problems can be spotted locally, I'd suggest that we stick with the previous format string, but map the enum to a string literal locally. That way all the components of the message are available locally, and there's no potentially error-prone indirection through integers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a phrasing issue; it's one of translatability. For example, let's say someone wants to translate diagnostics to French. If "generic" is part of the hardcoded inputs, how does someone provide a translation for it?

In this model, someone just needs to provide an appropriate format string, and there should be sufficient context that they can adjust ordering as needed.

To be clear, it's not the ability to spot bugs. It's avoiding fundamentally English-centric design.

Copy link
Contributor Author

@jonmeow jonmeow Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note too, we'd still need to add the ability to replace [ed: format] strings (that's just not here right now). But the intent here is to stop [ed: or at least reduce] building up technical debt in terms of diagnostics that would be incompatible with translation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see; that makes sense. I'm still concerned about the risk of bugs from the indirection through integers, and I feel like we can address that in a way that's consistent with translatability, but I'm happy to treat that as future work.

" expecting {2} argument{2:s}",
IntAsSelect, IntAsSelect, IntAsSelect);
CARBON_DIAGNOSTIC(
InCallToEntity, Note,
"calling {0:=0:function|=1:generic class|=2:generic interface}"
" declared here",
IntAsSelect);
context.emitter()
.Build(loc_id, CallArgCountMismatch, arg_ids.size(),
entity_kind_for_diagnostic, params.size())
static_cast<int>(entity_kind_for_diagnostic), params.size())
.Note(callee_info.callee_loc, InCallToEntity,
entity_kind_for_diagnostic)
static_cast<int>(entity_kind_for_diagnostic))
.Emit();
return std::nullopt;
}
Expand Down Expand Up @@ -80,8 +93,9 @@ static auto PerformCallToGenericClass(Context& context, SemIR::LocId loc_id,
-> SemIR::InstId {
const auto& generic_class = context.classes().Get(class_id);
auto callee_specific_id = ResolveCalleeInCall(
context, loc_id, generic_class, "generic class", generic_class.generic_id,
enclosing_specific_id, /*self_id=*/SemIR::InstId::Invalid, arg_ids);
context, loc_id, generic_class, EntityKind::GenericClass,
generic_class.generic_id, enclosing_specific_id,
/*self_id=*/SemIR::InstId::Invalid, arg_ids);
if (!callee_specific_id) {
return SemIR::InstId::BuiltinError;
}
Expand All @@ -99,8 +113,9 @@ static auto PerformCallToGenericInterface(
llvm::ArrayRef<SemIR::InstId> arg_ids) -> SemIR::InstId {
const auto& interface = context.interfaces().Get(interface_id);
auto callee_specific_id = ResolveCalleeInCall(
context, loc_id, interface, "generic interface", interface.generic_id,
enclosing_specific_id, /*self_id=*/SemIR::InstId::Invalid, arg_ids);
context, loc_id, interface, EntityKind::GenericInterface,
interface.generic_id, enclosing_specific_id,
/*self_id=*/SemIR::InstId::Invalid, arg_ids);
if (!callee_specific_id) {
return SemIR::InstId::BuiltinError;
}
Expand Down Expand Up @@ -143,7 +158,7 @@ auto PerformCall(Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id,
// If the callee is a generic function, determine the generic argument values
// for the call.
auto callee_specific_id = ResolveCalleeInCall(
context, loc_id, callable, "function", callable.generic_id,
context, loc_id, callable, EntityKind::Function, callable.generic_id,
callee_function.enclosing_specific_id, callee_function.self_id, arg_ids);
if (!callee_specific_id) {
return SemIR::InstId::BuiltinError;
Expand Down
20 changes: 10 additions & 10 deletions toolchain/check/check.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "toolchain/check/sem_ir_diagnostic_converter.h"
#include "toolchain/diagnostics/diagnostic.h"
#include "toolchain/diagnostics/diagnostic_emitter.h"
#include "toolchain/diagnostics/format_providers.h"
#include "toolchain/lex/token_kind.h"
#include "toolchain/parse/node_ids.h"
#include "toolchain/parse/tree.h"
Expand Down Expand Up @@ -1189,19 +1190,18 @@ static auto BuildApiMapAndDiagnosePackaging(
bool is_api_with_impl_ext = !is_impl && filename.ends_with(ImplExt);
auto want_ext = is_impl ? ImplExt : ApiExt;
if (is_api_with_impl_ext || !filename.ends_with(want_ext)) {
CARBON_DIAGNOSTIC(IncorrectExtension, Error,
"file extension of `{0}` required for `{1}`",
llvm::StringLiteral, Lex::TokenKind);
CARBON_DIAGNOSTIC(
IncorrectExtension, Error,
"file extension of `{0:.impl|}.carbon` required for {0:`impl`|api}",
BoolAsSelect);
auto diag = unit_info.emitter.Build(
packaging ? packaging->names.node_id : Parse::NodeId::Invalid,
IncorrectExtension, want_ext,
is_impl ? Lex::TokenKind::Impl : Lex::TokenKind::Api);
IncorrectExtension, is_impl);
if (is_api_with_impl_ext) {
CARBON_DIAGNOSTIC(IncorrectExtensionImplNote, Note,
"file extension of `{0}` only allowed for `{1}`",
llvm::StringLiteral, Lex::TokenKind);
diag.Note(Parse::NodeId::Invalid, IncorrectExtensionImplNote, ImplExt,
Lex::TokenKind::Impl);
CARBON_DIAGNOSTIC(
IncorrectExtensionImplNote, Note,
"file extension of `.impl.carbon` only allowed for `impl`");
diag.Note(Parse::NodeId::Invalid, IncorrectExtensionImplNote);
}
diag.Emit();
}
Expand Down
32 changes: 16 additions & 16 deletions toolchain/check/eval.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -682,65 +682,65 @@ static auto PerformBuiltinBinaryIntOp(Context& context, SemIRLoc loc,

bool overflow = false;
llvm::APInt result_val;
llvm::StringLiteral op_str = "<error>";
Lex::TokenKind op_token = Lex::TokenKind::Not;
switch (builtin_kind) {
// Arithmetic.
case SemIR::BuiltinFunctionKind::IntSAdd:
result_val = lhs_val.sadd_ov(rhs_val, overflow);
op_str = "+";
op_token = Lex::TokenKind::Plus;
break;
case SemIR::BuiltinFunctionKind::IntSSub:
result_val = lhs_val.ssub_ov(rhs_val, overflow);
op_str = "-";
op_token = Lex::TokenKind::Minus;
break;
case SemIR::BuiltinFunctionKind::IntSMul:
result_val = lhs_val.smul_ov(rhs_val, overflow);
op_str = "*";
op_token = Lex::TokenKind::Star;
break;
case SemIR::BuiltinFunctionKind::IntSDiv:
result_val = lhs_val.sdiv_ov(rhs_val, overflow);
op_str = "/";
op_token = Lex::TokenKind::Slash;
break;
case SemIR::BuiltinFunctionKind::IntSMod:
result_val = lhs_val.srem(rhs_val);
// LLVM weirdly lacks `srem_ov`, so we work it out for ourselves:
// <signed min> % -1 overflows because <signed min> / -1 overflows.
overflow = lhs_val.isMinSignedValue() && rhs_val.isAllOnes();
op_str = "%";
op_token = Lex::TokenKind::Percent;
break;
case SemIR::BuiltinFunctionKind::IntUAdd:
result_val = lhs_val + rhs_val;
op_str = "+";
op_token = Lex::TokenKind::Plus;
break;
case SemIR::BuiltinFunctionKind::IntUSub:
result_val = lhs_val - rhs_val;
op_str = "-";
op_token = Lex::TokenKind::Minus;
break;
case SemIR::BuiltinFunctionKind::IntUMul:
result_val = lhs_val * rhs_val;
op_str = "*";
op_token = Lex::TokenKind::Star;
break;
case SemIR::BuiltinFunctionKind::IntUDiv:
result_val = lhs_val.udiv(rhs_val);
op_str = "/";
op_token = Lex::TokenKind::Slash;
break;
case SemIR::BuiltinFunctionKind::IntUMod:
result_val = lhs_val.urem(rhs_val);
op_str = "%";
op_token = Lex::TokenKind::Percent;
break;

// Bitwise.
case SemIR::BuiltinFunctionKind::IntAnd:
result_val = lhs_val & rhs_val;
op_str = "&";
op_token = Lex::TokenKind::And;
break;
case SemIR::BuiltinFunctionKind::IntOr:
result_val = lhs_val | rhs_val;
op_str = "|";
op_token = Lex::TokenKind::Pipe;
break;
case SemIR::BuiltinFunctionKind::IntXor:
result_val = lhs_val ^ rhs_val;
op_str = "^";
op_token = Lex::TokenKind::Caret;
break;

// Bit shift.
Expand Down Expand Up @@ -777,9 +777,9 @@ static auto PerformBuiltinBinaryIntOp(Context& context, SemIRLoc loc,
if (overflow) {
CARBON_DIAGNOSTIC(CompileTimeIntegerOverflow, Error,
"integer overflow in calculation {0} {1} {2}", TypedInt,
llvm::StringLiteral, TypedInt);
Lex::TokenKind, TypedInt);
context.emitter().Emit(loc, CompileTimeIntegerOverflow,
{.type = lhs.type_id, .value = lhs_val}, op_str,
{.type = lhs.type_id, .value = lhs_val}, op_token,
{.type = rhs.type_id, .value = rhs_val});
}

Expand Down
12 changes: 6 additions & 6 deletions toolchain/check/testdata/packages/fail_extension.carbon
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/packages/fail_extension.carbon
// TIP: To dump output, run:
// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/packages/fail_extension.carbon
// CHECK:STDERR: fail_main.incorrect: error(IncorrectExtension): file extension of `.carbon` required for `api`
// CHECK:STDERR: fail_main.incorrect: error(IncorrectExtension): file extension of `.carbon` required for api
// CHECK:STDERR:
// CHECK:STDERR: fail_main_redundant_with_swapped_ext.impl.carbon: error(DuplicateMainApi): `Main//default` previously provided by `fail_main.incorrect`
// CHECK:STDERR:
// CHECK:STDERR: fail_main_redundant_with_swapped_ext.impl.carbon: error(IncorrectExtension): file extension of `.carbon` required for `api`
// CHECK:STDERR: fail_main_redundant_with_swapped_ext.impl.carbon: error(IncorrectExtension): file extension of `.carbon` required for api
// CHECK:STDERR: fail_main_redundant_with_swapped_ext.impl.carbon: note(IncorrectExtensionImplNote): file extension of `.impl.carbon` only allowed for `impl`
// CHECK:STDERR:

Expand All @@ -21,7 +21,7 @@

// --- fail_main_lib.incorrect

// CHECK:STDERR: fail_main_lib.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for `api`
// CHECK:STDERR: fail_main_lib.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for api
// CHECK:STDERR: library "main_lib";
// CHECK:STDERR: ^~~~~~~
// CHECK:STDERR:
Expand All @@ -37,7 +37,7 @@ impl library "[[@TEST_NAME]]";

// --- fail_package.incorrect

// CHECK:STDERR: fail_package.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for `api`
// CHECK:STDERR: fail_package.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for api
// CHECK:STDERR: package Package;
// CHECK:STDERR: ^~~~~~~
// CHECK:STDERR:
Expand All @@ -53,7 +53,7 @@ impl package Package;

// --- fail_package_lib.incorrect

// CHECK:STDERR: fail_package_lib.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for `api`
// CHECK:STDERR: fail_package_lib.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for api
// CHECK:STDERR: package Package library "package_lib";
// CHECK:STDERR: ^~~~~~~
// CHECK:STDERR:
Expand All @@ -69,7 +69,7 @@ impl package Package library "[[@TEST_NAME]]";

// --- fail_swapped_ext.impl.carbon

// CHECK:STDERR: fail_swapped_ext.impl.carbon:[[@LINE+5]]:1: error(IncorrectExtension): file extension of `.carbon` required for `api`
// CHECK:STDERR: fail_swapped_ext.impl.carbon:[[@LINE+5]]:1: error(IncorrectExtension): file extension of `.carbon` required for api
// CHECK:STDERR: package SwappedExt;
// CHECK:STDERR: ^~~~~~~
// CHECK:STDERR: fail_swapped_ext.impl.carbon: note(IncorrectExtensionImplNote): file extension of `.impl.carbon` only allowed for `impl`
Expand Down
8 changes: 5 additions & 3 deletions toolchain/diagnostics/diagnostic.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,11 @@ struct DiagnosticBase {
explicit constexpr DiagnosticBase(DiagnosticKind kind, DiagnosticLevel level,
llvm::StringLiteral format)
: Kind(kind), Level(level), Format(format) {
static_assert((... && !std::is_same_v<Args, llvm::StringRef>),
"Use std::string or llvm::StringLiteral for diagnostics to "
"avoid lifetime issues.");
static_assert((... && !(std::is_same_v<Args, llvm::StringRef> ||
std::is_same_v<Args, llvm::StringLiteral>)),
"String type disallowed in diagnostics. See "
"https://github.com/carbon-language/carbon-lang/blob/trunk/"
"toolchain/docs/diagnostics.md#diagnostic-parameter-types");
}

// The diagnostic's kind.
Expand Down
2 changes: 1 addition & 1 deletion toolchain/diagnostics/diagnostic_emitter_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ TEST_F(DiagnosticEmitterTest, EmitSimpleWarning) {
}

TEST_F(DiagnosticEmitterTest, EmitOneArgDiagnostic) {
CARBON_DIAGNOSTIC(TestDiagnostic, Error, "arg: `{0}`", llvm::StringLiteral);
CARBON_DIAGNOSTIC(TestDiagnostic, Error, "arg: `{0}`", std::string);
EXPECT_CALL(consumer_, HandleDiagnostic(IsSingleDiagnostic(
DiagnosticKind::TestDiagnostic,
DiagnosticLevel::Error, 1, 1, "arg: `str`")));
Expand Down
14 changes: 7 additions & 7 deletions toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace {
using ::Carbon::Testing::IsSingleDiagnostic;
using ::testing::InSequence;

CARBON_DIAGNOSTIC(TestDiagnostic, Error, "{0}", llvm::StringLiteral);
CARBON_DIAGNOSTIC(TestDiagnostic, Error, "M{0}", int);

struct FakeDiagnosticConverter : DiagnosticConverter<DiagnosticLoc> {
auto ConvertLoc(DiagnosticLoc loc, ContextFnT /*context_fn*/) const
Expand All @@ -32,12 +32,12 @@ TEST(SortedDiagnosticEmitterTest, SortErrors) {
SortingDiagnosticConsumer sorting_consumer(consumer);
DiagnosticEmitter<DiagnosticLoc> emitter(converter, sorting_consumer);

emitter.Emit({"f", "line", 2, 1}, TestDiagnostic, "M1");
emitter.Emit({"f", "line", 1, 1}, TestDiagnostic, "M2");
emitter.Emit({"f", "line", 1, 3}, TestDiagnostic, "M3");
emitter.Emit({"f", "line", 3, 4}, TestDiagnostic, "M4");
emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, "M5");
emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, "M6");
emitter.Emit({"f", "line", 2, 1}, TestDiagnostic, 1);
emitter.Emit({"f", "line", 1, 1}, TestDiagnostic, 2);
emitter.Emit({"f", "line", 1, 3}, TestDiagnostic, 3);
emitter.Emit({"f", "line", 3, 4}, TestDiagnostic, 4);
emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, 5);
emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, 6);

InSequence s;
EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
Expand Down
7 changes: 5 additions & 2 deletions toolchain/docs/diagnostics.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,12 @@ methods for formatting arguments:
- This includes `char` and integer types (`int`, `int32_t`, and so on).
- String types can be added as needed, but stringifying values using the
methods noted below is preferred.
- Use `llvm::StringLiteral` where appropriate; use `std::string` when
allocations are required.
- Use `std::string` when allocations are required.
- `llvm::StringRef` is disallowed due to lifetime issues.
- `llvm::StringLiteral` is disallowed because format providers such as
`BoolAsSelect` should work in cases where a `StringLiteral` could be
used, and because string literal parameters tend to make the
resulting diagnostics hard to translate.
- `llvm::format_provider<...>` specializations.
- `BoolAsSelect` and `IntAsSelect` from
[format_providers.h](/toolchain/diagnostics/format_providers.h) are
Expand Down
1 change: 0 additions & 1 deletion toolchain/lex/token_kind.def
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ CARBON_KEYWORD_TOKEN(Abstract, "abstract")
CARBON_KEYWORD_TOKEN(Addr, "addr")
CARBON_TOKEN_WITH_VIRTUAL_NODE(
CARBON_KEYWORD_TOKEN(And, "and"))
CARBON_KEYWORD_TOKEN(Api, "api")
CARBON_KEYWORD_TOKEN(As, "as")
CARBON_KEYWORD_TOKEN(Auto, "auto")
CARBON_KEYWORD_TOKEN(Bool, "bool")
Expand Down
4 changes: 2 additions & 2 deletions toolchain/parse/context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,9 @@ auto Context::ConsumeAndAddCloseSymbol(Lex::TokenIndex expected_open,
// TODO: Include the location of the matching opening delimiter in the
// diagnostic.
CARBON_DIAGNOSTIC(ExpectedCloseSymbol, Error,
"unexpected tokens before `{0}`", llvm::StringLiteral);
"unexpected tokens before `{0}`", Lex::TokenKind);
emitter_->Emit(*position_, ExpectedCloseSymbol,
open_token_kind.closing_symbol().fixed_spelling());
open_token_kind.closing_symbol());

SkipTo(tokens().GetMatchedClosingToken(expected_open));
AddNode(close_kind, Consume(), /*has_error=*/true);
Expand Down
12 changes: 7 additions & 5 deletions toolchain/parse/handle_binding_pattern.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include "toolchain/diagnostics/format_providers.h"
#include "toolchain/parse/context.h"
#include "toolchain/parse/handle.h"

Expand All @@ -25,12 +26,13 @@ auto HandleBindingPattern(Context& context) -> void {
}

// Handle an invalid pattern introducer for parameters and variables.
auto on_error = [&](llvm::StringLiteral expected) {
auto on_error = [&](bool expected_name) {
if (!state.has_error) {
CARBON_DIAGNOSTIC(ExpectedBindingPattern, Error,
"expected {0} in binding pattern", llvm::StringLiteral);
"expected {0:name|`:` or `:!`} in binding pattern",
BoolAsSelect);
context.emitter().Emit(*context.position(), ExpectedBindingPattern,
expected);
expected_name);
state.has_error = true;
}
};
Expand All @@ -51,7 +53,7 @@ auto HandleBindingPattern(Context& context) -> void {
// Add a placeholder for the name.
context.AddLeafNode(NodeKind::IdentifierName, *context.position(),
/*has_error=*/true);
on_error("name");
on_error(/*expected_name=*/true);
}

if (auto kind = context.PositionKind();
Expand All @@ -64,7 +66,7 @@ auto HandleBindingPattern(Context& context) -> void {
context.PushState(state);
context.PushStateForExpr(PrecedenceGroup::ForType());
} else {
on_error("`:` or `:!`");
on_error(/*expected_name=*/false);
// Add a placeholder for the type.
context.AddLeafNode(NodeKind::InvalidParse, *context.position(),
/*has_error=*/true);
Expand Down
Loading