Explicitly mark as unsupported instead of crashing mapping a parameter record type defined not in a namespace #5777

bricknerb · 2025-07-07T14:27:22Z

Before this change we crash in this case when trying to use the outer type.
After this change we diagnose a TODO.
Will add the missing support for this in a separate PR.

Part of #5533.

jonmeow · 2025-07-07T18:05:31Z

toolchain/check/testdata/interop/cpp/function/class.carbon

+};
+
+auto foo1(P::C) -> void;
+auto foo2(P) -> void;


In each of these, you have this foo2 with a call. However, is it necessary for the test (it's not inside the namespace)? If not, I'd suggest removing it; I'm not clear it's testing distinct behavior. If so, please add a comment clarifying what it's doing.

Or, if you mean to test behavior of a type that contains another type, maybe that should be a separate test to clarify what you're trying to test?

Thanks, done.
I've changed this PR to be made of:

First commit: Just adds tests that don't diagnose for class and struct and wrongly treat the outer record as a namespace. This can be a separate PR.

The rest fixes that and adds the tests that would have crashed without the fix.

You've split tests. But I don't think you're addressing my question of what the test is achieving.

That is, why is "Pass outer class after passing inner class" an interesting thing to test, particularly when including all the SemIR for both calls? I agree that's what it does, but what is the test going to be testing that is meaningfully different? (if you just want to test passing the outer class that has a nested class, why not only test that and have less IR? why is "after" important?)

To be clear, I'm asking you to comment your code.

jonmeow · 2025-07-08T17:44:18Z

toolchain/check/testdata/interop/cpp/function/class.carbon

+// --- inner_and_outer_value_param_type.h
+
+class O {
+ public:
+  class C {};
+};
+
+auto foo1(O::C) -> void;
+auto foo2(O) -> void;


Can this same header be shared, rather than copying it?

jonmeow · 2025-07-08T17:45:45Z

toolchain/check/testdata/interop/cpp/function/class.carbon

+import Cpp library "inner_and_outer_value_param_type.h";
+
+fn F() {
+  //@dump-sem-ir-begin


Does the SemIR need to be captured for the first call? Will it differ from the above test?

jonmeow · 2025-07-08T17:45:48Z

toolchain/check/testdata/interop/cpp/function/class.carbon

+};
+
+auto foo1(P::C) -> void;
+auto foo2(P) -> void;


You've split tests. But I don't think you're addressing my question of what the test is achieving.

That is, why is "Pass outer class after passing inner class" an interesting thing to test, particularly when including all the SemIR for both calls? I agree that's what it does, but what is the test going to be testing that is meaningfully different? (if you just want to test passing the outer class that has a nested class, why not only test that and have less IR? why is "after" important?)

jonmeow · 2025-07-08T18:07:11Z

toolchain/check/testdata/interop/cpp/function/class.carbon

+  // CHECK:STDERR:   ^~~~~~~~
+  // CHECK:STDERR:
+  Cpp.foo1({});
+  Cpp.foo2({});


If you're just trying to verify that Cpp.O is a type, maybe:

Suggested change

Cpp.foo2({});

//@dump-sem-ir-begin

// Verify `O` is a valid type.

var x: Cpp.O;

Trying this out actually revealed another bug here, which I've sent a fix for in #5789.
I think it's better to fix that first.

…other record type. The code assumes the outer record is a namespace.

…r record type defined not in a namespace Before this change we crash in this case. After this change we diagnose a TODO. Will add the missing support for this in a separate PR. Part of carbon-language#5533.

…ggers importing the record.

…ating it.

bricknerb · 2025-07-17T16:18:54Z

I believe I've addressed all comments.
Had to rebase and ended up basically recreating this change, so some history is lost. :(

jonmeow

Basically LG, see comment

jonmeow · 2025-07-17T20:42:14Z

toolchain/check/testdata/interop/cpp/function/class.carbon

+  // CHECK:STDERR:
+  Cpp.foo({});
+  //@dump-sem-ir-begin
+  var x: Cpp.O;


Since the second check (x: Cpp.O) is no longer calling, I think it'd make sense to re-combine the two tests now. It just hadn't been clear to me what you intended to test before: if you were testing the type was usable post-Cpp.foo, or if you were testing that a type containing a type could be used at all.

bricknerb changed the title ~~Explicitly unsupport and avoid crashing mapping parameter record type defined not in a namespace~~ Explicitly mark as unsupported instead of crashing mapping a parameter record type defined not in a namespace Jul 7, 2025

github-actions bot requested a review from jonmeow July 7, 2025 14:27

github-actions bot added the toolchain label Jul 7, 2025

jonmeow reviewed Jul 7, 2025

View reviewed changes

bricknerb force-pushed the class branch from 42d56f5 to bbecd00 Compare July 8, 2025 10:36

jonmeow reviewed Jul 8, 2025

View reviewed changes

bricknerb added 6 commits July 16, 2025 17:58

Add tests for passing a param by value of a record type defined in an…

f66ea13

…other record type. The code assumes the outer record is a namespace.

Explicitly mark as unsupported instead of crashing mapping a paramete…

2ba17cd

…r record type defined not in a namespace Before this change we crash in this case. After this change we diagnose a TODO. Will add the missing support for this in a separate PR. Part of carbon-language#5533.

Don't output the SemIR generated by the call to the function that tri…

1be804f

…ggers importing the record.

Remove unused foo2()

5b95d2e

Reuse the definition_in_*_value_param_type header instead of duplic…

12d9f64

…ating it.

Rename files to be more consistent

ea89433

bricknerb force-pushed the class branch from 61212f1 to ea89433 Compare July 17, 2025 16:16

bricknerb requested a review from jonmeow July 17, 2025 16:18

jonmeow approved these changes Jul 17, 2025

View reviewed changes

Merge definition in outer definition tests

7177387

bricknerb enabled auto-merge July 18, 2025 02:32

bricknerb added this pull request to the merge queue Jul 18, 2025

Merged via the queue into carbon-language:trunk with commit f4f521d Jul 18, 2025
9 checks passed

bricknerb deleted the class branch July 18, 2025 02:54

Explicitly mark as unsupported instead of crashing mapping a parameter record type defined not in a namespace #5777

Explicitly mark as unsupported instead of crashing mapping a parameter record type defined not in a namespace #5777

Uh oh!

Conversation

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

Uh oh!

Uh oh!

jonmeow Jul 7, 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!

bricknerb Jul 8, 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 Jul 8, 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!

jonmeow Jul 8, 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 Jul 8, 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!

bricknerb commented Jul 17, 2025

Uh oh!

jonmeow left a comment

Choose a reason for hiding this comment

Uh oh!

jonmeow Jul 17, 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!

Uh oh!

Uh oh!

bricknerb commented Jul 7, 2025 •

edited

Loading

jonmeow Jul 7, 2025 •

edited

Loading

bricknerb Jul 8, 2025 •

edited

Loading

jonmeow Jul 8, 2025 •

edited

Loading

jonmeow Jul 8, 2025 •

edited

Loading

jonmeow Jul 8, 2025 •

edited

Loading

jonmeow Jul 17, 2025 •

edited

Loading