Fix a few mismatches between the two link resolver implementations (#375)

* Avoid reporting mismatches for symbols that won't have pages

* Avoid traversing symbols that are not in the symbol index

* Handle collisions that only occur when symbol names are mapped to file names

* Don't add default implementation symbols to the target's parent path

* Fix a data race when gathering link resolution mismatches.

Also, avoid percent encoding in the gathered info.

* Fix incorrect behaviors when both link resolvers run to gather mismatches

* Remove real file paths in test data

* Combine two consecutive if-statements with same condition.

rdar://99891308

Author: d-ronnqvist

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

@@ -1137,7 +1137,7 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
             // Build references for all symbols in all of this module's symbol graphs.
             let symbolReferences: [SymbolGraph.Symbol.Identifier : [ResolvedTopicReference]]
             if LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver {
-                symbolReferences = hierarchyBasedLinkResolver!.referencesForSymbols(in:symbolGraphLoader.unifiedGraphs, bundle: bundle, context: self)
+                symbolReferences = hierarchyBasedLinkResolver!.referencesForSymbols(in: symbolGraphLoader.unifiedGraphs, bundle: bundle, context: self)
                     .mapValues({ [$0] }) // The documentation cache implementation uses an array of values to handle multi languages
             } else {
                 symbolReferences = documentationCacheBasedLinkResolver.referencesForSymbols(in: symbolGraphLoader.unifiedGraphs, bundle: bundle, context: self)
@@ -1275,9 +1275,11 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
 
             try shouldContinueRegistration()
 
-            if let hierarchyBasedLinkResolver = hierarchyBasedLinkResolver {
-                // Map the resolved references with their identifiers
-                hierarchyBasedLinkResolver.addMappingForSymbols(symbolIndex: symbolIndex)
+            // Only add the symbol mapping now if the path hierarchy based resolver is the main implementation.
+            // If it is only used for mismatch checking then we must wait until the documentation cache code path has traversed and updated all the colliding nodes.
+            // Otherwise the mappings will save the unmodified references and the hierarchy based resolver won't find the expected parent nodes when resolving links.
+            if LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver {
+                hierarchyBasedLinkResolver!.addMappingForSymbols(symbolIndex: symbolIndex)
             }
 
             if hierarchyBasedLinkResolver == nil || LinkResolutionMigrationConfiguration.shouldReportLinkResolutionMismatches {
@@ -1311,20 +1313,22 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
                         })
                 }
 
-                
                 // Only update the nodes and symbol index when using the documentation cache-based link resolver otherwise the context will end up in an inconsistent state.
-                if hierarchyBasedLinkResolver == nil {
+                if !LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver {
                     // Update the children of collision URLs. Walk the tree and update any dependents of updated URLs
                     for moduleReference in moduleReferences.values {
                         try symbolsURLHierarchy.traversePreOrder(from: moduleReference) { reference in
                             try self.documentationCacheBasedLinkResolver.updateNodeWithReferenceIfCollisionChild(reference, symbolsURLHierarchy: &symbolsURLHierarchy, symbolIndex: &symbolIndex, context: self)
                         }
                     }
+                    
+                    // Now that the colliding references have been updated, the hierarchy based resolver can save the reference to identifier mapping.
+                    hierarchyBasedLinkResolver?.addMappingForSymbols(symbolIndex: symbolIndex)
                 }
 
                 if LinkResolutionMigrationConfiguration.shouldReportLinkResolutionPathMismatches {
                     // The way that symbol path mismatches are gathered depend on which link resolution implementation is used to resolve links.
-                    if let hierarchyBasedLinkResolver = hierarchyBasedLinkResolver {
+                    if LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver {
                         // Attempting to use the The documentation cache based link resolver to compute the disambiguated symbol paths when it is not in full control of the symbol index,
                         // topic graph, and documentation cache will result in the wrong behavior. The issues range from incorrectly computed paths to precondition failures.
                         //
@@ -1337,7 +1341,7 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
                         // Luckily the path hierarchy describe the hierarchical relationships between the symbols in a way where it's possible to get the `Symbol.Identifier` for each symbol.
                         // By sorting the pairs of `(symbol, parent symbol)` by the symbol's number of path components it's possible to traverse the hierarchy breath first and update the initial disambiguated
                         // paths from the documentation cache based link resolver so that each child path matches the disambiguation from the parent path.
-                        let sortedSymbolAndParentPairs = hierarchyBasedLinkResolver.pathHierarchy.lookup.values.lazy
+                        let sortedSymbolAndParentPairs = hierarchyBasedLinkResolver!.pathHierarchy.lookup.values.lazy
                             .filter { $0.symbol != nil && $0.parent?.symbol != nil }
                             .sorted(by: \.symbol!.pathComponents.count)
                             .map { ($0.symbol!.identifier, $0.parent!.symbol!.identifier) }
@@ -1362,7 +1366,7 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
                                 linkResolutionMismatches.pathsWithMismatchedDisambiguation[hierarchyBasedReference.path] = cacheBasedMainReference.path
                             }
                         }
-                        for (id, cacheBasedReference) in cacheBasedReferences where !hierarchyBasedReferences.keys.contains(id) {
+                        for (id, cacheBasedReference) in cacheBasedReferences where !hierarchyBasedReferences.keys.contains(id) && symbolGraphLoader.hasPrimaryURL(moduleName: cacheBasedReference.pathComponents[2]) {
                             linkResolutionMismatches.missingPathsInHierarchyBasedLinkResolver.append(cacheBasedReference.path)
                         }
                     } else {
@@ -2553,15 +2557,15 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
                 let cacheBasedResult = documentationCacheBasedLinkResolver.resolve(unresolvedReference, in: parent, fromSymbolLink: isCurrentlyResolvingSymbolLink, context: self)
 
                 let inputInfo = LinkResolutionMismatches.FailedLinkInfo(
-                    path: unresolvedReference.topicURL.url.withoutHostAndPortAndScheme().absoluteString,
-                    parent: parent.url.withoutHostAndPortAndScheme().absoluteString,
+                    path: unresolvedReference.topicURL.url.path + (unresolvedReference.topicURL.url.fragment.map { "#" + $0 } ?? ""),
+                    parent: parent.url.path,
                     asSymbolLink: isCurrentlyResolvingSymbolLink
                 )
                 switch (hierarchyBasedResult, cacheBasedResult) {
                 case (.success, .failure):
-                    linkResolutionMismatches.mismatchedLinksThatCacheBasedLinkResolverFailedToResolve.insert(inputInfo)
+                    linkResolutionMismatches.mismatchedLinksThatCacheBasedLinkResolverFailedToResolve.sync { $0.insert(inputInfo) }
                 case (.failure, .success):
-                    linkResolutionMismatches.mismatchedLinksThatHierarchyBasedLinkResolverFailedToResolve.insert(inputInfo)
+                    linkResolutionMismatches.mismatchedLinksThatHierarchyBasedLinkResolverFailedToResolve.sync { $0.insert(inputInfo) }
                 default:
                     break // No difference to report
                 }

Sources/SwiftDocC/Infrastructure/Link Resolution/LinkResolutionMigration.swift

@@ -80,10 +80,10 @@ final class LinkResolutionMismatches {
     }
 
     /// Links that resolved in the cache-based implementation but not the path hierarchy-based implementation
-    var mismatchedLinksThatHierarchyBasedLinkResolverFailedToResolve = Set<FailedLinkInfo>()
+    var mismatchedLinksThatHierarchyBasedLinkResolverFailedToResolve: Synchronized<Set<FailedLinkInfo>> = .init([])
 
     /// Links that resolved in the path hierarchy-based implementation but not the cache-based implementation.
-    var mismatchedLinksThatCacheBasedLinkResolverFailedToResolve = Set<FailedLinkInfo>()
+    var mismatchedLinksThatCacheBasedLinkResolverFailedToResolve: Synchronized<Set<FailedLinkInfo>> = .init([])
 }
 
 // MARK: Reporting mismatches
@@ -122,8 +122,8 @@ extension LinkResolutionMismatches {
             return
         }
 
-        let mismatchedFailedCacheResults = mismatchedLinksThatCacheBasedLinkResolverFailedToResolve
-        let mismatchedFailedHierarchyResults = mismatchedLinksThatHierarchyBasedLinkResolverFailedToResolve
+        let mismatchedFailedCacheResults = mismatchedLinksThatCacheBasedLinkResolverFailedToResolve.sync({ $0 })
+        let mismatchedFailedHierarchyResults = mismatchedLinksThatHierarchyBasedLinkResolverFailedToResolve.sync({ $0 })
 
         if mismatchedFailedCacheResults.isEmpty && mismatchedFailedHierarchyResults.isEmpty {
             print("\(prefix) Both link resolver implementations succeeded and failed to resolve the same links.")

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift

@@ -133,18 +133,6 @@ struct PathHierarchy {
                     continue
                 }
             }
-            for relationship in graph.relationships where [.defaultImplementationOf].contains(relationship.kind) {
-                guard let sourceNode = nodes[relationship.source], sourceNode.parent == nil else {
-                    continue
-                }
-                topLevelCandidates.removeValue(forKey: relationship.source)
-                guard let targetParent = nodes[relationship.target]?.parent else {
-                    continue
-                }
-                if targetParent.add(symbolChild: sourceNode) {
-                    nodes[relationship.source] = nil
-                }
-            }
 
             // The hierarchy doesn't contain any non-symbol nodes yet. It's OK to unwrap the `symbol` property.
             for topLevelNode in topLevelCandidates.values where topLevelNode.symbol!.pathComponents.count == 1 {
@@ -688,6 +676,12 @@ extension PathHierarchy {
 
 // MARK: Determining disambiguated paths
 
+private let nonAllowedPathCharacters = CharacterSet.urlPathAllowed.inverted
+
+private func symbolFileName(_ symbolName: String) -> String {
+    return symbolName.components(separatedBy: nonAllowedPathCharacters).joined(separator: "_")
+}
+
 extension PathHierarchy {
     /// Determines the least disambiguated paths for all symbols in the path hierarchy.
     ///
@@ -701,7 +695,7 @@ extension PathHierarchy {
     ) -> [String: String] {
         func descend(_ node: Node, accumulatedPath: String) -> [(String, (String, Bool))] {
             var results: [(String, (String, Bool))] = []
-            let caseInsensitiveChildren = [String: DisambiguationTree](node.children.map { ($0.key.lowercased(), $0.value) }, uniquingKeysWith: { $0.merge(with: $1) })
+            let caseInsensitiveChildren = [String: DisambiguationTree](node.children.map { (symbolFileName($0.key.lowercased()), $0.value) }, uniquingKeysWith: { $0.merge(with: $1) })
 
             for (_, tree) in caseInsensitiveChildren {
                 let disambiguatedChildren = tree.disambiguatedValuesWithCollapsedUniqueSymbols(includeLanguage: includeLanguage)
@@ -720,9 +714,9 @@ extension PathHierarchy {
                         if hash != "_" {
                             knownDisambiguation += "-\(hash)"
                         }
-                        path = accumulatedPath + "/" + node.name + knownDisambiguation
+                        path = accumulatedPath + "/" + symbolFileName(node.name) + knownDisambiguation
                     } else {
-                        path = accumulatedPath + "/" + node.name
+                        path = accumulatedPath + "/" + symbolFileName(node.name)
                     }
                     if let symbol = node.symbol {
                         results.append(
@@ -749,7 +743,23 @@ extension PathHierarchy {
         }
 
         // If a symbol node exist in multiple languages, prioritize the Swift variant.
-        return [String: (String, Bool)](gathered, uniquingKeysWith: { lhs, rhs in lhs.1 ? lhs : rhs }).mapValues({ $0.0 })
+        let result = [String: (String, Bool)](gathered, uniquingKeysWith: { lhs, rhs in lhs.1 ? lhs : rhs }).mapValues({ $0.0 })
+        
+        assert(
+            Set(result.values).count == result.keys.count,
+            {
+                let collisionDescriptions = result
+                    .reduce(into: [String: [String]](), { $0[$1.value, default: []].append($1.key) })
+                    .filter({ $0.value.count > 1 })
+                    .map { "\($0.key)\n\($0.value.map({ "  " + $0 }).joined(separator: "\n"))" }
+                return """
+                Disambiguated paths contain \(collisionDescriptions.count) collision(s):
+                \(collisionDescriptions.joined(separator: "\n"))
+                """
+            }()
+        )
+        
+        return result
     }
 }
 

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchyBasedLinkResolver.swift

@@ -51,7 +51,10 @@ final class PathHierarchyBasedLinkResolver {
             guard node.symbol != nil else { continue }
 
             guard let parentID = node.parent?.identifier else { continue }
-            observe(resolvedReferenceMap[id]!, resolvedReferenceMap[parentID]!)
+            
+            // Only symbols in the symbol index are added to the reference map.
+            guard let reference = resolvedReferenceMap[id], let parentReference = resolvedReferenceMap[parentID] else { continue }
+            observe(reference, parentReference)
         }
     }