Fix package diamond resolution by removing package target filtering

[yimajo]

Problem

When an app links multiple package products that share the same package target, the shared target needs to be made dynamic so that it is not statically linked through multiple paths.

However, after a package product is linked by both the app and a dynamic framework and is made dynamic as a product-level diamond, that product's dependency target could be excluded from target-level diamond detection. As a result, the package target that should be shared could fail to be replaced with its dynamic variant and remain split across another product's static link path. In that case, types with the same name from the same package target can exist multiple times in the same process, which breaks runtime type identity.

Cause

Before this change, dependency targets of an already-dynamic package product were added to packageTargetsToSkip and excluded from later target-level diamond detection.

Because this exclusion was done by target identity, it also removed the same target from target-level diamond detection when that target entered the same process through another product.

The already-dynamic package product itself is still excluded by the existing dynamicallyBuildingTargets check. This change does not add an already-dynamic package product back to product-level diamond detection.

The packageTargetsToIgnore behavior is also kept, so diagnostics for product-level and target-level diamonds are not duplicated within the same pass.

With this change, even when a dependency target belongs to an already-dynamic package product, it remains eligible for target-level diamond detection if it also enters the same process through another product. It can then be replaced with its dynamic variant.

Not addressed in this PR

There is a separate issue that this Pull Request does not address: diamond resolution runs in a loop, but the loop can terminate before all diamond problems are resolved in order to avoid an infinite loop. This allows an unresolved diamond problem to pass through the build without being diagnosed, so the problem is only discovered at runtime.

Changes

• Fix package diamond resolution
  • Remove the package target exclusion done by packageTargetsToSkip
• Add test
• Improve performance
  • Use reverse dependency traversal for package diamond detection
  • Avoid redundant traversal when computing static transitive linked dependencies

Fix package diamond resolution

This PR removes the package target exclusion done by packageTargetsToSkip.

packageTargetsToSkip could skip package targets that still needed target-level diamond detection. As a result, Swift Build could fail to rewrite the build graph to use the required dynamic variants, leaving the same package target linked through multiple static paths.

That allowed the build to complete even though duplicated package code remained, so the problem could surface only at runtime as duplicated types with different runtime identities.

Test case

The following configuration reproduces the issue, so this PR adds it as a test case.

App
├── iOSFwk
│   └── PackageLibProduct
│       └── PackageLib
│           └── Common
├── PackageLibProduct
│   └── PackageLib
│       └── Common
└── PackageLibProduct2
    └── PackageLib2
        └── Common

In this configuration, PackageLibProduct is linked by both the app and iOSFwk, which is a dynamic framework, so it is first made dynamic as a product-level diamond.

Before this change, PackageLib, which is a dependency of PackageLibProduct, was then added to packageTargetsToSkip and excluded from target-level diamond detection.

As a result, PackageLib was not replaced with PackageLib-dynamic. The Common reached through PackageLibProduct2 -> PackageLib2 could still be replaced with Common-dynamic, but the Common reached through PackageLibProduct-dynamic -> PackageLib remained statically linked inside PackageLibProduct-dynamic.

This left two copies of Common in the process, as shown below: one copy was statically linked inside PackageLibProduct-dynamic, and the other was linked through Common-dynamic.

App
├── iOSFwk
│   └── PackageLibProduct-dynamic
│       └── PackageLib
│           └── Common
│
├── PackageLibProduct-dynamic
│   └── PackageLib
│       └── Common
│
└── PackageLibProduct2
    └── PackageLib2
        └── Common-dynamic

Improve performance

The primary goal of this PR is to fix package diamond resolution.

Removing packageTargetsToSkip brings package targets that still need target-level diamond detection back into the check. That work is necessary to produce the correct build graph, but with the existing traversal strategy it also exposes traversal cost that was previously hidden by the incorrect skip.

Instead of reducing the checked targets incorrectly, this PR changes the diamond detection traversal so shared dependency subgraphs are not walked repeatedly.

The performance-related changes are:

Use reverse dependency traversal for package diamond detection

This PR changes the package diamond detection algorithm to use a DFS-style walk over a reverse direct-dependency graph. Instead of collecting top-level linking targets while iterating each dynamic Mach-O target's dependencies, it now uses a separate TopLevelLinkingTargetResolver type to resolve the top-level linking targets for each package product or package target from the reverse graph.

This avoids repeatedly walking dependency subgraphs from each dynamic Mach-O target when determining which top-level targets link a package product or package target.

Avoid redundant traversal when computing static transitive linked dependencies

On main, static transitive linked dependencies were computed by running transitiveClosure once for each direct dependency.

As a result, when multiple direct dependencies reached the same static dependency subgraph, that subgraph was traversed once per direct dependency. Those repeated traversals did not change the final dependency set, so they were unnecessary work.

This PR changes the computation to seed a single transitiveClosure with all direct linked dependencies. This lets each shared static dependency subgraph be traversed only once.

[owenv]

@swift-ci test

[owenv]

Without this set, the traversal here can easily be exponential on highly connected graphs. Could we do this correctly if we reversed the edges and did a single DFS pass?

[yimajo]

Thank you for the review. I agree that handling this with DFS is probably the best long-term direction.

However, changing the current logic, which is based ondependenciesByTarget: [ConfiguredTarget: OrderedSet<LinkedDependency>] passed to resolveDiamondProblemsInPackages, to a DFS-based approach would also require revisiting the product-level vs target-level check ordering, dynamic variant replacement, and diagnostic suppression within the same pass. I think that would make the scope of the change larger.

As a separate alternative, I noticed that the current code keeps some values in Arrays even though they are used for membership checks. Since Sets are not used there, the same membership-style checks can do unnecessary repeated work. I think that part would be a relatively small local improvement.

https://github.com/yimajo/swift-build/blob/d6a606ebdee57c8300237465048f6f2fb4acb5bc/Sources/SWBTaskConstruction/ProductPlanning/ProductPlan.swift#L909

https://github.com/yimajo/swift-build/blob/d6a606ebdee57c8300237465048f6f2fb4acb5bc/Sources/SWBTaskConstruction/ProductPlanning/ProductPlan.swift#L960

[yimajo]

@owenv I updated the implementation to use the suggested reverse-edge DFS-style traversal.

I also addressed the LinkedDependency overhead mentioned in my previous comment: staticTransitive no longer keeps the unused origin, and its transitive closure is computed once from all direct dependencies instead of once per direct dependency.

So this now combines both improvements. I expect the combination to significantly reduce the extra work on highly connected graphs.

[yimajo]

@swift-ci test

[mbrandonw]

Hi @yimajo, just wanted to mention that this fixes a gnarly problem that has plagued Xcode's build system for many years (at least since 2019). I have an example project here that demonstrates the problem, and using your swift-build fixes it.

[owenv]

Thanks, this approach looks reasonable, left a few smaller comments. Could you please squash the commits as well?

[owenv]

This has the potential to go quadratic, can we look up the ConfiguredTargets in a dictionary by ID here?

[yimajo]

That makes sense.

This was code on main that searched in a loop, and I moved it into a method early on without thinking it through carefully.

let dependencies = configuredTarget.target.dependencies.compactMap { targetDependency in
    planRequest.buildGraph.allTargets.first(where: { $0.target.guid == targetDependency.guid })
}

After thinking about it in response to the review, I realized that this simply needs the dependencies from the Package Product's configuredTarget.

So there is no need to loop and search for them in the first place.

let dependencies = planRequest.buildGraph.dependencies(of: configuredTarget)

This seems likely to remove quite a bit of unnecessary work, more than I expected.

[owenv]

I don't think it's necessarily correct to only consider the first dependency here

[owenv]

The old code was inconsistent, but we should try to use conforms(to:) checks for product types instead of string matching

[owenv]

This would be good to switch over to a conforms(to:) check as well

[yimajo]

Just to confirm, conforms(to:) does not seem to exist here, so I will use conformsTo(identifier:) instead.

Please let me know if I am missing something.

[owenv]

yeah sorry, I meant conformsTo(identifier:).

[owenv]

What's the purpose of unioning these two, shouldn't reverseDirectDependenciesByTarget.keys be a subset of dependenciesByTarget.keys?

[owenv]

Please add this new file to CMakeLists.txt

[owenv]

@swift-ci test

[yimajo]

@swift-ci test

[owenv]

This pretty much LGTM but I want to review the test results before approving

[owenv]

@swift-ci test

[owenv]

LGTM, thanks for the contribution. I'm going to run a cross-repo test on this before merging to be safe

[Brett-Best]

This ended up causing my builds to fail due to the copy files build phase, Claude came up with the below which does fix my build and also confirms that this PR fixes the duplicated symbols issue I was seeing.

Let me know if you want me to try and put together a minimal reproduction for it or if this diff is clear enough / provides direction for a fix.

diff --git a/Sources/SWBTaskConstruction/TaskProducers/BuildPhaseTaskProducers/CopyFilesTaskProducer.swift b/Sources/SWBTaskConstruction/TaskProducers/BuildPhaseTaskProducers/CopyFilesTaskProducer.swift
index d3ff8cab..58192569 100644
--- a/Sources/SWBTaskConstruction/TaskProducers/BuildPhaseTaskProducers/CopyFilesTaskProducer.swift
+++ b/Sources/SWBTaskConstruction/TaskProducers/BuildPhaseTaskProducers/CopyFilesTaskProducer.swift
@@ -204,6 +204,20 @@ class CopyFilesTaskProducer: FilesBasedBuildPhaseTaskProducerBase, FilesBasedBui
     func addTasksForUngroupedFile(_ ftb: FileToBuild, _ buildFilesContext: BuildFilesProcessingContext, _ scope: MacroEvaluationScope, _ tasks: inout [any PlannedTask]) async {
         let dstFolder = computeOutputDirectory(scope)
 
+        // swift-build #1424 regression fix: a package target converted to build dynamically as a
+        // framework (a diamond-resolved dependency) has a dynamic-variant product reference whose
+        // name is `$(EXECUTABLE_NAME)`, which resolves to the bare binary path (e.g.
+        // `.../PackageFrameworks/_NumericsShims`) rather than the `.framework` wrapper. The framework
+        // bundle IS built (`.../PackageFrameworks/_NumericsShims.framework`), so embedding the bare
+        // path fails with "No such file or directory". When copying a framework whose resolved path
+        // lacks the wrapper, correct it to the `.framework`. This is a no-op for normal framework
+        // references, whose resolved path already ends in `.framework`.
+        var ftb = ftb
+        if ftb.fileType.isFramework, ftb.absolutePath.fileExtension != "framework" {
+            let wrapperPath = ftb.absolutePath.dirname.join("\(ftb.absolutePath.basename).framework")
+            ftb = FileToBuild(absolutePath: wrapperPath, fileType: ftb.fileType, buildFile: ftb.buildFile, uniquingSuffix: ftb.uniquingSuffix, shouldUsePrefixHeader: ftb.shouldUsePrefixHeader, regionVariantName: ftb.regionVariantName, indexingInputReplacement: ftb.indexingInputReplacement)
+        }
+
         // If the file is already in the destination path (e.g., a build rule placed it there), we are done and should not continue processing.
         if dstFolder.isAncestorOrEqual(of: ftb.absolutePath.dirname) {
             return

[danielpunkass]

I filed #1482 for the issue @Brett-Best identified above.