Add cxx interop support to symbolgraph-extract #7610

rauhul · 2024-05-29T20:27:38Z

In order to extract symbol graphs for modules with transitive imports on C++ modules we need parse headers in C++ interop mode using the option: -cxx-interoperability-mode=default. This commit updates swift-symbolgraph-extract to pass the option when there is a C++ module in the import graph.

This option is a new addition to swift-symbolgraph-extract added in swiftlang/swift#73963.

Prior to this option's introduction, extracting the symbol graph for C++ dependent modules would fail with an error similar to: "unknown type name 'namespace'".

With this SwiftPM commit and swift-symbolgraph-extract prior to swiftlang/swift#73963, users will instead see an argument parsing error like: "unknown option -cxx-interoperability-mode".

With this change and swift-symbolgraph-extract change in swiftlang/swift#73963, the symbol graph is extracted properly.

MaxDesiatov

Would you mind adding a test to make sure this doesn't regress in the future/

egorzhdan

This logic mostly duplicates the existing solution that forwards the C++ interop compat version flag to the test discovery target: https://github.com/apple/swift-package-manager/blob/8d419298fe880c44692a757c92ee5ab5d3b6d6f3/Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift#L555-L574

I think it's worth extracting and reusing it.

egorzhdan · 2024-05-30T11:33:50Z

Sources/Commands/Utilities/SymbolGraphExtract.swift

+            }
+        }
+        if cxxInterop {
+            commandLine += ["-cxx-interoperability-mode=default"]


Some projects use a non-default compat version (e.g. swift-5.9), we should forward the actual version here.

I dont see anything in SwiftPM that currents sets a non-"default" value and based on the PackageModel I dont see how a user can even select a "non-default compat version"

It seems like this support was removed in #6583

It is still possible to pass a custom C++ interop compat version, for example:

swiftSettings: [ .unsafeFlags(["-cxx-interoperability-mode=upcoming-swift"]) ]

Sources/Commands/Utilities/SymbolGraphExtract.swift

rauhul · 2024-05-31T00:32:32Z

@MaxDesiatov @egorzhdan @daniel-grumberg I've refactored this with (a lot of) guidance from @neonichu, please take another look

rauhul · 2024-05-31T00:51:44Z

Using:

This branch rebased on rel/6.0
A local copy of swift-symbolgraph-extract with support for cxx interop
A local copy of swift-mmio with swift-docc-plugin

Via:

DOCC_EXEC=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/docc SWIFT_EXEC=/Volumes/Developer/org.swift/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/bin/swiftc .build/debug/swift-package --package-path /Users/rauhul/Developer/org.swift/swift-mmio generate-documentation

I am able to properly generate docs.

Running based on main, I see the following errors unrelated to this change:

/Volumes/Developer/org.swift/swift-mmio/.build/arm64-apple-macosx/debug/MMIO-tool.build/module.modulemap:1:8: error: redefinition of module 'MMIO'
module MMIO {
       ^
/Volumes/Developer/org.swift/swift-mmio/.build/arm64-apple-macosx/debug/MMIO.build/module.modulemap:1:8: note: previously defined here
module MMIO {
       ^

When running swift-package dump-symbol-graph, I see a crash in swift-symbolgraph-extract in llvm::DenseMapBase::FindAndConstruct after very deep recursion in Module.cpp::collectExportedImports.

I think both of these issues are unrelated to my change here and should be handled independently.

In order to extract symbol graphs for modules with transitive imports on cxx modules we need parse headers in cxx mode using the option: `-cxx-interoperability-mode=default`. This commit updates `swift-symbolgraph-extract` to pass the option when there is a Cxx module in the import graph. This option is a new addition to `swift-symbolgraph-extract` added in swiftlang/swift#73963. Prior to this option's introduction, extracting the symbol graph for cxx dependent modules would fail with an error similar to: "unknown type name 'namespace'". With this SwiftPM commit and `swift-symbolgraph-extract` _prior_ to 73963, users will instead see an argument parsing error like: "unknown option -cxx-interoperability-mode". With this change and `swift-symbolgraph-extract` 73963, the symbol graph is extracted properly.

egorzhdan · 2024-05-31T14:25:11Z

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

+    /// If the current module or any of its linked dependencies requires cxx
+    /// interop, cxx interop will be enabled on the current module.


We don't actually want to implicitly enable C++ interop for anything other than the test discovery target or the symbolgraph target.
One of the reasons for that is that C++ interop is incompatible with Swift's library evolution. If a dependency of your project uses C++ internally, without re-exporting any C++ symbols, you don't need to enable C++ interop and can keep library evolution enabled.

egorzhdan · 2024-05-31T14:27:26Z

Tests/BuildTests/BuildPlanTests.swift

+            try XCTAssertMatch(
+                result.target(for: "swiftLib2").swiftTarget().compileArguments(),
+                [.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++20", .anySequence]
+            )


This assertion needs to be reversed: unless the Swift target explicitly enables C++ interop, we should not pass these flags to the compiler.

rauhul · 2024-05-31T17:50:05Z

@swift-ci test

rauhul · 2024-05-31T17:50:17Z

@swift-ci smoke test

egorzhdan

LGTM from the perspective of C++ interop.
I would probably wait for someone from the SwiftPM side of things to give a thumbs up as well before merging 😉

egorzhdan · 2024-06-03T12:04:40Z

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

+        var args = [String]()
+        args += try self.cxxInteroperabilityModeArguments(
+            propagateFromCurrentModuleOtherSwiftFlags: true)
+        return args


Can this be simplified to just return self.cxxInteroperabilityModeArguments(propagateFromCurrentModuleOtherSwiftFlags: true)?

A follow up PR adds to args so I'd like to leave this as is

MaxDesiatov · 2024-06-03T17:09:28Z

Sources/Build/BuildDescription/TargetBuildDescription.swift

+
+    /// Determines the arguments needed to run `swift-symbolgraph-extract` for
+    /// this module.
+    public func symbolGraphExtractArguments() throws -> [String] {


Suggested change

public func symbolGraphExtractArguments() throws -> [String] {

package func symbolGraphExtractArguments() throws -> [String] {

MaxDesiatov · 2024-06-03T17:09:40Z

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

+
+    /// Determines the arguments needed to run `swift-symbolgraph-extract` for
+    /// this module.
+    public func symbolGraphExtractArguments() throws -> [String] {


Suggested change

public func symbolGraphExtractArguments() throws -> [String] {

package func symbolGraphExtractArguments() throws -> [String] {

MaxDesiatov · 2024-06-03T17:09:47Z

Sources/Build/BuildDescription/ClangTargetBuildDescription.swift

@@ -207,6 +207,20 @@ public final class ClangTargetBuildDescription {
        }
    }

+    /// Determines the arguments needed to run `swift-symbolgraph-extract` for
+    /// this module.
+    public func symbolGraphExtractArguments() throws -> [String] {


Suggested change

public func symbolGraphExtractArguments() throws -> [String] {

package func symbolGraphExtractArguments() throws -> [String] {

rauhul · 2024-06-03T17:21:16Z

@MaxDesiatov do you mind if I fix the public -> package change in my follow up PR here #7621

xedin · 2024-06-03T17:33:38Z

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

+        func cxxInteroperabilityModeAndStandard(
+            for module: ResolvedModule
+        ) -> [String]? {
+            let scope = self.buildParameters.createScope(for: module)


Is this correct to use buildParameters of the context here for dependency scopes? What if if this i.e. executable target that has a plugin or a macro dependency and that in turn has other dependencies, we'd use "destination" parameters for their scope, which seems counter-intuitive?

It seems like we need to avoid adding new uses of recursiveTargetDependencies() in this context when build parameters are involved, because they are strongly connected to a destination, and instead traverse a build plan graph and use TargetDependencyDescription.

I'd rather not change this logic in this PR, im simply moving the existing code such that symbol graph can use it.

Sure, I'm mentioning this because of the comment - Move logic from PackageBuilder here., I don't think that would be possible to do because of this.

I guess the point of that comment is to say, we should not throw away the typed interop information at that layer and try to recover it here by parsing flags, we should instead determine the flags at this layer

Maybe you could do the same thing as .SWIFT_VERSION and add a new declaration for assigments like that which means that you won't need to deal with prefixes here?

I don't mean it has to be in this PR, just an idea how to avoid having to prefix match things...

xedin

Conditional onpackage changes Max suggested.

In order to extract symbol graphs for modules with transitive imports on C++ modules we need parse headers in C++ interop mode using the option: `-cxx-interoperability-mode=default`. This commit updates `swift-symbolgraph-extract` to pass the option when there is a C++ module in the import graph. This option is a new addition to `swift-symbolgraph-extract` added in swiftlang/swift#73963. Prior to this option's introduction, extracting the symbol graph for C++ dependent modules would fail with an error similar to: "unknown type name 'namespace'". With this SwiftPM commit and `swift-symbolgraph-extract` _prior_ to swiftlang/swift#73963, users will instead see an argument parsing error like: "unknown option -cxx-interoperability-mode". With this change and `swift-symbolgraph-extract` change in swiftlang/swift#73963, the symbol graph is extracted properly.

rauhul requested review from bnbarham, MaxDesiatov, jakepetroules, francescomikulis and xedin as code owners May 29, 2024 20:27

rauhul force-pushed the cxx-swift-symbolgraph-extract branch from 98ef23c to 0cd1433 Compare May 29, 2024 20:28

rauhul requested a review from egorzhdan May 29, 2024 20:28

MaxDesiatov added the C++ interop label May 30, 2024

MaxDesiatov reviewed May 30, 2024

View reviewed changes

MaxDesiatov added the needs tests This change needs test coverage label May 30, 2024

egorzhdan requested changes May 30, 2024

View reviewed changes

daniel-grumberg reviewed May 30, 2024

View reviewed changes

Sources/Commands/Utilities/SymbolGraphExtract.swift Outdated Show resolved Hide resolved

rauhul force-pushed the cxx-swift-symbolgraph-extract branch 2 times, most recently from 345fe41 to f9bb69f Compare May 31, 2024 00:31

rauhul requested review from egorzhdan, MaxDesiatov and daniel-grumberg May 31, 2024 00:31

rauhul added 2 commits May 31, 2024 00:05

Address review comments

9e5cfa2

rauhul force-pushed the cxx-swift-symbolgraph-extract branch from 4dcd4e4 to 9e5cfa2 Compare May 31, 2024 07:10

egorzhdan requested changes May 31, 2024

View reviewed changes

rauhul added 2 commits May 31, 2024 10:35

review feedback 2

c65aff0

update tests

af9b1b3

rauhul requested a review from egorzhdan May 31, 2024 17:50

rauhul mentioned this pull request Jun 1, 2024

Fix swift-symbolgraph-extract search paths #7621

Merged

egorzhdan approved these changes Jun 3, 2024

View reviewed changes

MaxDesiatov reviewed Jun 3, 2024

View reviewed changes

MaxDesiatov approved these changes Jun 3, 2024

View reviewed changes

xedin reviewed Jun 3, 2024

View reviewed changes

xedin approved these changes Jun 3, 2024

View reviewed changes

rauhul merged commit 73b0b26 into main Jun 3, 2024
5 checks passed

rauhul deleted the cxx-swift-symbolgraph-extract branch June 3, 2024 17:58

rauhul mentioned this pull request Jun 12, 2024

[6.0] Symbol graph cherry picks #7659

Merged

grynspan mentioned this pull request Oct 8, 2024

Swift test targets using C targets don't seem to respect C language standard. #7997

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cxx interop support to symbolgraph-extract #7610

Add cxx interop support to symbolgraph-extract #7610

rauhul commented May 29, 2024 •

edited by MaxDesiatov

Loading

MaxDesiatov left a comment

egorzhdan left a comment

egorzhdan May 30, 2024

rauhul May 30, 2024

rauhul May 30, 2024

egorzhdan May 31, 2024

rauhul commented May 31, 2024

rauhul commented May 31, 2024 •

edited

Loading

egorzhdan May 31, 2024

egorzhdan May 31, 2024

rauhul commented May 31, 2024

rauhul commented May 31, 2024

egorzhdan left a comment

egorzhdan Jun 3, 2024

rauhul Jun 3, 2024

MaxDesiatov Jun 3, 2024

MaxDesiatov Jun 3, 2024

MaxDesiatov Jun 3, 2024

rauhul commented Jun 3, 2024

xedin Jun 3, 2024

rauhul Jun 3, 2024 •

edited

Loading

xedin Jun 3, 2024

rauhul Jun 3, 2024

xedin Jun 3, 2024

xedin Jun 3, 2024

xedin left a comment

		/// If the current module or any of its linked dependencies requires cxx
		/// interop, cxx interop will be enabled on the current module.

	public func symbolGraphExtractArguments() throws -> [String] {
	package func symbolGraphExtractArguments() throws -> [String] {

Add cxx interop support to symbolgraph-extract #7610

Add cxx interop support to symbolgraph-extract #7610

Conversation

rauhul commented May 29, 2024 • edited by MaxDesiatov Loading

MaxDesiatov left a comment

Choose a reason for hiding this comment

egorzhdan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauhul commented May 31, 2024

rauhul commented May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauhul commented May 31, 2024

rauhul commented May 31, 2024

egorzhdan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauhul commented Jun 3, 2024

Choose a reason for hiding this comment

rauhul Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xedin left a comment

Choose a reason for hiding this comment

rauhul commented May 29, 2024 •

edited by MaxDesiatov

Loading

rauhul commented May 31, 2024 •

edited

Loading

rauhul Jun 3, 2024 •

edited

Loading