From f3d72b993aa42607887ca7fefa3b7f08b415d796 Mon Sep 17 00:00:00 2001 From: danakj Date: Wed, 5 Feb 2025 13:30:50 -0500 Subject: [PATCH 1/2] Add a test and check-support for positional params with a return type Functions with positional parameters omit any implicit or explicit parameter lists. This causes them to not have a pattern block, which crashes if there is a return type that needs to add to the pattern block. Add a test covering this and handle it by having the ReturnTypeId handler peek at the node stack and conditionally add the missing pattern block. To do so it looks to see if the previous node is a `IdentifierNameNotBeforeParams` which implies it was not expecting a pattern (since there are no params) and thus the pattern block was not added to the stack. Note that lambdas also allow functions to omit an identifier, which will need a pattern block on the stack for implicit parameters, explicit parameters or a return type, without seeing any IdentifierName-like parse nodes. To handle this, we will need to look for additional nodes in the future and add the missing pattern block to the stack - possibly for the FunctionInitializer, but the parse support needs to be created for lambdas first. --- toolchain/check/handle_function.cpp | 17 ++++++++++ toolchain/check/name_component.cpp | 3 +- .../no_prelude/fail_todo_no_params.carbon | 34 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index 566427356b08f..c9116b283bb38 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -26,6 +26,14 @@ namespace Carbon::Check { auto HandleParseNode(Context& context, Parse::FunctionIntroducerId node_id) -> bool { + // Normally the `IdentifierNameBeforeParamsId` node starts these blocks, but a + // `fn` may or may not have an identifier or parameters or a return type. And + // the pattern stack needs to exist if there are either parameters or a return + // type. + context.pattern_block_stack().Push(); + context.full_pattern_stack().PushFullPattern( + FullPatternStack::Kind::ImplicitParamList); + // Create an instruction block to hold the instructions created as part of the // function signature, such as parameter and return types. context.inst_block_stack().Push(); @@ -185,6 +193,15 @@ static auto BuildFunctionDecl(Context& context, } auto name = PopNameComponent(context, return_slot_pattern_id); + if (name.param_patterns_id.has_value() || + !name.pattern_block_id.has_value()) { + // If there were parameters then pattern stack entries were added from the + // parameters, and we can drop the entries from the FunctionIntroducer. If + // there were no parameters, but the pattern block was not used for a return + // type, then we can also drop them. + context.pattern_block_stack().PopAndDiscard(); + context.full_pattern_stack().PopFullPattern(); + } if (!name.param_patterns_id.has_value()) { context.TODO(node_id, "function with positional parameters"); name.param_patterns_id = SemIR::InstBlockId::Empty; diff --git a/toolchain/check/name_component.cpp b/toolchain/check/name_component.cpp index 43593e906bd31..0535c26783ad5 100644 --- a/toolchain/check/name_component.cpp +++ b/toolchain/check/name_component.cpp @@ -46,7 +46,8 @@ auto PopNameComponent(Context& context, SemIR::InstId return_slot_pattern_id) auto call_params_id = SemIR::InstBlockId::None; auto pattern_block_id = SemIR::InstBlockId::None; if (param_patterns_id->has_value() || - implicit_param_patterns_id->has_value()) { + implicit_param_patterns_id->has_value() || + return_slot_pattern_id.has_value()) { call_params_id = CalleePatternMatch(context, *implicit_param_patterns_id, *param_patterns_id, return_slot_pattern_id); diff --git a/toolchain/check/testdata/function/declaration/no_prelude/fail_todo_no_params.carbon b/toolchain/check/testdata/function/declaration/no_prelude/fail_todo_no_params.carbon index 864b09dfc0a8d..1305fcfdec509 100644 --- a/toolchain/check/testdata/function/declaration/no_prelude/fail_todo_no_params.carbon +++ b/toolchain/check/testdata/function/declaration/no_prelude/fail_todo_no_params.carbon @@ -27,6 +27,15 @@ library "[[@TEST_NAME]]"; fn A { } +// --- fail_todo_return_type.carbon + +library "[[@TEST_NAME]]"; +// CHECK:STDERR: fail_todo_return_type.carbon:[[@LINE+4]]:1: error: semantics TODO: `function with positional parameters` [SemanticsTodo] +// CHECK:STDERR: fn A -> (); +// CHECK:STDERR: ^~~~~~~~~~~ +// CHECK:STDERR: +fn A -> (); + // --- fail_todo_arrow_body.carbon // TODO: We don't have parsing support for this yet. @@ -94,6 +103,31 @@ fn A { // CHECK:STDOUT: return // CHECK:STDOUT: } // CHECK:STDOUT: +// CHECK:STDOUT: --- fail_todo_return_type.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %empty_tuple.type: type = tuple_type () [template] +// CHECK:STDOUT: %A.type: type = fn_type @A [template] +// CHECK:STDOUT: %A: %A.type = struct_value () [template] +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file { +// CHECK:STDOUT: package: = namespace [template] { +// CHECK:STDOUT: .A = %A.decl +// CHECK:STDOUT: } +// CHECK:STDOUT: %A.decl: %A.type = fn_decl @A [template = constants.%A] { +// CHECK:STDOUT: %return.patt: %empty_tuple.type = return_slot_pattern +// CHECK:STDOUT: %return.param_patt: %empty_tuple.type = out_param_pattern %return.patt, runtime_param0 +// CHECK:STDOUT: } { +// CHECK:STDOUT: %.loc7_10.1: %empty_tuple.type = tuple_literal () +// CHECK:STDOUT: %.loc7_10.2: type = converted %.loc7_10.1, constants.%empty_tuple.type [template = constants.%empty_tuple.type] +// CHECK:STDOUT: %return.param: ref %empty_tuple.type = out_param runtime_param0 +// CHECK:STDOUT: %return: ref %empty_tuple.type = return_slot %return.param +// CHECK:STDOUT: } +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @A() -> %empty_tuple.type; +// CHECK:STDOUT: // CHECK:STDOUT: --- fail_todo_arrow_body.carbon // CHECK:STDOUT: // CHECK:STDOUT: file {} From b728f4f661e6a9cedd5f46753877fbcaf2117d34 Mon Sep 17 00:00:00 2001 From: danakj Date: Thu, 6 Feb 2025 11:39:45 -0500 Subject: [PATCH 2/2] peek-in-return-type --- toolchain/check/handle_function.cpp | 31 +++++++++++++---------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index c9116b283bb38..146297e888060 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -26,14 +26,6 @@ namespace Carbon::Check { auto HandleParseNode(Context& context, Parse::FunctionIntroducerId node_id) -> bool { - // Normally the `IdentifierNameBeforeParamsId` node starts these blocks, but a - // `fn` may or may not have an identifier or parameters or a return type. And - // the pattern stack needs to exist if there are either parameters or a return - // type. - context.pattern_block_stack().Push(); - context.full_pattern_stack().PushFullPattern( - FullPatternStack::Kind::ImplicitParamList); - // Create an instruction block to hold the instructions created as part of the // function signature, such as parameter and return types. context.inst_block_stack().Push(); @@ -51,6 +43,20 @@ auto HandleParseNode(Context& context, Parse::ReturnTypeId node_id) -> bool { // Propagate the type expression. auto [type_node_id, type_inst_id] = context.node_stack().PopExprWithNodeId(); auto type_id = ExprAsType(context, type_node_id, type_inst_id).type_id; + + // If the previous node was `IdentifierNameBeforeParams`, then it would have + // caused these entries to be pushed to the pattern stacks. But it's possible + // to have a fn declaration without any parameters, in which case we find + // `IdentifierNameNotBeforeParams` on the node stack. Then these entries are + // not on the pattern stacks yet. They are only needed in that case if we have + // a return type, which we now know that we do. + if (context.node_stack().PeekNodeKind() == + Parse::NodeKind::IdentifierNameNotBeforeParams) { + context.pattern_block_stack().Push(); + context.full_pattern_stack().PushFullPattern( + FullPatternStack::Kind::ExplicitParamList); + } + auto return_slot_pattern_id = context.AddPatternInst( node_id, {.type_id = type_id, .type_inst_id = type_inst_id}); @@ -193,15 +199,6 @@ static auto BuildFunctionDecl(Context& context, } auto name = PopNameComponent(context, return_slot_pattern_id); - if (name.param_patterns_id.has_value() || - !name.pattern_block_id.has_value()) { - // If there were parameters then pattern stack entries were added from the - // parameters, and we can drop the entries from the FunctionIntroducer. If - // there were no parameters, but the pattern block was not used for a return - // type, then we can also drop them. - context.pattern_block_stack().PopAndDiscard(); - context.full_pattern_stack().PopFullPattern(); - } if (!name.param_patterns_id.has_value()) { context.TODO(node_id, "function with positional parameters"); name.param_patterns_id = SemIR::InstBlockId::Empty;