Fix crash where multi-language symbols could get parents not in the tree (#759)

* Fix crash where multi-language symbols could get parents not in the tree

rdar://118892994

* Add additional assertion that the node that's skipped is already added

* Add parenthesis to logic expression for readability

Author: d-ronnqvist

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

@@ -102,7 +102,12 @@ struct PathHierarchy {
             var nodes: [String: Node] = [:]
             nodes.reserveCapacity(graph.symbols.count)
             for (id, symbol) in graph.symbols {
-                if let existingNode = allNodes[id]?.first(where: { $0.symbol!.identifier == symbol.identifier }) {
+                if let existingNode = allNodes[id]?.first(where: {
+                    // If both identifiers are in the same language, they are the same symbol
+                    $0.symbol!.identifier.interfaceLanguage == symbol.identifier.interfaceLanguage
+                    // Otherwise, if both have the same name and kind their differences doesn't matter for link resolution purposes
+                    || ($0.name == symbol.pathComponents.last && $0.symbol!.kind.identifier == symbol.kind.identifier)
+                }) {
                     nodes[id] = existingNode
                 } else {
                     assert(!symbol.pathComponents.isEmpty, "A symbol should have at least its own name in its path components.")
@@ -201,9 +206,21 @@ struct PathHierarchy {
         }
 
         assert(
-            allNodes.allSatisfy({ $0.value[0].parent != nil || roots[$0.key] != nil }),
-            "Every node should either have a parent node or be a root node. This wasn't true for \(allNodes.filter({ $0.value[0].parent != nil || roots[$0.key] != nil }).map(\.key).sorted())"
+            allNodes.allSatisfy({ $0.value[0].parent != nil || roots[$0.key] != nil }), """
+            Every node should either have a parent node or be a root node. \
+            This wasn't true for \(allNodes.filter({ $0.value[0].parent != nil || roots[$0.key] != nil }).map(\.key).sorted())
+            """
         )
+        
+        assert(
+            allNodes.values.allSatisfy({ nodesWithSameUSR in nodesWithSameUSR.allSatisfy({ node in
+                Array(sequence(first: node, next: \.parent)).last!.symbol!.kind.identifier == .module })
+            }), """
+            Every node should reach a root node by following its parents up. \
+            This wasn't true for \(allNodes.filter({ $0.value.allSatisfy({ Array(sequence(first: $0, next: \.parent)).last!.symbol!.kind.identifier == .module }) }).map(\.key).sorted())
+            """
+        )
+        
         allNodes.removeAll()
 
         // build the lookup list by traversing the hierarchy and adding identifiers to each node
@@ -220,8 +237,21 @@ struct PathHierarchy {
             }
             for tree in node.children.values {
                 for (_, subtree) in tree.storage {
-                    for (_, node) in subtree {
-                        descend(node)
+                    for (_, childNode) in subtree {
+                        assert(childNode.parent === node, {
+                            func describe(_ node: Node?) -> String {
+                                guard let node = node else { return "<nil>" }
+                                guard let identifier = node.symbol?.identifier else { return node.name }
+                                return "\(identifier.precise) (\(identifier.interfaceLanguage))"
+                            }
+                            return """
+                            Every child node should point back to its parent so that the tree can be traversed both up and down without any dead-ends. \
+                            This wasn't true for '\(describe(childNode))' which pointed to '\(describe(childNode.parent))' but should have pointed to '\(describe(node))'.
+                            """ }()
+                        )
+                        // In release builds we close off any dead-ends in the tree as a precaution for what shouldn't happen.
+                        childNode.parent = node
+                        descend(childNode)
                     }
                 }
             }
@@ -231,6 +261,13 @@ struct PathHierarchy {
             descend(module)
         }
 
+        assert(
+            lookup.allSatisfy({ $0.value.parent != nil || roots[$0.value.name] != nil }), """
+            Every node should either have a parent node or be a root node. \
+            This wasn't true for \(allNodes.filter({ $0.value[0].parent != nil || roots[$0.key] != nil }).map(\.key).sorted())
+            """
+        )
+        
         func newNode(_ name: String) -> Node {
             let id = ResolvedIdentifier()
             let node = Node(name: name)
@@ -247,6 +284,13 @@ struct PathHierarchy {
             "Every node lookup should match a node with that identifier."
         )
 
+        assert(
+            lookup.values.allSatisfy({ $0.parent?.identifier == nil || lookup[$0.parent!.identifier] != nil }), """
+            Every node's findable parent should exist in the lookup. \
+            This wasn't true for \(lookup.values.filter({ $0.parent?.identifier == nil || lookup[$0.parent!.identifier] != nil }).map(\.symbol!.identifier.precise).sorted())
+            """
+        )
+        
         self.modules = roots
         self.lookup = lookup
 
@@ -327,7 +371,7 @@ extension PathHierarchy {
         /// Each name maps to a disambiguation tree that handles
         private(set) var children: [String: DisambiguationContainer]
 
-        private(set) unowned var parent: Node?
+        fileprivate(set) unowned var parent: Node?
         /// The symbol, if a node has one.
         private(set) var symbol: SymbolGraph.Symbol?
 
@@ -367,9 +411,18 @@ extension PathHierarchy {
 
         /// Adds a descendant of this node.
         fileprivate func add(child: Node, kind: String?, hash: String?) {
+            guard child.parent !== self else { 
+                assert(
+                    (try? children[child.name]?.find(kind, hash)) === child,
+                    "If the new child node already has this node as its parent it should already exist among this node's children."
+                )
+                return
+            }
             // If the name was passed explicitly, then the node could have spaces in its name
             child.parent = self
             children[child.name, default: .init()].add(kind ?? "_", hash ?? "_", child)
+            
+            assert(child.parent === self, "Potentially merging nodes shouldn't break the child node's reference to its parent.")
         }
 
         /// Combines this node with another node.

Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

@@ -1544,31 +1544,11 @@ class PathHierarchyTests: XCTestCase {
             ["X", "Y"],
             ["X", "Y2", "Z", "W"],
         ]
-        let graph = SymbolGraph(
-            metadata: SymbolGraph.Metadata(
-                formatVersion: SymbolGraph.SemanticVersion(major: 1, minor: 1, patch: 1),
-                generator: "unit-test"
-            ),
-            module: SymbolGraph.Module(
-                name: "Module",
-                platform: SymbolGraph.Platform(architecture: nil, vendor: nil, operatingSystem: nil)
-            ),
-            symbols: symbolPaths.map {
-                SymbolGraph.Symbol(
-                    identifier: SymbolGraph.Symbol.Identifier(precise: $0.joined(separator: "."), interfaceLanguage: "swift"),
-                    names: SymbolGraph.Symbol.Names(title: "Title", navigator: nil, subHeading: nil, prose: nil), // names doesn't matter for path disambiguation
-                    pathComponents: $0,
-                    docComment: nil,
-                    accessLevel: SymbolGraph.Symbol.AccessControl(rawValue: "public"),
-                    kind: SymbolGraph.Symbol.Kind(parsedIdentifier: .class, displayName: "Kind Display Name"), // kind display names doesn't matter for path disambiguation
-                    mixins: [:]
-                )
-            },
-            relationships: []
-        )
-        let exampleDocumentation = Folder(name: "MyKit.docc", content: [
-            try TextFile(name: "mykit.symbols.json", utf8Content: XCTUnwrap(String(data: JSONEncoder().encode(graph), encoding: .utf8))),
-            InfoPlist(displayName: "MyKit", identifier: "com.test.MyKit"),
+        let exampleDocumentation = Folder(name: "unit-test.docc", content: [
+            JSONFile(name: "Module.symbols.json", content: makeSymbolGraph(
+                moduleName: "Module",
+                symbols: symbolPaths.map { ($0.joined(separator: "."), .swift, $0) }
+            )),
         ])
         let tempURL = try createTemporaryDirectory()
         let bundleURL = try exampleDocumentation.write(inside: tempURL)
@@ -1595,6 +1575,49 @@ class PathHierarchyTests: XCTestCase {
         XCTAssertEqual(paths["X.Y2.Z.W"], "/Module/X/Y2/Z/W")
     }
 
+    func testMixedLanguageSymbolWithItsExtendingModule() throws {
+        let containerID = "some-container-symbol-id"
+        let memberID = "some-member-symbol-id"
+        
+        let exampleDocumentation = Folder(name: "unit-test.docc", content: [
+            Folder(name: "clang", content: [
+                JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph(
+                    moduleName: "ModuleName", 
+                    symbols: [
+                        (containerID, .objectiveC, ["ContainerName"])
+                    ]
+                )),
+            ]),
+            
+            Folder(name: "swift", content: [
+                JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph(
+                    moduleName: "ModuleName",
+                    symbols: [
+                        (containerID, .swift, ["ContainerName"])
+                    ]
+                )),
+                
+                JSONFile(name: "[email protected]", content: makeSymbolGraph(
+                    moduleName: "ExtendingModule",
+                    symbols: [
+                        (memberID, .swift, ["ContainerName", "MemberName"])
+                    ],
+                    relationships: [
+                        .init(source: memberID, target: containerID, kind: .memberOf, targetFallback: nil)
+                    ]
+                )),
+            ])
+        ])
+        
+        let tempURL = try createTempFolder(content: [exampleDocumentation])
+        let (_, _, context) = try loadBundle(from: tempURL)
+        let tree = try XCTUnwrap(context.linkResolver.localResolver?.pathHierarchy)
+        
+        let paths = tree.caseInsensitiveDisambiguatedPaths()
+        XCTAssertEqual(paths[containerID], "/ModuleName/ContainerName")
+        XCTAssertEqual(paths[memberID], "/ModuleName/ContainerName/MemberName")
+    }
+    
     func testMultiPlatformModuleWithExtension() throws {
         let (_, context) = try testBundleAndContext(named: "MultiPlatformModuleWithExtension")
         let tree = try XCTUnwrap(context.linkResolver.localResolver?.pathHierarchy)
@@ -1636,6 +1659,29 @@ class PathHierarchyTests: XCTestCase {
 
     // MARK: Test helpers
 
+    private func makeSymbolGraph(
+        moduleName: String,
+        symbols: [(identifier: String, language: SourceLanguage, pathComponents: [String])],
+        relationships: [SymbolGraph.Relationship] = []
+    ) -> SymbolGraph {
+        return SymbolGraph(
+            metadata: SymbolGraph.Metadata(formatVersion: .init(major: 0, minor: 5, patch: 3), generator: "unit-test"),
+            module: SymbolGraph.Module(name: moduleName, platform: .init()),
+            symbols: symbols.map { identifier, language, pathComponents in
+                SymbolGraph.Symbol(
+                    identifier: .init(precise: identifier, interfaceLanguage: language.id),
+                    names: .init(title: "SymbolName", navigator: nil, subHeading: nil, prose: nil), // names doesn't matter for path disambiguation
+                    pathComponents: pathComponents,
+                    docComment: nil,
+                    accessLevel: .public,
+                    kind: .init(parsedIdentifier: .class, displayName: "Kind Display Name"), // kind display names doesn't matter for path disambiguation
+                    mixins: [:]
+                )
+            },
+            relationships: relationships
+        )
+    }
+    
     private func assertFindsPath(_ path: String, in tree: PathHierarchy, asSymbolID symbolID: String, file: StaticString = #file, line: UInt = #line) throws {
         do {
             let symbol = try tree.findSymbol(path: path)