carbon-language/carbon-lang
1
0
Fork 0
mirror of https://github.com/carbon-language/carbon-lang.git synced 2026-10-04 12:11:04 +01:00
Code Issues Packages Projects Releases 10 Wiki Activity

Allow impl redecl in match_first after a definition (#7491)

Browse Source 
Previously the last decl had to be the definition. Now we allow a
declaration after a definition, so that the user can write a match_first
block last, and put (re-)declarations of impls in it, after the
definitions have already been written elsewhere.

We track the location of the decl that was associated with a match_first
block so that we can correctly point to it in diagnostics when an impl
is written twice in match_first blocks. Since impls may not be
redeclared across an import boundary, we will never have a `SemIR::Impl`
with a match_first from a different file in a redeclaration, so we don't
need to import the location of a previous decl that was in a match_first
for diagnostics. As such we just store a LocId on the `SemIR::Impl`
struct.
This commit is contained in:
Dana Jansens
2026-07-15 14:12:37 +00:00
committed by GitHub
parent 474090f439
commit 168420f805
7 changed files with 92 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files
toolchain/check/handle_impl.cpp
+19 -12
View File
@@ -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<SemIR::ImplId, SemIR::InstId> {
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());
toolchain/check/impl.cpp
+1 -1
View File
@@ -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,
toolchain/check/import_ref.cpp
+1
View File
@@ -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,
toolchain/check/testdata/match_first/basic.carbon
Vendored
+42 -25
View File
@@ -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: <elided>
// 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: <elided>
// 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: <elided>
// 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:
toolchain/sem_ir/formatter.cpp
+9
View File
@@ -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);
toolchain/sem_ir/impl.h
+10 -4
View File
@@ -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.
toolchain/sem_ir/typed_insts.h
+10 -10
View File
@@ -942,16 +942,6 @@ struct GenericNamedConstraintType {
SpecificId enclosing_specific_id;
};
// A `match_first` declaration.
struct MatchFirstDecl {
static constexpr auto Kind =
InstKind::MatchFirstDecl.Define<Parse::MatchFirstDefinitionStartId>(
{.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<Parse::AnyImplDeclId>(
@@ -1301,6 +1291,16 @@ struct MarkInPlaceInit {
DestInstId dest_id;
};
// A `match_first` declaration.
struct MatchFirstDecl {
static constexpr auto Kind =
InstKind::MatchFirstDecl.Define<Parse::MatchFirstDefinitionStartId>(
{.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.