When importing C++ records (struct/class/union), make the imported type complete #5534
Conversation
…pe complete This is required in order to pass these types as parameters / return values.
This doesn't support actually passing the value of the struct, which is planned to be implemented using thunks. I plan to add more tests in a separate PR. Based on carbon-language#5534 and carbon-language#5537. Part of carbon-language#5533.
| @@ -605,6 +606,9 @@ static auto ImportCXXRecordDecl(Context& context, SemIR::LocId loc_id, | |||
| // TODO: Set block. | |||
| /*body=*/{}); | |||
|
|
|||
| CompleteTypeOrCheckFail(context, | |||
There was a problem hiding this comment.
I'm surprised that this doesn't have a more substantial effect on any of the tests. Were all the types already complete?
(I'm also troubled that this change causes the inst names to get disambiguating suffixes that look unnecessary, but I'm not sure what to do about it.)
There was a problem hiding this comment.
I'm surprised that this doesn't have a more substantial effect on any of the tests. Were all the types already complete?
From ComputeClassObjectRepr:
auto complete_type_witness_id = CheckCompleteClassType(
context, node_id, class_id, field_decls, vtable_contents, body);
That said, maybe there's an edge case that was being missed; maybe a test should be added?
I'm also troubled that this change causes the inst names to get disambiguating suffixes that look unnecessary, but I'm not sure what to do about it.
This probably reflects the addition of instructions which the formatter would name the same, but don't get printed in output due to being unreferenced.
There was a problem hiding this comment.
This is required to allow passing these types as parameters / return values.
Note, it's not clear to me that this is the right way to fix if this issue can't be tested in this PR. The code without this PR looks equivalent to Carbon's class behavior.
There was a problem hiding this comment.
Thanks!
Tests definitely fail (crashes) in #5538 without this so we can continue the review there if you prefer.
I wanted to make the review easier so I split it to two PRs, but I can see the value of having it in one.
FWIW, in ImportCXXRecordDecl(), context.types().IsComplete(context.classes().Get(class_id).self_type_id) always returns false before CompleteTypeOrCheckFail() and true afterwards.
While ComputeClassObjectRepr() calls CheckCompleteClassType(), it doesn't seem to make this type complete, even though it sets complete_type_witness_id.
I believe that Carbon classes that aren't imported from C++ are being completed through a different pass.
This is required to allow passing these types as parameters / return values.
Part of #5533.