[cxx-interop] Do not give the default C++ constructor's return statement a result.

[zoecarver]

The default C++ object constructor assigns the newly created object out of the function so, it should not return a value. Returning a value will trigger SILGen assertions.

Refs #32402.

[jckarter]

Makes sense! As a followup, it'd be good to check that the elementwise constructor still does the right thing with possibly nontrivial fields if the imported type is non-empty.

[zoecarver]

@jckarter good idea, will do. Thanks.

[zoecarver]

@swift-ci please smoke test and merge.

[zoecarver]

@swift-ci please smoke test and merge.

[gribozavr]

@zoecarver Sorry for the late review, but could you adjust the test to have a stronger postcondition than just "does not crash"? Could you add a SILGen test that checks the actual SIL instructions in the body of the generated no-arg initializer in the test/Interop directory?

[gribozavr]

@zoecarver Another question is why is this issue only caught by assertions (I assume AST or SIL verifier?) only for C++ types? Are we missing some AST or SIL verifier checks?

[zoecarver]

Could you add a SILGen test that checks the actual SIL instructions in the body of the generated no-arg initializer in the test/Interop directory?

Sure. Will do.

Another question is why is this issue only caught by assertions (I assume AST or SIL verifier?) only for C++ types? Are we missing some AST or SIL verifier checks?

The assertion was only caught for C++ types because the issue with this constructor was only visible for address types. I'm not sure if there are any non-c++ address only types that are default constructible (and if so, we should make tests for them).

[zoecarver]

Also, for what it's worth, this crashed in SILGen IRC. If it had been able to generate the function, the SILVerifier probably would have errored.

[gribozavr]

The assertion was only caught for C++ types because the issue with this constructor was only visible for address types.

WDYM by "visible"? Is it valid to return () from an initializer for a loadable type? Shouldn't the AST verifier catch this problem regardless of whether the type is loadable or address-only?

[zoecarver]

Is it valid to return () from an initializer for a loadable type? Shouldn't the AST verifier catch this problem regardless of whether the type is loadable or address-only?

Hmm, I think you're right. It would make sense that the verifier should catch that sort of error. I'll make a patch to add a verifier check for this tomorrow.

Looking at it again, for a loadable C++ type, we sill generate a return statement with an empty tuple, but it looks like the SIL function actually does return a value. Any idea how that happens?

[jckarter]

@zoecarver The conversion to indirect return is part of SILGen's job. The AST should still formally return a result, but because the type is not loadable when lowered to SIL, it should result in the SIL function getting an indirect return argument, and writing the result to that, instead of returning by value. If you've already modified SIL type lowering so that nontrivial C++ types are classified as address-only, then that should just work, and if it isn't, it sounds like we still have work to do to figure out why.

[zoecarver]

@jckarter my confusion is because in synthesizeStructDefaultConstructorBody we create an AssignExpr to initialize self and (unconditionally) emit a ReturnStmt with no result.

If you've already modified SIL type lowering so that nontrivial C++ types are classified as address-only, then that should just work, and if it isn't, it sounds like we still have work to do to figure out why.

Yes, currently everything works. I'm just curious as to why. It seems like what we maybe should have done is returned the call to zeroInitializer (address or not) and removed the assignment completely. That would let SILGen figure out what's the best course of action (either return the value directly or copy into an indirect result).

[zoecarver]

my confusion is because in synthesizeStructDefaultConstructorBody we create an AssignExpr to initialize self and (unconditionally) emit a ReturnStmt with no result.

To elaborate a bit: Assigning self out of the function seems like it shouldn't work but, it does. Especially for loadable structs, assigning one value type to another seems like it should have no effect in the worst case, and error in the best case. The reason this works is SILGen somehow figures out what we're trying to do is emit a return of the newly created object. I don't know much about SILGen so, I'm not sure if this is the conventional way to do things but, I suspect it's not.

[jckarter]

It could be that the code path in synthesizeStructDefaultConstructorBody has not ever had to deal with address-only types in the past (which would make sense, because an empty struct in Swift would always be a trivial type), so we need to add those code paths now. You can look at how code generation for regular functions works to get a sense of how this is supposed to work, by comparing:

func foo(x: Int) -> Int { return x }

with

func bar(x: Any) -> Any { return x }

Since Int is a trivial loadable type, foo should accept its argument and return by value, but because Any is address-only, it will be loaded from memory and stored to the indirect return.

[zoecarver]

It could be that the code path in synthesizeStructDefaultConstructorBody has not ever had to deal with address-only types in the past.

I agree. It's likely it hasn't.

I'll create a new patch to update this function and similar ones to work with address types. When building the AST, we don't have to create a special path for address type, or do we? In other words, can I just create a ReturnStmt with the object and let SILGen figure out whether that can be returned directly or needs to be stored to the indirect return address? I'll give it a try and see what works.

Thanks for the help :)

[gribozavr]

When building the AST, we don't have to create a special path for address type, or do we?

Right -- at the AST level we don't even know whether the type is address-only or loadable, so the AST for the no-arg initializer should be identical between the two. My suggestion regarding the verifier seems to be relevant for the AST verifier then. If SILGen wants a specific shape of ReturnStmt in the AST for initializers of address-only types, but does not care about that detail for loadable types, we should still treat it as a universal invariant and always enforce it in the verifier.

[zoecarver]

Sorry for all the back and forth. I went to go make this change and I realized because of how the constructor body synthesizer works, we actually aren't allowed to directly create the return statement in the synthesizer function. So, I think the change this PR made is good.

I will add the AST verifier check, though.

[jckarter]

@zoecarver Yeah, if we're able to make it so that the intended semantics of the initializer can be synthesized into the AST, then we should be able to allow SILGen to just do its thing lowering a regular function body without special code.

[zoecarver]

@jckarter Sounds good. I think we're on the same page. That's how it works on TOT (after this PR) so, I don't think we need to make any changes, right?

Here's the added AST verifier check: #32656.