Skip to content

use SymbolKit's unified graph overload grouping #986

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.

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

Merged
merged 6 commits into from
Jul 25, 2024

Conversation

QuietMisdreavus
Copy link
Contributor

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

Summary

This PR uses a new overload grouping implementation in SymbolKit to create overload group symbols in the unified symbol graph, rather than unifying individual symbol graphs' overload groups after the fact. This fixes a bug where disjoint overload groups (whether between platforms or via extensions) were not being combined.

Dependencies

swiftlang/swift-docc-symbolkit#81

Testing

This example doc catalog contains two symbol graphs, one for macOS and one for iOS, each with one symbol that technically overload. It was generated from the following source code:

@available(macOS, unavailable)
public func myFunc(param: String) {}

@available(iOS, unavailable)
public func myFunc(param: Int) {}

Steps:

  1. Download and extract asdf.docc, linked above.
  2. swift run docc preview --enable-experimental-overloaded-symbol-presentation /path/to/asdf.docc
  3. Navigate to the resulting documentation for CrossPlatformOverloads.
  4. Ensure that only one page for myFunc(param:) is curated at the top level.
  5. If you have a recent main-branch build of Swift-DocC-Render set into the DOCC_HTML_DIR environment variable, ensure that both the Int and String variants of myFunc(param:) are shown in the resulting overload page.

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
Copy link
Contributor Author

@swift-ci Please test

Comment on lines 109 to 129
var symbols = graph.symbols
var relationships = graph.relationships

// Overload group symbols are added directly to the unified graph, not any individual
// graphs, so load up any relevant symbols and relationships before continuing
if let unifiedSelector = UnifiedSymbolGraph.Selector(forSymbolGraph: graph),
let unifiedGraph = loader.unifiedGraphs[moduleNode.name],
!unifiedGraph.overloadGroupSymbols.isEmpty
{
for overloadGroupIdentifier in unifiedGraph.overloadGroupSymbols {
if let unifiedSymbol = unifiedGraph.symbols[overloadGroupIdentifier],
let overloadSymbol = SymbolGraph.Symbol(fromUnifiedSymbol: unifiedSymbol, selector: unifiedSelector) {
symbols[overloadGroupIdentifier] = overloadSymbol
}
}

let overloadRelationships = unifiedGraph.relationshipsByLanguage[unifiedSelector]?.filter({ relationship in
unifiedGraph.overloadGroupSymbols.contains(relationship.source) || unifiedGraph.overloadGroupSymbols.contains(relationship.target)
}) ?? []
relationships.append(contentsOf: overloadRelationships)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than mixing symbol graph data and unified symbol graph data. Can we add the overload groups after all the symbol graphs (outside this loop)? That would allow for all the overload specific logic to move into one place.

I can't checkout that branch of SymbolKit, so I couldn't test this but I think the only thing that the path hierarchy needs to do with overload groups are:

  • Disfavor the overloaded symbols in the overload group
  • Add a node for the overload group in the same location as the overloaded symbols

If so, we should be able to do that using something like this:

// Overload group don't exist in the individual symbol graphs.
// Since overload groups don't change the _structure_ of the path hierarchy, we can add them after after all symbols for all platforms have already been added.
for unifiedGraph in loader.unifiedGraphs.values {
    // Create nodes for all the overload groups
    let overloadGroupNodes: [String: Node] = unifiedGraph.overloadGroupSymbols.reduce(into: [:]) { acc, uniqueID in
        assert(allNodes[uniqueID] == nil,
               "Overload group ID \(uniqueID) already has a symbol node in the hierarchy: \(allNodes[uniqueID]!.map(\.name).sorted().joined(separator: ","))")
        guard let unifiedSymbol = unifiedGraph.symbols[uniqueID] else { return }
        guard let symbol = unifiedSymbol.defaultSymbol else {
            fatalError("Overload group \(uniqueID) doesn't have a default symbol.")
        }
        acc[uniqueID] = Node(symbol: symbol, name: symbol.pathComponents.last!)
    }
    
    for relationship in unifiedGraph.relationships where relationship.kind == .overloadOf {
        guard let groupNode = overloadGroupNodes[relationship.source], let overloadedSymbolNodes = allNodes[relationship.target] else {
            continue
        }
        
        for overloadedSymbolNode in overloadedSymbolNodes {
            // We want to disfavor the individual overload symbols in favor of resolving links to their overload group symbol.
            overloadedSymbolNode.specialBehaviors.formUnion([.disfavorInLinkCollision, .excludeFromAutomaticCuration])
            
            guard let parent = overloadedSymbolNode.parent else { continue }
            
            assert(groupNode.parent == nil || groupNode.parent === parent, """
            Unexpectedly grouped symbols with different locations in the symbol hierarchy:
            Group ID: \(groupNode.symbol!.identifier.precise)
            Locations: \(Set(overloadedSymbolNodes.map { $0.symbol!.pathComponents.joined(separator: "/") }.sorted()))
            """)
            parent.add(symbolChild: groupNode)
        }
        assert(groupNode.parent != nil, "Unexpectedly found no location in the hierarchy for overload group \(relationship.source)")
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started trying to do this, but i wound up trying to reimplement the single-symbol logic instead. I'll try your version and see if that works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't checkout that branch of SymbolKit, so I couldn't test this

Whoops, i forgot to update DocC's Package.resolved after i amended my commit. 😅 You should be able to run swift package update swift-docc-symbolkit to get a valid commit. I'll push that change here once i get your code worked in.

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@@ -170,7 +148,7 @@ struct PathHierarchy {
}
}

for relationship in relationships where relationship.kind == .overloadOf {
for relationship in graph.relationships where relationship.kind == .overloadOf {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly we should be able to remove this entire for loop since the individual symbol graph files wouldn't contain any .overloadOf relationships anymore.

Copy link
Contributor

@d-ronnqvist d-ronnqvist Jul 17, 2024

Choose a reason for hiding this comment

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

Similarly, if I understand correctly we should be able to remove this if-statement since the individual symbol graph files wouldn't contain any overload group symbols—with overloadGroupIdentifierSuffix suffixes in their unique identifiers—anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, good catch. I'll take those out.

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus QuietMisdreavus merged commit 081ad56 into swiftlang:main Jul 25, 2024
2 checks passed
@QuietMisdreavus QuietMisdreavus deleted the split-overloads branch July 25, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants