use unified overload data for initial overload variants

[QuietMisdreavus]

Bug/issue #, if applicable: rdar://127130136

Summary

When symbol graphs reflect overload groups that are different across different platforms, Swift-DocC runs into a situation where it can load inconsistent information based on which platform's mixins are loaded for the initial overloadsVariants. In some situations (where a symbol is only overloaded on one platform), this can even create a crash in debug builds since Swift-DocC trips an assertion about missing overload data from SymbolKit.

This PR builds on swiftlang/swift-docc-symbolkit#72 to use unified overload groups when calculating overload information. This allows Swift-DocC to present a unified view of overloads across every platform.

Dependencies

swiftlang/swift-docc-symbolkit#72

Testing

Use the following documentation catalog to test: asdf.docc.zip

This catalog contains two symbol graphs, one for macOS and one for iOS. These symbol graphs describe the following structure:

APhoneOnlyClass // iOS only
MyClass
- myFunc(param: APhoneOnlyClass) // iOS only
- myFunc(param: Double) // macOS only
- myFunc(param: Int)
- myFunc(param: String)

Prior to this PR, this would have created two overload groups: One based on myFunc(param: APhoneOnlyClass), and one based on myFunc(param: Double).

Steps:

1. With a recent Swift-DocC-Render set in $DOCC_HTML_DIR, run swift run docc preview /path/to/asdf.docc
2. Navigate to MyClass.
3. Ensure that only one link to myFunc(param:) exists.
4. Navigate to myFunc(param:) and expand the overloaded declarations.
5. Verify that the overloaded declaration listing contains all four overloaded methods.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• Added tests
• Ran the ./bin/test script and it succeeded
• [ n/a ] Updated documentation if necessary

[QuietMisdreavus]

@swift-ci Please test

[QuietMisdreavus]

@swift-ci Please test

[patshaughnessy]

Looks great - just nerd sniped some of your code to try to make it a bit more readable. The tests pass with my version but let me know what you think.

[patshaughnessy]

How about:

Suggested change

	let overloadGroups: [String: Set<String>] =
	unifiedSymbolGraph.relationshipsByLanguage.values.joined().filter({
	$0.kind == .overloadOf
	}).map({
	(key: $0.target, value: $0.source)
	}).reduce([:], { acc, kv in
	var acc = acc
	acc[kv.key, default: []].insert(kv.value)
	return acc
	})
	let overloadRelationships = unifiedSymbolGraph.relationshipsByLanguage.values.joined().filter({
	$0.kind == .overloadOf
	})
	let relationshipsByTarget = [String: [SymbolGraph.Relationship]](grouping: overloadRelationships, by: { $0.target })
	let overloadGroups = relationshipsByTarget.mapValues({
	Set($0.map{relationship in relationship.source})
	})

This might be a bit slower and/or allocate a bit of memory unnecessarily, but maybe splitting up the expression a bit with some intermediate values like this makes the code a lot easier to read.

[patshaughnessy]

Also maybe extract this into a separate function.

[QuietMisdreavus]

I do admit i was particularly Haskell-brained when i wrote these PRs, but i'm not sure i want to use this version that creates multiple intermediate variable bindings just to break up the code.

@d-ronnqvist What do you think? I could have sworn there would be a better Dictionary initializer i could use, but this reduce was the closest thing i could find.

[d-ronnqvist]

I believe that it's possible to use init(_:uniquingKeysWith:) to combine the values, like this

let overloadGroups = [String: Set<String>](
    unifiedSymbolGraph.relationshipsByLanguage.values.joined().filter({
        $0.kind == .overloadOf
    }).map({
        (key: $0.target, value: $0.source)
    }),
    uniquingKeysWith: { $0.union($1) }
)

Using reduce(into:_:) is also fine but reduce(_:_:) with an unlabeled first parameter will end up making copies of the Set for each iteration of the loop. This accidentally makes an O(n) looking implementation O(n²).

This was actually the motivation for adding reduce(into:_:) in SE-0171:

The current version of reduce needs to make copies of the result type for each element in the sequence. The proposed version can eliminate the need to make copies (when the inout is optimized away).

Regarding the sequence portion; I suspect that only a very small number of relationships will be .overloadOf. As such, I suspect that it's faster to filter the allocations first and join them after. It's also possible to combine filter and map into one iteration using compactMap:

let overloadRelationships: [(String, String)] = unifiedSymbolGraph.relationshipsByLanguage.values.flatMap {
    $0.compactMap {
        guard $0.kind == .overloadOf else { return nil }
        return (key: $0.target, value: $0.source)
    }
}
let overloadGroups = [String: Set<String>](overloadRelationships, uniquingKeysWith: { $0.union($1) })

Compared to this, using reduce(into:_:) has a slight readability benefit because it means that the value doesn't need to be mapped to a key-value-pair in the sequence. This means that we can use filter and access the source and target directly in the reduce(into:_:) closure:

let overloadGroups: [String: Set<String>] = unifiedSymbolGraph.relationshipsByLanguage
    .values
    .flatMap {
        $0.filter { $0.kind == .overloadOf }
    }.reduce(into: [:]) { acc, rel in
        acc[rel.target, default: []].insert(rel.source)
    }

[d-ronnqvist]

You could also move the filter and loop inside the reduce(into:_:) closure:

let overloadGroups: [String: Set<String>] = unifiedSymbolGraph.relationshipsByLanguage
    .values
    .reduce(into: [:]) { acc, relationships in
        for rel in relationships where rel.kind == .overloadOf {
            acc[rel.target, default: []].insert(rel.source)
        }
    }

[QuietMisdreavus]

Aha! Thanks for the note about reduce(into:_:). I was a bit concerned about rebinding the accumulator like that; i'm glad there's an alternative that can also save an allocation.

[QuietMisdreavus]

@swift-ci Please test

[mayaepps]

Is this comment intended to be here? It makes it sound like there should be an iOS symbol graph, which there isn't for this test

[QuietMisdreavus]

Good eye! I copy/pasted the assertion code between tests and i guess this comment slipped past me. 😅

[QuietMisdreavus]

@swift-ci Please test