Skip to content

Fix a rarely occurring crash when processing certain symbols for many platforms #1227

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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 94 additions & 58 deletions Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,15 @@ struct PathHierarchy {
var nodes: [String: Node] = [:]
nodes.reserveCapacity(graph.symbols.count)
for (id, symbol) in graph.symbols {
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
// If both have the same path components and kind, their differences don't matter for link resolution purposes.
|| ($0.symbol!.pathComponents == symbol.pathComponents && $0.symbol!.kind.identifier == symbol.kind.identifier)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This || expression sometimes find the language representation of a symbol that's different from the language of the current symbol graph file. This in turn results in cloning a language specific symbol that's in the "wrong" language which removes the only language from that node. Later, when computing the paths for all symbols, the cloned node without any language is skipped, resulting in a symbol that never receives a reference.

}) {
if let possibleNodes = allNodes[id],
let existingNode = possibleNodes.first(where: {
// If both identifiers are in the same language, they are the same symbol.
$0.symbol!.identifier.interfaceLanguage == symbol.identifier.interfaceLanguage
}) ?? possibleNodes.first(where: {
// Otherwise, if both have the same path components and kind, their differences don't matter for link resolution purposes.
$0.symbol!.pathComponents == symbol.pathComponents && $0.symbol!.kind.identifier == symbol.kind.identifier
})
{
nodes[id] = existingNode
if existingNode.counterpart?.languages.contains(language!) != true {
// Unless this symbol is already split into language counterparts, add the languages to this node.
Expand Down Expand Up @@ -158,46 +161,17 @@ struct PathHierarchy {
if sourceNode.parent == nil {
targetNode.add(symbolChild: sourceNode)
} else if sourceNode.parent !== targetNode && sourceNode.counterpart?.parent !== targetNode {
// If the node we have for the child has an existing parent that doesn't
// match the parent from this symbol graph, we need to clone the child to
// ensure that the hierarchy remains consistent.
let clonedSourceNode = Node(
cloning: sourceNode,
symbol: graph.symbols[relationship.source],
children: [:],
languages: [language!]
)

// The original node no longer represents this symbol graph's language,
// so remove that data from there.
sourceNode.languages.remove(language!)

// Make sure that the clone's children can all line up with symbols from this symbol graph.
for children in sourceNode.children.values {
for child in children.storage {
guard let childSymbol = child.node.symbol else {
// We shouldn't come across any non-symbol nodes here,
// but assume they can work as child of both variants.
clonedSourceNode.add(child: child.node, kind: child.kind, hash: child.hash)
continue
}
if nodes[childSymbol.identifier.precise] === child.node {
clonedSourceNode.add(symbolChild: child.node)
}
// 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.
// 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.
let clonedSourceNode = sourceNode.deepClone(
separating: language!,
keeping: sourceNode.languages.subtracting([language!]),
symbolsByUSR: graph.symbols,
didCloneNode: { newNode, newSymbol in
nodes[newSymbol.identifier.precise] = newNode
allNodes[newSymbol.identifier.precise, default: []].append(newNode)
}
}

// Track the cloned node in the lists of nodes.
nodes[relationship.source] = clonedSourceNode
if let existingNodes = allNodes[relationship.source] {
clonedSourceNode.counterpart = existingNodes.first
for other in existingNodes {
other.counterpart = clonedSourceNode
}
}
allNodes[relationship.source, default: []].append(clonedSourceNode)

// Finally, add the cloned node as a child of its parent.
)
targetNode.add(symbolChild: clonedSourceNode)
}
topLevelCandidates.removeValue(forKey: relationship.source)
Expand Down Expand Up @@ -597,19 +571,81 @@ extension PathHierarchy {
self.children = [:]
self.specialBehaviors = []
}

/// Initializes a node with a new identifier but the data from an existing node.
fileprivate init(
cloning source: Node,
symbol: SymbolGraph.Symbol?? = nil,
children: [String: DisambiguationContainer]? = nil,
languages: Set<SourceLanguage>? = nil
) {
self.symbol = symbol ?? source.symbol
self.name = source.name
self.children = children ?? source.children
self.specialBehaviors = source.specialBehaviors
self.languages = languages ?? source.languages

fileprivate func deepClone(
separating separatedLanguage: SourceLanguage,
keeping otherLanguages: Set<SourceLanguage>,
symbolsByUSR: borrowing [String: SymbolGraph.Symbol],
didCloneNode: (Node, SymbolGraph.Symbol) -> Void
) -> Node {
assert(!otherLanguages.contains(separatedLanguage), "The caller should have already removed '\(separatedLanguage.id)' from '\(languages.sorted().map(\.id).joined(separator: ", "))'")

let clone: Node
if let currentSymbol = symbol {
// If a representation of the symbol exist in the current local symbol graph, prefer that for more correct disambiguation information.
let symbol = symbolsByUSR[currentSymbol.identifier.precise] ?? currentSymbol
clone = Node(symbol: symbol, name: name)
didCloneNode(clone, symbol)
} else {
assertionFailure("Unexpectedly cloned a non-symbol node '\(name)' into separate language representations ('\(separatedLanguage.id)' vs '\(otherLanguages.sorted().map(\.id).joined(separator: ", "))').")
clone = Node(name: name)
}
// Update languages and counterparts
clone.languages = [separatedLanguage]
languages.remove(separatedLanguage)
assert(!languages.isEmpty, """
Unexpectedly cloned '\(symbol?.identifier.precise ?? "non-symbol named \(name)")' for '\(separatedLanguage.id)' when it was already the only language it was available for.
""")

clone.counterpart = self
self.counterpart = clone

// Assign all the children to either the original, the clone, or both.
let originalChildren = children
children.removeAll(keepingCapacity: true)

func addOrMove(_ node: Node, to containerNode: Node) {
if node.symbol != nil {
containerNode.add(symbolChild: node)
} else {
containerNode.add(child: node, kind: nil, hash: nil)
}
assert(!containerNode.languages.isDisjoint(with: node.languages), """
Unexpectedly added a node to a container without any overlapping languages.
Child node languages: \(node.languages.sorted().map(\.id).joined(separator: ", "))
Parent node languages: \(node.languages.sorted().map(\.id).joined(separator: ", "))
""")
}

for elements in originalChildren.values {
for element in elements.storage {
let node = element.node
node.parent = nil // Remove the association with the original container. This node will be added to either the original (again) or to the clone.
let nodeLanguages = node.languages

switch (nodeLanguages.contains(separatedLanguage), !nodeLanguages.isDisjoint(with: languages)) {
case (true, false):
// This node only exist for the separated language, so it only belongs in the clone. No recursive copying needed.
addOrMove(node, to: clone)

case (false, true):
// This node doesn't exist for the separated language, so it only belongs in the original. No recursive copying needed.
addOrMove(node, to: self)

case (true, true):
// This node needs to have deep copies for both the original and the clone.
let innerClone = node.deepClone(separating: separatedLanguage, keeping: otherLanguages, symbolsByUSR: symbolsByUSR, didCloneNode: didCloneNode)
addOrMove(node, to: self)
addOrMove(innerClone, to: clone)

case (false, false):
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: ", "))'.")
continue
}
}
}

return clone
}

/// Adds a descendant to this node, providing disambiguation information from the node's symbol.
Expand Down
70 changes: 70 additions & 0 deletions Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3328,6 +3328,76 @@ class PathHierarchyTests: XCTestCase {
try assertFindsPath("/ModuleName/ContainerName", in: tree, asSymbolID: containerID)
}

func testMissingMemberOfAnonymousStructInsideUnion() throws {
let outerContainerID = "some-outer-container-symbol-id"
let innerContainerID = "some-inner-container-symbol-id"
let memberID = "some-member-symbol-id"

// Repeat the same symbols in both languages for many platforms.
let platforms = (1...10).map {
let name = "Platform\($0)"
return (name: name, availability: [makeAvailabilityItem(domainName: name)])
}

let catalog = Folder(name: "unit-test.docc", content: [
// union Outer {
// struct {
// uint32_t member;
// } inner;
// };
Folder(name: "clang", content: platforms.map { platform in
JSONFile(name: "ModuleName-\(platform.name).symbols.json", content: makeSymbolGraph(
moduleName: "ModuleName",
symbols: [
makeSymbol(id: outerContainerID, language: .objectiveC, kind: .union, pathComponents: ["Outer"], availability: platform.availability),
makeSymbol(id: innerContainerID, language: .objectiveC, kind: .property, pathComponents: ["Outer", "inner"], availability: platform.availability),
makeSymbol(id: memberID, language: .objectiveC, kind: .property, pathComponents: ["Outer", "inner", "member"], availability: platform.availability),
],
relationships: [
.init(source: memberID, target: innerContainerID, kind: .memberOf, targetFallback: nil),
.init(source: innerContainerID, target: outerContainerID, kind: .memberOf, targetFallback: nil),
]
))
}),

// struct Outer {
// struct __Unnamed_struct_inner {
// var member: UInt32 // <-- This symbol is missing due to rdar://152157610
// }
// var inner: Outer.__Unnamed_struct_inner
// }
Folder(name: "swift", content: platforms.map { platform in
JSONFile(name: "ModuleName-\(platform.name).symbols.json", content: makeSymbolGraph(
moduleName: "ModuleName",
symbols: [
makeSymbol(id: outerContainerID, language: .swift, kind: .struct, pathComponents: ["Outer"], availability: platform.availability),
makeSymbol(id: innerContainerID, language: .swift, kind: .property, pathComponents: ["Outer", "inner"], availability: platform.availability),
// The `member` property is missing due to rdar://152157610
],
relationships: [
.init(source: innerContainerID, target: outerContainerID, kind: .memberOf, targetFallback: nil),
]
))
})
])

let (_, context) = try loadBundle(catalog: catalog)
let tree = context.linkResolver.localResolver.pathHierarchy

let paths = tree.caseInsensitiveDisambiguatedPaths()
XCTAssertEqual(paths[outerContainerID], "/ModuleName/Outer")
XCTAssertEqual(paths[innerContainerID], "/ModuleName/Outer/inner")
XCTAssertEqual(paths[memberID], "/ModuleName/Outer/inner/member")

try assertFindsPath("/ModuleName/Outer-union", in: tree, asSymbolID: outerContainerID)
try assertFindsPath("/ModuleName/Outer-union/inner", in: tree, asSymbolID: innerContainerID)
try assertFindsPath("/ModuleName/Outer-union/inner/member", in: tree, asSymbolID: memberID)

try assertFindsPath("/ModuleName/Outer-struct", in: tree, asSymbolID: outerContainerID)
try assertFindsPath("/ModuleName/Outer-struct/inner", in: tree, asSymbolID: innerContainerID)
try assertPathNotFound("/ModuleName/Outer-struct/inner/member", in: tree)
}

func testLinksToCxxOperators() throws {
let (_, context) = try testBundleAndContext(named: "CxxOperators")
let tree = context.linkResolver.localResolver.pathHierarchy
Expand Down