Update most `toolchain/check/testdata/interop/cpp` tests to use sem ir ranges #5594

bricknerb · 2025-06-03T09:37:17Z

All except toolchain/check/testdata/interop/cpp/function_param_int*.carbon and toolchain/check/testdata/interop/cpp/function_return.carbon.

Don't output SemIR for cases that are intended to fail.

… ranges. Except `no_prelude/function/struct.carbon`, which is being updated in carbon-language#5540.

ivanaivanovska · 2025-06-03T13:15:53Z

toolchain/check/testdata/interop/cpp/function_param_int16.carbon

 // CHECK:STDOUT: fn @F() {
 // CHECK:STDOUT: !entry:
 // CHECK:STDOUT:   %Cpp.ref: <namespace> = name_ref Cpp, imports.%Cpp [concrete = imports.%Cpp]
-// CHECK:STDOUT:   %int_16.1: Core.IntLiteral = int_value 16 [concrete = constants.%int_16]
-// CHECK:STDOUT:   %i16.1: type = class_type @Int, @Int(constants.%int_16) [concrete = constants.%i16]
-// CHECK:STDOUT:   %foo.decl: %foo.type = fn_decl @foo [concrete = constants.%foo] {} {}


Where are now these fn_decl lines? I don't see them anymore, and they contain useful information for the parameters and the return values as fixed in #5468 .

I've tried to expand the range and debugged this a bit and AFAIU, this is because we ignore imported instructions.

carbon-lang/toolchain/sem_ir/formatter.cpp

Lines 236 to 240 in fb33f7c

// When there are dump ranges, ignore imported instructions.

auto loc_id = sem_ir_->insts().GetCanonicalLocId(inst_id);

if (loc_id.kind() != LocId::Kind::NodeId) {

return false;

}

However, I think we should have a way to format imported C++ instructions.
@jonmeow, WDYT?

Two things:

These should be in the imports block instead of the user block -- Move imported C++ entities to the import block #5616. Note, this may also affect the output you get since imports should be printed when a reference is found, and the name_ref should count. (I'd suggest merging in Move imported C++ entities to the import block #5616 and seeing the resulting behavior, I'm not actually checking)

If Move imported C++ entities to the import block #5616 doesn't get you were you want to be, and/or if you also want to see the declaration, e.g. fn @foo() on line 381, you might keep some files with --dump-sem-ir-ranges=if-present and no ranges specifically for that. At least right now we aren't including those based on range.

That said, if there's a mix where some test cases which want no SemIR and want full SemIR, both can only be indicated by an absence of ranges right now; the only way to do that is to have separate test files, one with =only set and the other with =if-present set.

Thanks @jonmeow! Merged.
AFAICT, #5616 has removed the fn_decl lines that @ivanaivanovska referred to.

@ivanaivanovska, can you let me know what important parts of the SemIR are missing now, if any?

It's still there, it's just moved to the imports section. In this PR, see line 342/354:

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

https://github.com/carbon-language/carbon-lang/pull/5594/files#diff-697388ec3d2472a66b115558398c52229dd253c95f4c02767ff139724e66723eR354

Thanks @jonmeow. Missed that part.

The fn_decl lines are now there - that is great, thanks!

The only part that is missing and I think is useful are the fn @foo() lines (e.g on line 381 in function_param_int16.carbon). But as @jonmeow mentions above, it looks like we'll need to refactor the files a bit and have files without ranges to have them, or have no ranges for some of the existing files. For now, I'd keep some of the existing files without ranges before we refactor the files in a separate PR, so that we don't lose these lines completely (e.g. in function_param_int16.carbon, function_param_int32.carbon, function_param_refactor.carbon). But any solution is fine with me as long as we get these lines somewhere.

I've revert toolchain/check/testdata/interop/cpp/function_param_*.carbon, so we can continue this discussion in a separate PR, to avoid blocking the rest of the changes.

the last file is function_return.carbon, sorry for the typo. If you could also revert that, that would be great.

LGTM otherwise.

jonmeow · 2025-06-05T17:24:45Z

toolchain/check/testdata/interop/cpp/no_prelude/bad_import.carbon

@@ -15,16 +14,19 @@

 library "[[@TEST_NAME]]";

+//@dump-sem-ir-begin
 // CHECK:STDERR: fail_import_cpp.carbon:[[@LINE+4]]:1: error: `Cpp` import missing library [CppInteropMissingLibrary]


Just to note, for cases that are intended to fail (trying to avoid commenting on a TODO-should-pass), you might consider not dumping semir at all. Failures often get a bit boilerplate and it might not be worth keeping all the relevant SemIR around.

I removed the ranges from cases that are intended to fail.

jonmeow · 2025-06-05T17:28:42Z

toolchain/check/testdata/interop/cpp/function_param_int16.carbon

 // CHECK:STDOUT: fn @F() {
 // CHECK:STDOUT: !entry:
 // CHECK:STDOUT:   %Cpp.ref: <namespace> = name_ref Cpp, imports.%Cpp [concrete = imports.%Cpp]
-// CHECK:STDOUT:   %int_16.1: Core.IntLiteral = int_value 16 [concrete = constants.%int_16]
-// CHECK:STDOUT:   %i16.1: type = class_type @Int, @Int(constants.%int_16) [concrete = constants.%i16]
-// CHECK:STDOUT:   %foo.decl: %foo.type = fn_decl @foo [concrete = constants.%foo] {} {}


Two things:

These should be in the imports block instead of the user block -- Move imported C++ entities to the import block #5616. Note, this may also affect the output you get since imports should be printed when a reference is found, and the name_ref should count. (I'd suggest merging in Move imported C++ entities to the import block #5616 and seeing the resulting behavior, I'm not actually checking)

If Move imported C++ entities to the import block #5616 doesn't get you were you want to be, and/or if you also want to see the declaration, e.g. fn @foo() on line 381, you might keep some files with --dump-sem-ir-ranges=if-present and no ranges specifically for that. At least right now we aren't including those based on range.

That said, if there's a mix where some test cases which want no SemIR and want full SemIR, both can only be indicated by an absence of ranges right now; the only way to do that is to have separate test files, one with =only set and the other with =if-present set.

…-dump-sem-ir-ranges=only`

jonmeow

To note, approving for clarity (and to avoid blocking) -- I'm fine with this as-is, as long as @ivanaivanovska is okay.

…and update `toolchain/check/testdata/interop/cpp/function_param_unsupported.carbon`

…on` and `toolchain/check/testdata/interop/cpp/function_return.carbon` tests to use sem ir ranges (#5645) Follow up of #5594. Trying to compromise SemIR size, having enough information and complexity of tests, I've duplicated representative tests to a separate test file with `--dump-sem-ir-ranges=if-present`.

Update all toolchain/check/testdata/interop/cpp tests to use sem ir…

a0eeb57

… ranges. Except `no_prelude/function/struct.carbon`, which is being updated in carbon-language#5540.

github-actions bot added the toolchain label Jun 3, 2025

github-actions bot requested a review from zygoloid June 3, 2025 09:38

bricknerb changed the title ~~Update all toolchain/check/testdata/interop/cpp tests to use sem ir ranges.~~ Update all toolchain/check/testdata/interop/cpp tests to use sem ir ranges Jun 3, 2025

ivanaivanovska reviewed Jun 3, 2025

View reviewed changes

commit

bab609c

jonmeow reviewed Jun 5, 2025

View reviewed changes

bricknerb added 4 commits June 10, 2025 12:58

Merge branch 'main' into ranges

78bc0b8

Update tests following merge and add back missing //@dump-sem-ir-begin

3dca8e5

Make sure all tests in toolchain/check/testdata/interop/cpp have `-…

dc26ca5

…-dump-sem-ir-ranges=only`

Remove SemIR ranges from cases that are intended to fail

4bf82a6

bricknerb requested a review from ivanaivanovska June 10, 2025 13:23

jonmeow approved these changes Jun 10, 2025

View reviewed changes

bricknerb added 3 commits June 11, 2025 11:37

Change ranges for import_short.carbon to focus on the relevant parts.

367d94c

Revert toolchain/check/testdata/interop/cpp/function_param_*.carbon

dc86e78

Revert toolchain/check/testdata/interop/cpp/function_return.carbon …

66c8249

…and update `toolchain/check/testdata/interop/cpp/function_param_unsupported.carbon`

ivanaivanovska approved these changes Jun 11, 2025

View reviewed changes

Merge branch 'main' into ranges

d9d06be

bricknerb enabled auto-merge June 11, 2025 12:23

bricknerb changed the title ~~Update all toolchain/check/testdata/interop/cpp tests to use sem ir ranges~~ Update most toolchain/check/testdata/interop/cpp tests to use sem ir ranges Jun 11, 2025

bricknerb added this pull request to the merge queue Jun 11, 2025

Merged via the queue into carbon-language:trunk with commit 29d4aae Jun 11, 2025
8 checks passed

bricknerb deleted the ranges branch June 11, 2025 12:48

bricknerb mentioned this pull request Jun 11, 2025

Update toolchain/check/testdata/interop/cpp/function_param_int*.carbon and toolchain/check/testdata/interop/cpp/function_return.carbon tests to use sem ir ranges #5645

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update most `toolchain/check/testdata/interop/cpp` tests to use sem ir ranges #5594

Update most `toolchain/check/testdata/interop/cpp` tests to use sem ir ranges #5594

Uh oh!

bricknerb commented Jun 3, 2025 •

edited

Loading

Uh oh!

ivanaivanovska Jun 3, 2025

Uh oh!

bricknerb Jun 4, 2025

Uh oh!

jonmeow Jun 5, 2025

Uh oh!

bricknerb Jun 10, 2025 •

edited

Loading

Uh oh!

jonmeow Jun 10, 2025 •

edited

Loading

Uh oh!

bricknerb Jun 10, 2025

Uh oh!

ivanaivanovska Jun 11, 2025

Uh oh!

bricknerb Jun 11, 2025

Uh oh!

ivanaivanovska Jun 11, 2025

Uh oh!

bricknerb Jun 11, 2025

Uh oh!

jonmeow Jun 5, 2025

Uh oh!

bricknerb Jun 10, 2025

Uh oh!

jonmeow Jun 5, 2025

Uh oh!

jonmeow left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

	// When there are dump ranges, ignore imported instructions.
	auto loc_id = sem_ir_->insts().GetCanonicalLocId(inst_id);
	if (loc_id.kind() != LocId::Kind::NodeId) {
	return false;
	}

Update most toolchain/check/testdata/interop/cpp tests to use sem ir ranges #5594

Update most toolchain/check/testdata/interop/cpp tests to use sem ir ranges #5594

Uh oh!

Conversation

bricknerb commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bricknerb Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonmeow Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonmeow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Update most `toolchain/check/testdata/interop/cpp` tests to use sem ir ranges #5594

Update most `toolchain/check/testdata/interop/cpp` tests to use sem ir ranges #5594

bricknerb commented Jun 3, 2025 •

edited

Loading

bricknerb Jun 10, 2025 •

edited

Loading

jonmeow Jun 10, 2025 •

edited

Loading

jonmeow left a comment •

edited

Loading