Fix #438 (Structure suggestions for link resolution warnings as fixits) (#455)

* fix #438 (Structure suggestions for link resolution warnings as fixits)

* address review feedback

 - create TopicReferenceResolutionError with minimal public interface
 - replace TopicReferenceResolutionResult.failure's errorMessage with TopicReferenceResolutionError
 - add support for non-symbol reference syntax
 - add Solution for PathHierarchy.Error.nonSymbolMatchForSymbolLink
 - add meaningful information to Solution.summary
 - scope Replacements as small as possible (on disambiguation, path segment, or reference delimiters)
 - add note listing all available candidates even if near-miss suggestions are available
 - add Solutions if no near-miss is available, but there are only three candidates or less
 - make offsetWithRange mutating
 - remove XCTAssertElements test helper

* 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

* add assertions for solutions in SymbolTests/testUnresolvedReferenceWarningsInDocComment() + improve improve documentation about indices in source locations

---------

Co-authored-by: David Rönnqvist <[email protected]>

rdar://103279313

Author: theMomax

Sources/SwiftDocC/Infrastructure/Diagnostics/Diagnostic.swift

@@ -87,21 +87,13 @@ public struct Diagnostic: DescribedError {
 
 public extension Diagnostic {
 
-    /// Returns a copy of the diagnostic but offset using a certain `SourceRange`.
+    /// Offsets the diagnostic using a certain SymbolKit `SourceRange`.
+    ///
     /// Useful when validating a doc comment that needs to be projected in its containing file "space".
-    func offsetedWithRange(_ docRange: SymbolGraph.LineList.SourceRange) -> Diagnostic {
-        guard let diagnosticRange = range else {
-            // No location information in the source diagnostic, might be removed for safety reasons.
-            return self
-        }
-
-        let start = SourceLocation(line: diagnosticRange.lowerBound.line + docRange.start.line, column: diagnosticRange.lowerBound.column + docRange.start.character, source: nil)
-        let end = SourceLocation(line: diagnosticRange.upperBound.line + docRange.start.line, column: diagnosticRange.upperBound.column + docRange.start.character, source: nil)
-
-        // Use the updated source range.
-        var result = self
-        result.range = start..<end
-        return result
+    mutating func offsetWithRange(_ docRange: SymbolGraph.LineList.SourceRange) {
+        // If there is no location information in the source diagnostic, the diagnostic might be removed for safety reasons.
+        range?.offsetWithRange(docRange)
+        
     }
 
     var localizedDescription: String {

Sources/SwiftDocC/Infrastructure/Diagnostics/Problem.swift

@@ -8,6 +8,8 @@
  See https://swift.org/CONTRIBUTORS.txt for Swift project authors
 */
 
+import SymbolKit
+
 /**
  A problem with a document along with possible solutions to the problem.
  */
@@ -45,7 +47,22 @@ extension Problem {
             })
         })
 
-        return description +  fixitString
+        return description + fixitString
+    }
+}
+
+extension Problem {
+    /// Offsets the problem using a certain SymbolKit `SourceRange`.
+    ///
+    /// Useful when validating a doc comment that needs to be projected in its containing file "space".
+    mutating func offsetWithRange(_ docRange: SymbolGraph.LineList.SourceRange) {
+        diagnostic.offsetWithRange(docRange)
+        
+        for i in possibleSolutions.indices {
+            for j in possibleSolutions[i].replacements.indices {
+                possibleSolutions[i].replacements[j].offsetWithRange(docRange)
+            }
+        }
     }
 }
 

Sources/SwiftDocC/Infrastructure/Diagnostics/Replacement.swift

@@ -9,11 +9,12 @@
 */
 
 import Markdown
+import SymbolKit
 
 /**
  A textual replacement.
  */
-public struct Replacement {
+public struct Replacement: Hashable {
     /// The range to replace.
     public var range: SourceRange
 
@@ -25,3 +26,22 @@ public struct Replacement {
         self.replacement = replacement
     }
 }
+
+extension Replacement {
+    /// Offsets the replacement using a certain `SourceRange`.
+    ///
+    /// Useful when validating a doc comment that needs to be projected in its containing file "space".
+    ///
+    /// - Warning: Absolute `SourceRange`s index line and column from 1. Thus, at least one
+    /// of `self` or `range` must be a relative range indexed from 0.
+    mutating func offsetWithRange(_ range: SourceRange) {
+        self.range.offsetWithRange(range)
+    }
+    
+    /// Offsets the replacement using a certain SymbolKit `SourceRange`.
+    /// 
+    /// Useful when validating a doc comment that needs to be projected in its containing file "space".
+    mutating func offsetWithRange(_ docRange: SymbolGraph.LineList.SourceRange) {
+        self.offsetWithRange(SourceRange(from: docRange))
+    }
+}

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
 

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

@@ -624,10 +624,12 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
                        let docs = documentationNode.symbol?.docComment {
                         // Offset all problem ranges by the start location of the
                         // source comment in the context of the complete file.
-                        problems = problems.compactMap { problem -> Problem? in
-                            guard let docRange = docs.lines.first?.range else { return nil }
-                            return Problem(diagnostic: problem.diagnostic.offsetedWithRange(docRange),
-                                           possibleSolutions: problem.possibleSolutions)
+                        if let docRange = docs.lines.first?.range {
+                            for i in problems.indices {
+                                problems[i].offsetWithRange(docRange)
+                            }
+                        } else {
+                            problems.removeAll()
                         }
                     }
 

Sources/SwiftDocC/Infrastructure/External Data/OutOfProcessReferenceResolver.swift

@@ -123,7 +123,7 @@ public class OutOfProcessReferenceResolver: ExternalReferenceResolver, FallbackR
             do {
                 guard let unresolvedTopicURL = unresolvedReference.topicURL.components.url else {
                     // Return the unresolved reference if the underlying URL is not valid
-                    return .failure(unresolvedReference, errorMessage: "URL \(unresolvedReference.topicURL.absoluteString.singleQuoted) is not valid.")
+                    return .failure(unresolvedReference, TopicReferenceResolutionError("URL \(unresolvedReference.topicURL.absoluteString.singleQuoted) is not valid."))
                 }
                 let metadata = try resolveInformationForTopicURL(unresolvedTopicURL)
                 // Don't do anything with this URL. The external URL will be resolved during conversion to render nodes
@@ -136,7 +136,7 @@ public class OutOfProcessReferenceResolver: ExternalReferenceResolver, FallbackR
                     )
                 )
             } catch let error {
-                return .failure(unresolvedReference, errorMessage: error.localizedDescription)
+                return .failure(unresolvedReference, TopicReferenceResolutionError(error))
             }
         }
     }

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

@@ -129,7 +129,7 @@ final class DocumentationCacheBasedLinkResolver {
         // Ensure we are resolving either relative links or "doc:" scheme links
         guard unresolvedReference.topicURL.url.scheme == nil || ResolvedTopicReference.urlHasResolvedTopicScheme(unresolvedReference.topicURL.url) else {
             // Not resolvable in the topic graph
-            return .failure(unresolvedReference, errorMessage: "Reference URL \(unresolvedReference.description.singleQuoted) doesn't have \"doc:\" scheme.")
+            return .failure(unresolvedReference, TopicReferenceResolutionError("Reference URL \(unresolvedReference.description.singleQuoted) doesn't have \"doc:\" scheme."))
         }
 
         // Fall back on the parent's bundle identifier for relative paths
@@ -267,7 +267,7 @@ final class DocumentationCacheBasedLinkResolver {
                 // Return the successful or failed externally resolved reference.
                 return resolvedExternalReference
             } else if !context.registeredBundles.contains(where: { $0.identifier == bundleID }) {
-                return .failure(unresolvedReference, errorMessage: "No external resolver registered for \(bundleID.singleQuoted).")
+                return .failure(unresolvedReference, TopicReferenceResolutionError("No external resolver registered for \(bundleID.singleQuoted)."))
             }
         }
 
@@ -295,7 +295,7 @@ final class DocumentationCacheBasedLinkResolver {
         // Give up: there is no local or external document for this reference.
 
         // External references which failed to resolve will already have returned a more specific error message.
-        return .failure(unresolvedReference, errorMessage: "No local documentation matches this reference.")
+        return .failure(unresolvedReference, TopicReferenceResolutionError("No local documentation matches this reference."))
     }
 
 

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

@@ -10,6 +10,7 @@
 
 import Foundation
 import SymbolKit
+import Markdown
 
 /// An opaque identifier that uniquely identifies a resolved entry in the path hierarchy,
 ///
@@ -321,7 +322,9 @@ struct PathHierarchy {
                 }
             } catch DisambiguationTree.Error.lookupCollision(let collisions) {
                 func wrappedCollisionError() -> Error {
-                    Error.lookupCollision(partialResult: node, collisions: collisions)
+                    Error.lookupCollision(partialResult: node,
+                                          remainingSubpath: remaining.map(\.full).joined(separator: "/"),
+                                          collisions: collisions)
                 }
 
                 // See if the collision can be resolved by looking ahead on level deeper.
@@ -365,6 +368,7 @@ struct PathHierarchy {
                 // Couldn't resolve the collision by look ahead.
                 throw Error.lookupCollision(
                     partialResult: node,
+                    remainingSubpath: remaining.map(\.full).joined(separator: "/"),
                     collisions: collisions.map { ($0.node, $0.disambiguation) }
                 )
             }
@@ -814,8 +818,9 @@ extension PathHierarchy {
         ///
         /// Includes information about:
         /// - The partial result for as much of the path that could be found unambiguously.
+        /// - The remaining portion of the path.
         /// - A list of possible matches paired with the disambiguation suffixes needed to distinguish them.
-        case lookupCollision(partialResult: Node, collisions: [(node: Node, disambiguation: String)])
+        case lookupCollision(partialResult: Node, remainingSubpath: String, collisions: [(node: Node, disambiguation: String)])
     }
 }
 
@@ -825,30 +830,101 @@ private func availableChildNameIsBefore(_ lhs: String, _ rhs: String) -> Bool {
 }
 
 extension PathHierarchy.Error {
-    /// Formats the error into an error message suitable for presentation
-    func errorMessage(context: DocumentationContext) -> String {
+    /// Generate a ``TopicReferenceResolutionError`` from this error using the given `context` and `originalReference`.
+    ///
+    /// The resulting ``TopicReferenceResolutionError`` is human-readable and provides helpful solutions.
+    ///
+    /// - 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. 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.
+    func asTopicReferenceResolutionError(context: DocumentationContext, originalReference: String) -> TopicReferenceResolutionError {
         switch self {
-        case .partialResult(let partialResult, let remaining, let available):
-            let nearMisses = NearMiss.bestMatches(for: available, against: remaining)
-            let suggestion: String
-            switch nearMisses.count {
-            case 0:
-                suggestion = "No similar pages. Available children: \(available.joined(separator: ", "))."
-            case 1:
-                suggestion = "Did you mean: \(nearMisses[0])?"
-            default:
-                suggestion = "Did you mean one of: \(nearMisses.joined(separator: ", "))?"
+        case .notFound(availableChildren: let availableChildren):
+            return TopicReferenceResolutionError("No local documentation matches this reference.", note: availabilityNote(category: "top-level elements", candidates: availableChildren))
+        case .unfindableMatch:
+            return TopicReferenceResolutionError("No local documentation matches this reference.")
+        case .nonSymbolMatchForSymbolLink:
+            return TopicReferenceResolutionError("Symbol links can only resolve symbols.", solutions: [
+                Solution(summary: "Use a '<doc:>' style reference.", replacements: [
+                    // the SourceRange points to the opening double-backtick
+                    Replacement(range: SourceLocation(line: 0, column: -2, source: nil)..<SourceLocation(line: 0, column: 0, source: nil), replacement: "<doc:"),
+                    // the SourceRange points to the closing double-backtick
+                    Replacement(range: SourceLocation(line: 0, column: originalReference.count, source: nil)..<SourceLocation(line: 0, column: originalReference.count+2, source: nil), replacement: ">"),
+                ])
+            ])
+        case .partialResult(partialResult: let partialResult, remainingSubpath: let remainingSubpath, availableChildren: let availableChildren):
+            let nearMisses = NearMiss.bestMatches(for: availableChildren, against: remainingSubpath)
+            
+            let validPrefix = originalReference.dropLast(remainingSubpath.count)
+            
+            let unprocessedSuffix = remainingSubpath.suffix(from: remainingSubpath.firstIndex(of: "/") ?? remainingSubpath.endIndex)
+            
+            let replacementRange = SourceLocation(line: 0, column: validPrefix.count, source: nil)..<SourceLocation(line: 0, column: originalReference.count-unprocessedSuffix.count, source: nil)
+            
+            // we don't want to provide an endless amount of unrelated fixits, but if there are only three
+            // or less valid options, we still suggest them as fixits
+            let fixitCandidates = nearMisses.isEmpty && availableChildren.count <= 3 ? availableChildren : nearMisses
+            
+            let solutions = fixitCandidates.map { candidate in
+                Solution(summary: "Correct reference to \(validPrefix + candidate + unprocessedSuffix).", replacements: [
+                    Replacement(range: replacementRange, replacement: candidate)
+                ])
             }
-            return "Reference at \(partialResult.pathWithoutDisambiguation().singleQuoted) can't resolve \(remaining.singleQuoted). \(suggestion)"
-        case .notFound, .unfindableMatch:
-            return "No local documentation matches this reference."
 
-        case .nonSymbolMatchForSymbolLink:
-            return "Symbol links can only resolve symbols."
 
-        case .lookupCollision(let partialResult, let collisions):
-            let collisionDescription = collisions.map { "Append '-\($0.disambiguation)' to refer to \($0.node.fullNameOfValue(context: context).singleQuoted)" }.sorted()
-            return "Reference is ambiguous after \(partialResult.pathWithoutDisambiguation().singleQuoted): \(collisionDescription.joined(separator: ". "))."
+            var message: String {
+                var suggestion: String {
+                    switch nearMisses.count {
+                    case 0:
+                        return "No similar pages."
+                    case 1:
+                        return "Did you mean \(nearMisses[0])?"
+                    default:
+                        return "Did you mean one of: \(nearMisses.joined(separator: ", "))?"
+                    }
+                }
+                
+                return "Reference at \(partialResult.pathWithoutDisambiguation().singleQuoted) can't resolve \(remainingSubpath.singleQuoted). \(suggestion)"
+            }
+            
+            return TopicReferenceResolutionError(message, note: availabilityNote(category: "children", candidates: availableChildren), solutions: solutions)
+            
+        case .lookupCollision(partialResult: let partialResult, remainingSubpath: let remainingSubpath, collisions: let collisions):
+            let unprocessedSuffix = remainingSubpath.suffix(from: remainingSubpath.firstIndex(of: "/") ?? remainingSubpath.endIndex)
+            
+            let prefixPotentiallyIncludingInvalidDisambiguation = originalReference.dropLast(unprocessedSuffix.count)
+            let lowerBoundOfLastSegment = prefixPotentiallyIncludingInvalidDisambiguation.lastIndex(of: "/")
+                ?? prefixPotentiallyIncludingInvalidDisambiguation.startIndex
+            let lowerBoundOfLastDisambiguation = prefixPotentiallyIncludingInvalidDisambiguation.lastIndex(of: "-")
+                ?? prefixPotentiallyIncludingInvalidDisambiguation.startIndex
+            let lowerBoundOfInvalidDisambiguation = lowerBoundOfLastDisambiguation > lowerBoundOfLastSegment ? lowerBoundOfLastDisambiguation : nil
+            let validPrefix = prefixPotentiallyIncludingInvalidDisambiguation[..<(lowerBoundOfInvalidDisambiguation ?? prefixPotentiallyIncludingInvalidDisambiguation.endIndex)]
+            
+            let replacementRange = SourceLocation(line: 0, column: validPrefix.count, source: nil)..<SourceLocation(line: 0, column: originalReference.count-unprocessedSuffix.count, source: nil)
+            
+            let solutions = collisions.sorted(by: {
+                $0.node.fullNameOfValue(context: context) + $0.disambiguation
+                    < $1.node.fullNameOfValue(context: context) + $1.disambiguation
+            }).map { collision in
+                Solution(summary: "Insert disambiguation suffix for \(collision.node.fullNameOfValue(context: context).singleQuoted)", replacements: [
+                    Replacement(range: replacementRange, replacement: "-" + collision.disambiguation)
+                ])
+            }
+            
+            return TopicReferenceResolutionError("Reference is ambiguous after \(partialResult.pathWithoutDisambiguation().singleQuoted).", solutions: solutions)
+        }
+    }
+    
+    private func availabilityNote(category: String, candidates: [String]) -> String {
+        switch candidates.count {
+        case 0:
+            return "No \(category) available."
+        default:
+            return "Available \(category): \(candidates.joined(separator: ", "))."
         }
     }
 }

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

@@ -207,7 +207,7 @@ final class PathHierarchyBasedLinkResolver {
                 // Return the successful or failed externally resolved reference.
                 return resolvedExternalReference
             } else if !context.registeredBundles.contains(where: { $0.identifier == bundleID }) {
-                return .failure(unresolvedReference, errorMessage: "No external resolver registered for \(bundleID.singleQuoted).")
+                return .failure(unresolvedReference, TopicReferenceResolutionError("No external resolver registered for \(bundleID.singleQuoted)."))
             }
         }
 
@@ -222,7 +222,12 @@ final class PathHierarchyBasedLinkResolver {
             if let resolvedFallbackReference = fallbackResolver.resolve(unresolvedReference, in: parent, fromSymbolLink: isCurrentlyResolvingSymbolLink, context: context) {
                 return .success(resolvedFallbackReference)
             } else {
-                return .failure(unresolvedReference, errorMessage: error.errorMessage(context: context))
+                var originalReferenceString = unresolvedReference.path
+                if let fragment = unresolvedReference.topicURL.components.fragment {
+                    originalReferenceString += "#" + fragment
+                }
+                
+                return .failure(unresolvedReference, error.asTopicReferenceResolutionError(context: context, originalReference: originalReferenceString))
             }
         } catch {
             fatalError("Only SymbolPathTree.Error errors are raised from the symbol link resolution code above.")