Add logic to look for overload groups when emitting warnings (#885)

* Add logic to look for overload groups when emitting warnings and
suggestions about ambiguous symbols. If exactly one of the ambiguous
symbols is an overload group, then suggest the author simply remove
the incorrect disambiguation.

* Refactor logic for ambiguous symbol warnings to first emit a message
about selecting the preferred symbol without any disambiguation hash,
followed by a list of all the remaining symbols to select from with
their hashes.

* PR Feedback:

- PathHierarchy.makePartialResultError checks for a preferred symbol,
normally an overload group, when throwing processing unknownDisambiguation
errors.

- Simplify makeTopicReferenceResolutionErrorInfo and limit it to formatting the
warning message.

* PR Feedback: Simplify PathHierarchy+Error.swift and
PathHierarchy.makePartialResultError further.

* PR Feedback:
- Further simplify logic for displaying unknown disambiguation warnings

Author: patshaughnessy

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

@@ -150,8 +150,8 @@ extension PathHierarchy.Error {
             let solutions: [Solution] = candidates
                 .sorted(by: collisionIsBefore)
                 .map { (node: PathHierarchy.Node, disambiguation: String) -> Solution in
-                    return Solution(summary: "\(Self.replacementOperationDescription(from: disambiguations.dropFirst(), to: disambiguation)) for\n\(fullNameOfNode(node).singleQuoted)", replacements: [
-                        Replacement(range: replacementRange, replacement: "-" + disambiguation)
+                    return Solution(summary: "\(Self.replacementOperationDescription(from: disambiguations, to: disambiguation)) for\n\(fullNameOfNode(node).singleQuoted)", replacements: [
+                        Replacement(range: replacementRange, replacement: disambiguation)
                     ])
                 }
 

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

@@ -355,29 +355,52 @@ extension PathHierarchy {
         remaining: ArraySlice<PathComponent>,
         rawPathForError: String
     ) -> Error {
-        if let disambiguationTree = node.children[String(remaining.first!.name)] {
-            return Error.unknownDisambiguation(
+        guard let disambiguationTree = node.children[String(remaining.first!.name)] else {
+            return Error.unknownName(
                 partialResult: (
                     node,
                     pathForError(of: rawPathForError, droppingLast: remaining.count)
                 ),
                 remaining: Array(remaining),
-                candidates: disambiguationTree.disambiguatedValues().map {
-                    (node: $0.value, disambiguation: String($0.disambiguation.makeSuffix().dropFirst()))
-                }
+                availableChildren: Set(node.children.keys)
             )
         }
-        
-        return Error.unknownName(
+
+        // 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),
-            availableChildren: Set(node.children.keys)
+            candidates: candidatesAndSuffixes
         )
     }
-    
+
+    /// 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

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

@@ -251,7 +251,8 @@ final class PathHierarchyBasedLinkResolver {
     func fullName(of node: PathHierarchy.Node, in context: DocumentationContext) -> String {
         guard let identifier = node.identifier else { return node.name }
         if let symbol = node.symbol {
-            if let fragments = symbol.declarationFragments {
+            // Use the simple title for overload group symbols to avoid showing detailed type info
+            if !symbol.isOverloadGroup, let fragments = symbol.declarationFragments {
                 return fragments.map(\.spelling).joined().split(whereSeparator: { $0.isWhitespace || $0.isNewline }).joined(separator: " ")
             }
             return symbol.names.title

Tests/SwiftDocCTests/Infrastructure/ExternalPathHierarchyResolverTests.swift

@@ -379,8 +379,8 @@ class ExternalPathHierarchyResolverTests: XCTestCase {
             authoredLink: "/MixedFramework/CollisionsWithDifferentKinds/something-class",
             errorMessage: "'class' isn't a disambiguation for 'something' at '/MixedFramework/CollisionsWithDifferentKinds'",
             solutions: [
-                .init(summary: "Replace 'class' with 'enum.case' for\n'case something'", replacement: ("-enum.case", 54, 60)),
-                .init(summary: "Replace 'class' with 'property' for\n'var something: String { get }'", replacement: ("-property", 54, 60)),
+                .init(summary: "Replace '-class' with '-enum.case' for\n'case something'", replacement: ("-enum.case", 54, 60)),
+                .init(summary: "Replace '-class' with '-property' for\n'var something: String { get }'", replacement: ("-property", 54, 60)),
             ]
         )
 
@@ -407,9 +407,9 @@ class ExternalPathHierarchyResolverTests: XCTestCase {
             authoredLink: "/MixedFramework/CollisionsWithEscapedKeywords/init()-abc123",
             errorMessage: "'abc123' isn't a disambiguation for 'init()' at '/MixedFramework/CollisionsWithEscapedKeywords'",
             solutions: [
-                .init(summary: "Replace 'abc123' with 'method' for\n'func `init`()'", replacement: ("-method", 52, 59)),
-                .init(summary: "Replace 'abc123' with 'init' for\n'init()'", replacement: ("-init", 52, 59)),
-                .init(summary: "Replace 'abc123' with 'type.method' for\n'static func `init`()'", replacement: ("-type.method", 52, 59)),
+                .init(summary: "Replace '-abc123' with '-method' for\n'func `init`()'", replacement: ("-method", 52, 59)),
+                .init(summary: "Replace '-abc123' with '-init' for\n'init()'", replacement: ("-init", 52, 59)),
+                .init(summary: "Replace '-abc123' with '-type.method' for\n'static func `init`()'", replacement: ("-type.method", 52, 59)),
             ]
         )
         // Providing disambiguation will narrow down the suggestions. Note that `()` is missing in the last path component
@@ -469,8 +469,8 @@ class ExternalPathHierarchyResolverTests: XCTestCase {
             authoredLink: "/MixedFramework/CollisionsWithDifferentFunctionArguments/something(argument:)-abc123",
             errorMessage: "'abc123' isn't a disambiguation for 'something(argument:)' at '/MixedFramework/CollisionsWithDifferentFunctionArguments'",
             solutions: [
-                .init(summary: "Replace 'abc123' with '1cyvp' for\n'func something(argument: Int) -> Int'", replacement: ("-1cyvp", 77, 84)),
-                .init(summary: "Replace 'abc123' with '2vke2' for\n'func something(argument: String) -> Int'", replacement: ("-2vke2", 77, 84)),
+                .init(summary: "Replace '-abc123' with '-1cyvp' for\n'func something(argument: Int) -> Int'", replacement: ("-1cyvp", 77, 84)),
+                .init(summary: "Replace '-abc123' with '-2vke2' for\n'func something(argument: String) -> Int'", replacement: ("-2vke2", 77, 84)),
             ]
         )
         // Providing disambiguation will narrow down the suggestions. Note that `argument` label is missing in the last path component

Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

@@ -327,8 +327,8 @@ class PathHierarchyTests: XCTestCase {
         'class' isn't a disambiguation for 'something' at '/MixedFramework/CollisionsWithDifferentKinds'
         """) { error in
             XCTAssertEqual(error.solutions, [
-                .init(summary: "Replace 'class' with 'enum.case' for\n'case something'", replacements: [("-enum.case", 54, 60)]),
-                .init(summary: "Replace 'class' with 'property' for\n'var something: String { get }'", replacements: [("-property", 54, 60)]),
+                .init(summary: "Replace '-class' with '-enum.case' for\n'case something'", replacements: [("-enum.case", 54, 60)]),
+                .init(summary: "Replace '-class' with '-property' for\n'var something: String { get }'", replacements: [("-property", 54, 60)]),
             ])
         }
 
@@ -350,9 +350,9 @@ class PathHierarchyTests: XCTestCase {
         'abc123' isn't a disambiguation for 'init()' at '/MixedFramework/CollisionsWithEscapedKeywords'
         """) { error in
             XCTAssertEqual(error.solutions, [
-                .init(summary: "Replace 'abc123' with 'method' for\n'func `init`()'", replacements: [("-method", 52, 59)]),
-                .init(summary: "Replace 'abc123' with 'init' for\n'init()'", replacements: [("-init", 52, 59)]),
-                .init(summary: "Replace 'abc123' with 'type.method' for\n'static func `init`()'", replacements: [("-type.method", 52, 59)]),
+                .init(summary: "Replace '-abc123' with '-method' for\n'func `init`()'", replacements: [("-method", 52, 59)]),
+                .init(summary: "Replace '-abc123' with '-init' for\n'init()'", replacements: [("-init", 52, 59)]),
+                .init(summary: "Replace '-abc123' with '-type.method' for\n'static func `init`()'", replacements: [("-type.method", 52, 59)]),
             ])
         }
         try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithEscapedKeywords/init()", in: tree, context: context, expectedErrorMessage: """
@@ -431,8 +431,8 @@ class PathHierarchyTests: XCTestCase {
         'abc123' isn't a disambiguation for 'something(argument:)' at '/MixedFramework/CollisionsWithDifferentFunctionArguments'
         """) { error in
             XCTAssertEqual(error.solutions, [
-                .init(summary: "Replace 'abc123' with '1cyvp' for\n'func something(argument: Int) -> Int'", replacements: [("-1cyvp", 77, 84)]),
-                .init(summary: "Replace 'abc123' with '2vke2' for\n'func something(argument: String) -> Int'", replacements: [("-2vke2", 77, 84)]),
+                .init(summary: "Replace '-abc123' with '-1cyvp' for\n'func something(argument: Int) -> Int'", replacements: [("-1cyvp", 77, 84)]),
+                .init(summary: "Replace '-abc123' with '-2vke2' for\n'func something(argument: String) -> Int'", replacements: [("-2vke2", 77, 84)]),
             ])
         }
         // Providing disambiguation will narrow down the suggestions. Note that `argument` label is missing in the last path component
@@ -1318,6 +1318,24 @@ class PathHierarchyTests: XCTestCase {
         // This overloaded protocol method should be able to resolve without a suffix at all, since it doesn't conflict with anything
         let overloadedProtocolMethod = try tree.findNode(path: "/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)", onlyFindSymbols: true)
         XCTAssert(overloadedProtocolMethod.symbol?.identifier.precise.hasSuffix(SymbolGraph.Symbol.overloadGroupIdentifierSuffix) == true)
+
+    }
+
+    func testAmbiguousPathsForOverloadedGroupSymbols() throws {
+        enableFeatureFlag(\.isExperimentalOverloadedSymbolPresentationEnabled)
+        let (_, context) = try testBundleAndContext(named: "OverloadedSymbols")
+        let tree = context.linkResolver.localResolver.pathHierarchy
+        try assertPathRaisesErrorMessage("/ShapeKit/OverloadedProtocol/fourthTestMemberName(test:)-abc123", in: tree, context: context, expectedErrorMessage: """
+        'abc123' isn't a disambiguation for 'fourthTestMemberName(test:)' at '/ShapeKit/OverloadedProtocol'
+        """) { error in
+            XCTAssertEqual(error.solutions, [
+                .init(summary: "Remove '-abc123' for\n'fourthTestMemberName(test:)'", replacements: [("", 56, 63)]),
+                .init(summary: "Replace '-abc123' with '-8iuz7' for\n'func fourthTestMemberName(test: String) -> Double\'", replacements: [("-8iuz7", 56, 63)]),
+                .init(summary: "Replace '-abc123' with '-1h173' for\n'func fourthTestMemberName(test: String) -> Float\'", replacements: [("-1h173", 56, 63)]),
+                .init(summary: "Replace '-abc123' with '-91hxs' for\n'func fourthTestMemberName(test: String) -> Int\'", replacements: [("-91hxs", 56, 63)]),
+                .init(summary: "Replace '-abc123' with '-961zx' for\n'func fourthTestMemberName(test: String) -> String\'", replacements: [("-961zx", 56, 63)]),
+            ])
+        }
     }
 
     func testDoesNotSuggestBundleNameForSymbolLink() throws {