Skip to content

Commit

Permalink
Refactor CheckIsAllowedRedecl and stop function definition merging (#…
Browse files Browse the repository at this point in the history
…4800)

Rename `CheckIsAllowedRedecl` to `DiagnoseIfInvalidRedecl` to try to
better document behavior, and clean up comments.

This extends the no-merge-if-defined behavior to functions. It was
already the case for class/interface, and just added for impl, so if
anything functions were now inconsistent. I was kind of tempted to make
a helper for it, but I didn't think of a great structure/name to get
there: `DiagnoseRedef` isn't always called when it's a redefinition, for
example due to `extern` diagnostics, it's hard to combine.

Cleans up `is_defined` calls to rely more on `has_definition_started`,
removing some code paths that are unused since definitions aren't
merged.
  • Loading branch information
jonmeow authored Jan 14, 2025
1 parent 5b70a3e commit d958caa
Showing 15 changed files with 195 additions and 142 deletions.
15 changes: 7 additions & 8 deletions toolchain/check/handle_class.cpp
Original file line number Diff line number Diff line change
@@ -65,13 +65,13 @@ static auto MergeClassRedecl(Context& context, SemIRLoc new_loc,
return false;
}

CheckIsAllowedRedecl(
DiagnoseIfInvalidRedecl(
context, Lex::TokenKind::Class, prev_class.name_id,
RedeclInfo(new_class, new_loc, new_is_definition),
RedeclInfo(prev_class, prev_loc, prev_class.is_defined()),
RedeclInfo(prev_class, prev_loc, prev_class.has_definition_started()),
prev_import_ir_id);

if (new_is_definition && prev_class.is_defined()) {
if (new_is_definition && prev_class.has_definition_started()) {
// Don't attempt to merge multiple definitions.
return false;
}
@@ -280,11 +280,10 @@ auto HandleParseNode(Context& context, Parse::ClassDefinitionStartId node_id)
auto& class_info = context.classes().Get(class_id);

// Track that this declaration is the definition.
if (!class_info.is_defined()) {
class_info.definition_id = class_decl_id;
class_info.scope_id = context.name_scopes().Add(
class_decl_id, SemIR::NameId::Invalid, class_info.parent_scope_id);
}
CARBON_CHECK(!class_info.has_definition_started());
class_info.definition_id = class_decl_id;
class_info.scope_id = context.name_scopes().Add(
class_decl_id, SemIR::NameId::Invalid, class_info.parent_scope_id);

// Enter the class scope.
context.scope_stack().Push(
14 changes: 9 additions & 5 deletions toolchain/check/handle_function.cpp
Original file line number Diff line number Diff line change
@@ -88,11 +88,15 @@ static auto MergeFunctionRedecl(Context& context, SemIRLoc new_loc,
return false;
}

CheckIsAllowedRedecl(context, Lex::TokenKind::Fn, prev_function.name_id,
RedeclInfo(new_function, new_loc, new_is_definition),
RedeclInfo(prev_function, prev_function.latest_decl_id(),
prev_function.has_definition_started()),
prev_import_ir_id);
DiagnoseIfInvalidRedecl(
context, Lex::TokenKind::Fn, prev_function.name_id,
RedeclInfo(new_function, new_loc, new_is_definition),
RedeclInfo(prev_function, prev_function.latest_decl_id(),
prev_function.has_definition_started()),
prev_import_ir_id);
if (new_is_definition && prev_function.has_definition_started()) {
return false;
}

if (!prev_function.first_owning_decl_id.is_valid()) {
prev_function.first_owning_decl_id = new_function.first_owning_decl_id;
13 changes: 4 additions & 9 deletions toolchain/check/handle_impl.cpp
Original file line number Diff line number Diff line change
@@ -453,10 +453,7 @@ auto HandleParseNode(Context& context, Parse::ImplDefinitionStartId node_id)
impl_decl_id, impl_info.scope_id,
context.generics().GetSelfSpecific(impl_info.generic_id));
StartGenericDefinition(context);

if (!impl_info.is_defined()) {
ImplWitnessStartDefinition(context, impl_info);
}
ImplWitnessStartDefinition(context, impl_info);
context.inst_block_stack().Push();
context.node_stack().Push(node_id, impl_id);

@@ -479,11 +476,9 @@ auto HandleParseNode(Context& context, Parse::ImplDefinitionId /*node_id*/)
context.node_stack().Pop<Parse::NodeKind::ImplDefinitionStart>();

auto& impl_info = context.impls().Get(impl_id);
if (!impl_info.is_defined()) {
FinishImplWitness(context, impl_info);
impl_info.defined = true;
}

CARBON_CHECK(!impl_info.is_defined());
FinishImplWitness(context, impl_info);
impl_info.defined = true;
FinishGenericDefinition(context, impl_info.generic_id);

context.inst_block_stack().Pop();
53 changes: 25 additions & 28 deletions toolchain/check/handle_interface.cpp
Original file line number Diff line number Diff line change
@@ -79,16 +79,15 @@ static auto BuildInterfaceDecl(Context& context,
// TODO: This should be refactored a little, particularly for
// prev_import_ir_id. See similar logic for classes and functions, which
// might also be refactored to merge.
CheckIsAllowedRedecl(
DiagnoseIfInvalidRedecl(
context, Lex::TokenKind::Interface, existing_interface.name_id,
RedeclInfo(interface_info, node_id, is_definition),
RedeclInfo(existing_interface, existing_interface.latest_decl_id(),
existing_interface.is_defined()),
existing_interface.has_definition_started()),
/*prev_import_ir_id=*/SemIR::ImportIRId::Invalid);

// Can't merge interface definitions due to the generic requirements.
// TODO: Should this also be mirrored to classes/functions for generics?
if (!is_definition || !existing_interface.is_defined()) {
if (!is_definition || !existing_interface.has_definition_started()) {
// This is a redeclaration of an existing interface.
interface_decl.interface_id = existing_interface_decl->interface_id;
interface_decl.type_id = existing_interface_decl->type_id;
@@ -140,7 +139,7 @@ auto HandleParseNode(Context& context,
auto& interface_info = context.interfaces().Get(interface_id);

// Track that this declaration is the definition.
CARBON_CHECK(!interface_info.is_defined(),
CARBON_CHECK(!interface_info.has_definition_started(),
"Can't merge with defined interfaces.");
interface_info.definition_id = interface_decl_id;
interface_info.scope_id =
@@ -159,29 +158,27 @@ auto HandleParseNode(Context& context,
context.args_type_info_stack().Push();

// Declare and introduce `Self`.
if (!interface_info.is_defined()) {
SemIR::FacetType facet_type =
context.FacetTypeFromInterface(interface_id, self_specific_id);
SemIR::TypeId self_type_id = context.GetTypeIdForTypeConstant(
TryEvalInst(context, SemIR::InstId::Invalid, facet_type));

// We model `Self` as a symbolic binding whose type is the interface.
// Because there is no equivalent non-symbolic value, we use `Invalid` as
// the `value_id` on the `BindSymbolicName`.
auto entity_name_id = context.entity_names().Add(
{.name_id = SemIR::NameId::SelfType,
.parent_scope_id = interface_info.scope_id,
.bind_index = context.scope_stack().AddCompileTimeBinding()});
interface_info.self_param_id =
context.AddInst(SemIR::LocIdAndInst::NoLoc<SemIR::BindSymbolicName>(
{.type_id = self_type_id,
.entity_name_id = entity_name_id,
.value_id = SemIR::InstId::Invalid}));
context.scope_stack().PushCompileTimeBinding(interface_info.self_param_id);
context.name_scopes().AddRequiredName(interface_info.scope_id,
SemIR::NameId::SelfType,
interface_info.self_param_id);
}
SemIR::FacetType facet_type =
context.FacetTypeFromInterface(interface_id, self_specific_id);
SemIR::TypeId self_type_id = context.GetTypeIdForTypeConstant(
TryEvalInst(context, SemIR::InstId::Invalid, facet_type));

// We model `Self` as a symbolic binding whose type is the interface.
// Because there is no equivalent non-symbolic value, we use `Invalid` as
// the `value_id` on the `BindSymbolicName`.
auto entity_name_id = context.entity_names().Add(
{.name_id = SemIR::NameId::SelfType,
.parent_scope_id = interface_info.scope_id,
.bind_index = context.scope_stack().AddCompileTimeBinding()});
interface_info.self_param_id =
context.AddInst(SemIR::LocIdAndInst::NoLoc<SemIR::BindSymbolicName>(
{.type_id = self_type_id,
.entity_name_id = entity_name_id,
.value_id = SemIR::InstId::Invalid}));
context.scope_stack().PushCompileTimeBinding(interface_info.self_param_id);
context.name_scopes().AddRequiredName(interface_info.scope_id,
SemIR::NameId::SelfType,
interface_info.self_param_id);

// Enter the interface scope.
context.scope_stack().Push(interface_decl_id, interface_info.scope_id,
10 changes: 4 additions & 6 deletions toolchain/check/merge.cpp
Original file line number Diff line number Diff line change
@@ -91,12 +91,10 @@ auto DiagnoseExternRequiresDeclInApiFile(Context& context, SemIRLoc loc)
context.emitter().Build(loc, ExternRequiresDeclInApiFile).Emit();
}

// Checks to see if a structurally valid redeclaration is allowed in context.
// These all still merge.
auto CheckIsAllowedRedecl(Context& context, Lex::TokenKind decl_kind,
SemIR::NameId name_id, RedeclInfo new_decl,
RedeclInfo prev_decl, SemIR::ImportIRId import_ir_id)
-> void {
auto DiagnoseIfInvalidRedecl(Context& context, Lex::TokenKind decl_kind,
SemIR::NameId name_id, RedeclInfo new_decl,
RedeclInfo prev_decl,
SemIR::ImportIRId import_ir_id) -> void {
if (!import_ir_id.is_valid()) {
// Check for disallowed redeclarations in the same file.
if (!new_decl.is_definition) {
15 changes: 8 additions & 7 deletions toolchain/check/merge.h
Original file line number Diff line number Diff line change
@@ -16,7 +16,7 @@ namespace Carbon::Check {
auto DiagnoseExternRequiresDeclInApiFile(Context& context, SemIRLoc loc)
-> void;

// Information on new and previous declarations for CheckIsAllowedRedecl.
// Information on new and previous declarations for DiagnoseIfInvalidRedecl.
struct RedeclInfo {
explicit RedeclInfo(SemIR::EntityWithParamsBase params, SemIRLoc loc,
bool is_definition)
@@ -35,18 +35,19 @@ struct RedeclInfo {
SemIR::LibraryNameId extern_library_id;
};

// Checks if a redeclaration is allowed prior to merging. This may emit a
// diagnostic, but diagnostics do not prevent merging.
// Checks for various invalid redeclarations. This can emit diagnostics.
// However, merging is still often appropriate for error recovery, so this
// doesn't return whether a diagnostic occurred.
//
// The kinds of things this verifies are:
// - A declaration is not redundant.
// - A definition doesn't redefine a prior definition.
// - The use of `extern` is consistent within a library.
// - Multiple libraries do not declare non-`extern`.
auto CheckIsAllowedRedecl(Context& context, Lex::TokenKind decl_kind,
SemIR::NameId name_id, RedeclInfo new_decl,
RedeclInfo prev_decl,
SemIR::ImportIRId prev_import_ir_id) -> void;
auto DiagnoseIfInvalidRedecl(Context& context, Lex::TokenKind decl_kind,
SemIR::NameId name_id, RedeclInfo new_decl,
RedeclInfo prev_decl,
SemIR::ImportIRId prev_import_ir_id) -> void;

// When the prior name lookup result is an import and we are successfully
// merging, replace the name lookup result with the reference in the current
13 changes: 10 additions & 3 deletions toolchain/check/testdata/class/fail_method_redefinition.carbon
Original file line number Diff line number Diff line change
@@ -25,6 +25,8 @@ class Class {
// CHECK:STDOUT: %Class: type = class_type @Class [template]
// CHECK:STDOUT: %F.type: type = fn_type @F [template]
// CHECK:STDOUT: %F: %F.type = struct_value () [template]
// CHECK:STDOUT: %.type: type = fn_type @.1 [template]
// CHECK:STDOUT: %.d22: %.type = struct_value () [template]
// CHECK:STDOUT: %empty_struct_type: type = struct_type {} [template]
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %empty_struct_type [template]
// CHECK:STDOUT: }
@@ -46,13 +48,13 @@ class Class {
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @Class {
// CHECK:STDOUT: %F.decl.loc12: %F.type = fn_decl @F [template = constants.%F] {} {}
// CHECK:STDOUT: %F.decl.loc19: %F.type = fn_decl @F [template = constants.%F] {} {}
// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} {}
// CHECK:STDOUT: %.decl: %.type = fn_decl @.1 [template = constants.%.d22] {} {}
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %empty_struct_type [template = constants.%complete_type]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%Class
// CHECK:STDOUT: .F = %F.decl.loc12
// CHECK:STDOUT: .F = %F.decl
// CHECK:STDOUT: complete_type_witness = %complete_type
// CHECK:STDOUT: }
// CHECK:STDOUT:
@@ -61,3 +63,8 @@ class Class {
// CHECK:STDOUT: return
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @.1() {
// CHECK:STDOUT: !entry:
// CHECK:STDOUT: return
// CHECK:STDOUT: }
// CHECK:STDOUT:
15 changes: 11 additions & 4 deletions toolchain/check/testdata/class/fail_redefinition.carbon
Original file line number Diff line number Diff line change
@@ -54,7 +54,7 @@ fn Class.I() {}
// CHECK:STDOUT: %I.c9a: %I.type.2b6 = struct_value () [template]
// CHECK:STDOUT: %empty_struct_type: type = struct_type {} [template]
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %empty_struct_type [template]
// CHECK:STDOUT: %.a95: type = class_type @.1 [template]
// CHECK:STDOUT: %.a95: type = class_type @.2 [template]
// CHECK:STDOUT: %G.type.bf6: type = fn_type @G.1 [template]
// CHECK:STDOUT: %G.e39: %G.type.bf6 = struct_value () [template]
// CHECK:STDOUT: %H.type.e2f: type = fn_type @H.2 [template]
@@ -63,6 +63,8 @@ fn Class.I() {}
// CHECK:STDOUT: %I.a7f: %I.type.b27 = struct_value () [template]
// CHECK:STDOUT: %G.type.621: type = fn_type @G.2 [template]
// CHECK:STDOUT: %G.f0c: %G.type.621 = struct_value () [template]
// CHECK:STDOUT: %.type: type = fn_type @.1 [template]
// CHECK:STDOUT: %.d22: %.type = struct_value () [template]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: imports {
@@ -79,11 +81,11 @@ fn Class.I() {}
// CHECK:STDOUT: }
// CHECK:STDOUT: %Core.import = import Core
// CHECK:STDOUT: %Class.decl: type = class_decl @Class [template = constants.%Class] {} {}
// CHECK:STDOUT: %.decl: type = class_decl @.1 [template = constants.%.a95] {} {}
// CHECK:STDOUT: %.decl.loc24: type = class_decl @.2 [template = constants.%.a95] {} {}
// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} {}
// CHECK:STDOUT: %G.decl: %G.type.621 = fn_decl @G.2 [template = constants.%G.f0c] {} {}
// CHECK:STDOUT: %H.decl: %H.type.91d = fn_decl @H.1 [template = constants.%H.d38] {} {}
// CHECK:STDOUT: %I.decl: %I.type.2b6 = fn_decl @I.1 [template = constants.%I.c9a] {} {}
// CHECK:STDOUT: %.decl.loc43: %.type = fn_decl @.1 [template = constants.%.d22] {} {}
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @Class {
@@ -101,7 +103,7 @@ fn Class.I() {}
// CHECK:STDOUT: complete_type_witness = %complete_type
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @.1 {
// CHECK:STDOUT: class @.2 {
// CHECK:STDOUT: %G.decl: %G.type.bf6 = fn_decl @G.1 [template = constants.%G.e39] {} {}
// CHECK:STDOUT: %H.decl: %H.type.e2f = fn_decl @H.2 [template = constants.%H.382] {} {}
// CHECK:STDOUT: %I.decl: %I.type.b27 = fn_decl @I.2 [template = constants.%I.a7f] {} {}
@@ -144,3 +146,8 @@ fn Class.I() {}
// CHECK:STDOUT: return
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @.1() {
// CHECK:STDOUT: !entry:
// CHECK:STDOUT: return
// CHECK:STDOUT: }
// CHECK:STDOUT:
Loading

0 comments on commit d958caa

Please sign in to comment.