Skip to content

Commit

Permalink
Start adding extern logic for functions. (#3809)
Browse files Browse the repository at this point in the history 
This starts propagating is_extern on import, and warns when merging an
imported non-extern declaration with a local non-extern declaration.
Note this doesn't address import conflicts yet (i.e., two libraries
define an equivalent name) because they don't call merge logic.

On merge, I'm only setting values when new_is_definition because I think
it better matches the comment and resulting behavior. Note I now set
them even for bad redefinitions; I think this matches the comment, and
there's not a perfect choice here. I could change the flow back if
preferred.

Note this puts a spotlight on invalid nodes on imported decls, which I
think I'm going to need to address now. This isn't addressed by
ImportRef logic directly because the Function's decl_id is a
FunctionDecl, rather than the ImportRef that led to it. While I could
add the ImportRef link to each decl, I think adjusting the associated
NodeId is a better approach.
  • Loading branch information
@jonmeow
jonmeow authored Mar 27, 2024
1 parent 6c458ff commit a790271
Show file tree
Hide file tree
Showing 11 changed files with 748 additions and 163 deletions.
121 changes: 77 additions & 44 deletions toolchain/check/function.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,60 +191,93 @@ auto CheckFunctionTypeMatches(Context& context,
context.functions().Get(prev_function_id), substitutions);
}

// Checks to see if a structurally valid redeclaration is allowed in context.
// These all still merge.
static auto CheckIsAllowedRedecl(Context& context, Parse::NodeId node_id,
SemIR::Function& new_function,
bool new_is_definition,
SemIR::Function& prev_function,
bool prev_is_import) -> void {
CARBON_DIAGNOSTIC(FunctionPreviousDecl, Note, "Previously declared here.");
if (prev_is_import) {
// TODO: Allow non-extern declarations in the same library.
if (!new_function.is_extern && !prev_function.is_extern) {
CARBON_DIAGNOSTIC(
FunctionNonExternRedecl, Error,
"Only one library can declare function {0} without `extern`.",
SemIR::NameId);
context.emitter()
.Build(node_id, FunctionNonExternRedecl, prev_function.name_id)
.Note(prev_function.decl_id, FunctionPreviousDecl)
.Emit();
return;
}
} else {
if (!new_is_definition) {
CARBON_DIAGNOSTIC(FunctionRedecl, Error,
"Redundant redeclaration of function {0}.",
SemIR::NameId);
context.emitter()
.Build(node_id, FunctionRedecl, prev_function.name_id)
.Note(prev_function.decl_id, FunctionPreviousDecl)
.Emit();
return;
}
if (prev_function.definition_id.is_valid()) {
CARBON_DIAGNOSTIC(FunctionRedefinition, Error,
"Redefinition of function {0}.", SemIR::NameId);
CARBON_DIAGNOSTIC(FunctionPreviousDefinition, Note,
"Previously defined here.");
context.emitter()
.Build(node_id, FunctionRedefinition, prev_function.name_id)
.Note(prev_function.definition_id, FunctionPreviousDefinition)
.Emit();
return;
}
// `extern` definitions are prevented in handle_function.cpp; this is only
// checking for a non-`extern` definition after an `extern` declaration.
if (prev_function.is_extern) {
CARBON_DIAGNOSTIC(FunctionDefiningExtern, Error,
"Redeclaring `extern` function `{0}` as non-`extern`.",
SemIR::NameId);
CARBON_DIAGNOSTIC(FunctionPreviousExternDecl, Note,
"Previously declared `extern` here.");
context.emitter()
.Build(node_id, FunctionDefiningExtern, prev_function.name_id)
.Note(prev_function.decl_id, FunctionPreviousExternDecl)
.Emit();
return;
}
}
}

// TODO: Detect conflicting cross-file declarations, as well as uses of imported
// declarations followed by a redeclaration.
auto MergeFunctionRedecl(Context& context, Parse::NodeId node_id,
SemIR::Function& new_function,
SemIR::FunctionId prev_function_id, bool is_definition)
-> bool {
SemIR::Function& new_function, bool new_is_definition,
SemIR::FunctionId prev_function_id,
bool prev_is_import) -> bool {
auto& prev_function = context.functions().Get(prev_function_id);

if (!CheckRedecl(context, new_function, prev_function, {})) {
return false;
}

if (!is_definition) {
CARBON_DIAGNOSTIC(FunctionRedecl, Error,
"Redundant redeclaration of function {0}.",
SemIR::NameId);
CARBON_DIAGNOSTIC(FunctionPreviousDecl, Note, "Previously declared here.");
context.emitter()
.Build(node_id, FunctionRedecl, prev_function.name_id)
.Note(prev_function.decl_id, FunctionPreviousDecl)
.Emit();
// The diagnostic doesn't prevent a merge.
return true;
} else if (prev_function.definition_id.is_valid()) {
CARBON_DIAGNOSTIC(FunctionRedefinition, Error,
"Redefinition of function {0}.", SemIR::NameId);
CARBON_DIAGNOSTIC(FunctionPreviousDefinition, Note,
"Previously defined here.");
context.emitter()
.Build(node_id, FunctionRedefinition, prev_function.name_id)
.Note(prev_function.definition_id, FunctionPreviousDefinition)
.Emit();
// The second definition will be unused as a consequence of the error.
return true;
} else if (prev_function.is_extern) {
CARBON_DIAGNOSTIC(FunctionDefiningExtern, Error,
"Cannot define `extern` function `{0}`.", SemIR::NameId);
CARBON_DIAGNOSTIC(FunctionPreviousExternDecl, Note,
"Previously declared `extern` here.");
context.emitter()
.Build(node_id, FunctionDefiningExtern, prev_function.name_id)
.Note(prev_function.decl_id, FunctionPreviousExternDecl)
.Emit();
// The diagnostic doesn't prevent a merge.
return true;
}
CheckIsAllowedRedecl(context, node_id, new_function, new_is_definition,
prev_function, prev_is_import);

// Track the signature from the definition, so that IDs in the body
// match IDs in the signature.
prev_function.definition_id = new_function.definition_id;
prev_function.implicit_param_refs_id = new_function.implicit_param_refs_id;
prev_function.param_refs_id = new_function.param_refs_id;
prev_function.return_type_id = new_function.return_type_id;
prev_function.return_slot_id = new_function.return_slot_id;
if (new_is_definition) {
// Track the signature from the definition, so that IDs in the body
// match IDs in the signature.
prev_function.definition_id = new_function.definition_id;
prev_function.implicit_param_refs_id = new_function.implicit_param_refs_id;
prev_function.param_refs_id = new_function.param_refs_id;
prev_function.return_type_id = new_function.return_type_id;
prev_function.return_slot_id = new_function.return_slot_id;
}
if (!new_function.is_extern) {
prev_function.is_extern = false;
}
return true;
}

Expand Down
6 changes: 3 additions & 3 deletions toolchain/check/function.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ auto CheckFunctionTypeMatches(Context& context,
// If merging is successful, updates the FunctionId on new_function and returns
// true. Otherwise, returns false. Prints a diagnostic when appropriate.
auto MergeFunctionRedecl(Context& context, Parse::NodeId node_id,
SemIR::Function& new_function,
SemIR::FunctionId prev_function_id, bool is_definition)
-> bool;
SemIR::Function& new_function, bool new_is_definition,
SemIR::FunctionId prev_function_id,
bool prev_is_imported) -> bool;

} // namespace Carbon::Check

Expand Down
22 changes: 15 additions & 7 deletions toolchain/check/handle_function.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,22 +165,30 @@ static auto BuildFunctionDecl(Context& context,
}

// Check whether this is a redeclaration.
auto existing_id =
auto prev_id =
context.decl_name_stack().LookupOrAddName(name_context, lookup_result_id);
if (existing_id.is_valid()) {
if (auto existing_function_decl =
context.insts().Get(existing_id).TryAs<SemIR::FunctionDecl>()) {
if (MergeFunctionRedecl(context, node_id, function_info,
if (prev_id.is_valid()) {
auto prev_inst = context.insts().Get(prev_id);
bool prev_is_import = false;

if (prev_inst.Is<SemIR::ImportRefUsed>()) {
prev_inst =
context.insts().Get(context.constant_values().Get(prev_id).inst_id());
prev_is_import = true;
}

if (auto existing_function_decl = prev_inst.TryAs<SemIR::FunctionDecl>()) {
if (MergeFunctionRedecl(context, node_id, function_info, is_definition,
existing_function_decl->function_id,
is_definition)) {
prev_is_import)) {
// When merging, use the existing function rather than adding a new one.
function_decl.function_id = existing_function_decl->function_id;
}
} else {
// This is a redeclaration of something other than a function. This
// includes the case where an associated function redeclares another
// associated function.
context.DiagnoseDuplicateName(function_info.decl_id, existing_id);
context.DiagnoseDuplicateName(function_info.decl_id, prev_id);
}
}

Expand Down
3 changes: 3 additions & 0 deletions toolchain/check/import_ref.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -707,8 +707,11 @@ class ImportRefResolver {
GetLocalParamRefsId(function.param_refs_id, param_const_ids),
.return_type_id = new_return_type_id,
.return_slot_id = new_return_slot,
.is_extern = function.is_extern,
.builtin_kind = function.builtin_kind});
// Write the function ID into the FunctionDecl.
// TODO: The Invalid NodeId means it's hard to associate diagnostics. We
// should replace this.
context_.ReplaceInstBeforeConstantUse(
function_decl_id, {Parse::NodeId::Invalid, function_decl});
return {context_.constant_values().Get(function_decl_id)};
Expand Down
28 changes: 14 additions & 14 deletions toolchain/check/testdata/function/builtin/fail_redefined.carbon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,57 +43,57 @@ fn C(n: i32, m: i32) -> i32 = "int.add";
// CHECK:STDOUT: }
// CHECK:STDOUT: %A.loc7: <function> = fn_decl @A [template] {
// CHECK:STDOUT: %n.loc7_6.1: i32 = param n
// CHECK:STDOUT: @A.%n: i32 = bind_name n, %n.loc7_6.1
// CHECK:STDOUT: %n.loc7_6.2: i32 = bind_name n, %n.loc7_6.1
// CHECK:STDOUT: %m.loc7_14.1: i32 = param m
// CHECK:STDOUT: @A.%m: i32 = bind_name m, %m.loc7_14.1
// CHECK:STDOUT: %m.loc7_14.2: i32 = bind_name m, %m.loc7_14.1
// CHECK:STDOUT: %return.var.loc7: ref i32 = var <return slot>
// CHECK:STDOUT: }
// CHECK:STDOUT: %A.loc15: <function> = fn_decl @A [template] {
// CHECK:STDOUT: %n.loc15_6.1: i32 = param n
// CHECK:STDOUT: %n.loc15_6.2: i32 = bind_name n, %n.loc15_6.1
// CHECK:STDOUT: @A.%n: i32 = bind_name n, %n.loc15_6.1
// CHECK:STDOUT: %m.loc15_14.1: i32 = param m
// CHECK:STDOUT: %m.loc15_14.2: i32 = bind_name m, %m.loc15_14.1
// CHECK:STDOUT: @A.%m: i32 = bind_name m, %m.loc15_14.1
// CHECK:STDOUT: %return.var.loc15: ref i32 = var <return slot>
// CHECK:STDOUT: }
// CHECK:STDOUT: %B.loc17: <function> = fn_decl @B [template] {
// CHECK:STDOUT: %n.loc17_6.1: i32 = param n
// CHECK:STDOUT: @B.%n: i32 = bind_name n, %n.loc17_6.1
// CHECK:STDOUT: %n.loc17_6.2: i32 = bind_name n, %n.loc17_6.1
// CHECK:STDOUT: %m.loc17_14.1: i32 = param m
// CHECK:STDOUT: @B.%m: i32 = bind_name m, %m.loc17_14.1
// CHECK:STDOUT: %m.loc17_14.2: i32 = bind_name m, %m.loc17_14.1
// CHECK:STDOUT: %return.var.loc17: ref i32 = var <return slot>
// CHECK:STDOUT: }
// CHECK:STDOUT: %B.loc25: <function> = fn_decl @B [template] {
// CHECK:STDOUT: %n.loc25_6.1: i32 = param n
// CHECK:STDOUT: %n.loc25_6.2: i32 = bind_name n, %n.loc25_6.1
// CHECK:STDOUT: @B.%n: i32 = bind_name n, %n.loc25_6.1
// CHECK:STDOUT: %m.loc25_14.1: i32 = param m
// CHECK:STDOUT: %m.loc25_14.2: i32 = bind_name m, %m.loc25_14.1
// CHECK:STDOUT: @B.%m: i32 = bind_name m, %m.loc25_14.1
// CHECK:STDOUT: %return.var.loc25: ref i32 = var <return slot>
// CHECK:STDOUT: }
// CHECK:STDOUT: %C.loc27: <function> = fn_decl @C [template] {
// CHECK:STDOUT: %n.loc27_6.1: i32 = param n
// CHECK:STDOUT: @C.%n: i32 = bind_name n, %n.loc27_6.1
// CHECK:STDOUT: %n.loc27_6.2: i32 = bind_name n, %n.loc27_6.1
// CHECK:STDOUT: %m.loc27_14.1: i32 = param m
// CHECK:STDOUT: @C.%m: i32 = bind_name m, %m.loc27_14.1
// CHECK:STDOUT: %m.loc27_14.2: i32 = bind_name m, %m.loc27_14.1
// CHECK:STDOUT: %return.var.loc27: ref i32 = var <return slot>
// CHECK:STDOUT: }
// CHECK:STDOUT: %C.loc34: <function> = fn_decl @C [template] {
// CHECK:STDOUT: %n.loc34_6.1: i32 = param n
// CHECK:STDOUT: %n.loc34_6.2: i32 = bind_name n, %n.loc34_6.1
// CHECK:STDOUT: @C.%n: i32 = bind_name n, %n.loc34_6.1
// CHECK:STDOUT: %m.loc34_14.1: i32 = param m
// CHECK:STDOUT: %m.loc34_14.2: i32 = bind_name m, %m.loc34_14.1
// CHECK:STDOUT: @C.%m: i32 = bind_name m, %m.loc34_14.1
// CHECK:STDOUT: %return.var.loc34: ref i32 = var <return slot>
// CHECK:STDOUT: }
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @A(%n: i32, %m: i32) -> i32 = "int.add" {
// CHECK:STDOUT: !entry:
// CHECK:STDOUT: %n.ref: i32 = name_ref n, file.%n.loc15_6.2
// CHECK:STDOUT: %n.ref: i32 = name_ref n, %n
// CHECK:STDOUT: return %n.ref
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @B(%n: i32, %m: i32) -> i32 = "int.add" {
// CHECK:STDOUT: !entry:
// CHECK:STDOUT: %n.ref: i32 = name_ref n, %n
// CHECK:STDOUT: %n.ref: i32 = name_ref n, file.%n.loc17_6.2
// CHECK:STDOUT: return %n.ref
// CHECK:STDOUT: }
// CHECK:STDOUT:
Expand Down
Loading

0 comments on commit a790271

Please sign in to comment.