Skip to content

Prevent a manual technology root from overriding the module node with the same name #770

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
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
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ public class LinkResolver {
try ExternalPathHierarchyResolver(dependencyArchive: $0)
}
for resolver in resolvers {
for moduleName in resolver.pathHierarchy.modules.keys {
self.externalResolvers[moduleName] = resolver
for moduleNode in resolver.pathHierarchy.modules {
self.externalResolvers[moduleNode.name] = resolver
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@ extension PathHierarchy {

var gathered: [(String, (String, Bool))] = []

for (moduleName, node) in modules {
let path = "/" + moduleName
for node in modules {
let path = "/" + node.name
gathered.append(
(moduleName, (path, node.symbol == nil || node.symbol!.identifier.interfaceLanguage == "swift"))
(node.name, (path, node.symbol == nil || node.symbol!.identifier.interfaceLanguage == "swift"))
)
gathered += descend(node, accumulatedPath: path)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ private extension PathHierarchy.Node {
extension PathHierarchy {
/// Creates a visual representation or the path hierarchy for debugging.
func dump() -> String {
var children = modules.sorted(by: \.key).map { $0.value.dumpableNode() }
var children = modules.sorted(by: \.name).map { $0.dumpableNode() }
if articlesContainer.symbol == nil {
children.append(articlesContainer.dumpableNode()) // The article parent can be the same node as the module
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,12 @@ extension PathHierarchy {
// A function to avoid repeating the
func searchForNodeInModules() throws -> Node {
// Note: This captures `parentID`, `remaining`, and `rawPathForError`.
if let moduleMatch = modules[firstComponent.full] ?? modules[String(firstComponent.name)] {
if let moduleMatch = modules.first(where: { $0.matches(firstComponent) }) {
return try searchForNode(descendingFrom: moduleMatch, pathComponents: remaining.dropFirst(), onlyFindSymbols: onlyFindSymbols, rawPathForError: rawPath)
}
if modules.count == 1 {
do {
return try searchForNode(descendingFrom: modules.first!.value, pathComponents: remaining, onlyFindSymbols: onlyFindSymbols, rawPathForError: rawPath)
return try searchForNode(descendingFrom: modules.first!, pathComponents: remaining, onlyFindSymbols: onlyFindSymbols, rawPathForError: rawPath)
} catch let error as PathHierarchy.Error {
switch error {
case .notFound:
Expand All @@ -115,13 +115,13 @@ extension PathHierarchy {
}
}
}
let topLevelNames = Set(modules.keys + [articlesContainer.name, tutorialContainer.name])
let topLevelNames = Set(modules.map(\.name) + [articlesContainer.name, tutorialContainer.name])

if isAbsolute, FeatureFlags.current.isExperimentalLinkHierarchySerializationEnabled {
throw Error.moduleNotFound(
pathPrefix: pathForError(of: rawPath, droppingLast: remaining.count),
remaining: Array(remaining),
availableChildren: Set(modules.keys)
availableChildren: Set(modules.map(\.name))
)
} else {
throw Error.notFound(
Expand Down Expand Up @@ -475,7 +475,12 @@ private extension Sequence {

private extension PathHierarchy.Node {
func matches(_ component: PathHierarchy.PathComponent) -> Bool {
if let symbol = symbol {
// Check the full path component first in case the node's name has a suffix that could be mistaken for a hash disambiguation.
if name == component.full {
return true
}
// Otherwise, check if the node's symbol matches the provided disambiguation
else if let symbol = symbol {
guard name == component.name else {
return false
}
Expand All @@ -486,9 +491,9 @@ private extension PathHierarchy.Node {
return false
}
return true
} else {
return name == component.full
}

return false
}

func anyChildMatches(_ component: PathHierarchy.PathComponent) -> Bool {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ extension PathHierarchy.FileRepresentation {
}

self.nodes = nodes
self.modules = pathHierarchy.modules.mapValues({ identifierMap[$0.identifier]! })
self.modules = pathHierarchy.modules.map({ identifierMap[$0.identifier]! })
self.articlesContainer = identifierMap[pathHierarchy.articlesContainer.identifier]!
self.tutorialContainer = identifierMap[pathHierarchy.tutorialContainer.identifier]!
self.tutorialOverviewContainer = identifierMap[pathHierarchy.tutorialOverviewContainer.identifier]!
Expand Down Expand Up @@ -93,7 +93,7 @@ extension PathHierarchy {
var nodes: [Node]

/// The module nodes in this hierarchy.
var modules: [String: Int]
var modules: [Int]
/// The container for articles and reference documentation.
var articlesContainer: Int
/// The container of tutorials.
Expand Down Expand Up @@ -174,7 +174,7 @@ extension PathHierarchyBasedLinkResolver {
}

return SerializableLinkResolutionInformation(
version: .init(major: 0, minor: 0, patch: 1), // This is still in development
version: .init(major: 0, minor: 1, patch: 0), // This is still in development
Copy link
Contributor

Choose a reason for hiding this comment

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

How this is related to the crash?

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 is a consequence of changing the modules from a dictionary to an array which also changed the file format of the serialized path hierarchy. Since that file format change isn't backwards compatible I bumped the version number.

bundleID: bundleID,
pathHierarchy: hierarchyFileRepresentation,
nonSymbolPaths: nonSymbolPaths
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ struct ResolvedIdentifier: Equatable, Hashable {
/// After a path hierarchy has been fully created — with both symbols and non-symbols — it can be used to find elements in the hierarchy and to determine the least disambiguated paths for all elements.
struct PathHierarchy {

/// A map of module names to module nodes.
private(set) var modules: [String: Node]
/// The list of module nodes.
private(set) var modules: [Node]
/// The container of top-level articles in the documentation hierarchy.
let articlesContainer: Node
/// The container of tutorials in the documentation hierarchy.
Expand Down Expand Up @@ -295,7 +295,7 @@ struct PathHierarchy {
"""
)

self.modules = roots
self.modules = Array(roots.values)
self.lookup = lookup

assert(topLevelSymbols().allSatisfy({ lookup[$0] != nil }))
Expand Down Expand Up @@ -352,7 +352,7 @@ struct PathHierarchy {
newNode.identifier = newReference
self.lookup[newReference] = newNode

modules[name] = newNode
modules.append(newNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @d-ronnqvist. It seems that this is what fixes the bug, and the rest is refactoring as a consequence of changing modules from a dictionary to an array.

However, I'm confused about some other changes that don't appear to have any impact on resolving the bug. I'll add a comment to each of the things I believe are unnecessary in this PR, so you can explain me why these changes were made.


return newReference
}
Expand All @@ -377,7 +377,7 @@ extension PathHierarchy {

fileprivate(set) unowned var parent: Node?
/// The symbol, if a node has one.
private(set) var symbol: SymbolGraph.Symbol?
fileprivate(set) var symbol: SymbolGraph.Symbol?

/// If the path hierarchy should disfavor this node in a link collision.
///
Expand Down Expand Up @@ -452,7 +452,7 @@ extension PathHierarchy {
func topLevelSymbols() -> [ResolvedIdentifier] {
var result: Set<ResolvedIdentifier> = []
// Roots represent modules and only have direct symbol descendants.
for root in modules.values {
for root in modules {
for (_, tree) in root.children {
for subtree in tree.storage.values {
for node in subtree.values where node.symbol != nil {
Expand All @@ -461,7 +461,7 @@ extension PathHierarchy {
}
}
}
return Array(result) + modules.values.map { $0.identifier }
return Array(result) + modules.map { $0.identifier }
}
}

Expand Down Expand Up @@ -566,6 +566,8 @@ extension PathHierarchy {
pathComponents: [],
docComment: nil,
accessLevel: .public,
// To make the file format smaller we don't store the symbol kind identifiers with each node. Instead, the kind identifier is stored
// as disambiguation and is filled in while building up the hierarchy below.
kind: SymbolGraph.Symbol.Kind(rawIdentifier: "", displayName: ""),
mixins: [:]
)
Expand All @@ -584,11 +586,21 @@ extension PathHierarchy {
let childNode = lookup[identifiers[child.nodeID]]!
// Even if this is a symbol node, explicitly pass the kind and hash disambiguation.
node.add(child: childNode, kind: child.kind, hash: child.hash)
if let kind = child.kind {
// Since the symbol was created with an empty symbol kind, fill in its kind identifier here.
childNode.symbol?.kind.identifier = .init(identifier: kind)
}
}
}

self.lookup = lookup
self.modules = fileRepresentation.modules.mapValues({ lookup[identifiers[$0]]! })
let modules = fileRepresentation.modules.map({ lookup[identifiers[$0]]! })
// Fill in the symbol kind of all modules. This is needed since the modules were created with empty symbol kinds and since no other symbol has a
// module as its child, so the modules didn't get their symbol kind set when building up the hierarchy above.
for node in modules {
node.symbol?.kind.identifier = .module
}
self.modules = modules
self.articlesContainer = lookup[identifiers[fileRepresentation.articlesContainer]]!
self.tutorialContainer = lookup[identifiers[fileRepresentation.tutorialContainer]]!
self.tutorialOverviewContainer = lookup[identifiers[fileRepresentation.tutorialOverviewContainer]]!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ final class PathHierarchyBasedLinkResolver {

/// Returns a list of all module symbols.
func modules() -> [ResolvedTopicReference] {
return pathHierarchy.modules.values.map { resolvedReferenceMap[$0.identifier]! }
return pathHierarchy.modules.map { resolvedReferenceMap[$0.identifier]! }
}

// MARK: - Adding non-symbols
Expand Down
31 changes: 31 additions & 0 deletions Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1755,6 +1755,37 @@ class PathHierarchyTests: XCTestCase {
XCTAssertEqual(paths[memberID], "/ModuleName/ContainerName-qwwf/MemberName1")
}

func testModuleAndCollidingTechnologyRootHasPathsForItsSymbols() throws {
let symbolID = "some-symbol-id"

let exampleDocumentation = Folder(name: "unit-test.docc", content: [
JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph(
moduleName: "ModuleName",
symbols: [
(symbolID, .swift, ["SymbolName"]),
],
relationships: []
)),

TextFile(name: "ModuleName.md", utf8Content: """
# Manual Technology Root

@Metadata {
@TechnologyRoot
}

A technology root with the same file name as the module name.
""")
])

let tempURL = try createTempFolder(content: [exampleDocumentation])
let (_, _, context) = try loadBundle(from: tempURL)
let tree = try XCTUnwrap(context.linkResolver.localResolver?.pathHierarchy)

let paths = tree.caseInsensitiveDisambiguatedPaths(includeDisambiguationForUnambiguousChildren: true)
XCTAssertEqual(paths[symbolID], "/ModuleName/SymbolName")
}

func testMultiPlatformModuleWithExtension() throws {
let (_, context) = try testBundleAndContext(named: "MultiPlatformModuleWithExtension")
let tree = try XCTUnwrap(context.linkResolver.localResolver?.pathHierarchy)
Expand Down