From aa71f317877f4012fd7a6f14fbd1d185126a0f3c Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Thu, 13 Feb 2025 11:07:17 -0800 Subject: [PATCH] Refactor implicit Self param into a member on SemIR::Function (#4928) This ensures the data is available for more uses (specifically for diagnosing virtual/abstract/impl functions on non-instance methods). It still doesn't quite address the TODO to move the Self param search all the way back to the param walk in all cases. To do that in the case that still has a separate search loop, I think we'd have to change `Check::NameComponent` to carry this information (as it carries the implicit_param_patterns-id) - though there's comments in NameComponent suggesting it shouldn't carry function-specific things like `call_params_id` and `return_slot_pattern_id` - so I wasn't sure if it was suitable to add more there, but I can - possibly in a follow-up change. --------- Co-authored-by: Jon Ross-Perkins --- toolchain/check/convert.cpp | 17 ++--------------- toolchain/check/handle_function.cpp | 20 +++++++++++++++++++- toolchain/check/import_ref.cpp | 16 ++++++++++++++-- toolchain/sem_ir/function.h | 4 ++++ 4 files changed, 39 insertions(+), 18 deletions(-) diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index 59daad64b98b..19a8783ccb6d 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -1319,8 +1319,6 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id, // 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; @@ -1328,18 +1326,7 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id, // The caller should have ensured this callee has the right arity. CARBON_CHECK(arg_refs.size() == param_patterns.size()); - // Find self parameter pattern. - // TODO: Do this during initial traversal of implicit params. - auto self_param_id = SemIR::InstId::None; - for (auto implicit_param_id : implicit_param_patterns) { - if (SemIR::Function::GetNameFromPatternId( - context.sem_ir(), implicit_param_id) == SemIR::NameId::SelfValue) { - CARBON_CHECK(!self_param_id.has_value()); - self_param_id = implicit_param_id; - } - } - - if (self_param_id.has_value() && !self_id.has_value()) { + if (callee.self_param_id.has_value() && !self_id.has_value()) { CARBON_DIAGNOSTIC(MissingObjectInMethodCall, Error, "missing object argument in method call"); CARBON_DIAGNOSTIC(InCallToFunction, Note, "calling function declared here"); @@ -1350,7 +1337,7 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id, self_id = SemIR::ErrorInst::SingletonInstId; } - return CallerPatternMatch(context, callee_specific_id, self_param_id, + return CallerPatternMatch(context, callee_specific_id, callee.self_param_id, callee.param_patterns_id, return_slot_pattern_id, self_id, arg_refs, return_slot_arg_id); } diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index 776c7d9ebd6a..02c41743e1a1 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -125,6 +125,7 @@ static auto MergeFunctionRedecl(Context& context, // match IDs in the signature. prev_function.MergeDefinition(new_function); prev_function.return_slot_pattern_id = new_function.return_slot_pattern_id; + prev_function.self_param_id = new_function.self_param_id; } if (prev_import_ir_id.has_value()) { ReplacePrevInstForMerge(context, new_function.parent_scope_id, @@ -258,13 +259,30 @@ static auto BuildFunctionDecl(Context& context, auto decl_id = context.AddPlaceholderInst(SemIR::LocIdAndInst(node_id, function_decl)); + // Find self parameter pattern. + // TODO: Do this during initial traversal of implicit params. + auto self_param_id = SemIR::InstId::None; + auto implicit_param_patterns = + context.inst_blocks().GetOrEmpty(name.implicit_param_patterns_id); + if (const auto* i = + llvm::find_if(implicit_param_patterns, + [&](auto implicit_param_id) { + return SemIR::Function::GetNameFromPatternId( + context.sem_ir(), implicit_param_id) == + SemIR::NameId::SelfValue; + }); + i != implicit_param_patterns.end()) { + self_param_id = *i; + } + // Build the function entity. This will be merged into an existing function if // there is one, or otherwise added to the function store. auto function_info = SemIR::Function{{name_context.MakeEntityWithParamsBase( name, decl_id, is_extern, introducer.extern_library)}, {.return_slot_pattern_id = name.return_slot_pattern_id, - .virtual_modifier = virtual_modifier}}; + .virtual_modifier = virtual_modifier, + .self_param_id = self_param_id}}; if (is_definition) { function_info.definition_id = decl_id; } diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index b6cde2c5c9cb..02e807622ebb 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -986,9 +986,14 @@ static auto LoadLocalPatternConstantIds(ImportRefResolver& resolver, // take a holistic look at how to balance those concerns. For example, // could the same function be used to load the constants and use them, with // a parameter to select between the two? +// +// `self_param_id` is an optional out parameter, populated with the InstId in +// the resulting parameter patterns that represents the Self parameter. static auto GetLocalParamPatternsId(ImportContext& context, - SemIR::InstBlockId param_patterns_id) + SemIR::InstBlockId param_patterns_id, + SemIR::InstId* self_param_id = nullptr) -> SemIR::InstBlockId { + CARBON_CHECK(!self_param_id || !self_param_id->has_value()); if (!param_patterns_id.has_value() || param_patterns_id == SemIR::InstBlockId::Empty) { return param_patterns_id; @@ -1064,6 +1069,11 @@ static auto GetLocalParamPatternsId(ImportContext& context, AddImportIRInst(context, addr_pattern_id), {.type_id = type_id, .inner_id = new_param_id})); } + if (self_param_id && + context.import_entity_names().Get(binding.entity_name_id).name_id == + SemIR::NameId::SelfValue) { + *self_param_id = new_param_id; + } new_patterns.push_back(new_param_id); } return context.local_inst_blocks().Add(new_patterns); @@ -1934,8 +1944,10 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver, // Add the function declaration. new_function.parent_scope_id = parent_scope_id; + SemIR::InstId self_param_id = SemIR::InstId::None; new_function.implicit_param_patterns_id = GetLocalParamPatternsId( - resolver, import_function.implicit_param_patterns_id); + resolver, import_function.implicit_param_patterns_id, &self_param_id); + new_function.self_param_id = self_param_id; new_function.param_patterns_id = GetLocalParamPatternsId(resolver, import_function.param_patterns_id); new_function.return_slot_pattern_id = GetLocalReturnSlotPatternId( diff --git a/toolchain/sem_ir/function.h b/toolchain/sem_ir/function.h index fe35b0a1966a..113303195692 100644 --- a/toolchain/sem_ir/function.h +++ b/toolchain/sem_ir/function.h @@ -31,6 +31,10 @@ struct FunctionFields { // this function. VirtualModifier virtual_modifier; + // The implicit self parameter, if any, in implicit_param_patterns_id from + // EntityWithParamsBase. + InstId self_param_id = SemIR::InstId::None; + // The following member is set on the first call to the function, or at the // point where the function is defined.