C++ function params and return values not printed in the SemIR anymore

[ivanaivanovska]

Description of the bug:

Parameters and return values were printed in the SemIR for the C++ function declarations as well, but they are not printed anymore.

E.g. in toolchain/check/testdata/interop/cpp/function_decl.carbon:

• Case 1: Function params:

// --- single_int_param_function_decl.h

auto foo(int b) -> void;

Before: // CHECK:STDOUT: fn @foo[](%b.param_patt: %i32);

Now: // CHECK:STDOUT: fn @foo();

• Case 2: Return values

// --- int_return_function_decl.h

auto foo_int() -> int;

Before: // CHECK:STDOUT: fn @foo_int[]() -> %i32;

Now: // CHECK:STDOUT: fn @foo_int();

The tests still work, the issue seems to be only in the printed SemIR.

It is still useful to be able to see the C++ parameter/return values types, especially for the ongoing efforts of enabling these types.

[ivanaivanovska]

@zygoloid It looks like this change has been introduced with PR #5342. Is this behaviour intended? Does it mean that the function parameters and the return values are wrongly filled in the interop when importing the function declarations?

[zygoloid]

We now print the call params as part of printing the fn declaration in SemIR, which matches what we do for a call instruction, where we print the call arguments (rather than the syntactic arguments). If those are not being created by C++ import, then they won't show up here; generally the way to create them is by calling CalleePatternMatch, and you can look for other callers of that function to see how.

The parameter patterns are generally printed as part of the SemIR printed for the fn_decl instruction. In the SemIR I'm seeing this:

// CHECK:STDOUT:   %foo.decl: %foo.type = fn_decl @foo [concrete = constants.%foo] {} {}

... where the two empty {}s indicate that we're not tracking the list of pattern instructions as part of the function declaration. We probably should, so that they would show up here in the formatted SemIR. The way to do this would be to push a block onto the pattern block stack before calling MakeParamPatternsBlockId in import_cpp.cpp, make it use AddPatternInst instead of AddInstInNoBlock, and then pop that pattern block after the call and use it as the Function's pattern_block_id.

[ivanaivanovska]

Thanks @zygoloid !

I tried to follow your input, but could only solve part of the issue. Two questions remain:

1. Now I get only one of the {} filled in. Is it correct to assume that both {}{} should be filled in, same as for the Carbon functions?

// CHECK:STDOUT:   %foo.decl: %foo.type = fn_decl @foo [concrete = constants.%foo] {
// CHECK:STDOUT:     %a.patt: %pattern_type.7ce = binding_pattern a [concrete]
// CHECK:STDOUT:     %a.param_patt: %pattern_type.7ce = value_param_pattern %a.patt, call_param0 [concrete]
// CHECK:STDOUT:   } {}

2. I get a crash when calling:

auto call_params_id =  CalleePatternMatch(context, SemIR::InstBlockId::None, param_patterns_id,   
                   return_slot_pattern_id);

There is some issue with the param_pattern.index, which I fill in manually here.

• If I leave the code as is, there is an assertion failure:

CHECK failure at toolchain/check/pattern_match.cpp:306: !param_pattern.index.has_value()

• If I set the index to None, then I get a crash but no obvious error/failure is printed.

Do you maybe have any hints on this? Thanks.

The Draft PR for a reference: #5468.

[ivanaivanovska]

cc: @bricknerb

[geoffromer]

Thanks @zygoloid !

I tried to follow your input, but could only solve part of the issue. Two questions remain:

1. Now I get only one of the {} filled in. Is it correct to assume that both {}{} should be filled in, same as for the Carbon functions?

Yes, they should both be filled in. Populating the second one is CalleePatternMatch's responsibility, so the fact that it's empty I think just reflects the fact that you've disabled that call.

2. I get a crash when calling:

auto call_params_id =  CalleePatternMatch(context, SemIR::InstBlockId::None, param_patterns_id,   
                   return_slot_pattern_id);

There is some issue with the param_pattern.index, which I fill in manually here.

• If I leave the code as is, there is an assertion failure:

CHECK failure at toolchain/check/pattern_match.cpp:306: !param_pattern.index.has_value()

Yeah, populating that field is also CalleePatternMatch's responsibility, so it shouldn't be set manually. I'll send a PR to add a message to that CHECK to try to explain the problem (Edit: Sent #5504 for this).

• If I set the index to None, then I get a crash but no obvious error/failure is printed.

The crash is happening here:

carbon-lang/toolchain/check/pattern_match.cpp

Lines 213 to 215 in 8e666ce

	std::exchange(context.bind_name_map().Lookup(entry.pattern_id).value(),
	{.bind_name_id = SemIR::InstId::None,
	.type_expr_region_id = SemIR::ExprRegionId::None});

My guess is that the Lookup call is failing, which causes the exchange operation to crash. This code expects bind_name_map to have an entry for every BindingPattern inst, and it looks like your code doesn't add one when it adds a BindingPattern. I wonder if you could use AddBindingPattern instead of calling AddPatternInst with a BindingPattern, because that takes care of populating bind_name_map. If not, you can at least use it as an example of how to do it.