Improve the diagnostics about cyclic curation (#984)

* Improve diagnostics about disallowed cyclic curation

rdar://131477513

* Shorten cyclic diagnostic summary message
Add debug assert when task group link has no list item
Use consistent arrow thickness in cyclic diagnostic explanation
Fix grammar in internal code comment

Author: d-ronnqvist

Sources/SwiftDocC/Infrastructure/DocumentationCurator.swift

@@ -231,6 +231,22 @@ struct DocumentationCurator {
                     (childDocumentationNode.kind == .article || childDocumentationNode.kind.isSymbol || childDocumentationNode.kind == .tutorial || childDocumentationNode.kind == .tutorialArticle) else {
                         continue
                 }
+                
+                // A solution that suggests removing the list item that contain this link
+                var removeListItemSolutions: [Solution] {
+                    // Traverse the markup parents up to the nearest list item
+                    guard let listItem = sequence(first: link as (any Markup), next: \.parent).mapFirst(where: { $0 as? ListItem }),
+                          let listItemRange = listItem.range
+                    else {
+                        assertionFailure("Unable to find the list item element that contains \(link.format()) in the markup for \(describeForDiagnostic(nodeReference).singleQuoted)")
+                        return []
+                    }
+                    return [Solution(
+                        summary: "Remove \(listItem.format().trimmingCharacters(in: .whitespacesAndNewlines).singleQuoted)",
+                        replacements: [Replacement(range: listItemRange, replacement: "")]
+                    )]
+                }
+                let topicSectionBaseExplanation = "Links in a \"Topics section\" are used to organize documentation into a hierarchy"
 
                 // Allow curating a module node from a manual technology root.
                 if childDocumentationNode.kind == .module {
@@ -242,19 +258,51 @@ struct DocumentationCurator {
                     let hasTechnologyRoot = isTechnologyRoot(nodeReference) || context.reachableRoots(from: nodeReference).contains(where: isTechnologyRoot)
 
                     if !hasTechnologyRoot {
-                        problems.append(Problem(diagnostic: Diagnostic(source: source(), severity: .warning, range: range(), identifier: "org.swift.docc.ModuleCuration", summary: "Linking to \((link.destination ?? "").singleQuoted) from a Topics group in \(nodeReference.absoluteString.singleQuoted) isn't allowed", explanation: "The former is a module, and modules only exist at the root"), possibleSolutions: []))
+                        problems.append(Problem(
+                            diagnostic: Diagnostic(
+                                source: source(), severity: .warning, range: range(), identifier: "org.swift.docc.ModuleCuration",
+                                summary: "Organizing the module \(childReference.lastPathComponent.singleQuoted) under \(describeForDiagnostic(nodeReference).singleQuoted) isn't allowed",
+                                explanation: "\(topicSectionBaseExplanation). Modules should be roots in the documentation hierarchy."),
+                            possibleSolutions: removeListItemSolutions))
                         continue
                     }
                 }
 
                 // Verify we are not creating a graph cyclic relationship.
                 guard childReference != nodeReference else {
-                    problems.append(Problem(diagnostic: Diagnostic(source: source(), severity: .warning, range: range(), identifier: "org.swift.docc.CyclicReference", summary: "A symbol can't link to itself from within its Topics group in \(nodeReference.absoluteString.singleQuoted)"), possibleSolutions: []))
+                    problems.append(Problem(
+                        diagnostic: Diagnostic(
+                            source: source(), severity: .warning, range: range(), identifier: "org.swift.docc.CyclicReference",
+                            summary: "Organizing \(describeForDiagnostic(childReference).singleQuoted) under itself forms a cycle",
+                            explanation: "\(topicSectionBaseExplanation). The documentation hierarchy shouldn't contain cycles."),
+                        possibleSolutions: removeListItemSolutions
+                    ))
                     continue
                 }
 
                 guard !isReference(childReference, anAncestorOf: nodeReference) else {
-                    problems.append(Problem(diagnostic: Diagnostic(source: source(), severity: .warning, range: range(), identifier: "org.swift.docc.CyclicReference", summary: "Linking to \((link.destination ?? "").singleQuoted) from a Topics group in \(nodeReference.absoluteString.singleQuoted) isn't allowed", explanation: "The former is an ancestor of the latter"), possibleSolutions: []))
+                    // Adding this edge in the topic graph _would_ introduce a cycle.
+                    // In order to produce more actionable diagnostics, create a new graph that has this cycle.
+                    var edges = context.topicGraph.edges
+                    edges[nodeReference, default: []].append(childReference)
+                    let graph = DirectedGraph(edges: edges)
+                    
+                    let cycleDescriptions: [String] = graph.cycles(from: nodeReference).map { prettyPrint(cycle: $0) }
+                    
+                    problems.append(Problem(
+                        diagnostic: Diagnostic(
+                            source: source(), severity: .warning, range: range(), identifier: "org.swift.docc.CyclicReference",
+                            summary: """
+                            Organizing \(describeForDiagnostic(childReference).singleQuoted) under \(describeForDiagnostic(nodeReference).singleQuoted) \
+                            forms \(cycleDescriptions.count == 1 ? "a cycle" : "\(cycleDescriptions.count) cycles")
+                            """,
+                            explanation: """
+                            \(topicSectionBaseExplanation). The documentation hierarchy shouldn't contain cycles.
+                            If this link contributed to the documentation hierarchy it would introduce \(cycleDescriptions.count == 1 ? "this cycle" : "these \(cycleDescriptions.count) cycles"):
+                            \(cycleDescriptions.joined(separator: "\n"))
+                            """),
+                        possibleSolutions: removeListItemSolutions
+                    ))
                     continue
                 }
 
@@ -272,3 +320,18 @@ struct DocumentationCurator {
         }
     }
 }
+
+private func prettyPrint(cycle: [ResolvedTopicReference]) -> String {
+    let pathDescription = cycle.map { describeForDiagnostic($0, withoutModuleName: true)}.joined(separator: " ─▶︎ ")
+    return """
+    ╭─▶︎ \(pathDescription) ─╮
+    ╰\(String(repeating:"─", count: pathDescription.count + 3 /* "─▶︎ "*/ + 2 /* " ─"*/))╯
+    """
+}
+
+private func describeForDiagnostic(_ reference: ResolvedTopicReference, withoutModuleName: Bool = false) -> String {
+    // The references that the curator crawls wouldn't result in a warning if they weren't in the same module.
+    // To avoid repeating a common prefix. The first two path components are the leading slash and "documentation".
+    // The third path component is the module name.
+    reference.pathComponents.dropFirst(withoutModuleName ? 3 : 2).joined(separator: "/")
+}

Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift

@@ -13,6 +13,7 @@ import Foundation
 import XCTest
 import SymbolKit
 @testable import SwiftDocC
+import SwiftDocCTestUtilities
 import Markdown
 
 class DocumentationCuratorTests: XCTestCase {
@@ -129,8 +130,11 @@ class DocumentationCuratorTests: XCTestCase {
         )
         XCTAssertEqual(
             moduleCurationProblem?.diagnostic.summary,
-            "Linking to \'doc://org.swift.docc.example/documentation/MyKit\' from a Topics group in \'doc://org.swift.docc.example/documentation/MyKit/MyClass/myFunction()\' isn't allowed"
+            "Organizing the module 'MyKit' under 'MyKit/MyClass/myFunction()' isn't allowed"
         )
+        XCTAssertEqual(moduleCurationProblem?.diagnostic.explanation, """
+            Links in a "Topics section" are used to organize documentation into a hierarchy. Modules should be roots in the documentation hierarchy.
+            """)
 
         let cyclicReferenceProblem = myClassProblems.first(where: { $0.diagnostic.identifier == "org.swift.docc.CyclicReference" })
         XCTAssertNotNil(cyclicReferenceProblem)
@@ -141,8 +145,77 @@ class DocumentationCuratorTests: XCTestCase {
         )
         XCTAssertEqual(
             cyclicReferenceProblem?.diagnostic.summary,
-            "A symbol can't link to itself from within its Topics group in \'doc://org.swift.docc.example/documentation/MyKit/MyClass/myFunction()\'"
+            "Organizing 'MyKit/MyClass/myFunction()' under itself forms a cycle"
         )
+        XCTAssertEqual(cyclicReferenceProblem?.diagnostic.explanation, """
+            Links in a "Topics section" are used to organize documentation into a hierarchy. The documentation hierarchy shouldn't contain cycles.
+            """)
+    }
+    
+    func testCyclicCurationDiagnostic() throws {
+        let tempURL = try createTempFolder(content: [
+            Folder(name: "unit-test.docc", content: [
+                // A number of articles with this cyclic curation:
+                //
+                // Root──▶First──▶Second──▶Third─┐
+                //          ▲                    │
+                //          └────────────────────┘
+                TextFile(name: "Root.md", utf8Content: """
+                # Root
+                
+                @Metadata {
+                  @TechnologyRoot
+                }
+                
+                Curate the first article
+                
+                ## Topics
+                - <doc:First>
+                """),
+                
+                TextFile(name: "First.md", utf8Content: """
+                # First
+                
+                Curate the second article
+                
+                ## Topics
+                - <doc:Second>
+                """),
+                
+                TextFile(name: "Second.md", utf8Content: """
+                # Second
+                
+                Curate the third article
+                
+                ## Topics
+                - <doc:Third>
+                """),
+                
+                TextFile(name: "Third.md", utf8Content: """
+                # Third
+                
+                Form a cycle by curating the first article
+                ## Topics
+                - <doc:First>
+                """),
+            ])
+        ])
+        
+        let (_, _, context) = try loadBundle(from: tempURL)
+        XCTAssertEqual(context.problems.map(\.diagnostic.identifier), ["org.swift.docc.CyclicReference"])
+        let curationProblem = try XCTUnwrap(context.problems.first)
+        
+        XCTAssertEqual(curationProblem.diagnostic.source?.lastPathComponent, "Third.md")
+        XCTAssertEqual(curationProblem.diagnostic.summary, "Organizing 'unit-test/First' under 'unit-test/Third' forms a cycle")
+        
+        XCTAssertEqual(curationProblem.diagnostic.explanation, """
+            Links in a "Topics section" are used to organize documentation into a hierarchy. The documentation hierarchy shouldn't contain cycles.
+            If this link contributed to the documentation hierarchy it would introduce this cycle:
+            ╭─▶︎ Third ─▶︎ First ─▶︎ Second ─╮
+            ╰─────────────────────────────╯
+            """)
+        
+        XCTAssertEqual(curationProblem.possibleSolutions.map(\.summary), ["Remove '- <doc:First>'"])
     }
 
     func testModuleUnderTechnologyRoot() throws {