From 7d6cd3da6dae9fc41ffdef159dc2fed9500953d7 Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Thu, 6 Feb 2025 12:49:00 -0500 Subject: [PATCH] Add a test and check-support for positional params with a return type (#4899) 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 | 14 ++++++++ toolchain/check/name_component.cpp | 3 +- .../no_prelude/fail_todo_no_params.carbon | 34 +++++++++++++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index 566427356b08..146297e88806 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -43,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}); diff --git a/toolchain/check/name_component.cpp b/toolchain/check/name_component.cpp index 43593e906bd3..0535c26783ad 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 864b09dfc0a8..1305fcfdec50 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 {}