Skip to content

[cxx-interop] Add fix for corner case where NS_OPTIONS typedef has to be desugared #66619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

plotfi
Copy link
Contributor

@plotfi plotfi commented Jun 14, 2023

@NuriAmari added code to do consistent mangling for NS_OPTIONS types in the context of C++-Interop, but I think with UIControlState we have a corner case where the typedef has surgar in the form of a clang::ElaboratedType.

Still trying to get the specifics right on this one and will add test cases soon. This does fix the mangling for the case in #66602

Commit Message:

This patch is an add-on to https://github.com/apple/swift/pull/64043.
Essentially when encountering NS_OPTIONS enums, in C++-Interop mode
if they are not specially handled then they can mangle differently than
they do without C++-Interop. This patch adds logic to handle when a
typedef and enum have additional clang::ElaboratedType sugar, but
otherwise it does the same as the existing 64043 patch.

The test case provided was encountered in a real app build. The problem
came from when two modules are each compiled one with and one without
C++-Interop. For the test case code provided the mangling of the
protocol conformance is not consistent and the code in
SILGenLazyConformance.cpp crashes on an invalid conformance with reason
"Invalid conformance in type-checked AST".

@plotfi plotfi requested a review from NuriAmari June 14, 2023 06:01
@plotfi plotfi added the c++ interop Feature: Interoperability with C++ label Jun 14, 2023
@plotfi plotfi force-pushed the plotfi-cxx-interop-uicontrolstate-mangling branch 2 times, most recently from b241346 to 21a3c89 Compare June 14, 2023 23:09
@plotfi
Copy link
Contributor Author

plotfi commented Jun 14, 2023

@egorzhdan Can I get a quick review and test run?

Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small comments but approach looks good. Thanks, Puyan!

@zoecarver
Copy link
Contributor

@plotfi do you not have commit access?

@zoecarver
Copy link
Contributor

@swift-ci please test

@plotfi
Copy link
Contributor Author

plotfi commented Jun 14, 2023

@plotfi do you not have commit access?

I do but I don’t think I have CI access.

@zoecarver
Copy link
Contributor

Can you ping @shahmishal?

@shahmishal
Copy link
Member

@plotfi you should have access let me know if you can't start it.

@plotfi
Copy link
Contributor Author

plotfi commented Jun 15, 2023

@plotfi you should have access let me know if you can't start it.

Thanks @shahmishal for confirming!

@plotfi plotfi force-pushed the plotfi-cxx-interop-uicontrolstate-mangling branch from 21a3c89 to 82fe5e3 Compare June 15, 2023 17:51
… be desugared

This patch is an add-on to swiftlang#64043.
Essentially when encountering NS_OPTIONS enums, in C++-Interop mode
if they are not specially handled then they can mangle differently than
they do without C++-Interop. This patch adds logic to handle when a
typedef and enum have additional clang::ElaboratedType sugar, but
otherwise it does the same as the existing 64043 patch.

The test case provided was encountered in a real app build. The problem
came from when two modules are each compiled one with and one without
C++-Interop. For the test case code provided the mangling of the
protocol conformance is not consistent and the code in
SILGenLazyConformance.cpp crashes on an invalid conformance with reason
"Invalid conformance in type-checked AST".
@plotfi plotfi force-pushed the plotfi-cxx-interop-uicontrolstate-mangling branch from 82fe5e3 to fe6ccd7 Compare June 15, 2023 18:18
@plotfi
Copy link
Contributor Author

plotfi commented Jun 15, 2023

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants