From 73e6994d44922da8da7d706c1d06f3d8f3838ff1 Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Tue, 2 Dec 2025 12:21:53 -0500 Subject: [PATCH] Add a diagnostic note for errors during identifying facet types (#6445) Errors that occur while constructing a specific should be tied back to the facet type being identified. We don't have an InstId for the facet type during identify, so provide the means to Stringify a FacetTypeId. Depends on https://github.com/carbon-language/carbon-lang/pull/6435 --- toolchain/check/diagnostic_emitter.cpp | 3 + .../identify_specific_facet_type.carbon | 14 +- toolchain/check/type_completion.cpp | 18 ++- toolchain/diagnostics/diagnostic_kind.def | 3 + toolchain/sem_ir/ids.h | 1 + toolchain/sem_ir/stringify.cpp | 136 ++++++++++-------- toolchain/sem_ir/stringify.h | 8 +- 7 files changed, 115 insertions(+), 68 deletions(-) diff --git a/toolchain/check/diagnostic_emitter.cpp b/toolchain/check/diagnostic_emitter.cpp index 002c6c405cc6..345e033cd6c8 100644 --- a/toolchain/check/diagnostic_emitter.cpp +++ b/toolchain/check/diagnostic_emitter.cpp @@ -115,6 +115,9 @@ auto DiagnosticEmitter::ConvertArg(llvm::Any arg) const -> llvm::Any { sem_ir_->types().GetInstId(*type_id)) + "`"; } + if (auto* facet_type_id = llvm::any_cast(&arg)) { + return "`" + StringifyFacetType(*sem_ir_, *facet_type_id) + "`"; + } if (auto* specific_id = llvm::any_cast(&arg)) { return "`" + StringifySpecific(*sem_ir_, *specific_id) + "`"; } diff --git a/toolchain/check/testdata/generic/identify_specific_facet_type.carbon b/toolchain/check/testdata/generic/identify_specific_facet_type.carbon index ba8061b91a2a..c348fa3b90e9 100644 --- a/toolchain/check/testdata/generic/identify_specific_facet_type.carbon +++ b/toolchain/check/testdata/generic/identify_specific_facet_type.carbon @@ -18,13 +18,16 @@ library "[[@TEST_NAME]]"; interface K(T:! type) {} constraint J(N:! i32) { - // CHECK:STDERR: fail_error_in_identifying_require_facet_type.carbon:[[@LINE+3]]:30: error: array bound of -1 is negative [ArrayBoundNegative] + // CHECK:STDERR: fail_error_in_identifying_require_facet_type.carbon:[[@LINE+6]]:30: error: array bound of -1 is negative [ArrayBoundNegative] // CHECK:STDERR: require impls K(array(i32, N)); // CHECK:STDERR: ^ + // CHECK:STDERR: fail_error_in_identifying_require_facet_type.carbon:[[@LINE+3]]:3: note: in `require` used here [ResolvingSpecificHere] + // CHECK:STDERR: require impls K(array(i32, N)); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ require impls K(array(i32, N)); } -// CHECK:STDERR: fail_error_in_identifying_require_facet_type.carbon:[[@LINE+8]]:12: note: in `require` used here [ResolvingSpecificHere] +// CHECK:STDERR: fail_error_in_identifying_require_facet_type.carbon:[[@LINE+8]]:12: note: identifying facet type `J(-1)` here [IdentifyingFacetTypeHere] // CHECK:STDERR: impl {} as J(-1) {} // CHECK:STDERR: ^~~~~ // CHECK:STDERR: @@ -44,13 +47,16 @@ library "[[@TEST_NAME]]"; interface K(T:! type) {} constraint J(N:! i32) { - // CHECK:STDERR: fail_error_in_identifying_extend_require_facet_type.carbon:[[@LINE+3]]:37: error: array bound of -1 is negative [ArrayBoundNegative] + // CHECK:STDERR: fail_error_in_identifying_extend_require_facet_type.carbon:[[@LINE+6]]:37: error: array bound of -1 is negative [ArrayBoundNegative] // CHECK:STDERR: extend require impls K(array(i32, N)); // CHECK:STDERR: ^ + // CHECK:STDERR: fail_error_in_identifying_extend_require_facet_type.carbon:[[@LINE+3]]:3: note: in `require` used here [ResolvingSpecificHere] + // CHECK:STDERR: extend require impls K(array(i32, N)); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ extend require impls K(array(i32, N)); } -// CHECK:STDERR: fail_error_in_identifying_extend_require_facet_type.carbon:[[@LINE+8]]:12: note: in `require` used here [ResolvingSpecificHere] +// CHECK:STDERR: fail_error_in_identifying_extend_require_facet_type.carbon:[[@LINE+8]]:12: note: identifying facet type `J(-1)` here [IdentifyingFacetTypeHere] // CHECK:STDERR: impl {} as J(-1) {} // CHECK:STDERR: ^~~~~ // CHECK:STDERR: diff --git a/toolchain/check/type_completion.cpp b/toolchain/check/type_completion.cpp index b0c43d568444..18e80c0be934 100644 --- a/toolchain/check/type_completion.cpp +++ b/toolchain/check/type_completion.cpp @@ -811,7 +811,7 @@ static auto RequireIdentifiedNamedConstraints( // introduce new generic bindings, the specific for the RequireImpls can be // constructed from the enclosing one. static auto GetRequireImplsSpecificFromEnclosingSpecific( - Context& context, SemIR::LocId loc_id, const SemIR::RequireImpls& require, + Context& context, const SemIR::RequireImpls& require, SemIR::SpecificId enclosing_specific_id) -> SemIR::SpecificId { auto enclosing_specific_args_id = context.specifics().GetArgsOrEmpty(enclosing_specific_id); @@ -836,7 +836,8 @@ static auto GetRequireImplsSpecificFromEnclosingSpecific( CARBON_CHECK(context.insts().Is(self_inst_id)); arg_ids.push_back(self_inst_id); - return MakeSpecific(context, loc_id, require.generic_id, arg_ids); + return MakeSpecific(context, SemIR::LocId(require.decl_id), + require.generic_id, arg_ids); } // Returns the `facet_type` mapped into `specific_id`. If an error results, it @@ -903,6 +904,15 @@ auto RequireIdentifiedFacetType(Context& context, SemIR::LocId loc_id, } llvm::append_range(self_impls, facet_type_info.self_impls_constraints); + Diagnostics::AnnotationScope annotate_diagnostics( + &context.emitter(), [&](auto& builder) { + CARBON_DIAGNOSTIC(IdentifyingFacetTypeHere, Note, + "identifying facet type {0} here", + SemIR::FacetTypeId); + builder.Note(loc_id, IdentifyingFacetTypeHere, + facet_type.facet_type_id); + }); + for (auto extends : facet_type_info.extend_named_constraints) { const auto& constraint = context.named_constraints().Get(extends.named_constraint_id); @@ -910,7 +920,7 @@ auto RequireIdentifiedFacetType(Context& context, SemIR::LocId loc_id, constraint.require_impls_block_id)) { const auto& require = context.require_impls().Get(require_impls_id); auto require_specific_id = GetRequireImplsSpecificFromEnclosingSpecific( - context, loc_id, require, extends.specific_id); + context, require, extends.specific_id); auto facet_type_id = TryGetFacetTypeInSpecific( context, require.facet_type_inst_id, require_specific_id); if (facet_type_id.has_value()) { @@ -930,7 +940,7 @@ auto RequireIdentifiedFacetType(Context& context, SemIR::LocId loc_id, constraint.require_impls_block_id)) { const auto& require = context.require_impls().Get(require_impls_id); auto require_specific_id = GetRequireImplsSpecificFromEnclosingSpecific( - context, loc_id, require, impls.specific_id); + context, require, impls.specific_id); auto facet_type_id = TryGetFacetTypeInSpecific( context, require.facet_type_inst_id, require_specific_id); if (facet_type_id.has_value()) { diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index 377d24857c30..30a458e4e92e 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -530,6 +530,9 @@ CARBON_DIAGNOSTIC_KIND(FacetTypeConstraintCycle) // Facet type combination. CARBON_DIAGNOSTIC_KIND(FacetTypeRequiredForTypeAndOperator) +// Facet type completeness. +CARBON_DIAGNOSTIC_KIND(IdentifyingFacetTypeHere) + // Generics. CARBON_DIAGNOSTIC_KIND(GenericMissingExplicitParameters) diff --git a/toolchain/sem_ir/ids.h b/toolchain/sem_ir/ids.h index fd0521ca6470..74eec8109d03 100644 --- a/toolchain/sem_ir/ids.h +++ b/toolchain/sem_ir/ids.h @@ -354,6 +354,7 @@ struct AssociatedConstantId : public IdBase { // The ID of a `FacetTypeInfo`. struct FacetTypeId : public IdBase { static constexpr llvm::StringLiteral Label = "facet_type"; + using DiagnosticType = Diagnostics::TypeInfo; using IdBase::IdBase; }; diff --git a/toolchain/sem_ir/stringify.cpp b/toolchain/sem_ir/stringify.cpp index 84f7d0ae4bbb..d58fe1795ddf 100644 --- a/toolchain/sem_ir/stringify.cpp +++ b/toolchain/sem_ir/stringify.cpp @@ -48,7 +48,8 @@ class StepStack { public: // An individual step in the stack, which stringifies some component of a type // name. - using Step = std::variant; + using Step = + std::variant; // Support `Push` for a qualified name. e.g., `A.B.C`. using QualifiedNameItem = std::pair; @@ -74,6 +75,9 @@ class StepStack { auto PushElementIndex(ElementIndex element_index) -> void { steps_.push_back(element_index); } + auto PushFacetType(FacetTypeId facet_type_id) -> void { + steps_.push_back(facet_type_id); + } // Pushes all components of a qualified name (`A.B.C`) onto the stack. auto PushQualifiedName(NameScopeId name_scope_id, NameId name_id) -> void { @@ -335,64 +339,7 @@ class Stringifier { } auto StringifyInst(InstId /*inst_id*/, FacetType inst) -> void { - const FacetTypeInfo& facet_type_info = - sem_ir_->facet_types().Get(inst.facet_type_id); - // Output `where` restrictions. - bool some_where = false; - if (facet_type_info.other_requirements) { - step_stack_->PushString("..."); - some_where = true; - } - if (facet_type_info.builtin_constraint_mask.HasAnyOf( - SemIR::BuiltinConstraintMask::TypeCanDestroy)) { - if (some_where) { - step_stack_->PushString(" and"); - } - step_stack_->PushString(" .Self impls Core.CanDestroy"); - some_where = true; - } - for (auto rewrite : llvm::reverse(facet_type_info.rewrite_constraints)) { - if (some_where) { - step_stack_->PushString(" and"); - } - step_stack_->Push(" ", rewrite.lhs_id, " = ", rewrite.rhs_id); - some_where = true; - } - if (!facet_type_info.self_impls_constraints.empty() || - !facet_type_info.self_impls_named_constraints.empty()) { - if (some_where) { - step_stack_->PushString(" and"); - } - llvm::ListSeparator sep(" & "); - for (auto impls : - llvm::reverse(facet_type_info.self_impls_named_constraints)) { - step_stack_->Push(impls, &sep); - } - for (auto impls : llvm::reverse(facet_type_info.self_impls_constraints)) { - step_stack_->Push(impls, &sep); - } - step_stack_->PushString(" .Self impls "); - some_where = true; - } - // TODO: Other restrictions from facet_type_info. - if (some_where) { - step_stack_->PushString(" where"); - } - - // Output extend interface and named constraint requirements. - if (facet_type_info.extend_constraints.empty() && - facet_type_info.extend_named_constraints.empty()) { - step_stack_->PushString("type"); - return; - } - llvm::ListSeparator sep(" & "); - for (auto extend : - llvm::reverse(facet_type_info.extend_named_constraints)) { - step_stack_->Push(extend, &sep); - } - for (auto extend : llvm::reverse(facet_type_info.extend_constraints)) { - step_stack_->Push(extend, &sep); - } + step_stack_->PushFacetType(inst.facet_type_id); } auto StringifyInst(InstId /*inst_id*/, FacetValue inst) -> void { @@ -721,6 +668,67 @@ class Stringifier { *out_ << ""; } + auto StringifyFacetType(FacetTypeId facet_type_id) -> void { + const FacetTypeInfo& facet_type_info = + sem_ir_->facet_types().Get(facet_type_id); + // Output `where` restrictions. + bool some_where = false; + if (facet_type_info.other_requirements) { + step_stack_->PushString("..."); + some_where = true; + } + if (facet_type_info.builtin_constraint_mask.HasAnyOf( + SemIR::BuiltinConstraintMask::TypeCanDestroy)) { + if (some_where) { + step_stack_->PushString(" and"); + } + step_stack_->PushString(" .Self impls Core.CanDestroy"); + some_where = true; + } + for (auto rewrite : llvm::reverse(facet_type_info.rewrite_constraints)) { + if (some_where) { + step_stack_->PushString(" and"); + } + step_stack_->Push(" ", rewrite.lhs_id, " = ", rewrite.rhs_id); + some_where = true; + } + if (!facet_type_info.self_impls_constraints.empty() || + !facet_type_info.self_impls_named_constraints.empty()) { + if (some_where) { + step_stack_->PushString(" and"); + } + llvm::ListSeparator sep(" & "); + for (auto impls : + llvm::reverse(facet_type_info.self_impls_named_constraints)) { + step_stack_->Push(impls, &sep); + } + for (auto impls : llvm::reverse(facet_type_info.self_impls_constraints)) { + step_stack_->Push(impls, &sep); + } + step_stack_->PushString(" .Self impls "); + some_where = true; + } + // TODO: Other restrictions from facet_type_info. + if (some_where) { + step_stack_->PushString(" where"); + } + + // Output extend interface and named constraint requirements. + if (facet_type_info.extend_constraints.empty() && + facet_type_info.extend_named_constraints.empty()) { + step_stack_->PushString("type"); + return; + } + llvm::ListSeparator sep(" & "); + for (auto extend : + llvm::reverse(facet_type_info.extend_named_constraints)) { + step_stack_->Push(extend, &sep); + } + for (auto extend : llvm::reverse(facet_type_info.extend_constraints)) { + step_stack_->Push(extend, &sep); + } + } + private: const File* sem_ir_; StepStack* step_stack_; @@ -763,6 +771,9 @@ static auto Stringify(const File& sem_ir, StepStack& step_stack) case CARBON_KIND(ElementIndex element_index): out << element_index.index; break; + case CARBON_KIND(FacetTypeId facet_type_id): + stringifier.StringifyFacetType(facet_type_id); + break; } } @@ -841,4 +852,11 @@ auto StringifySpecificInterface(const File& sem_ir, } } +auto StringifyFacetType(const File& sem_ir, FacetTypeId facet_type_id) + -> std::string { + StepStack step_stack(&sem_ir); + step_stack.PushFacetType(facet_type_id); + return Stringify(sem_ir, step_stack); +} + } // namespace Carbon::SemIR diff --git a/toolchain/sem_ir/stringify.h b/toolchain/sem_ir/stringify.h index 300f8e429716..935b8fe4e124 100644 --- a/toolchain/sem_ir/stringify.h +++ b/toolchain/sem_ir/stringify.h @@ -26,7 +26,7 @@ auto StringifySpecific(const File& sem_ir, SpecificId specific_id) -> std::string; // Produces a string version of the name of a specific interface. If the -// interface is not generic, this is just the name of the interface. Otheewise, +// interface is not generic, this is just the name of the interface. Otherwise, // it is the interface name and its generic arguments. Generally, this should // not be called directly. To format a string into a diagnostic, use a // diagnostic parameter of type `SpecificInterface`. @@ -34,6 +34,12 @@ auto StringifySpecificInterface(const File& sem_ir, SpecificInterface specific_interface) -> std::string; +// Produces a string version of the facet type. This contains the name of the +// interfaces or named constraints that the facet type names, and any +// requirements such as rewrites. +auto StringifyFacetType(const File& sem_ir, FacetTypeId facet_type_id) + -> std::string; + } // namespace Carbon::SemIR #endif // CARBON_TOOLCHAIN_SEM_IR_STRINGIFY_H_