Prefer non-symbols in general documentation links (#594)

* Prefer non-symbols in general documentation links

rdar://109583745

* Elaborate comment about filtering symbol and non-symbol collisions.

Also, add test where  symbol and non-symbol have same path.

Author: d-ronnqvist

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,15 +420,15 @@ 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
                 }
             }
             // 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<PathComponent>,
-        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<PathComponent>,
-        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 {

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
+            
+            - <doc:Wrapper>
+            """.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

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!
     }
 }