diff --git a/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift b/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift index c27211ba43..f6842763f3 100644 --- a/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift +++ b/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift @@ -361,20 +361,20 @@ struct PathHierarchy { break lookForArticleRoot } } - return try searchForNode(descendingFrom: articlesContainer, pathComponents: remaining.dropFirst(), parsedPathForError: parsedPathForError) + return try searchForNode(descendingFrom: articlesContainer, pathComponents: remaining.dropFirst(), parsedPathForError: parsedPathForError, onlyFindSymbols: onlyFindSymbols) } else if articlesContainer.anyChildMatches(firstComponent) { - return try searchForNode(descendingFrom: articlesContainer, pathComponents: remaining, parsedPathForError: parsedPathForError) + return try searchForNode(descendingFrom: articlesContainer, pathComponents: remaining, parsedPathForError: parsedPathForError, onlyFindSymbols: onlyFindSymbols) } } if !isKnownDocumentationPath { if tutorialContainer.matches(firstComponent) { - return try searchForNode(descendingFrom: tutorialContainer, pathComponents: remaining.dropFirst(), parsedPathForError: parsedPathForError) + return try searchForNode(descendingFrom: tutorialContainer, pathComponents: remaining.dropFirst(), parsedPathForError: parsedPathForError, onlyFindSymbols: onlyFindSymbols) } else if tutorialContainer.anyChildMatches(firstComponent) { - return try searchForNode(descendingFrom: tutorialContainer, pathComponents: remaining, parsedPathForError: parsedPathForError) + return try searchForNode(descendingFrom: tutorialContainer, pathComponents: remaining, parsedPathForError: parsedPathForError, onlyFindSymbols: onlyFindSymbols) } // The parent for tutorial overviews / technologies is "tutorials" which has already been removed above, so no need to check against that name. else if tutorialOverviewContainer.anyChildMatches(firstComponent) { - return try searchForNode(descendingFrom: tutorialOverviewContainer, pathComponents: remaining, parsedPathForError: parsedPathForError) + return try searchForNode(descendingFrom: tutorialOverviewContainer, pathComponents: remaining, parsedPathForError: parsedPathForError, onlyFindSymbols: onlyFindSymbols) } } } @@ -383,11 +383,11 @@ struct PathHierarchy { func searchForNodeInModules() throws -> Node { // Note: This captures `parentID`, `remaining`, and `parsedPathForError`. if let moduleMatch = modules[firstComponent.full] ?? modules[firstComponent.name] { - return try searchForNode(descendingFrom: moduleMatch, pathComponents: remaining.dropFirst(), parsedPathForError: parsedPathForError) + return try searchForNode(descendingFrom: moduleMatch, pathComponents: remaining.dropFirst(), parsedPathForError: parsedPathForError, onlyFindSymbols: onlyFindSymbols) } if modules.count == 1 { do { - return try searchForNode(descendingFrom: modules.first!.value, pathComponents: remaining, parsedPathForError: parsedPathForError) + return try searchForNode(descendingFrom: modules.first!.value, pathComponents: remaining, parsedPathForError: parsedPathForError, onlyFindSymbols: onlyFindSymbols) } catch { // Ignore this error and raise an error about not finding the module instead. } @@ -405,7 +405,7 @@ struct PathHierarchy { } catch { // If the node couldn't be found in the modules, search the non-matching parent to achieve a more specific error message if let parentID = parentID { - return try searchForNode(descendingFrom: lookup[parentID]!, pathComponents: path, parsedPathForError: parsedPathForError) + return try searchForNode(descendingFrom: lookup[parentID]!, pathComponents: path, parsedPathForError: parsedPathForError, onlyFindSymbols: onlyFindSymbols) } throw error } @@ -420,7 +420,7 @@ struct PathHierarchy { // If the starting point's children match this component, descend the path hierarchy from there. if possibleStartingPoint.anyChildMatches(firstComponent) { do { - return try searchForNode(descendingFrom: possibleStartingPoint, pathComponents: path, parsedPathForError: parsedPathForError) + return try searchForNode(descendingFrom: possibleStartingPoint, pathComponents: path, parsedPathForError: parsedPathForError, onlyFindSymbols: onlyFindSymbols) } catch { innerMostError = error } @@ -428,7 +428,7 @@ struct PathHierarchy { // It's possible that the component is ambiguous at the parent. Checking if this node matches the first component avoids that ambiguity. if possibleStartingPoint.matches(firstComponent) { do { - return try searchForNode(descendingFrom: possibleStartingPoint, pathComponents: path.dropFirst(), parsedPathForError: parsedPathForError) + return try searchForNode(descendingFrom: possibleStartingPoint, pathComponents: path.dropFirst(), parsedPathForError: parsedPathForError, onlyFindSymbols: onlyFindSymbols) } catch { if innerMostError == nil { innerMostError = error @@ -453,7 +453,8 @@ struct PathHierarchy { private func searchForNode( descendingFrom startingPoint: Node, pathComponents: ArraySlice, - parsedPathForError: () -> [PathComponent] + parsedPathForError: () -> [PathComponent], + onlyFindSymbols: Bool ) throws -> Node { var node = startingPoint var remaining = pathComponents[...] @@ -481,7 +482,7 @@ struct PathHierarchy { } } catch DisambiguationTree.Error.lookupCollision(let collisions) { func handleWrappedCollision() throws -> Node { - try handleCollision(node: node, parsedPath: parsedPathForError(), remaining: remaining, collisions: collisions) + try handleCollision(node: node, parsedPath: parsedPathForError, remaining: remaining, collisions: collisions, onlyFindSymbols: onlyFindSymbols) } // See if the collision can be resolved by looking ahead on level deeper. @@ -523,26 +524,45 @@ struct PathHierarchy { return possibleMatches.first(where: { $0.symbol?.identifier.interfaceLanguage == "swift" }) ?? possibleMatches.first! } // Couldn't resolve the collision by look ahead. - return try handleCollision(node: node, parsedPath: parsedPathForError(), remaining: remaining, collisions: collisions) + return try handleCollision(node: node, parsedPath: parsedPathForError, remaining: remaining, collisions: collisions, onlyFindSymbols: onlyFindSymbols) } } } private func handleCollision( node: Node, - parsedPath: [PathComponent], + parsedPath: () -> [PathComponent], remaining: ArraySlice, - collisions: [(node: PathHierarchy.Node, disambiguation: String)] + collisions: [(node: PathHierarchy.Node, disambiguation: String)], + onlyFindSymbols: Bool ) throws -> Node { - let favoredNodes = collisions.filter { $0.node.isDisfavoredInCollision == false } - if favoredNodes.count == 1 { - return favoredNodes.first!.node + if let favoredMatch = collisions.singleMatch({ $0.node.isDisfavoredInCollision == false }) { + return favoredMatch.node + } + // If a module has the same name as the article root (which is named after the bundle display name) then its possible + // for an article a symbol to collide. Articles aren't supported in symbol links but symbols are supported in general + // documentation links (although the non-symbol result is prioritized). + // + // There is a later check that the returned node is a symbol for symbol links, but that won't happen if the link is a + // collision. To fully handle the collision in both directions, the check below uses `onlyFindSymbols` in the closure + // so that only symbol matches are returned for symbol links (when `onlyFindSymbols` is `true`) and non-symbol matches + // for general documentation links (when `onlyFindSymbols` is `false`). + // + // It's a more compact way to write + // + // if onlyFindSymbols { + // return $0.node.symbol != nil + // } else { + // return $0.node.symbol == nil + // } + if let symbolOrNonSymbolMatch = collisions.singleMatch({ ($0.node.symbol != nil) == onlyFindSymbols }) { + return symbolOrNonSymbolMatch.node } throw Error.lookupCollision( partialResult: ( node, - Array(parsedPath.dropLast(remaining.count)) + Array(parsedPath().dropLast(remaining.count)) ), remaining: Array(remaining), collisions: collisions.map { ($0.node, $0.disambiguation) } @@ -598,6 +618,25 @@ struct PathHierarchy { } } +private extension Sequence { + /// Returns the only element of the sequence that satisfies the given predicate. + /// - Parameters: + /// - predicate: A closure that takes an element of the sequence as its argument and returns a Boolean value indicating whether the element is a match. + /// - Returns: The only element of the sequence that satisfies `predicate`, or `nil` if multiple elements satisfy the predicate or if no element satisfy the predicate. + /// - Complexity: O(_n_), where _n_ is the length of the sequence. + func singleMatch(_ predicate: (Element) -> Bool) -> Element? { + var match: Element? + for element in self where predicate(element) { + guard match == nil else { + // Found a second match. No need to check the rest of the sequence. + return nil + } + match = element + } + return match + } +} + extension PathHierarchy { /// A node in the path hierarchy. final class Node { diff --git a/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift b/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift index e628d8bcfd..6e6c956352 100644 --- a/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift +++ b/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift @@ -2133,6 +2133,50 @@ let expected = """ XCTAssertEqual(context.problems.filter { $0.diagnostic.source?.path.hasSuffix(newArticle1URL.lastPathComponent) == true }.count, 0) } + func testPrefersNonSymbolsInDocLink() throws { + try XCTSkipUnless(LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver) + + let (_, bundle, context) = try testBundleAndContext(copying: "SymbolsWithSameNameAsModule") { url in + // This bundle has a top-level struct named "Wrapper". Adding an article named "Wrapper.md" introduces a possibility for a link collision + try """ + # An article + + This is an article with the same name as a top-level symbol + """.write(to: url.appendingPathComponent("Wrapper.md"), atomically: true, encoding: .utf8) + + // Also change the display name so that the article container has the same name as the module. + try InfoPlist(displayName: "Something", identifier: "com.example.Something").write(inside: url) + + // Use a doc-link to curate the article. + try """ + # ``Something`` + + Curate the article and the symbol top-level. + + ## Topics + + - + """.write(to: url.appendingPathComponent("Something.md"), atomically: true, encoding: .utf8) + } + + let moduleReference = try XCTUnwrap(context.rootModules.first) + let moduleNode = try context.entity(with: moduleReference) + + let renderContext = RenderContext(documentationContext: context, bundle: bundle) + let converter = DocumentationContextConverter(bundle: bundle, context: context, renderContext: renderContext) + let source = context.documentURL(for: moduleReference) + + let renderNode = try XCTUnwrap(converter.renderNode(for: moduleNode, at: source)) + let curatedTopic = try XCTUnwrap(renderNode.topicSections.first?.identifiers.first) + + let topicReference = try XCTUnwrap(renderNode.references[curatedTopic] as? TopicRenderReference) + XCTAssertEqual(topicReference.title, "An article") + + // This test also reproduce https://github.com/apple/swift-docc/issues/593 + // When that's fixed this test should also use a symbol link to curate the top-level symbol and verify that + // the symbol link resolves to the symbol. + } + // Modules that are being extended should not have their own symbol in the current bundle's graph. func testNoSymbolForTertiarySymbolGraphModules() throws { // Add an article without curating it anywhere diff --git a/Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift b/Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift index f3160c47b7..d31de01a40 100644 --- a/Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift +++ b/Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift @@ -1115,6 +1115,24 @@ class PathHierarchyTests: XCTestCase { "/MixedLanguageFramework/SwiftOnlyStruct/tada()") } + func testArticleAndSymbolCollisions() throws { + try XCTSkipUnless(LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver) + let (_, _, context) = try testBundleAndContext(copying: "MixedLanguageFramework") { url in + try """ + # An article + + This article has the same path as a symbol + """.write(to: url.appendingPathComponent("Bar.md"), atomically: true, encoding: .utf8) + } + let tree = try XCTUnwrap(context.hierarchyBasedLinkResolver?.pathHierarchy) + + // The added article above has the same path as an existing symbol in the this module. + let symbolNode = try tree.findNode(path: "/MixedLanguageFramework/Bar", onlyFindSymbols: true) + XCTAssertNotNil(symbolNode.symbol, "Symbol link finds the symbol") + let articleNode = try tree.findNode(path: "/MixedLanguageFramework/Bar", onlyFindSymbols: false) + XCTAssertNil(articleNode.symbol, "General documentation link find the article") + } + func testOverloadedSymbols() throws { try XCTSkipUnless(LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver) let (_, context) = try testBundleAndContext(named: "OverloadedSymbols") @@ -1319,6 +1337,40 @@ class PathHierarchyTests: XCTestCase { XCTAssertEqual(try tree.findSymbol(path: "Something/SomethingElse", parent: moduleID).absolutePath, "Something/SomethingElse") } + func testPrefersNonSymbolsWhenOnlyFindSymbolIsFalse() throws { + try XCTSkipUnless(LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver) + + let (_, _, context) = try testBundleAndContext(copying: "SymbolsWithSameNameAsModule") { url in + // This bundle has a top-level struct named "Wrapper". Adding an article named "Wrapper.md" introduces a possibility for a link collision + try """ + # An article + + This is an article with the same name as a top-level symbol + """.write(to: url.appendingPathComponent("Wrapper.md"), atomically: true, encoding: .utf8) + + // Also change the display name so that the article container has the same name as the module. + try InfoPlist(displayName: "Something", identifier: "com.example.Something").write(inside: url) + } + let tree = try XCTUnwrap(context.hierarchyBasedLinkResolver?.pathHierarchy) + + do { + // Links to non-symbols can use only the file name, without specifying the module or catalog name. + let articleID = try tree.find(path: "Wrapper", onlyFindSymbols: false) + let articleMatch = try XCTUnwrap(tree.lookup[articleID]) + XCTAssertNil(articleMatch.symbol, "Should have found the article") + } + do { + // Links to non-symbols can also use module-relative links. + let articleID = try tree.find(path: "/Something/Wrapper", onlyFindSymbols: false) + let articleMatch = try XCTUnwrap(tree.lookup[articleID]) + XCTAssertNil(articleMatch.symbol, "Should have found the article") + } + // Symbols can only use absolute links or be found relative to another page. + let symbolID = try tree.find(path: "/Something/Wrapper", onlyFindSymbols: true) + let symbolMatch = try XCTUnwrap(tree.lookup[symbolID]) + XCTAssertNotNil(symbolMatch.symbol, "Should have found the struct") + } + func testOneSymbolPathsWithKnownDisambiguation() throws { try XCTSkipUnless(LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver) let exampleDocumentation = Folder(name: "MyKit.docc", content: [ @@ -1596,9 +1648,13 @@ class PathHierarchyTests: XCTestCase { } extension PathHierarchy { + func findNode(path rawPath: String, onlyFindSymbols: Bool, parent: ResolvedIdentifier? = nil) throws -> PathHierarchy.Node { + let id = try find(path: rawPath, parent: parent, onlyFindSymbols: onlyFindSymbols) + return lookup[id]! + } + func findSymbol(path rawPath: String, parent: ResolvedIdentifier? = nil) throws -> SymbolGraph.Symbol { - let id = try find(path: rawPath, parent: parent, onlyFindSymbols: true) - return lookup[id]!.symbol! + return try findNode(path: rawPath, onlyFindSymbols: true, parent: parent).symbol! } }