Skip to content

Commit fc79e78

Browse files
d-ronnqvistQuietMisdreavus
d-ronnqvist
and
QuietMisdreavus
authored
Fix a rarely occurring crash when processing certain symbols for many platforms (#1227)
* Fix a rare crash that sometimes (depending on file read order) happen when processing members of structs nested in unions for many platforms. rdar://151854292 * update test to correctly trigger underlying issue * recursively clone children and transplant those with mismatched languages * Simplify and correct deep cloning of path hierarchy nodes --------- Co-authored-by: Vera Mitchell <[email protected]>
1 parent a0c57b8 commit fc79e78

File tree

2 files changed

+164
-58
lines changed

2 files changed

+164
-58
lines changed

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

Lines changed: 94 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -112,12 +112,15 @@ struct PathHierarchy {
112112
var nodes: [String: Node] = [:]
113113
nodes.reserveCapacity(graph.symbols.count)
114114
for (id, symbol) in graph.symbols {
115-
if let existingNode = allNodes[id]?.first(where: {
116-
// If both identifiers are in the same language, they are the same symbol.
117-
$0.symbol!.identifier.interfaceLanguage == symbol.identifier.interfaceLanguage
118-
// If both have the same path components and kind, their differences don't matter for link resolution purposes.
119-
|| ($0.symbol!.pathComponents == symbol.pathComponents && $0.symbol!.kind.identifier == symbol.kind.identifier)
120-
}) {
115+
if let possibleNodes = allNodes[id],
116+
let existingNode = possibleNodes.first(where: {
117+
// If both identifiers are in the same language, they are the same symbol.
118+
$0.symbol!.identifier.interfaceLanguage == symbol.identifier.interfaceLanguage
119+
}) ?? possibleNodes.first(where: {
120+
// Otherwise, if both have the same path components and kind, their differences don't matter for link resolution purposes.
121+
$0.symbol!.pathComponents == symbol.pathComponents && $0.symbol!.kind.identifier == symbol.kind.identifier
122+
})
123+
{
121124
nodes[id] = existingNode
122125
if existingNode.counterpart?.languages.contains(language!) != true {
123126
// Unless this symbol is already split into language counterparts, add the languages to this node.
@@ -158,46 +161,17 @@ struct PathHierarchy {
158161
if sourceNode.parent == nil {
159162
targetNode.add(symbolChild: sourceNode)
160163
} else if sourceNode.parent !== targetNode && sourceNode.counterpart?.parent !== targetNode {
161-
// If the node we have for the child has an existing parent that doesn't
162-
// match the parent from this symbol graph, we need to clone the child to
163-
// ensure that the hierarchy remains consistent.
164-
let clonedSourceNode = Node(
165-
cloning: sourceNode,
166-
symbol: graph.symbols[relationship.source],
167-
children: [:],
168-
languages: [language!]
169-
)
170-
171-
// The original node no longer represents this symbol graph's language,
172-
// so remove that data from there.
173-
sourceNode.languages.remove(language!)
174-
175-
// Make sure that the clone's children can all line up with symbols from this symbol graph.
176-
for children in sourceNode.children.values {
177-
for child in children.storage {
178-
guard let childSymbol = child.node.symbol else {
179-
// We shouldn't come across any non-symbol nodes here,
180-
// but assume they can work as child of both variants.
181-
clonedSourceNode.add(child: child.node, kind: child.kind, hash: child.hash)
182-
continue
183-
}
184-
if nodes[childSymbol.identifier.precise] === child.node {
185-
clonedSourceNode.add(symbolChild: child.node)
186-
}
164+
// If the source node already exist in a different location in the hierarchy we need to split it into separate nodes for each language representation.
165+
// This ensures that each node has a single parent, so that the hierarchy can be unambiguously walked upwards to expand the "scope" of a search.
166+
let clonedSourceNode = sourceNode.deepClone(
167+
separating: language!,
168+
keeping: sourceNode.languages.subtracting([language!]),
169+
symbolsByUSR: graph.symbols,
170+
didCloneNode: { newNode, newSymbol in
171+
nodes[newSymbol.identifier.precise] = newNode
172+
allNodes[newSymbol.identifier.precise, default: []].append(newNode)
187173
}
188-
}
189-
190-
// Track the cloned node in the lists of nodes.
191-
nodes[relationship.source] = clonedSourceNode
192-
if let existingNodes = allNodes[relationship.source] {
193-
clonedSourceNode.counterpart = existingNodes.first
194-
for other in existingNodes {
195-
other.counterpart = clonedSourceNode
196-
}
197-
}
198-
allNodes[relationship.source, default: []].append(clonedSourceNode)
199-
200-
// Finally, add the cloned node as a child of its parent.
174+
)
201175
targetNode.add(symbolChild: clonedSourceNode)
202176
}
203177
topLevelCandidates.removeValue(forKey: relationship.source)
@@ -597,19 +571,81 @@ extension PathHierarchy {
597571
self.children = [:]
598572
self.specialBehaviors = []
599573
}
600-
601-
/// Initializes a node with a new identifier but the data from an existing node.
602-
fileprivate init(
603-
cloning source: Node,
604-
symbol: SymbolGraph.Symbol?? = nil,
605-
children: [String: DisambiguationContainer]? = nil,
606-
languages: Set<SourceLanguage>? = nil
607-
) {
608-
self.symbol = symbol ?? source.symbol
609-
self.name = source.name
610-
self.children = children ?? source.children
611-
self.specialBehaviors = source.specialBehaviors
612-
self.languages = languages ?? source.languages
574+
575+
fileprivate func deepClone(
576+
separating separatedLanguage: SourceLanguage,
577+
keeping otherLanguages: Set<SourceLanguage>,
578+
symbolsByUSR: borrowing [String: SymbolGraph.Symbol],
579+
didCloneNode: (Node, SymbolGraph.Symbol) -> Void
580+
) -> Node {
581+
assert(!otherLanguages.contains(separatedLanguage), "The caller should have already removed '\(separatedLanguage.id)' from '\(languages.sorted().map(\.id).joined(separator: ", "))'")
582+
583+
let clone: Node
584+
if let currentSymbol = symbol {
585+
// If a representation of the symbol exist in the current local symbol graph, prefer that for more correct disambiguation information.
586+
let symbol = symbolsByUSR[currentSymbol.identifier.precise] ?? currentSymbol
587+
clone = Node(symbol: symbol, name: name)
588+
didCloneNode(clone, symbol)
589+
} else {
590+
assertionFailure("Unexpectedly cloned a non-symbol node '\(name)' into separate language representations ('\(separatedLanguage.id)' vs '\(otherLanguages.sorted().map(\.id).joined(separator: ", "))').")
591+
clone = Node(name: name)
592+
}
593+
// Update languages and counterparts
594+
clone.languages = [separatedLanguage]
595+
languages.remove(separatedLanguage)
596+
assert(!languages.isEmpty, """
597+
Unexpectedly cloned '\(symbol?.identifier.precise ?? "non-symbol named \(name)")' for '\(separatedLanguage.id)' when it was already the only language it was available for.
598+
""")
599+
600+
clone.counterpart = self
601+
self.counterpart = clone
602+
603+
// Assign all the children to either the original, the clone, or both.
604+
let originalChildren = children
605+
children.removeAll(keepingCapacity: true)
606+
607+
func addOrMove(_ node: Node, to containerNode: Node) {
608+
if node.symbol != nil {
609+
containerNode.add(symbolChild: node)
610+
} else {
611+
containerNode.add(child: node, kind: nil, hash: nil)
612+
}
613+
assert(!containerNode.languages.isDisjoint(with: node.languages), """
614+
Unexpectedly added a node to a container without any overlapping languages.
615+
Child node languages: \(node.languages.sorted().map(\.id).joined(separator: ", "))
616+
Parent node languages: \(node.languages.sorted().map(\.id).joined(separator: ", "))
617+
""")
618+
}
619+
620+
for elements in originalChildren.values {
621+
for element in elements.storage {
622+
let node = element.node
623+
node.parent = nil // Remove the association with the original container. This node will be added to either the original (again) or to the clone.
624+
let nodeLanguages = node.languages
625+
626+
switch (nodeLanguages.contains(separatedLanguage), !nodeLanguages.isDisjoint(with: languages)) {
627+
case (true, false):
628+
// This node only exist for the separated language, so it only belongs in the clone. No recursive copying needed.
629+
addOrMove(node, to: clone)
630+
631+
case (false, true):
632+
// This node doesn't exist for the separated language, so it only belongs in the original. No recursive copying needed.
633+
addOrMove(node, to: self)
634+
635+
case (true, true):
636+
// This node needs to have deep copies for both the original and the clone.
637+
let innerClone = node.deepClone(separating: separatedLanguage, keeping: otherLanguages, symbolsByUSR: symbolsByUSR, didCloneNode: didCloneNode)
638+
addOrMove(node, to: self)
639+
addOrMove(innerClone, to: clone)
640+
641+
case (false, false):
642+
assertionFailure("Node \(node.name) (\(node.languages.sorted().map(\.id).joined(separator: ","))) doesn't belong in either '\(separatedLanguage.id)' or '\(otherLanguages.sorted().map(\.id).joined(separator: ", "))'.")
643+
continue
644+
}
645+
}
646+
}
647+
648+
return clone
613649
}
614650

615651
/// Adds a descendant to this node, providing disambiguation information from the node's symbol.

‎Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3328,6 +3328,76 @@ class PathHierarchyTests: XCTestCase {
33283328
try assertFindsPath("/ModuleName/ContainerName", in: tree, asSymbolID: containerID)
33293329
}
33303330

3331+
func testMissingMemberOfAnonymousStructInsideUnion() throws {
3332+
let outerContainerID = "some-outer-container-symbol-id"
3333+
let innerContainerID = "some-inner-container-symbol-id"
3334+
let memberID = "some-member-symbol-id"
3335+
3336+
// Repeat the same symbols in both languages for many platforms.
3337+
let platforms = (1...10).map {
3338+
let name = "Platform\($0)"
3339+
return (name: name, availability: [makeAvailabilityItem(domainName: name)])
3340+
}
3341+
3342+
let catalog = Folder(name: "unit-test.docc", content: [
3343+
// union Outer {
3344+
// struct {
3345+
// uint32_t member;
3346+
// } inner;
3347+
// };
3348+
Folder(name: "clang", content: platforms.map { platform in
3349+
JSONFile(name: "ModuleName-\(platform.name).symbols.json", content: makeSymbolGraph(
3350+
moduleName: "ModuleName",
3351+
symbols: [
3352+
makeSymbol(id: outerContainerID, language: .objectiveC, kind: .union, pathComponents: ["Outer"], availability: platform.availability),
3353+
makeSymbol(id: innerContainerID, language: .objectiveC, kind: .property, pathComponents: ["Outer", "inner"], availability: platform.availability),
3354+
makeSymbol(id: memberID, language: .objectiveC, kind: .property, pathComponents: ["Outer", "inner", "member"], availability: platform.availability),
3355+
],
3356+
relationships: [
3357+
.init(source: memberID, target: innerContainerID, kind: .memberOf, targetFallback: nil),
3358+
.init(source: innerContainerID, target: outerContainerID, kind: .memberOf, targetFallback: nil),
3359+
]
3360+
))
3361+
}),
3362+
3363+
// struct Outer {
3364+
// struct __Unnamed_struct_inner {
3365+
// var member: UInt32 // <-- This symbol is missing due to rdar://152157610
3366+
// }
3367+
// var inner: Outer.__Unnamed_struct_inner
3368+
// }
3369+
Folder(name: "swift", content: platforms.map { platform in
3370+
JSONFile(name: "ModuleName-\(platform.name).symbols.json", content: makeSymbolGraph(
3371+
moduleName: "ModuleName",
3372+
symbols: [
3373+
makeSymbol(id: outerContainerID, language: .swift, kind: .struct, pathComponents: ["Outer"], availability: platform.availability),
3374+
makeSymbol(id: innerContainerID, language: .swift, kind: .property, pathComponents: ["Outer", "inner"], availability: platform.availability),
3375+
// The `member` property is missing due to rdar://152157610
3376+
],
3377+
relationships: [
3378+
.init(source: innerContainerID, target: outerContainerID, kind: .memberOf, targetFallback: nil),
3379+
]
3380+
))
3381+
})
3382+
])
3383+
3384+
let (_, context) = try loadBundle(catalog: catalog)
3385+
let tree = context.linkResolver.localResolver.pathHierarchy
3386+
3387+
let paths = tree.caseInsensitiveDisambiguatedPaths()
3388+
XCTAssertEqual(paths[outerContainerID], "/ModuleName/Outer")
3389+
XCTAssertEqual(paths[innerContainerID], "/ModuleName/Outer/inner")
3390+
XCTAssertEqual(paths[memberID], "/ModuleName/Outer/inner/member")
3391+
3392+
try assertFindsPath("/ModuleName/Outer-union", in: tree, asSymbolID: outerContainerID)
3393+
try assertFindsPath("/ModuleName/Outer-union/inner", in: tree, asSymbolID: innerContainerID)
3394+
try assertFindsPath("/ModuleName/Outer-union/inner/member", in: tree, asSymbolID: memberID)
3395+
3396+
try assertFindsPath("/ModuleName/Outer-struct", in: tree, asSymbolID: outerContainerID)
3397+
try assertFindsPath("/ModuleName/Outer-struct/inner", in: tree, asSymbolID: innerContainerID)
3398+
try assertPathNotFound("/ModuleName/Outer-struct/inner/member", in: tree)
3399+
}
3400+
33313401
func testLinksToCxxOperators() throws {
33323402
let (_, context) = try testBundleAndContext(named: "CxxOperators")
33333403
let tree = context.linkResolver.localResolver.pathHierarchy

0 commit comments

Comments
 (0)