Don't remove leading and trailing parentheses for closure types in type signature disambiguation (#1231) (#1243)

* Fix issue where suggested link disambiguation removed leading and trailing parentheses for some closure types

rdar://151311221

* Minor unrelated correction of whitespace in symbol declaration test data

* Include more information in test failure messages about unknown disambiguation

Author: d-ronnqvist

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

@@ -45,7 +45,7 @@ extension PathHierarchy {
         }
         let spelling = utf8TypeSpelling(for: fragments, isSwift: isSwift)
 
-        guard isSwift, spelling[...].isTuple() else {
+        guard isSwift, spelling[...].shapeOfSwiftTypeSpelling() == .tuple else {
             return [String(decoding: spelling, as: UTF8.self)]
         }
 
@@ -194,14 +194,14 @@ extension PathHierarchy {
         }
 
         // Check if the type names are wrapped in redundant parenthesis and remove them
-        if accumulated.first == openParen, accumulated.last == closeParen, !accumulated[...].isTuple() {
+        if accumulated.first == openParen, accumulated.last == closeParen, accumulated[...].shapeOfSwiftTypeSpelling() == .scalar {
             // In case there are multiple
             // Use a temporary slice until all the layers of redundant parenthesis have been removed.
             var temp = accumulated[...]
 
             repeat {
                 temp = temp.dropFirst().dropLast()
-            } while temp.first == openParen && temp.last == closeParen && !temp.isTuple()
+            } while temp.first == openParen && temp.last == closeParen && temp.shapeOfSwiftTypeSpelling() == .scalar
 
             // Adjust the markers so that they align with the expected characters
             let difference = (accumulated.count - temp.count) / 2
@@ -282,26 +282,48 @@ private let question    = UTF8.CodeUnit(ascii: "?")
 private let colon       = UTF8.CodeUnit(ascii: ":")
 private let hyphen      = UTF8.CodeUnit(ascii: "-")
 
+/// A guesstimate of the "shape" of a Swift type based on its spelling.
+private enum ShapeOfSwiftTypeSpelling {
+    /// This type spelling looks like a scalar.
+    ///
+    /// For example `Name` or `(Name)`.
+    /// - Note: We treat `(Name)` as a non-tuple so that we can remove the redundant leading and trailing parenthesis.
+    case scalar
+    /// This type spelling looks like a tuple.
+    ///
+    /// For example `(First, Second)`.
+    case tuple
+    /// This type spelling looks like a closure.
+    ///
+    /// For example `(First)->Second` or `(First, Second)->()` or `()->()`.
+    case closure
+}
+
 private extension ContiguousArray<UTF8.CodeUnit>.SubSequence {
-     /// Checks if the UTF-8 string looks like a tuple with comma separated values.
+     /// Checks if the UTF-8 string looks like a tuple, scalar, or closure.
     ///
     /// This is used to remove redundant parenthesis around expressions.
-    func isTuple() -> Bool {
-        guard first == openParen, last == closeParen else { return false }
+    func shapeOfSwiftTypeSpelling() -> ShapeOfSwiftTypeSpelling {
+        guard first == openParen, last == closeParen else { return .scalar }
         var depth = 0
-        for char in self {
-            switch char {
+        for index in indices {
+            switch self[index] {
             case openParen:
                 depth += 1
             case closeParen:
                 depth -= 1
             case comma where depth == 1:
-                return true
+                // If we find "," in one level of parenthesis, we've found a tuple.
+                return .tuple
+            case closeAngle where depth == 0 && index > startIndex && self[index - 1] == hyphen:
+                // If we find "->" outside any parentheses, we've found a closure.
+                return .closure
             default:
                 continue
             }
         }
-        return false
+        // If we traversed the entire type name without finding a tuple or a closure we treat the type name as a scalar.
+        return .scalar
     }
 }
 

Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

@@ -3328,6 +3328,73 @@ class PathHierarchyTests: XCTestCase {
         try assertFindsPath("/ModuleName/ContainerName", in: tree, asSymbolID: containerID)
     }
 
+    func testMinimalTypeDisambiguationForClosureParameterWithVoidReturnType() throws {
+        // Create a `doSomething(with:and:)` function with a `String` parameter (same in every overload) and a `(TYPE)->()` closure parameter.
+        func makeSymbolOverload(closureParameterType: SymbolGraph.Symbol.DeclarationFragments.Fragment) -> SymbolGraph.Symbol {
+            makeSymbol(
+                id: "some-function-overload-\(closureParameterType.spelling.lowercased())",
+                kind: .method,
+                pathComponents: ["doSomething(with:and:)"],
+                signature: .init(
+                    parameters: [
+                        .init(name: "first", externalName: "with", declarationFragments: [
+                            .init(kind: .externalParameter, spelling: "with", preciseIdentifier: nil),
+                            .init(kind: .text, spelling: " ", preciseIdentifier: nil),
+                            .init(kind: .internalParameter, spelling: "first", preciseIdentifier: nil),
+                            .init(kind: .text, spelling: " ", preciseIdentifier: nil),
+                            .init(kind: .typeIdentifier, spelling: "String", preciseIdentifier: "s:SS")
+                        ], children: []),
+                        
+                        .init(name: "second", externalName: "and", declarationFragments: [
+                            .init(kind: .externalParameter, spelling: "and", preciseIdentifier: nil),
+                            .init(kind: .text, spelling: " ", preciseIdentifier: nil),
+                            .init(kind: .internalParameter, spelling: "second", preciseIdentifier: nil),
+                            .init(kind: .text, spelling: " (", preciseIdentifier: nil),
+                            closureParameterType,
+                            .init(kind: .text, spelling: ") -> ()", preciseIdentifier: nil),
+                        ], children: [])
+                    ],
+                    returns: [.init(kind: .typeIdentifier, spelling: "Void", preciseIdentifier: "s:s4Voida")]
+                )
+            )
+        }
+        
+        let catalog = Folder(name: "unit-test.docc", content: [
+            JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph(
+                moduleName: "ModuleName",
+                symbols: [
+                    makeSymbolOverload(closureParameterType: .init(kind: .typeIdentifier, spelling: "Int", preciseIdentifier: "s:Si")),    // (String, (Int)->()) -> Void
+                    makeSymbolOverload(closureParameterType: .init(kind: .typeIdentifier, spelling: "Double", preciseIdentifier: "s:Sd")), // (String, (Double)->()) -> Void
+                    makeSymbolOverload(closureParameterType: .init(kind: .typeIdentifier, spelling: "Float", preciseIdentifier: "s:Sf")),  // (String, (Float)->()) -> Void
+                ],
+                relationships: []
+            ))
+        ])
+        
+        let (_, context) = try loadBundle(catalog: catalog)
+        let tree = context.linkResolver.localResolver.pathHierarchy
+        
+        let link = "/ModuleName/doSomething(with:and:)"
+        try assertPathRaisesErrorMessage(link, in: tree, context: context, expectedErrorMessage: "'doSomething(with:and:)' is ambiguous at '/ModuleName'") { errorInfo in
+            XCTAssertEqual(errorInfo.solutions.count, 3, "There should be one suggestion per overload")
+            for solution in errorInfo.solutions {
+                // Apply the suggested replacements for each solution and verify that _that_ link resolves to a single symbol.
+                var linkWithSuggestion = link
+                XCTAssertFalse(solution.replacements.isEmpty, "Diagnostics about ambiguous links should have some replacements for each solution.")
+                for (replacementText, start, end) in solution.replacements {
+                    let range = linkWithSuggestion.index(linkWithSuggestion.startIndex, offsetBy: start) ..< linkWithSuggestion.index(linkWithSuggestion.startIndex, offsetBy: end)
+                    linkWithSuggestion.replaceSubrange(range, with: replacementText)
+                }
+                
+                XCTAssertNotNil(try? tree.findSymbol(path: linkWithSuggestion), """
+                Failed to resolve \(linkWithSuggestion) after applying replacements \(solution.replacements.map { "'\($0.0)'@\($0.start)-\($0.end)" }.joined(separator: ",")) to '\(link)'.
+                
+                The replacement that DocC suggests in its warnings should unambiguously refer to a single symbol match.
+                """)
+            }
+        }
+    }
+    
     func testMissingMemberOfAnonymousStructInsideUnion() throws {
         let outerContainerID = "some-outer-container-symbol-id"
         let innerContainerID = "some-inner-container-symbol-id"
@@ -3665,7 +3732,7 @@ class PathHierarchyTests: XCTestCase {
         let voidType       = DeclToken.typeIdentifier("Void",   precise: "s:s4Voida")
 
         func makeParameter(_ name: String, decl: [DeclToken]) -> SymbolGraph.Symbol.FunctionSignature.FunctionParameter {
-            .init(name: name,  externalName: nil, declarationFragments: makeFragments([.internalParameter(name),  .text("")] + decl),  children: [])
+            .init(name: name,  externalName: nil, declarationFragments: makeFragments([.internalParameter(name), .text(" ")] + decl), children: [])
         }
 
         func makeSignature(first: DeclToken..., second: DeclToken..., third: DeclToken...) -> SymbolGraph.Symbol.FunctionSignature {
@@ -3772,6 +3839,56 @@ class PathHierarchyTests: XCTestCase {
             ])
         }
 
+        // Each overload has a unique closure parameter with a "()" literal closure return type
+        do {
+            func makeSignature(first: DeclToken..., second: DeclToken...) -> SymbolGraph.Symbol.FunctionSignature {
+                .init(
+                    parameters: [
+                        .init(name: "first",  externalName: nil, declarationFragments: makeFragments(first),  children: []),
+                        .init(name: "second", externalName: nil, declarationFragments: makeFragments(second), children: [])
+                    ],
+                    returns: makeFragments([voidType])
+                )
+            }
+            
+            //  String   (Int)->()
+            //  String   (Double)->()
+            //  String   (Float)->()
+            let catalog = Folder(name: "unit-test.docc", content: [
+                JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph(
+                    moduleName: "ModuleName",
+                    symbols: [
+                        //  String   (Int)->Void
+                        makeSymbol(id: "function-overload-1", kind: .func, pathComponents: ["doSomething(first:second:)"], signature: makeSignature(
+                            first: stringType,              // String
+                            second: "(", intType, ") -> ()" // (Int)->()
+                        )),
+                        
+                        //  String   (Double)->Void
+                        makeSymbol(id: "function-overload-2", kind: .func, pathComponents: ["doSomething(first:second:)"], signature: makeSignature(
+                            first: stringType,                 // String
+                            second: "(", doubleType, ") -> ()" // (Double)->()
+                        )),
+                        
+                        //  String   (Float)->Void
+                        makeSymbol(id: "function-overload-3", kind: .func, pathComponents: ["doSomething(first:second:)"], signature: makeSignature(
+                            first: stringType,                // String
+                            second: "(", floatType, ") -> ()" // (Double)->()
+                        )),
+                    ]
+                ))
+            ])
+            
+            let (_, context) = try loadBundle(catalog: catalog)
+            let tree = context.linkResolver.localResolver.pathHierarchy
+            
+            try assertPathCollision("ModuleName/doSomething(first:second:)", in: tree, collisions: [
+                (symbolID: "function-overload-1", disambiguation: "-(_,(Int)->())"),    //  _     (Int)->()
+                (symbolID: "function-overload-2", disambiguation: "-(_,(Double)->())"), //  _     (Double)->()
+                (symbolID: "function-overload-3", disambiguation: "-(_,(Float)->())"),  //  _     (Float)->()
+            ])
+        }
+        
         // The second overload refers to the metatype of the parameter
         do {
             func makeSignature(first: DeclToken...) -> SymbolGraph.Symbol.FunctionSignature {
@@ -4350,8 +4467,8 @@ class PathHierarchyTests: XCTestCase {
             XCTFail("Symbol for \(path.singleQuoted) not found in tree", file: file, line: line)
         } catch PathHierarchy.Error.unknownName {
             XCTFail("Symbol for \(path.singleQuoted) not found in tree. Only part of path is found.", file: file, line: line)
-        } catch PathHierarchy.Error.unknownDisambiguation {
-            XCTFail("Symbol for \(path.singleQuoted) not found in tree. Unknown disambiguation.", file: file, line: line)
+        } catch PathHierarchy.Error.unknownDisambiguation(_, _, let candidates) {
+            XCTFail("Symbol for \(path.singleQuoted) not found in tree. Unknown disambiguation. Suggested disambiguations: \(candidates.map(\.disambiguation.singleQuoted).sorted().joined(separator: ", "))", file: file, line: line)
         } catch PathHierarchy.Error.lookupCollision(_, _, let collisions) {
             let symbols = collisions.map { $0.node.symbol! }
             XCTFail("Unexpected collision for \(path.singleQuoted); \(symbols.map { return "\($0.names.title) - \($0.kind.identifier.identifier) - \($0.identifier.precise.stableHashString)"})", file: file, line: line)