[Multiplatform] Update Graph to store filter data in edges.

[waltflanagan]

Short description 📝

This is a component of multiplatform support to capture PlatformFilter information within the graph. These are captured as edges and used when resolving GraphDependency into GraphDependencyReference types which then are used to generate Xcode projects.

How to test the changes locally 🧐

Tests should pass.

Contributor checklist ✅

• The code has been linted using run make workspace/lint-fix
• The change is tested via unit testing or acceptance testing, or both
• The title of the PR is formulated in a way that is usable as a changelog entry
• In case the PR introduces changes that affect users, the documentation has been updated

Reviewer checklist ✅

• The code architecture and patterns are consistent with the rest of the codebase
• Reviewer has checked that, if needed, the PR includes the label changelog:added, changelog:fixed, or changelog:changed, and the title is usable as a changelog entry

[kwridan]

Nice work @waltflanagan 🎉

[kwridan]

Thanks for the updates @waltflanagan

I did an integration test using #5381 (and merging those changes into it) on our project and oddly now I'm seeing some unexpected filters being applied - will investigate further

[kwridan]

I narrowed down the issue, I think we're incorrectly merging filters from sibling graph paths

e.g.

For example when asking for filters for App > Static Framework C - we expect there to be none however we get .iOS 🙈

//
    // App -> Static Framework A -> SDK A (platformFilter = [.iOS])
    //     -> Static Framework B -> Static Framework C
    //
    func test_platformFilters_transitivePlatformFilter_2() throws {
        // Given
        let app = Target.test(name: "App", destinations: [.iPad, .iPhone, .mac], product: .app)
        let staticFrameworkA = Target.test(
            name: "StaticFrameworkA",
            destinations: [.iPad, .iPhone, .mac],
            product: .staticLibrary
        )
        
        let staticFrameworkB = Target.test(
            name: "StaticFrameworkB",
            destinations: [.iPad, .iPhone, .mac],
            product: .staticLibrary
        )
        
        let staticFrameworkC = Target.test(
            name: "StaticFrameworkC",
            destinations: [.iPad, .iPhone, .mac],
            product: .staticLibrary
        )
        
        let project = Project.test(targets: [app, staticFrameworkA, staticFrameworkB, staticFrameworkC])
        
        let appkGraphDependency: GraphDependency = .target(name: app.name, path: project.path)
        let staticFrameworkAGraphDependency: GraphDependency = .target(name: staticFrameworkA.name, path: project.path)
        let staticFrameworkBGraphDependency: GraphDependency = .target(name: staticFrameworkB.name, path: project.path)
        let staticFrameworkCGraphDependency: GraphDependency = .target(name: staticFrameworkC.name, path: project.path)
        let sdkGraphDependency: GraphDependency = .testSDK(name: "CoreTelephony.framework")
        
        let graph = Graph.test(
            path: project.path,
            projects: [project.path: project],
            targets: [
                project.path: [
                    app.name: app,
                    staticFrameworkA.name: staticFrameworkA,
                    staticFrameworkB.name: staticFrameworkB,
                    staticFrameworkC.name: staticFrameworkC,
                ]
            ],
            dependencies: [
                appkGraphDependency: [
                    staticFrameworkAGraphDependency,
                    staticFrameworkBGraphDependency,
                ],
                staticFrameworkAGraphDependency: [
                    sdkGraphDependency,
                ],
                staticFrameworkBGraphDependency: [
                    staticFrameworkCGraphDependency,
                ]
            ],
            edges: [
                GraphEdge(from: staticFrameworkAGraphDependency, to: sdkGraphDependency): [.ios],
            ]
        )
        let subject = GraphTraverser(graph: graph)
        
        // When
        let results = subject.platformFilters(from: appkGraphDependency, to: staticFrameworkCGraphDependency)
        
        // Then
        XCTAssertEqual(results?.sorted(), [])
    }

[kwridan]

An attempt to fix the issue

func platformFilters(from rootDependency: GraphDependency, to transitiveDependency: GraphDependency) -> PlatformFilters? {
        enum PlatformFilterSearchResult {
            case invalid
            case noFilters
            case filters(PlatformFilters)
            case disjoint
        }
        
        var cache: [GraphEdge: PlatformFilterSearchResult] = [:]

        func find(from root: GraphDependency, to other: GraphDependency) -> PlatformFilterSearchResult {
            let edge = GraphEdge(from: root, to: other)
            if let result = cache[edge] {
                return result
            }
            
            let result: PlatformFilterSearchResult
            defer {
                cache[edge] = result
            }
            
            guard let dependencies = graph.dependencies[root] else {
                result = .invalid
                return result
            }
            
            if dependencies.contains(other) {
                // terminate search
                if let filters = graph.edges[edge] {
                    result = .filters(filters)
                    return result
                } else {
                    result = .noFilters
                    return result
                }
            } else {
                let dependencyResults = dependencies.map { dependency in
                    let dependencyResult = find(from: dependency, to: other)
                    if let dependencyFilters = graph.edges[(root, dependency)], !dependencyFilters.isEmpty {
                        switch dependencyResult {
                        case .invalid:
                            return PlatformFilterSearchResult.invalid
                        case .filters(let filters):
                            let commonFilters = dependencyFilters.intersection(filters)
                            if commonFilters.isEmpty {
                                return .disjoint
                            } else {
                                return .filters(commonFilters)
                            }
                        case .noFilters:
                            return .filters(dependencyFilters)
                        case .disjoint:
                            return dependencyResult
                        }
                    } else {
                        return dependencyResult
                    }
                }
                
                result = dependencyResults.reduce(.invalid) {
                    partialResult, dependencyResult in
                    switch (dependencyResult, partialResult) {
                    case (.invalid, _):
                        // ignore invalid paths (i.e. ones that don't contain the target dependency)
                        return partialResult
                    case (_, .invalid):
                        // ignore invalid paths (i.e. ones that don't contain the target dependency)
                        return dependencyResult
                    case (.noFilters, _):
                        // if one of the paths has no filters, that takes precedence over a path with filters
                        return .noFilters
                    case (_, .noFilters):
                        // if one of the paths has no filters, that takes precedence over a path with filters
                        return .noFilters
                    case (.filters(let dependencyFilters), .filters(let collectedFilters)):
                        return .filters(dependencyFilters.union(collectedFilters))
                    case (.disjoint, _):
                        return partialResult
                    case (_, .disjoint):
                        return dependencyResult
                    }
                }
                return result
            }
        }

        let result = find(from: rootDependency, to: transitiveDependency)
        switch result {
        case .invalid:
            return []
        case .noFilters:
            return []
        case .filters(let filters):
            return filters
        case .disjoint:
            return nil
        }
    }

Notes:

• I wonder do we need to handle the disjoint case at the platform filter level? perhaps in this case there's a user error which would already be caught at linting stage?
  • Does the disjoint case already occur in another case - when applying filters applicable to a target? (I recall the conclusion there Xcode may take care of it?)
• It can probably be simplified further (merge .noFilters and .filters cases as no filters is .filters([])
• The test test_platformFilterResolution_when_targetHasTransitiveDependencies a few adjustments to make it more representative of the use case
  • multiPlatformFramework was changed to .staticFramework
  • App > macFramework dependency needed to be removed
  • Results needed a re-order
• ⚠️ This extra traversal adds over a 1s to our generation time 🙈

[kwridan]

thanks for the updates @waltflanagan

[kwridan]

Thanks @waltflanagan

Testing against our project I'm seeing many missing dependencies - there's still some room to remove some of the ambiguity between no filters and disjoint case.

I'm thinking possibly we defer the disjoint case all together and tackle it separately subsequently after integrating the public API? as it will help better test it?

[kwridan]

Thanks @waltflanagan

• ✅ Testing this in conjunction with other PR I think we ironed out most of the traversal issues!
• ⚠️ The additional traversals sadly do increase generation time (~ +30% on our project)
  • I think it's unavoidable as the extra traversals are needed. We'll need to subsequently find optimisations to eliminate some unnecessary ones or consolidate some ... but that can come after we finalise the first iteration of the feature (correctness first, then we can look for optimisations! 🚀)
• 🟠API wise, I think we should try to retain some of the semantics where [] / no specified filters is the default and behaves as it does today as opposed to making the default .all (some rationale / thinking below) - happy for us to merge this PR and follow up ahead of public API to reduce the rebasing overhead (apologies for the slow review times)

Platform Filters

PlatformFilters is currently a set and we're trying to encode the following information into it

• 1. Some specific filters are explicitly specified
• 2. No filters are explicitly specified
• 3. Disjoint filters were resolved

It would be helpful to model this more explicitly rather than encode it into the set (I appreciate there's some convenience to that within the traversal method)

• Xcode semantics do somewhat confuse matters depending on how you view / interpret it 😅
  • By default no filters are explicitly specified and is displayed as "Always used"
  • Attempting to modify the filters only shows applicable filters as opposed to all possible ones
    • For example Xcode wouldn't allow specifying say a driver kit platform filter for an iOS + Mac multi platform app
    • As such semantically, defaulting edges to .all (read as [.ios, .macos, .tvos, .catalyst, .driverkit, .watchos, .visionos]) seems a bit odd / inconsistent with this
  • We can potentially offer lint rules to warn incase a target specifies filters that are not applicable (based on its declared destinations) - however this will not be possible if the default is .all (as it's ambiguous if users explicitly defined those or if they didn't specify it at all and just so happens to be our implementation detail for unspecified)

Thanks again @waltflanagan for all your hard work on this!!

[kwridan]

Thanks @waltflanagan

All my comments have been addressed in the follow up PR #5595 (review)

We can proceed with this as a first step and follow up with the introduction of Condition as you have in the other PR to make it clearer 🎉