From 1a916a66ad47055be53e29899b85b2f95cd89526 Mon Sep 17 00:00:00 2001 From: Max Obermeier Date: Sun, 29 Jan 2023 11:56:11 +0100 Subject: [PATCH] address review feedback - add DescribedError conformance TopicReferenceResolutionError - improve Hashable implementation of TopicReferenceResolutionError by storing properties demanded by DescribedError instead of raw Error - conform Replacement and Solution to Hashable to aid Hashable implementation of TopicReferenceResolutionError - add fixme regarding support for regular markdown link-style references when creating solutions (#470) - add basic semantic information about possible solutions back into PathHierarchyTests - remove debug print in SymbolTests --- .../Diagnostics/Replacement.swift | 2 +- .../Infrastructure/Diagnostics/Solution.swift | 2 +- .../Link Resolution/PathHierarchy.swift | 3 +- Sources/SwiftDocC/Model/Identifier.swift | 62 ++++++---------- .../Semantics/ReferenceResolver.swift | 6 +- .../Infrastructure/PathHierarchyTests.swift | 72 ++++++++++++++++--- .../Semantics/SymbolTests.swift | 2 - 7 files changed, 95 insertions(+), 54 deletions(-) diff --git a/Sources/SwiftDocC/Infrastructure/Diagnostics/Replacement.swift b/Sources/SwiftDocC/Infrastructure/Diagnostics/Replacement.swift index 9d8e8540a0..0ad2394b33 100644 --- a/Sources/SwiftDocC/Infrastructure/Diagnostics/Replacement.swift +++ b/Sources/SwiftDocC/Infrastructure/Diagnostics/Replacement.swift @@ -14,7 +14,7 @@ import SymbolKit /** A textual replacement. */ -public struct Replacement { +public struct Replacement: Hashable { /// The range to replace. public var range: SourceRange diff --git a/Sources/SwiftDocC/Infrastructure/Diagnostics/Solution.swift b/Sources/SwiftDocC/Infrastructure/Diagnostics/Solution.swift index 9c89c559a7..5d34f49992 100644 --- a/Sources/SwiftDocC/Infrastructure/Diagnostics/Solution.swift +++ b/Sources/SwiftDocC/Infrastructure/Diagnostics/Solution.swift @@ -11,7 +11,7 @@ /** A solution to a `Problem`. */ -public struct Solution { +public struct Solution: Hashable { /// A *brief* description of what the solution is. public var summary: String diff --git a/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift b/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift index b31ba2a22c..5c6ac1dbd3 100644 --- a/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift +++ b/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift @@ -836,7 +836,8 @@ extension PathHierarchy.Error { /// /// - Parameters: /// - context: The ``DocumentationContext`` the `originalReference` was resolved in. - /// - originalReference: The raw input string that represents the body of the reference that failed to resolve. + /// - originalReference: The raw input string that represents the body of the reference that failed to resolve. This string is + /// used to calculate the proper replacment-ranges for fixits. /// /// - Note: `Replacement`s produced by this function use `SourceLocation`s relative to the `originalReference`, i.e. the beginning /// of the _body_ of the original reference. diff --git a/Sources/SwiftDocC/Model/Identifier.swift b/Sources/SwiftDocC/Model/Identifier.swift index 8b6d12243c..c6798dc7a4 100644 --- a/Sources/SwiftDocC/Model/Identifier.swift +++ b/Sources/SwiftDocC/Model/Identifier.swift @@ -68,54 +68,36 @@ public enum TopicReferenceResolutionResult: Hashable, CustomStringConvertible { } /// The error causing the failure in the resolution of a ``TopicReference``. -public struct TopicReferenceResolutionError: Error { - // we store the base as an `Error` so that - // we can potentailly pass through a `DescribedError` - // conformance in the future - private let base: Error +public struct TopicReferenceResolutionError: DescribedError, Hashable { + public let errorDescription: String + public let recoverySuggestion: String? + public let failureReason: String? + public let helpAnchor: String? private let solutions: [Solution] - init(_ error: Error, solutions: [Solution] = []) { - self.base = error + init(_ message: String, note: String? = nil, solutions: [Solution] = []) { + self.errorDescription = message + self.recoverySuggestion = note + self.failureReason = nil + self.helpAnchor = nil self.solutions = solutions } - - var localizedDescription: String { - base.localizedDescription - } - - public var note: String? { - (base as? GenericBaseError)?.note - } } extension TopicReferenceResolutionError { - init(_ message: String, note: String? = nil, solutions: [Solution] = []) { - self.base = GenericBaseError(message: message, note: note) - self.solutions = solutions - } - - private struct GenericBaseError: DescribedError { - let message: String - let note: String? - - var errorDescription: String { - message - } - - var recoverySuggestion: String? { - note + init(_ error: Error, solutions: [Solution] = []) { + if let describedError = error as? DescribedError { + self.errorDescription = describedError.errorDescription + self.recoverySuggestion = describedError.recoverySuggestion + self.failureReason = describedError.failureReason + self.helpAnchor = describedError.helpAnchor + } else { + self.errorDescription = error.localizedDescription + self.recoverySuggestion = nil + self.failureReason = nil + self.helpAnchor = nil } - } -} - -extension TopicReferenceResolutionError: Hashable { - public static func == (lhs: TopicReferenceResolutionError, rhs: TopicReferenceResolutionError) -> Bool { - (lhs.base as CustomStringConvertible).description == (rhs.base as CustomStringConvertible).description - } - - public func hash(into hasher: inout Hasher) { - (base as CustomStringConvertible).description.hash(into: &hasher) + self.solutions = solutions } } diff --git a/Sources/SwiftDocC/Semantics/ReferenceResolver.swift b/Sources/SwiftDocC/Semantics/ReferenceResolver.swift index 64cc0a11cd..0a3b80a3ff 100644 --- a/Sources/SwiftDocC/Semantics/ReferenceResolver.swift +++ b/Sources/SwiftDocC/Semantics/ReferenceResolver.swift @@ -18,13 +18,17 @@ func unresolvedReferenceProblem(reference: TopicReference, source: URL?, range: if let source = source, let range = range, - let note = underlyingError.note { + let note = underlyingError.recoverySuggestion ?? underlyingError.failureReason ?? underlyingError.helpAnchor { notes.append(DiagnosticNote(source: source, range: range, message: note)) } var solutions: [Solution] = [] if let range = range { + // FIXME: The replacements currently only support DocC's predomentantly used custom link formats. + // We should also support the regular markdown link syntax ([doc:my/reference](doc:my/reference), or + // [custom title](doc:my/reference)), however don't have the necessary information yet. + // https://github.com/apple/swift-docc/issues/470 let innerRange = fromSymbolLink // ``my/reference`` ? SourceLocation(line: range.lowerBound.line, column: range.lowerBound.column+2, source: range.lowerBound.source).. Int { 0 } @@ -312,14 +318,26 @@ class PathHierarchyTests: XCTestCase { (symbolID: "s:14MixedFramework29CollisionsWithEscapedKeywordsC4inityyF", disambiguation: "method"), (symbolID: "s:14MixedFramework29CollisionsWithEscapedKeywordsC4inityyFZ", disambiguation: "type.method"), ]) - try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithEscapedKeywords/init()", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/MixedFramework/CollisionsWithEscapedKeywords'.") + try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithEscapedKeywords/init()", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/MixedFramework/CollisionsWithEscapedKeywords'.") { error in + XCTAssertEqual(error.solutions, [ + .init(summary: "Insert disambiguation suffix for 'func `init`()'", replacements: ["-method"]), + .init(summary: "Insert disambiguation suffix for 'init()'", replacements: ["-init"]), + .init(summary: "Insert disambiguation suffix for 'static func `init`()'", replacements: ["-type.method"]), + ]) + } try assertPathCollision("/MixedFramework/CollisionsWithEscapedKeywords/subscript()", in: tree, collisions: [ (symbolID: "s:14MixedFramework29CollisionsWithEscapedKeywordsC9subscriptyyF", disambiguation: "method"), (symbolID: "s:14MixedFramework29CollisionsWithEscapedKeywordsCSiycip", disambiguation: "subscript"), (symbolID: "s:14MixedFramework29CollisionsWithEscapedKeywordsC9subscriptyyFZ", disambiguation: "type.method"), ]) - try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithEscapedKeywords/subscript()", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/MixedFramework/CollisionsWithEscapedKeywords'.") + try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithEscapedKeywords/subscript()", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/MixedFramework/CollisionsWithEscapedKeywords'.") { error in + XCTAssertEqual(error.solutions, [ + .init(summary: "Insert disambiguation suffix for 'func `subscript`()'", replacements: ["-method"]), + .init(summary: "Insert disambiguation suffix for 'static func `subscript`()'", replacements: ["-type.method"]), + .init(summary: "Insert disambiguation suffix for 'subscript() -> Int { get }'", replacements: ["-subscript"]), + ]) + } // public enum CollisionsWithDifferentFunctionArguments { // public func something(argument: Int) -> Int { 0 } @@ -329,7 +347,15 @@ class PathHierarchyTests: XCTestCase { (symbolID: "s:14MixedFramework40CollisionsWithDifferentFunctionArgumentsO9something8argumentS2i_tF", disambiguation: "1cyvp"), (symbolID: "s:14MixedFramework40CollisionsWithDifferentFunctionArgumentsO9something8argumentSiSS_tF", disambiguation: "2vke2"), ]) - try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithDifferentFunctionArguments/something(argument:)", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/MixedFramework/CollisionsWithDifferentFunctionArguments'.") + try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithDifferentFunctionArguments/something(argument:)", + in: tree, + context: context, + expectedErrorMessage: "Reference is ambiguous after '/MixedFramework/CollisionsWithDifferentFunctionArguments'.") { error in + XCTAssertEqual(error.solutions, [ + .init(summary: "Insert disambiguation suffix for 'func something(argument: Int) -> Int'", replacements: ["-1cyvp"]), + .init(summary: "Insert disambiguation suffix for 'func something(argument: String) -> Int'", replacements: ["-2vke2"]), + ]) + } // public enum CollisionsWithDifferentSubscriptArguments { // public subscript(something: Int) -> Int { 0 } @@ -339,7 +365,12 @@ class PathHierarchyTests: XCTestCase { (symbolID: "s:14MixedFramework41CollisionsWithDifferentSubscriptArgumentsOyS2icip", disambiguation: "4fd0l"), (symbolID: "s:14MixedFramework41CollisionsWithDifferentSubscriptArgumentsOySiSScip", disambiguation: "757cj"), ]) - try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithDifferentSubscriptArguments/subscript(_:)", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/MixedFramework/CollisionsWithDifferentSubscriptArguments'.") + try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithDifferentSubscriptArguments/subscript(_:)", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/MixedFramework/CollisionsWithDifferentSubscriptArguments'.") { error in + XCTAssertEqual(error.solutions, [ + .init(summary: "Insert disambiguation suffix for 'subscript(something: Int) -> Int { get }'", replacements: ["-4fd0l"]), + .init(summary: "Insert disambiguation suffix for 'subscript(somethingElse: String) -> Int { get }'", replacements: ["-757cj"]), + ]) + } // typedef NS_OPTIONS(NSInteger, MyObjectiveCOption) { // MyObjectiveCOptionNone = 0, @@ -845,7 +876,12 @@ class PathHierarchyTests: XCTestCase { ("s:5MyKit0A5MyProtocol0Afunc()DefaultImp", "2dxqn"), ("s:5MyKit0A5MyProtocol0Afunc()", "6ijsi"), ]) - try assertPathRaisesErrorMessage("/SideKit/SideProtocol/func()", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/SideKit/SideProtocol'.") // This test data have the same declaration for both symbols. + try assertPathRaisesErrorMessage("/SideKit/SideProtocol/func()", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/SideKit/SideProtocol'.") { error in + XCTAssertEqual(error.solutions, [ + .init(summary: "Insert disambiguation suffix for 'func1()'", replacements: ["-2dxqn"]), + .init(summary: "Insert disambiguation suffix for 'func1()'", replacements: ["-6ijsi"]), + ]) + } // This test data have the same declaration for both symbols. try assertFindsPath("/FillIntroduced/iOSOnlyDeprecated()", in: tree, asSymbolID: "s:14FillIntroduced17iOSOnlyDeprecatedyyF") try assertFindsPath("/FillIntroduced/macCatalystOnlyIntroduced()", in: tree, asSymbolID: "s:14FillIntroduced015macCatalystOnlyB0yyF") @@ -886,7 +922,13 @@ class PathHierarchyTests: XCTestCase { ("c:@E@Foo", "struct"), ("c:MixedLanguageFramework.h@T@Foo", "typealias"), ]) - try assertPathRaisesErrorMessage("MixedLanguageFramework/Foo", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/MixedLanguageFramework'.") // The 'enum' and 'typealias' symbols have multi-line declarations that are presented on a single line + try assertPathRaisesErrorMessage("MixedLanguageFramework/Foo", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/MixedLanguageFramework'.") { error in + XCTAssertEqual(error.solutions, [ + .init(summary: "Insert disambiguation suffix for 'struct Foo'", replacements: ["-struct"]), + .init(summary: "Insert disambiguation suffix for 'typedef enum Foo : NSString { ... } Foo;'", replacements: ["-enum"]), + .init(summary: "Insert disambiguation suffix for 'typedef enum Foo : NSString { ... } Foo;'", replacements: ["-typealias"]), + ]) + } // The 'enum' and 'typealias' symbols have multi-line declarations that are presented on a single line try assertFindsPath("MixedLanguageFramework/Foo/first", in: tree, asSymbolID: "c:@E@Foo@first") @@ -1250,11 +1292,12 @@ class PathHierarchyTests: XCTestCase { } } - private func assertPathRaisesErrorMessage(_ path: String, in tree: PathHierarchy, context: DocumentationContext, expectedErrorMessage: String, file: StaticString = #file, line: UInt = #line) throws { + private func assertPathRaisesErrorMessage(_ path: String, in tree: PathHierarchy, context: DocumentationContext, expectedErrorMessage: String, file: StaticString = #file, line: UInt = #line, _ additionalAssertion: (TopicReferenceResolutionError) -> Void = { _ in }) throws { XCTAssertThrowsError(try tree.findSymbol(path: path), "Finding path \(path) didn't raise an error.",file: file,line: line) { untypedError in let error = untypedError as! PathHierarchy.Error let referenceError = error.asTopicReferenceResolutionError(context: context, originalReference: path) XCTAssertEqual(referenceError.localizedDescription, expectedErrorMessage, file: file, line: line) + additionalAssertion(referenceError) } } @@ -1275,3 +1318,16 @@ extension PathHierarchy { return lookup[id]!.symbol! } } + +private extension TopicReferenceResolutionError { + var solutions: [SimplifiedSolution] { + self.solutions(referenceSourceRange: SourceLocation(line: 0, column: 0, source: nil)..(_:))' couldn't be resolved. Reference at '/MyKit/MyClass' can't resolve 'UnresolvableSymbolLinkInMyClassOverview<>(_:))'. No similar pages." })) XCTAssertEqual(problem.diagnostic.notes.map(\.message), ["Available children: init(), myFunction()."]) XCTAssertEqual(problem.possibleSolutions.count, 2)