Skip to content

Commit

Permalink
address review feedback
Browse files Browse the repository at this point in the history 
 - 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
  • Loading branch information
@theMomax
theMomax committed Feb 8, 2023
1 parent df61ad6 commit 1a916a6
Show file tree
Hide file tree
Showing 7 changed files with 95 additions and 54 deletions.
2 changes: 1 addition & 1 deletion Sources/SwiftDocC/Infrastructure/Diagnostics/Replacement.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import SymbolKit
/**
A textual replacement.
*/
public struct Replacement {
public struct Replacement: Hashable {
/// The range to replace.
public var range: SourceRange

Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftDocC/Infrastructure/Diagnostics/Solution.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 2 additions & 1 deletion Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
62 changes: 22 additions & 40 deletions Sources/SwiftDocC/Model/Identifier.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down
6 changes: 5 additions & 1 deletion Sources/SwiftDocC/Semantics/ReferenceResolver.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)..<SourceLocation(line: range.upperBound.line, column: range.upperBound.column-2, source: range.upperBound.source)
Expand Down
72 changes: 64 additions & 8 deletions Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import XCTest
import SymbolKit
@testable import SwiftDocC
import SwiftDocCTestUtilities
import Markdown

class PathHierarchyTests: XCTestCase {

Expand Down Expand Up @@ -296,7 +297,12 @@ class PathHierarchyTests: XCTestCase {
])
try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithDifferentKinds/something", in: tree, context: context, expectedErrorMessage: """
Reference is ambiguous after '/MixedFramework/CollisionsWithDifferentKinds'.
""")
""") { error in
XCTAssertEqual(error.solutions, [
.init(summary: "Insert disambiguation suffix for 'case something'", replacements: ["-enum.case"]),
.init(summary: "Insert disambiguation suffix for 'var something: String { get }'", replacements: ["-property"]),
])
}

// public final class CollisionsWithEscapedKeywords {
// public subscript() -> Int { 0 }
Expand All @@ -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 }
Expand All @@ -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 }
Expand All @@ -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,
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")

Expand Down Expand Up @@ -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)
}
}

Expand All @@ -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)..<SourceLocation(line: 0, column: 0, source: nil)).map { solution in
SimplifiedSolution(summary: solution.summary, replacements: solution.replacements.map(\.replacement))
}
}
}

private struct SimplifiedSolution: Equatable {
let summary: String
let replacements: [String]
}
2 changes: 0 additions & 2 deletions Tests/SwiftDocCTests/Semantics/SymbolTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -564,8 +564,6 @@ class SymbolTests: XCTestCase {
if LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver {
var problem: Problem

print(unresolvedTopicProblems.map(\.diagnostic.localizedSummary))

problem = try XCTUnwrap(unresolvedTopicProblems.first(where: { $0.diagnostic.localizedSummary == "Topic reference 'UnresolvableSymbolLinkInMyClassOverview<>(_:))' 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)
Expand Down

0 comments on commit 1a916a6

Please sign in to comment.