From bd2fa3ace7c7508fb3974fdb268ce9a4b5f1efe6 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Mon, 4 Nov 2024 08:47:40 -0800 Subject: [PATCH] Remove CalleeParamsInfo (#4452) I'm seeing three issues with CalleeParamsInfo: 1. Although ResolveCalleeInCall had been extracted out and the EntityWithParamsBase could be used directly, that had not been cleaned up. 2. implicit_param_refs_id is unused; param_refs_id was only used by ResolveCalleeInCall. The factoring as a struct seems to be obscuring what's used and what isn't (creating unnecessary copies). 3. On #4446, CalleeParamsInfo seemed to be obfuscating what ConvertCallArgs actually worked with (a Function, not a generic entity). I'm thinking that just removing CalleeParamsInfo is the best resolution here, it looks like it's tripping people up more than it's helping. Note, I think Function used to be named Callable, which was where the "callable" name originally came from (I might be wrong about this). But "function" seems clearer about the type now. --- toolchain/check/call.cpp | 21 +++++++++------------ toolchain/check/convert.cpp | 18 ++++++++++++------ toolchain/check/convert.h | 35 +++++++---------------------------- 3 files changed, 28 insertions(+), 46 deletions(-) diff --git a/toolchain/check/call.cpp b/toolchain/check/call.cpp index 2ad8317e51c7..57ad47acc868 100644 --- a/toolchain/check/call.cpp +++ b/toolchain/check/call.cpp @@ -46,10 +46,8 @@ static auto ResolveCalleeInCall(Context& context, SemIR::LocId loc_id, SemIR::InstId self_id, llvm::ArrayRef arg_ids) -> std::optional { - CalleeParamsInfo callee_info(entity); - // Check that the arity matches. - auto params = context.inst_blocks().GetOrEmpty(callee_info.param_refs_id); + auto params = context.inst_blocks().GetOrEmpty(entity.param_refs_id); if (arg_ids.size() != params.size()) { CARBON_DIAGNOSTIC(CallArgCountMismatch, Error, "{0} argument{0:s} passed to " @@ -64,7 +62,7 @@ static auto ResolveCalleeInCall(Context& context, SemIR::LocId loc_id, context.emitter() .Build(loc_id, CallArgCountMismatch, arg_ids.size(), static_cast(entity_kind_for_diagnostic), params.size()) - .Note(callee_info.callee_loc, InCallToEntity, + .Note(entity.latest_decl_id(), InCallToEntity, static_cast(entity_kind_for_diagnostic)) .Emit(); return std::nullopt; @@ -75,8 +73,8 @@ static auto ResolveCalleeInCall(Context& context, SemIR::LocId loc_id, if (entity_generic_id.is_valid()) { specific_id = DeduceGenericCallArguments( context, loc_id, entity_generic_id, enclosing_specific_id, - callee_info.implicit_param_patterns_id, callee_info.param_patterns_id, - self_id, arg_ids); + entity.implicit_param_patterns_id, entity.param_patterns_id, self_id, + arg_ids); if (!specific_id.is_valid()) { return std::nullopt; } @@ -153,12 +151,12 @@ auto PerformCall(Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id, } } } - auto& callable = context.functions().Get(callee_function.function_id); + auto& function = context.functions().Get(callee_function.function_id); // If the callee is a generic function, determine the generic argument values // for the call. auto callee_specific_id = ResolveCalleeInCall( - context, loc_id, callable, EntityKind::Function, callable.generic_id, + context, loc_id, function, EntityKind::Function, function.generic_id, callee_function.enclosing_specific_id, callee_function.self_id, arg_ids); if (!callee_specific_id) { return SemIR::InstId::BuiltinError; @@ -181,9 +179,9 @@ auto PerformCall(Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id, &context.emitter(), [&](auto& builder) { CARBON_DIAGNOSTIC(IncompleteReturnTypeHere, Note, "return type declared here"); - builder.Note(callable.return_slot_id, IncompleteReturnTypeHere); + builder.Note(function.return_slot_id, IncompleteReturnTypeHere); }); - return CheckFunctionReturnType(context, callee_id, callable, + return CheckFunctionReturnType(context, callee_id, function, *callee_specific_id); }(); switch (return_info.init_repr.kind) { @@ -212,8 +210,7 @@ auto PerformCall(Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id, // Convert the arguments to match the parameters. auto converted_args_id = ConvertCallArgs(context, loc_id, callee_function.self_id, arg_ids, - return_slot_arg_id, CalleeParamsInfo(callable), - callable.return_slot_pattern_id, *callee_specific_id); + return_slot_arg_id, function, *callee_specific_id); auto call_inst_id = context.AddInst(loc_id, {.type_id = return_info.type_id, .callee_id = callee_id, diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index ef4a5643dbc9..57bc923c15c6 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -1160,15 +1160,21 @@ auto ConvertForExplicitAs(Context& context, Parse::NodeId as_node, } // TODO: consider moving this to pattern_match.h -auto ConvertCallArgs( - Context& context, SemIR::LocId call_loc_id, SemIR::InstId self_id, - llvm::ArrayRef arg_refs, SemIR::InstId return_slot_arg_id, - const CalleeParamsInfo& callee, SemIR::InstId return_slot_pattern_id, - SemIR::SpecificId callee_specific_id) -> SemIR::InstBlockId { +auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id, + SemIR::InstId self_id, + llvm::ArrayRef arg_refs, + SemIR::InstId return_slot_arg_id, + const SemIR::Function& callee, + SemIR::SpecificId callee_specific_id) + -> SemIR::InstBlockId { + // The callee reference can be invalidated by conversions, so ensure all reads + // from it are done before conversion calls. + auto callee_decl_id = callee.latest_decl_id(); auto implicit_param_patterns = context.inst_blocks().GetOrEmpty(callee.implicit_param_patterns_id); auto param_patterns = context.inst_blocks().GetOrEmpty(callee.param_patterns_id); + auto return_slot_pattern_id = callee.return_slot_pattern_id; // The caller should have ensured this callee has the right arity. CARBON_CHECK(arg_refs.size() == param_patterns.size()); @@ -1190,7 +1196,7 @@ auto ConvertCallArgs( CARBON_DIAGNOSTIC(InCallToFunction, Note, "calling function declared here"); context.emitter() .Build(call_loc_id, MissingObjectInMethodCall) - .Note(callee.callee_loc, InCallToFunction) + .Note(callee_decl_id, InCallToFunction) .Emit(); self_id = SemIR::InstId::BuiltinError; } diff --git a/toolchain/check/convert.h b/toolchain/check/convert.h index 9dee887c1cba..86cf9fdf85d1 100644 --- a/toolchain/check/convert.h +++ b/toolchain/check/convert.h @@ -89,37 +89,16 @@ auto ConvertForExplicitAs(Context& context, Parse::NodeId as_node, SemIR::InstId value_id, SemIR::TypeId type_id) -> SemIR::InstId; -// Information about the syntactic parameters of a callee (excluding the return -// slot, for example). This information is extracted from the -// EntityWithParamsBase before calling ConvertCallArgs, because conversion can -// trigger importing of more entities, which can invalidate the reference to the -// callee. -struct CalleeParamsInfo { - explicit CalleeParamsInfo(const SemIR::EntityWithParamsBase& callee) - : callee_loc(callee.latest_decl_id()), - implicit_param_refs_id(callee.implicit_param_refs_id), - implicit_param_patterns_id(callee.implicit_param_patterns_id), - param_refs_id(callee.param_refs_id), - param_patterns_id(callee.param_patterns_id) {} - - // The location of the callee to use in diagnostics. - SemIRLoc callee_loc; - // The implicit parameters of the callee. - SemIR::InstBlockId implicit_param_refs_id; - SemIR::InstBlockId implicit_param_patterns_id; - // The explicit parameters of the callee. - SemIR::InstBlockId param_refs_id; - SemIR::InstBlockId param_patterns_id; -}; - // Implicitly converts a set of arguments to match the parameter types in a // function call. Returns a block containing the converted implicit and explicit // argument values for runtime parameters. -auto ConvertCallArgs( - Context& context, SemIR::LocId call_loc_id, SemIR::InstId self_id, - llvm::ArrayRef arg_refs, SemIR::InstId return_slot_arg_id, - const CalleeParamsInfo& callee, SemIR::InstId return_slot_pattern_id, - SemIR::SpecificId callee_specific_id) -> SemIR::InstBlockId; +auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id, + SemIR::InstId self_id, + llvm::ArrayRef arg_refs, + SemIR::InstId return_slot_arg_id, + const SemIR::Function& callee, + SemIR::SpecificId callee_specific_id) + -> SemIR::InstBlockId; // A type that has been converted for use as a type expression. struct TypeExpr {