Fix new bug where overload groups prevent type disambiguation for one…

… of the overloaded symbols

Also, improve paths for overload groups and other preferred symbols

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+DisambiguatedPaths.swift

@@ -192,6 +192,22 @@ extension PathHierarchy.DisambiguationContainer {
         for elements: some Sequence<Element>,
         includeLanguage: Bool = false,
         allowAdvancedDisambiguation: Bool = true
+    ) -> [(value: PathHierarchy.Node, disambiguation: Disambiguation)] {
+        var collisions = _disambiguatedValues(for: elements, includeLanguage: includeLanguage, allowAdvancedDisambiguation: allowAdvancedDisambiguation)
+
+        // If all but one of the collisions are disfavored, remove the disambiguation for the only favored element.
+        if let onlyFavoredElementIndex = collisions.firstIndex(where: { !$0.value.specialBehaviors.contains(.disfavorInLinkCollision) }),
+           onlyFavoredElementIndex == collisions.lastIndex(where: { !$0.value.specialBehaviors.contains(.disfavorInLinkCollision) })
+        {
+            collisions[onlyFavoredElementIndex].disambiguation = .none
+        }
+        return collisions
+    }
+
+    private static func _disambiguatedValues(
+        for elements: some Sequence<Element>,
+        includeLanguage: Bool = false,
+        allowAdvancedDisambiguation: Bool = true
     ) -> [(value: PathHierarchy.Node, disambiguation: Disambiguation)] {
         var collisions: [(value: PathHierarchy.Node, disambiguation: Disambiguation)] = []
 
@@ -213,8 +229,10 @@ extension PathHierarchy.DisambiguationContainer {
         }
 
         if allowAdvancedDisambiguation {
+            let elementsThatSupportAdvancedDisambiguation = elements.filter { !$0.node.specialBehaviors.contains(.excludeFromAdvancedLinkDisambiguation) }
+
             // Next, if a symbol returns a tuple with a unique number of values, disambiguate by that (without specifying what those arguments are)
-            let groupedByReturnCount = [Int?: [Element]](grouping: elements, by: \.returnTypes?.count)
+            let groupedByReturnCount = [Int?: [Element]](grouping: elementsThatSupportAdvancedDisambiguation, by: \.returnTypes?.count)
             for (returnTypesCount, elements) in groupedByReturnCount  {
                 guard let returnTypesCount = returnTypesCount else { continue }
                 guard elements.count > 1 else {
@@ -245,7 +263,7 @@ extension PathHierarchy.DisambiguationContainer {
                 return collisions
             }
 
-            let groupedByParameterCount = [Int?: [Element]](grouping: elements, by: \.parameterTypes?.count)
+            let groupedByParameterCount = [Int?: [Element]](grouping: elementsThatSupportAdvancedDisambiguation, by: \.parameterTypes?.count)
             for (parameterTypesCount, elements) in groupedByParameterCount  {
                 guard let parameterTypesCount = parameterTypesCount else { continue }
                 guard elements.count > 1 else {

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Find.swift

@@ -386,41 +386,18 @@ extension PathHierarchy {
             )
         }
 
-        // Use a empty disambiguation suffix for the preferred symbol, if there
-        // is one, which will trigger the warning to suggest removing the
-        // suffix entirely.
-        let candidates = disambiguationTree.disambiguatedValues()
-        let favoredSuffix = favoredSuffix(from: candidates)
-        let suffixes = candidates.map { $0.disambiguation.makeSuffix() }
-        let candidatesAndSuffixes = zip(candidates, suffixes).map { (candidate, suffix) in
-            if suffix == favoredSuffix {
-                return (node: candidate.value, disambiguation: "")
-            } else {
-                return (node: candidate.value, disambiguation: suffix)
-            }
-        }
         return Error.unknownDisambiguation(
             partialResult: (
                 node,
                 pathForError(of: rawPathForError, droppingLast: remaining.count)
             ),
             remaining: Array(remaining),
-            candidates: candidatesAndSuffixes
+            candidates: disambiguationTree.disambiguatedValues().map {
+                (node: $0.value, disambiguation: String($0.disambiguation.makeSuffix()))
+            }
         )
     }
 
-    /// Check if exactly one of the given candidate symbols is preferred, because it is not disfavored
-    /// for link resolution and all the other symbols are.
-    /// - Parameters:
-    ///   - from: An array of candidate node and disambiguation tuples.
-    /// - Returns: An optional string set to the disambiguation suffix string, without the hyphen separator e.g. "abc123",
-    ///            or nil if there is no preferred symbol.
-    private func favoredSuffix(from candidates: [(value: PathHierarchy.Node, disambiguation: PathHierarchy.DisambiguationContainer.Disambiguation)]) -> String? {
-        return candidates.singleMatch({
-            !$0.value.specialBehaviors.contains(PathHierarchy.Node.SpecialBehaviors.disfavorInLinkCollision)
-        })?.disambiguation.makeSuffix()
-    }
-
     private func pathForError(
         of rawPath: String,
         droppingLast trailingComponentsToDrop: Int
@@ -477,8 +454,12 @@ extension PathHierarchy.DisambiguationContainer {
     ///  - Exactly one match is found; indicated by a non-nil return value.
     ///  - More than one match is found; indicated by a raised error listing the matches and their missing disambiguation.
     func find(_ disambiguation: PathHierarchy.PathComponent.Disambiguation?) throws -> PathHierarchy.Node? {
-        if storage.count <= 1, disambiguation == nil {
-            return storage.first?.node
+        if disambiguation == nil {
+            if storage.count <= 1 {
+                return storage.first?.node
+            } else if let favoredMatch = storage.singleMatch({ !$0.node.specialBehaviors.contains(.disfavorInLinkCollision) }) {
+                return favoredMatch.node
+            }
         }
 
         switch disambiguation {
@@ -503,6 +484,7 @@ extension PathHierarchy.DisambiguationContainer {
                 break
             }
         case .typeSignature(let parameterTypes, let returnTypes):
+            let storage = storage.filter { !$0.node.specialBehaviors.contains(.excludeFromAdvancedLinkDisambiguation )}
             switch (parameterTypes, returnTypes) {
             case (let parameterTypes?, let returnTypes?):
                 return storage.first(where: { typesMatch(provided: parameterTypes, actual: $0.parameterTypes) && typesMatch(provided: returnTypes, actual: $0.returnTypes) })?.node

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

@@ -123,7 +123,7 @@ struct PathHierarchy {
                     // Disfavor synthesized symbols when they collide with other symbol with the same path.
                     // FIXME: Get information about synthesized symbols from SymbolKit https://github.com/swiftlang/swift-docc-symbolkit/issues/58
                     if symbol.identifier.precise.contains("::SYNTHESIZED::") {
-                        node.specialBehaviors = [.disfavorInLinkCollision, .excludeFromAutomaticCuration]
+                        node.specialBehaviors.formUnion([.disfavorInLinkCollision, .excludeFromAutomaticCuration])
                     }
                     nodes[id] = node
 
@@ -286,6 +286,10 @@ struct PathHierarchy {
                 guard let groupNode = overloadGroupNodes[relationship.target], let overloadedSymbolNodes = allNodes[relationship.source] else {
                     continue
                 }
+
+                // The overload group symbol is cloned from a real symbol and has the same type signature as the clone. This prevents either symbol from using
+                // parameter type or return type disambiguation. Exclude the overload group from this, so that the real symbol can use it.
+                groupNode.specialBehaviors.insert(.excludeFromAdvancedLinkDisambiguation)
 
                 for overloadedSymbolNode in overloadedSymbolNodes {
                     // We want to disfavor the individual overload symbols in favor of resolving links to their overload group symbol.
@@ -494,6 +498,9 @@ extension PathHierarchy {
 
             /// This node is excluded from automatic curation.
             static let excludeFromAutomaticCuration = SpecialBehaviors(rawValue: 1 << 1)
+
+            /// This node is excluded from advanced link disambiguation.
+            static let excludeFromAdvancedLinkDisambiguation = SpecialBehaviors(rawValue: 1 << 2)
         }
 
         /// Initializes a symbol node.

Tests/SwiftDocCTests/DocumentationService/ConvertService/ConvertServiceTests.swift

@@ -1434,7 +1434,7 @@ class ConvertServiceTests: XCTestCase {
                 "/documentation/FillIntroduced/macCatalystOnlyDeprecated()",
                 "/documentation/MyKit/MyClass/myFunction()",
                 "/documentation/SideKit",
-                "/documentation/SideKit/SideProtocol/func()-6ijsi",
+                "/documentation/SideKit/SideProtocol/func()",
                 "/documentation/SideKit/SideClass/myFunction()",
                 "/documentation/SideKit/SideProtocol",
             ],

Tests/SwiftDocCTests/Infrastructure/AutomaticCurationTests.swift

@@ -735,11 +735,11 @@ class AutomaticCurationTests: XCTestCase {
 
         XCTAssertEqual(protocolTopicSection.title, "Instance Methods")
         XCTAssertEqual(protocolTopicSection.identifiers, [
-            "doc://com.shapes.ShapeKit/documentation/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)-9b6be"
+            "doc://com.shapes.ShapeKit/documentation/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)"
         ])
 
         let overloadGroupRenderNode = try renderNode(
-            atPath: "/documentation/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)-9b6be",
+            atPath: "/documentation/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)",
             fromTestBundleNamed: "OverloadedSymbols")
 
         XCTAssertEqual(
@@ -778,7 +778,7 @@ class AutomaticCurationTests: XCTestCase {
 
             XCTAssertEqual(overloadTopic.title, "Instance Methods", file: file, line: line)
             XCTAssertEqual(overloadTopic.references.map(\.absoluteString), [
-                "doc://com.shapes.ShapeKit/documentation/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)-9b6be"
+                "doc://com.shapes.ShapeKit/documentation/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)"
             ], file: file, line: line)
         }
 

Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift

@@ -1365,7 +1365,7 @@ let expected = """
  │ ├ doc://org.swift.docc.example/documentation/SideKit/SideClass/path
  │ ╰ doc://org.swift.docc.example/documentation/SideKit/SideClass/url
  ├ doc://org.swift.docc.example/documentation/SideKit/SideProtocol
- │ ╰ doc://org.swift.docc.example/documentation/SideKit/SideProtocol/func()-6ijsi
+ │ ╰ doc://org.swift.docc.example/documentation/SideKit/SideProtocol/func()
  │   ╰ doc://org.swift.docc.example/documentation/SideKit/SideProtocol/func()-2dxqn
  ╰ doc://org.swift.docc.example/documentation/SideKit/UncuratedClass
  doc://org.swift.docc.example/tutorials/TestOverview
@@ -1793,7 +1793,7 @@ let expected = """
 
         // "/" is a separator in URL paths so it's replaced with with "_" (adding disambiguation if the replacement introduces conflicts)
         XCTAssertEqual("/(_:_:)",   pageIdentifiersAndNames["/documentation/Operators/MyNumber/_(_:_:)-7am4"])
-        XCTAssertEqual("/=(_:_:)",  pageIdentifiersAndNames["/documentation/Operators/MyNumber/_=(_:_:)-3m4ko"])
+        XCTAssertEqual("/=(_:_:)",  pageIdentifiersAndNames["/documentation/Operators/MyNumber/_=(_:_:)"])
     }
 
     func testFileNamesWithDifferentPunctuation() throws {
@@ -2391,7 +2391,7 @@ let expected = """
         XCTAssertEqual(context.documentationCache.reference(symbolID: "s:7SideKit0A5ClassC10testEE")?.path, "/documentation/SideKit/SideClass/Test-swift.enum/NestedEnum-swift.enum")
         XCTAssertEqual(context.documentationCache.reference(symbolID: "s:7SideKit0A5ClassC10tEstPP")?.path, "/documentation/SideKit/SideClass/Test-swift.enum/NestedEnum-swift.enum/path")
 
-        XCTAssertEqual(context.documentationCache.reference(symbolID: "s:5MyKit0A5MyProtocol0Afunc()")?.path, "/documentation/SideKit/SideProtocol/func()-6ijsi")
+        XCTAssertEqual(context.documentationCache.reference(symbolID: "s:5MyKit0A5MyProtocol0Afunc()")?.path, "/documentation/SideKit/SideProtocol/func()")
         XCTAssertEqual(context.documentationCache.reference(symbolID: "s:5MyKit0A5MyProtocol0Afunc()DefaultImp")?.path, "/documentation/SideKit/SideProtocol/func()-2dxqn")
     }
 

Tests/SwiftDocCTests/Infrastructure/ExternalPathHierarchyResolverTests.swift

@@ -869,7 +869,7 @@ class ExternalPathHierarchyResolverTests: XCTestCase {
         // This overloaded protocol method should be able to resolve without a suffix at all, since it doesn't conflict with anything
         try linkResolvers.assertSuccessfullyResolves(
             authoredLink: "/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)",
-            to: "doc://com.shapes.ShapeKit/documentation/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)-9b6be"
+            to: "doc://com.shapes.ShapeKit/documentation/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)"
         )
     }
 

Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

@@ -705,7 +705,7 @@ class PathHierarchyTests: XCTestCase {
 
         // The protocol requirement and the default implementation both exist at the @_export imported Something protocol.
         let paths = tree.caseInsensitiveDisambiguatedPaths()
-        XCTAssertEqual(paths["s:5Inner9SomethingP02doB0yyF"],    "/DefaultImplementationsWithExportedImport/Something/doSomething()-8skxc")
+        XCTAssertEqual(paths["s:5Inner9SomethingP02doB0yyF"],    "/DefaultImplementationsWithExportedImport/Something/doSomething()") // This is the only favored symbol so it doesn't require any disambiguation
         XCTAssertEqual(paths["s:5Inner9SomethingPAAE02doB0yyF"], "/DefaultImplementationsWithExportedImport/Something/doSomething()-scj9")
 
         // Test disfavoring a default implementation in a symbol collision
@@ -911,15 +911,15 @@ class PathHierarchyTests: XCTestCase {
         XCTAssertEqual(
             // static func /= (lhs: inout MyNumber, rhs: MyNumber) -> MyNumber
             paths["s:9Operators8MyNumberV2deoiyA2Cz_ACtFZ"],
-            "/Operators/MyNumber/_=(_:_:)->MyNumber")
+            "/Operators/MyNumber/_=(_:_:)") // This is the only favored symbol so it doesn't require any disambiguation
         XCTAssertEqual(
             // static func / (lhs: MyNumber, rhs: MyNumber) -> MyNumber
             hashAndKindDisambiguatedPaths["s:9Operators8MyNumberV1doiyA2C_ACtFZ"],
             "/Operators/MyNumber/_(_:_:)-7am4")
         XCTAssertEqual(
             // static func /= (lhs: inout MyNumber, rhs: MyNumber) -> MyNumber
             hashAndKindDisambiguatedPaths["s:9Operators8MyNumberV2deoiyA2Cz_ACtFZ"],
-            "/Operators/MyNumber/_=(_:_:)-3m4ko")
+            "/Operators/MyNumber/_=(_:_:)") // This is the only favored symbol so it doesn't require any disambiguation
 
     }
 
@@ -1363,11 +1363,8 @@ class PathHierarchyTests: XCTestCase {
                        "/ShapeKit/OverloadedEnum/firstTestMemberName(_:)-(String)")
         XCTAssertEqual(paths["s:8ShapeKit14OverloadedEnumO19firstTestMemberNameySdSaySdGF"],
                        "/ShapeKit/OverloadedEnum/firstTestMemberName(_:)-([Double])")
-
-        // The overload group is cloned from this symbol and therefore have the same function signature.
-        // Because there are two collisions with the same signature, this method can only be uniquely disambiguated with its hash.
         XCTAssertEqual(paths["s:8ShapeKit14OverloadedEnumO19firstTestMemberNameyS2dF"],
-                       "/ShapeKit/OverloadedEnum/firstTestMemberName(_:)-4ja8m")
+                       "/ShapeKit/OverloadedEnum/firstTestMemberName(_:)-(Double)")
     }
 
     func testApplyingSyntaxSugarToTypeName() {
@@ -1756,7 +1753,9 @@ class PathHierarchyTests: XCTestCase {
             XCTAssertEqual(error.solutions.count, 5)
 
             XCTAssertEqual(error.solutions.dropFirst(0).first, .init(summary: "Remove '-abc123' for \n'fourthTestMemberName(test:)'", replacements: [("", 56, 63)]))
-            XCTAssertEqual(error.solutions.dropFirst(1).first, .init(summary: "Replace 'abc123' with '8iuz7' for \n'func fourthTestMemberName(test: String) -> Double\'", replacements: [("-8iuz7", 56, 63)]))
+            // The overload group is cloned from this symbol and therefore have the same function signature.
+            // Because there are two collisions with the same signature, this method can only be uniquely disambiguated with its hash.
+            XCTAssertEqual(error.solutions.dropFirst(1).first, .init(summary: "Replace '-abc123' with '->Double' for \n'func fourthTestMemberName(test: String) -> Double\'", replacements: [("->Double", 56, 63)]))
             XCTAssertEqual(error.solutions.dropFirst(2).first, .init(summary: "Replace '-abc123' with '->Float' for \n'func fourthTestMemberName(test: String) -> Float\'", replacements: [("->Float", 56, 63)]))
             XCTAssertEqual(error.solutions.dropFirst(3).first, .init(summary: "Replace '-abc123' with '->Int' for \n'func fourthTestMemberName(test: String) -> Int\'", replacements: [("->Int", 56, 63)]))
             XCTAssertEqual(error.solutions.dropFirst(4).first, .init(summary: "Replace '-abc123' with '->String' for \n'func fourthTestMemberName(test: String) -> String\'", replacements: [("->String", 56, 63)]))
@@ -2024,7 +2023,7 @@ class PathHierarchyTests: XCTestCase {
 
         // "/" is an allowed character in URL paths.
         XCTAssertEqual("/Operators/MyNumber/_(_:_:)-7am4", paths["s:9Operators8MyNumberV1doiyA2C_ACtFZ"])
-        XCTAssertEqual("/Operators/MyNumber/_=(_:_:)-3m4ko", paths["s:9Operators8MyNumberV2deoiyA2Cz_ACtFZ"])
+        XCTAssertEqual("/Operators/MyNumber/_=(_:_:)", paths["s:9Operators8MyNumberV2deoiyA2Cz_ACtFZ"]) // This is the only favored symbol so it doesn't require any disambiguation
 
         // Some of these have more human readable disambiguation alternatives
         let humanReadablePaths = tree.caseInsensitiveDisambiguatedPaths()
@@ -2035,7 +2034,7 @@ class PathHierarchyTests: XCTestCase {
         XCTAssertEqual("/Operators/MyNumber/_(_:_:)-(Self,_)",  /* >(_:_:) */ humanReadablePaths["s:SLsE1goiySbx_xtFZ::SYNTHESIZED::s:9Operators8MyNumberV"])
 
         XCTAssertEqual("/Operators/MyNumber/_(_:_:)->MyNumber", humanReadablePaths["s:9Operators8MyNumberV1doiyA2C_ACtFZ"])
-        XCTAssertEqual("/Operators/MyNumber/_=(_:_:)->MyNumber", humanReadablePaths["s:9Operators8MyNumberV2deoiyA2Cz_ACtFZ"])
+        XCTAssertEqual("/Operators/MyNumber/_=(_:_:)", humanReadablePaths["s:9Operators8MyNumberV2deoiyA2Cz_ACtFZ"]) // This is the only favored symbol so it doesn't require any disambiguation
 
         // Verify that all paths are unique
         let repeatedPaths: [String: Int] = paths.values.reduce(into: [:], { acc, path in acc[path, default: 0] += 1 })
@@ -2640,7 +2639,7 @@ class PathHierarchyTests: XCTestCase {
         let tree = context.linkResolver.localResolver.pathHierarchy
 
         let paths = tree.caseInsensitiveDisambiguatedPaths()
-        XCTAssertEqual(paths[protocolRequirementID], "/ModuleName/SomeProtocolName/someProtocolRequirement()-8lcpm")
+        XCTAssertEqual(paths[protocolRequirementID], "/ModuleName/SomeProtocolName/someProtocolRequirement()") // This is the only favored symbol so it doesn't require any disambiguation
         XCTAssertEqual(paths[defaultImplementationID], "/ModuleName/SomeProtocolName/someProtocolRequirement()-3docm")
 
         // Verify that the multi platform paths are the same as the single platform paths