Skip to content

[Explicit Module Builds] C++ Interoperability mode fixes #69706

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 21, 2024
Merged

[Explicit Module Builds] C++ Interoperability mode fixes #69706

merged 4 commits into from
May 21, 2024

Conversation

@artemcm
Copy link
Contributor

@artemcm artemcm commented Nov 7, 2023

No description provided.

hyp
hyp reviewed Nov 7, 2023
View reviewed changes
@artemcm artemcm force-pushed the DepScanCPPInterop branch 2 times, most recently from e442ef3 to 3245aae Compare November 8, 2023 20:51
@artemcm artemcm marked this pull request as ready for review November 15, 2023 21:01
@artemcm artemcm requested a review from tshortli as a code owner November 15, 2023 21:01
@artemcm
Copy link
Contributor Author

artemcm commented Nov 27, 2023

@swift-ci smoke test

1 similar comment
@artemcm
Copy link
Contributor Author

artemcm commented Dec 19, 2023

@swift-ci smoke test

@artemcm
Copy link
Contributor Author

artemcm commented Jan 8, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Feb 12, 2024

@swift-ci smoke test

@artemcm
Copy link
Contributor Author

artemcm commented Feb 13, 2024

@swift-ci test

@artemcm artemcm changed the title Propagate the C++ Interop mode to interface sub-invocations and dependency scanner [Explicit Module Builds] C++ Interoperability mode fixes Feb 13, 2024
@artemcm artemcm force-pushed the DepScanCPPInterop branch 2 times, most recently from c90fe9f to d5167fd Compare February 14, 2024 22:29
@artemcm
Copy link
Contributor Author

artemcm commented Feb 20, 2024

@swift-ci test

2 similar comments
@artemcm
Copy link
Contributor Author

artemcm commented Feb 22, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Feb 27, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Feb 28, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Feb 28, 2024

@swift-ci test

@artemcm artemcm force-pushed the DepScanCPPInterop branch from 444a056 to 7f9323d Compare March 18, 2024 20:14
@artemcm artemcm force-pushed the DepScanCPPInterop branch 2 times, most recently from 60f3a2f to 7fa769b Compare March 19, 2024 22:53
@artemcm
Copy link
Contributor Author

artemcm commented Mar 19, 2024

@swift-ci test

2 similar comments
@artemcm
Copy link
Contributor Author

artemcm commented Apr 3, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Apr 22, 2024

@swift-ci test

@artemcm artemcm force-pushed the DepScanCPPInterop branch from 7fa769b to 46dff8c Compare April 22, 2024 18:35
@artemcm
Copy link
Contributor Author

artemcm commented Apr 22, 2024

@swift-ci test

@artemcm artemcm force-pushed the DepScanCPPInterop branch from 46dff8c to 1b61dad Compare April 22, 2024 20:12
@artemcm
Copy link
Contributor Author

artemcm commented Apr 22, 2024

@swift-ci test

2 similar comments
@artemcm
Copy link
Contributor Author

artemcm commented May 7, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented May 10, 2024

@swift-ci test

@artemcm artemcm force-pushed the DepScanCPPInterop branch 3 times, most recently from 6561db0 to 90b2a72 Compare May 14, 2024 22:31
@artemcm
Copy link
Contributor Author

artemcm commented May 14, 2024

@swift-ci test

1 similar comment
@artemcm
Copy link
Contributor Author

artemcm commented May 15, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented May 16, 2024

@egorzhdan ping for review.

egorzhdan
egorzhdan reviewed May 20, 2024
View reviewed changes
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

Thanks @artemcm! I only have a few minor questions.

test/Interop/Cxx/library-evolution/allow-objc-in-cxx-mode-in-evolving-libraries.swift Outdated
Copy link
Contributor

@egorzhdan egorzhdan May 20, 2024

Choose a reason for hiding this comment

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

Why is the disable-implicit-cxx-module-import flag required in this test?

Copy link
Contributor Author

@artemcm artemcm May 20, 2024

Choose a reason for hiding this comment

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

<unknown>:0: error: unexpected warning produced: module 'Cxx' was not compiled with library evolution support; using it means binary compatibility for 'test' can't be guaranteed

Because we import Cxx module when enabling interop, enabling library evolution has to mean disabling this implicit import.

Copy link
Contributor Author

@artemcm artemcm May 20, 2024

Choose a reason for hiding this comment

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

And because I don't know how to handle error messages without source-location with -verify I thought I'd just disable it.

Copy link
Contributor Author

@artemcm artemcm May 20, 2024

Choose a reason for hiding this comment

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

I've refactored the decision to implicit import Cxx into a new shouldImportCxx method and made it default to false for when Library Evolution is enabled.

Copy link
Contributor

@egorzhdan egorzhdan May 20, 2024

Choose a reason for hiding this comment

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

Ah, I see! Thanks for explaining

@artemcm artemcm force-pushed the DepScanCPPInterop branch from 8d0525c to a9c347c Compare May 20, 2024 16:52
artemcm added 3 commits May 20, 2024 10:23
@artemcm
[Explicit Module Builds] Propagate the C++ Interop mode to interface …
bc17581 
…sub-invocations and dependency scanner
@artemcm
[Dependency Scanning] Add required additional C++ interop overlay mod…
eb1c0e7 
…ule queries
@artemcm
Always add an implicit import of 'Cxx' module when C++ Interop is ena…
6ea604b 
…bled.

We cannot always rely on being able to do so only as an overlay query upon loading 'requires cplusplus' modulemap modules. The 'requires' statement only applies to submodules, and we may not be able to query language feature modulemap attributes in dependency scanning context.
@artemcm artemcm force-pushed the DepScanCPPInterop branch from a9c347c to 7c4b05f Compare May 20, 2024 17:23
@artemcm artemcm force-pushed the DepScanCPPInterop branch from 7c4b05f to ec4e0e2 Compare May 20, 2024 18:18
@artemcm
Copy link
Contributor Author

artemcm commented May 20, 2024

@swift-ci test

egorzhdan
egorzhdan approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

LGTM!

@artemcm artemcm enabled auto-merge May 20, 2024 18:41
@artemcm artemcm mentioned this pull request May 20, 2024
Merged
@artemcm artemcm merged commit 31337dd into swiftlang:main May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hyp hyp hyp left review comments

@egorzhdan egorzhdan egorzhdan approved these changes

@tshortli tshortli Awaiting requested review from tshortli tshortli is a code owner

@zoecarver zoecarver Awaiting requested review from zoecarver

@hborla hborla Awaiting requested review from hborla hborla is a code owner

@slavapestov slavapestov Awaiting requested review from slavapestov slavapestov is a code owner

@xedin xedin Awaiting requested review from xedin xedin is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@artemcm @hyp @egorzhdan