diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index 9e68acc272c3..0f5b2c2f7f60 100644 --- a/toolchain/check/handle_impl.cpp +++ b/toolchain/check/handle_impl.cpp @@ -193,8 +193,10 @@ static auto PopImplIntroducerAndParamsAsNameComponent( } // Build an ImplDecl describing the signature of an impl. This handles the -// common logic shared by impl forward declarations and impl definitions. -static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id) +// common logic shared by impl forward declarations and impl definitions. It +// also sets the `definition_id` on the Impl structure. +static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id, + bool has_definition) -> std::pair { auto [constraint_node, constraint_id] = context.node_stack().PopExprWithNodeId(); @@ -269,6 +271,9 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id) .self_id = self_type_inst_id, .constraint_id = constraint_type_inst_id, .interface = specific_interface}}; + if (has_definition) { + impl.definition_id = impl_decl_id; + } // There's a bunch of places that may represent a diagnostic that occurred // in checking the impl up to this point, which we consolidate into this // bool. Due to lack of an instruction to set to `ErrorInst`, an @@ -306,6 +311,10 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id) auto& prev_impl = context.impls().Get(impl_id); FinishGenericRedecl(context, prev_impl.generic_id); + if (has_definition) { + prev_impl.definition_id = impl_decl_id; + } + if (auto match_first = context.match_first_context()) { if (prev_impl.match_first_id.has_value()) { if (!impl_had_error) { @@ -316,16 +325,18 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id) "previous declaration here"); context.emitter() .Build(node_id, ImplInTwoMatchFirst) - .Note(prev_impl.latest_decl_id(), ImplInTwoMatchFirstNote) + .Note(prev_impl.decl_loc_in_match_first, + ImplInTwoMatchFirstNote) .Emit(); } impl_had_error = true; } if (!impl_had_error) { - prev_impl.is_final = match_first->is_final; prev_impl.match_first_id = match_first->decl_id; + prev_impl.decl_loc_in_match_first = SemIR::LocId(impl_decl_id); prev_impl.match_first_position = match_first->block_size; + prev_impl.match_first_is_final = match_first->is_final; match_first->block_size += 1; } } @@ -362,12 +373,10 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id) impl.witness_block_id = context.inst_block_stack().Pop(); if (auto match_first = context.match_first_context()) { - // This should have been diagnosed above. - CARBON_CHECK(!impl.is_final); - - impl.is_final = match_first->is_final; impl.match_first_id = match_first->decl_id; + impl.decl_loc_in_match_first = SemIR::LocId(impl_decl_id); impl.match_first_position = match_first->block_size; + impl.match_first_is_final = match_first->is_final; match_first->block_size += 1; } } @@ -392,7 +401,7 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id) } auto HandleParseNode(Context& context, Parse::ImplDeclId node_id) -> bool { - auto [impl_id, impl_decl_id] = BuildImplDecl(context, node_id); + auto [impl_id, impl_decl_id] = BuildImplDecl(context, node_id, false); auto& impl = context.impls().Get(impl_id); context.decl_name_stack().PopScope(); @@ -409,13 +418,11 @@ auto HandleParseNode(Context& context, Parse::ImplDeclId node_id) -> bool { auto HandleParseNode(Context& context, Parse::ImplDefinitionStartId node_id) -> bool { - auto [impl_id, impl_decl_id] = BuildImplDecl(context, node_id); + auto [impl_id, impl_decl_id] = BuildImplDecl(context, node_id, true); auto& impl = context.impls().Get(impl_id); CheckRequireDeclsSatisfied(context, node_id, impl); - CARBON_CHECK(!impl.has_definition_started()); - impl.definition_id = impl_decl_id; impl.scope_id = context.name_scopes().Add(impl_decl_id, SemIR::NameId::None, context.decl_name_stack().PeekParentScopeId()); diff --git a/toolchain/check/impl.cpp b/toolchain/check/impl.cpp index 04e9a49a4f9a..2a7e28d1fe57 100644 --- a/toolchain/check/impl.cpp +++ b/toolchain/check/impl.cpp @@ -183,7 +183,7 @@ static auto VerifyImplRedecl(Context& context, const SemIR::Impl& new_impl, return ImplRedeclType::DiagnosedInvalidRedecl; } - if (prev_impl.has_definition_started()) { + if (new_impl.has_definition_started() && prev_impl.has_definition_started()) { // Impls aren't merged in order to avoid generic region lookup into a // mismatching table. CARBON_DIAGNOSTIC(ImplRedefinition, Error, diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index 62d86b68d6b3..6dc4c6529135 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -2760,6 +2760,7 @@ static auto ImportImplDecl(ImportContext& context, {.parent_scope_inst_id = SemIR::InstId::None, .is_final = import_impl.is_final, .match_first_id = SemIR::InstId::None, + .decl_loc_in_match_first = SemIR::LocId::None, .match_first_position = import_impl.match_first_position, .self_id = SemIR::TypeInstId::None, .constraint_id = SemIR::TypeInstId::None, diff --git a/toolchain/check/testdata/match_first/basic.carbon b/toolchain/check/testdata/match_first/basic.carbon index 24dd2ce3a2bc..f955b4d63d11 100644 --- a/toolchain/check/testdata/match_first/basic.carbon +++ b/toolchain/check/testdata/match_first/basic.carbon @@ -76,8 +76,8 @@ interface Z {} final impl () as Z; match_first { // This is not seen as a redecl of the `final impl`. It's not valid to write - // this `impl` since it overlaps the `final impl`, but we already have a - // diagnostic. + // an `impl` in `match_first` that overlaps a `final impl`, but we already + // have a diagnostic. impl () as Z {} } @@ -89,8 +89,8 @@ interface Z {} final impl () as Z {} match_first { // This is not seen as a redecl of the `final impl`. It's not valid to write - // this `impl` since it overlaps the `final impl`, but we already have a - // diagnostic. + // an `impl` in `match_first` that overlaps a `final impl`, but we already + // have a diagnostic. // CHECK:STDERR: fail_final_redecl_defined_once.carbon:[[@LINE+4]]:3: error: impl declared but not defined [ImplMissingDefinition] // CHECK:STDERR: impl () as Z; // CHECK:STDERR: ^~~~~~~~~~~~~ @@ -106,8 +106,8 @@ interface Z {} final impl () as Z {} match_first { // This is not seen as a redecl of the `final impl`. It's not valid to write - // this `impl` since it overlaps the `final impl`, but we already have a - // diagnostic. + // an `impl` in `match_first` that overlaps a `final impl`, but we already + // have a diagnostic. // CHECK:STDERR: fail_final_redecl_defined_twice.carbon:[[@LINE+7]]:3: error: `impl` will never be used [ImplFinalOverlapsNonFinal] // CHECK:STDERR: impl () as Z {} // CHECK:STDERR: ^~~~~~~~~~~~~~ @@ -125,9 +125,9 @@ interface Z {} final impl () as Z; match_first { - // This is not seen as a redecl of the `final impl`. It's not valid to write - // this `impl` since it overlaps the `final impl`, but we already have a - // diagnostic. + // This _is_ seen as a redecl of the `final impl`. It's not valid to write + // an `impl` in `match_first` that overlaps a `final impl`, but we already + // have a diagnostic. // CHECK:STDERR: fail_final_final_redecl.carbon:[[@LINE+7]]:3: error: `final impl` in `match_first` block [FinalImplInMatchFirst] // CHECK:STDERR: final impl () as Z {} // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~ @@ -184,21 +184,13 @@ match_first { } impl () as Z {} -// --- fail_todo_redecl_after_defined.carbon +// --- redecl_after_defined.carbon library "[[@TEST_NAME]]"; interface Z {} impl () as Z {} match_first { - // TODO: We want to allow a redecl in match_first after the definition. - // CHECK:STDERR: fail_todo_redecl_after_defined.carbon:[[@LINE+7]]:3: error: redefinition of `impl () as Z` [ImplRedefinition] - // CHECK:STDERR: impl () as Z; - // CHECK:STDERR: ^~~~~~~~~~~~~ - // CHECK:STDERR: fail_todo_redecl_after_defined.carbon:[[@LINE-6]]:1: note: previous definition was here [ImplPreviousDefinition] - // CHECK:STDERR: impl () as Z {} - // CHECK:STDERR: ^~~~~~~~~~~~~~ - // CHECK:STDERR: impl () as Z; } @@ -334,16 +326,35 @@ interface Z {} match_first { impl () as Z {} - // CHECK:STDERR: fail_redecl_after_defined_in_same_block.carbon:[[@LINE+7]]:3: error: redefinition of `impl () as Z` [ImplRedefinition] + // CHECK:STDERR: fail_redecl_after_defined_in_same_block.carbon:[[@LINE+7]]:3: error: impl declared in `match_first` more than once [ImplInTwoMatchFirst] // CHECK:STDERR: impl () as Z; // CHECK:STDERR: ^~~~~~~~~~~~~ - // CHECK:STDERR: fail_redecl_after_defined_in_same_block.carbon:[[@LINE-4]]:3: note: previous definition was here [ImplPreviousDefinition] + // CHECK:STDERR: fail_redecl_after_defined_in_same_block.carbon:[[@LINE-4]]:3: note: previous declaration here [ImplInTwoMatchFirstNote] // CHECK:STDERR: impl () as Z {} // CHECK:STDERR: ^~~~~~~~~~~~~~ // CHECK:STDERR: impl () as Z; } +// --- fail_two_redecl_in_same_block.carbon +library "[[@TEST_NAME]]"; + +interface Z {} + +impl () as Z {} +match_first { + impl () as Z; + // TODO: This "previous declaration" location is wrong. The redecl came after. + // CHECK:STDERR: fail_two_redecl_in_same_block.carbon:[[@LINE+7]]:3: error: impl declared in `match_first` more than once [ImplInTwoMatchFirst] + // CHECK:STDERR: impl () as Z; + // CHECK:STDERR: ^~~~~~~~~~~~~ + // CHECK:STDERR: fail_two_redecl_in_same_block.carbon:[[@LINE-5]]:3: note: previous declaration here [ImplInTwoMatchFirstNote] + // CHECK:STDERR: impl () as Z; + // CHECK:STDERR: ^~~~~~~~~~~~~ + // CHECK:STDERR: + impl () as Z; +} + // --- fail_redecl_in_different_block.carbon library "[[@TEST_NAME]]"; @@ -373,10 +384,10 @@ match_first { impl () as Z {} } match_first { - // CHECK:STDERR: fail_redecl_defined_in_different_block_firset.carbon:[[@LINE+7]]:3: error: redefinition of `impl () as Z` [ImplRedefinition] + // CHECK:STDERR: fail_redecl_defined_in_different_block_firset.carbon:[[@LINE+7]]:3: error: impl declared in `match_first` more than once [ImplInTwoMatchFirst] // CHECK:STDERR: impl () as Z; // CHECK:STDERR: ^~~~~~~~~~~~~ - // CHECK:STDERR: fail_redecl_defined_in_different_block_firset.carbon:[[@LINE-6]]:3: note: previous definition was here [ImplPreviousDefinition] + // CHECK:STDERR: fail_redecl_defined_in_different_block_firset.carbon:[[@LINE-6]]:3: note: previous declaration here [ImplInTwoMatchFirstNote] // CHECK:STDERR: impl () as Z {} // CHECK:STDERR: ^~~~~~~~~~~~~~ // CHECK:STDERR: @@ -460,6 +471,7 @@ final match_first { // CHECK:STDOUT: // CHECK:STDOUT: !members: // CHECK:STDOUT: extend %Z.ref.loc6 +// CHECK:STDOUT: match_first = position 0 // CHECK:STDOUT: witness = %Z.impl_witness // CHECK:STDOUT: } // CHECK:STDOUT: @@ -486,6 +498,7 @@ final match_first { // CHECK:STDOUT: // CHECK:STDOUT: !members: // CHECK:STDOUT: extend %Z.ref +// CHECK:STDOUT: match_first = position 0 // CHECK:STDOUT: witness = %Z.impl_witness // CHECK:STDOUT: } // CHECK:STDOUT: @@ -510,6 +523,7 @@ final match_first { // CHECK:STDOUT: // CHECK:STDOUT: !members: // CHECK:STDOUT: extend %Z.ref.loc5 +// CHECK:STDOUT: match_first = position 0 // CHECK:STDOUT: witness = %Z.impl_witness // CHECK:STDOUT: } // CHECK:STDOUT: @@ -529,11 +543,12 @@ final match_first { // CHECK:STDOUT: } // CHECK:STDOUT: } // CHECK:STDOUT: -// CHECK:STDOUT: final impl @empty_tuple.type.as.Z.impl: %.loc5_7.2 as %Z.ref.loc5 { +// CHECK:STDOUT: impl @empty_tuple.type.as.Z.impl: %.loc5_7.2 as %Z.ref.loc5 { // CHECK:STDOUT: // CHECK:STDOUT: // CHECK:STDOUT: !members: // CHECK:STDOUT: extend %Z.ref.loc5 +// CHECK:STDOUT: match_first = position 0, final // CHECK:STDOUT: witness = %Z.impl_witness // CHECK:STDOUT: } // CHECK:STDOUT: @@ -553,11 +568,12 @@ final match_first { // CHECK:STDOUT: } // CHECK:STDOUT: } // CHECK:STDOUT: -// CHECK:STDOUT: final impl @empty_tuple.type.as.Z.impl: %.loc6_9.2 as %Z.ref.loc6 { +// CHECK:STDOUT: impl @empty_tuple.type.as.Z.impl: %.loc6_9.2 as %Z.ref.loc6 { // CHECK:STDOUT: // CHECK:STDOUT: // CHECK:STDOUT: !members: // CHECK:STDOUT: extend %Z.ref.loc6 +// CHECK:STDOUT: match_first = position 0, final // CHECK:STDOUT: witness = %Z.impl_witness // CHECK:STDOUT: } // CHECK:STDOUT: @@ -577,11 +593,12 @@ final match_first { // CHECK:STDOUT: } // CHECK:STDOUT: } // CHECK:STDOUT: -// CHECK:STDOUT: final impl @empty_tuple.type.as.Z.impl: %.loc5_7.2 as %Z.ref.loc5 { +// CHECK:STDOUT: impl @empty_tuple.type.as.Z.impl: %.loc5_7.2 as %Z.ref.loc5 { // CHECK:STDOUT: // CHECK:STDOUT: // CHECK:STDOUT: !members: // CHECK:STDOUT: extend %Z.ref.loc5 +// CHECK:STDOUT: match_first = position 0, final // CHECK:STDOUT: witness = %Z.impl_witness // CHECK:STDOUT: } // CHECK:STDOUT: diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp index 4655e8221b75..e51711c5dd6b 100644 --- a/toolchain/sem_ir/formatter.cpp +++ b/toolchain/sem_ir/formatter.cpp @@ -518,6 +518,15 @@ auto Formatter::FormatImpl(ImplId id, const Impl& impl_info) -> void { FormatNameScope(impl_info.scope_id); } + if (impl_info.match_first_id.has_value()) { + Indent(); + out() << "match_first = position " << impl_info.match_first_position; + if (impl_info.match_first_is_final) { + out() << ", final"; + } + out() << "\n"; + } + Indent(); out() << "witness = "; FormatArg(impl_info.witness_id); diff --git a/toolchain/sem_ir/impl.h b/toolchain/sem_ir/impl.h index e3513323ba9d..2e3870d57bfc 100644 --- a/toolchain/sem_ir/impl.h +++ b/toolchain/sem_ir/impl.h @@ -22,18 +22,24 @@ struct ImplFields { // Note that this is None for an imported impl. InstId parent_scope_inst_id; + // Whether the impl declaration is marked `final`. This is false for impls in + // a `final match_first`. The `match_first_is_final` flag is used instead for + // that case. + bool is_final; + // The following members are set at the start of the impl declaration, and may // be modified at the start of redeclarations. - // Whether the impl declaration is marked `final`. - bool is_final; - // The `MatchFirstDecl` of the `match_first` block that the impl is associated // with. InstId match_first_id = SemIR::InstId::None; - + // The location of the `ImplDecl` that was associated with the + // `match_first_id`. Used for diagnostics, and not imported. + LocId decl_loc_in_match_first = SemIR::LocId::None; // The position of the impl in its associated `match_first` block. int match_first_position = 0; + // Whether the `match_first` block was modified as `final`. + bool match_first_is_final = false; // The following members always have values and do not change. diff --git a/toolchain/sem_ir/typed_insts.h b/toolchain/sem_ir/typed_insts.h index bef4f56bba2b..783c824b7373 100644 --- a/toolchain/sem_ir/typed_insts.h +++ b/toolchain/sem_ir/typed_insts.h @@ -942,16 +942,6 @@ struct GenericNamedConstraintType { SpecificId enclosing_specific_id; }; -// A `match_first` declaration. -struct MatchFirstDecl { - static constexpr auto Kind = - InstKind::MatchFirstDecl.Define( - {.ir_name = "match_first", - .constant_kind = InstConstantKind::AlwaysUnique, - .is_lowered = false}); - SemIR::InstId enclosing_scope_inst_id; -}; - // An `impl` declaration. struct ImplDecl { static constexpr auto Kind = InstKind::ImplDecl.Define( @@ -1301,6 +1291,16 @@ struct MarkInPlaceInit { DestInstId dest_id; }; +// A `match_first` declaration. +struct MatchFirstDecl { + static constexpr auto Kind = + InstKind::MatchFirstDecl.Define( + {.ir_name = "match_first", + .constant_kind = InstConstantKind::AlwaysUnique, + .is_lowered = false}); + SemIR::InstId enclosing_scope_inst_id; +}; + // A type that holds an object representation of another type, that may or may // not be a valid representation. In particular, it may also hold an unformed // state.