Refine logic for when curation removes a symbol from automatic curati…

…on (#1075)

* Refine logic for when curation removes a symbol from automatic curation

rdar://133338975

* Update curation in MixedManualAutomaticCuration to follow new logic

* Update tests to expect automatic curation of symbols that are manually curated outside their canonical container

* Fix bug where automatic overload topic groups could include unexpected symbols

* Stop using TopicGraph to find the overloads in a group

* Documentation that organizing member symbols outside their canonical container doesn't remove them from the default topic group.

* Add tip about dividing large topics into sub-topics

Also, remove member symbol from top-level symbol curation example

* Minor grammar correction in code comment

Co-authored-by: Andrea Fernandez Buitrago <[email protected]>

* Address code review feedback:

- Update reference to topic graph node property in code comment

* Address code review feedback:

- Rephrase public documentation about curating a symbol outside its canonical container

---------

Co-authored-by: Andrea Fernandez Buitrago <[email protected]>

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

@@ -2409,7 +2409,7 @@ public class DocumentationContext {
     ///
     /// - Parameter automaticallyCurated: A list of automatic curation records.
     func removeUnneededAutomaticCuration(_ automaticallyCurated: [AutoCuratedSymbolRecord]) {
-        // It might look like it would be correct to check `topicGraph.nodes[symbol]?.isManuallyCurated` here,
+        // It might look like it would be correct to check `topicGraph.nodes[symbol]?.shouldAutoCurateInCanonicalLocation` here,
         // but that would incorrectly remove the only parent if the manual curation and the automatic curation was the same.
         //
         // Similarly, it might look like it would be correct to only check `parents(of: symbol).count > 1` here,
@@ -2470,7 +2470,7 @@ public class DocumentationContext {
         linkResolver.localResolver.traverseSymbolAndParents { reference, parentReference, counterpartParentReference in
             guard let topicGraphNode = topicGraph.nodeWithReference(reference),
                   // Check that the node isn't already manually curated
-                  !topicGraphNode.isManuallyCurated
+                  topicGraphNode.shouldAutoCurateInCanonicalLocation
             else { return }
 
             // Check that the symbol doesn't already have parent's that aren't either language representation's hierarchical parent.
@@ -2595,9 +2595,99 @@ public class DocumentationContext {
         for reference in references {
             try crawler.crawlChildren(
                 of: reference,
-                relateNodes: {
-                    self.topicGraph.unsafelyAddEdge(source: $0, target: $1)
-                    self.topicGraph.nodes[$1]?.isManuallyCurated = true
+                relateNodes: { container, descendant in
+                    topicGraph.unsafelyAddEdge(source: container, target: descendant)
+
+                    guard topicGraph.nodes[descendant]?.shouldAutoCurateInCanonicalLocation == true else {
+                        // Descendant is already marked to be removed from automatic curation.
+                        return
+                    }
+
+                    // An inner function called below
+                    func stopAutoCuratingDescendant() {
+                        topicGraph.nodes[descendant]?.shouldAutoCurateInCanonicalLocation = false
+                    }
+
+                    guard let (canonicalContainer, counterpartContainer) = linkResolver.localResolver.nearestContainers(ofSymbol: descendant) else {
+                        // Any curation of a non-symbol removes it from automatic curation
+                        stopAutoCuratingDescendant()
+                        return
+                    }
+
+                    // For symbols we only stop automatic curation if they are curated within their canonical container's sub-hierarchy
+                    // or if a top-level symbol is curated under another top-level symbol (more on that below).
+                    //
+                    // For example, curating a member under an API collection within the container removes the member from automatic curation:
+                    //  ┆
+                    //  ├─SomeClass
+                    //  │ └─API Collection
+                    //  │   └─SomeClass/someMethod()  ◀︎━━ won't auto curate
+                    //
+                    // However, curating a member outside under another container _doesn't_ remove it from automatic curation:
+                    //  ┆
+                    //  ├─Other Container
+                    //  │ └─SomeClass/someMethod()  ◀︎━━  will still auto curate under `SomeClass`
+                    //  ├─SomeClass
+                    //
+                    // The same applies if the authored curation location is a another member of the canonical container:
+                    //  ┆
+                    //  ├─SomeClass
+                    //  │ └─SomeClass/SomeInnerClass
+                    //  │   └─SomeClass/someMethod()  ◀︎━━ will still auto curate under `SomeClass`
+                    //
+                    // Top-level symbols curated under other top-level is an exception to this rule.
+                    //  ┆
+                    //  ├─SomeClass
+                    //  │ └─OtherTopLevelClass  ◀︎━━ won't auto curate because it's top-level.
+                    //
+                    // The reason for this exception is to allow developers to group top-level types under one-another without requiring an API collection.
+                    // For example, in DocC one could curate `DiagnosticConsumer`, `DiagnosticFormattingOptions`, and `Diagnostic` under `DiagnosticEngine`,
+                    // treating the `DiagnosticEngine` as the top-level topic for all diagnostic related types.
+
+
+                    // To determine if `container` exists in the curated symbol's canonical container's sub-hierarchy,
+                    // first find its nearest container symbol (in case `container` is a series of API collections).
+                    //
+                    // If the `container` is a symbol, this returns the container.
+                    guard let nearestSymbolContainer = topicGraph.reverseEdgesGraph
+                        .breadthFirstSearch(from: container)
+                        .first(where: { topicGraph.nodes[$0]?.kind.isSymbol == true })
+                    else {
+                        // The container doesn't exist in the same module as the curated symbol.
+                        // Continue to automatically curate the descendant under its canonical container.
+                        return
+                    }
+
+                    if nearestSymbolContainer == canonicalContainer || nearestSymbolContainer == counterpartContainer {
+                        // The descendant is curated in its canonical container (in either language representation)
+                        stopAutoCuratingDescendant()
+                        return
+                    }
+
+                    // An inner function called below
+                    func isModule(_ reference: ResolvedTopicReference) -> Bool {
+                        topicGraph.nodes[reference]?.kind == .module
+                    }
+
+                    if isModule(canonicalContainer) || counterpartContainer.map(isModule) == true {
+                        guard let curationLocationContainers = linkResolver.localResolver.nearestContainers(ofSymbol: nearestSymbolContainer) else {
+                            assertionFailure("""
+                                Unexpectedly didn't find any canonical containers for symbol \(nearestSymbolContainer.absoluteString.singleQuoted).
+                                Every non-module symbol should have a canonical container.
+                                """)
+                            return
+                        }
+
+                        if canonicalContainer   == curationLocationContainers.main        ||
+                           canonicalContainer   == curationLocationContainers.counterpart ||
+                           counterpartContainer == curationLocationContainers.main        ||
+                           counterpartContainer == curationLocationContainers.counterpart && counterpartContainer != nil
+                        {
+                            // The descendant is a top-level symbol, curated under another top-level symbol in the same module
+                            stopAutoCuratingDescendant()
+                            return
+                        }
+                    }
                 }
             )
         }

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

@@ -13,7 +13,6 @@ import SymbolKit
 
 extension PathHierarchyBasedLinkResolver {
 
-    /// 
     /// Finds the canonical path, also called "breadcrumbs", to the given symbol in the path hierarchy.
     /// The path is a list of references that describe a walk through the path hierarchy descending from the module down to, but not including, the given `reference`.
     ///
@@ -59,4 +58,23 @@ extension PathHierarchyBasedLinkResolver {
                 $0.identifier.flatMap { resolvedReferenceMap[$0] }
             }
     }
+
+    /// Returns the nearest canonical containers for the different language representations of a given symbol.
+    /// - Parameter reference: The symbol reference to find the canonical containers for.
+    /// - Returns: The  canonical containers for the different language representations of a given symbol, or `nil` if the reference is a module or a non-symbol.
+    func nearestContainers(ofSymbol reference: ResolvedTopicReference) -> (main: ResolvedTopicReference, counterpart: ResolvedTopicReference?)? {
+        guard let nodeID = resolvedReferenceMap[reference] else { return nil }
+
+        let node = pathHierarchy.lookup[nodeID]! // Only the path hierarchy can create its IDs and a created ID always matches a node
+        guard node.symbol != nil else { return nil }
+
+        func containerReference(_ node: PathHierarchy.Node) -> ResolvedTopicReference? {
+            guard let containerID = node.parent?.identifier else { return nil }
+            return resolvedReferenceMap[containerID]
+        }
+
+        guard let main = containerReference(node) else { return nil }
+
+        return (main, node.counterpart.flatMap(containerReference))
+    }
 }

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

@@ -0,0 +1,52 @@
+/*
+ This source file is part of the Swift.org open source project
+
+ Copyright (c) 2024 Apple Inc. and the Swift project authors
+ Licensed under Apache License v2.0 with Runtime Library Exception
+
+ See https://swift.org/LICENSE.txt for license information
+ See https://swift.org/CONTRIBUTORS.txt for Swift project authors
+*/
+
+import Foundation
+import SymbolKit
+
+extension PathHierarchyBasedLinkResolver {
+
+    /// Returns the references for the overloaded symbols that belong to the given overload group.
+    /// - Parameter reference: The reference of an overload group.
+    /// - Returns: The references for overloaded symbols in the given group, or `nil` if the `reference` is not an overload group reference.
+    func overloads(ofGroup reference: ResolvedTopicReference) -> [ResolvedTopicReference]? {
+        guard let groupNodeID = resolvedReferenceMap[reference] else { return nil }
+        let groupNode = pathHierarchy.lookup[groupNodeID]! // Only the path hierarchy can create its IDs and a created ID always matches a node
+
+        guard let groupSymbol = groupNode.symbol, groupSymbol.isOverloadGroup else {
+            return nil
+        }
+        assert(groupNode.languages == [.swift], "Only Swift supports overload groups. The implementation makes assumptions based on this.")
+
+        let elementsWithSameName = groupNode.parent?.children[groupNode.name]?.storage ?? []
+
+        let groupSymbolKindID = groupSymbol.kind.identifier
+        return elementsWithSameName.compactMap {
+            let id = $0.node.identifier
+            guard id != groupNodeID, // Skip the overload group itself
+                  $0.node.symbol?.kind.identifier == groupSymbolKindID // Only symbols of the same kind as the group are overloads
+            else {
+                return nil
+            }
+
+            assert(
+                // The PathHierarchy doesn't track overloads (and I don't think it should) but we can check that the filtered elements
+                // have the behaviors that's expected of overloaded symbols as a proxy to verify that no unexpected values are returned.
+                $0.node.specialBehaviors == [.disfavorInLinkCollision, .excludeFromAutomaticCuration],
+                """
+                Node behaviors \($0.node.specialBehaviors) for \($0.node.symbol?.identifier.precise ?? "<non-symbol>") doesn't match an \
+                overloaded symbol's behaviors (\(PathHierarchy.Node.SpecialBehaviors(arrayLiteral: [.disfavorInLinkCollision, .excludeFromAutomaticCuration])))
+                """
+            )
+
+            return resolvedReferenceMap[$0.node.identifier]
+        }
+    }
+}

Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphRelationshipsBuilder.swift

@@ -476,7 +476,6 @@ struct SymbolGraphRelationshipsBuilder {
         else {
             return
         }
-        overloadGroupTopicGraphNode.isOverloadGroup = true
         context.topicGraph.addEdge(from: overloadGroupTopicGraphNode, to: overloadTopicGraphNode)
     }
 }

Sources/SwiftDocC/Infrastructure/Topic Graph/AutomaticCuration.swift

@@ -90,7 +90,7 @@ public struct AutomaticCuration {
             .reduce(into: AutomaticCuration.groups) { groupsIndex, reference in
                 guard let topicNode = context.topicGraph.nodeWithReference(reference),
                       !topicNode.isEmptyExtension,
-                      !topicNode.isManuallyCurated
+                      topicNode.shouldAutoCurateInCanonicalLocation
                 else {
                     return
                 }
@@ -105,7 +105,7 @@ public struct AutomaticCuration {
                 // If this symbol is an overload group and all its overloaded children were manually
                 // curated elsewhere, skip it so it doesn't clutter the curation hierarchy with a
                 // duplicate symbol.
-                if let overloads = context.topicGraph.overloads(of: reference), overloads.isEmpty {
+                if let overloads = context.linkResolver.localResolver.overloads(ofGroup: reference), overloads.isEmpty {
                     return
                 }
 

Sources/SwiftDocC/Infrastructure/Topic Graph/TopicGraph.swift

@@ -87,21 +87,18 @@ struct TopicGraph {
         /// If true, the topic has been removed from the hierarchy due to being an extension whose children have been curated elsewhere.
         let isEmptyExtension: Bool
 
-        /// If true, the topic has been manually organized into a topic section on some other page.
-        var isManuallyCurated: Bool = false
+        /// If true, the topic should automatically organize into a topic section in its canonical container page's hierarchy for each language representation.
+        var shouldAutoCurateInCanonicalLocation: Bool = true
 
-        /// If true, this topic is a generated "overload group" symbol page.
-        var isOverloadGroup: Bool = false
-
-        init(reference: ResolvedTopicReference, kind: DocumentationNode.Kind, source: ContentLocation, title: String, isResolvable: Bool = true, isVirtual: Bool = false, isEmptyExtension: Bool = false, isManuallyCurated: Bool = false) {
+        init(reference: ResolvedTopicReference, kind: DocumentationNode.Kind, source: ContentLocation, title: String, isResolvable: Bool = true, isVirtual: Bool = false, isEmptyExtension: Bool = false, shouldAutoCurateInCanonicalLocation: Bool = true) {
             self.reference = reference
             self.kind = kind
             self.source = source
             self.title = title
             self.isResolvable = isResolvable
             self.isVirtual = isVirtual
             self.isEmptyExtension = isEmptyExtension
-            self.isManuallyCurated = isManuallyCurated
+            self.shouldAutoCurateInCanonicalLocation = shouldAutoCurateInCanonicalLocation
         }
 
         func withReference(_ reference: ResolvedTopicReference) -> Node {
@@ -301,16 +298,6 @@ struct TopicGraph {
         DirectedGraph(edges: reverseEdges)
     }
 
-    /// Returns the children of this node that reference it as their overload group.
-    func overloads(of groupReference: ResolvedTopicReference) -> [ResolvedTopicReference]? {
-        guard nodes[groupReference]?.isOverloadGroup == true else {
-            return nil
-        }
-        return edges[groupReference, default: []].filter({ childReference in
-            nodes[childReference]?.isManuallyCurated == false
-        })
-    }
-
     /// Returns true if a node exists with the given reference and it's set as linkable.
     func isLinkable(_ reference: ResolvedTopicReference) -> Bool {
         // Sections (represented by the node path + fragment with the section name)

Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift

@@ -336,8 +336,8 @@ public extension DocumentationNode {
             default:
                 var topicSectionGroups: [LinkDestinationSummary.TaskGroup] = renderNode.topicSections.map { group in .init(title: group.title, identifiers: group.identifiers) }
 
-                if let overloadChildren = context.topicGraph.overloads(of: self.reference), !overloadChildren.isEmpty {
-                    topicSectionGroups.append(.init(title: "Overloads", identifiers: overloadChildren.map(\.absoluteString)))
+                if let overloads = context.linkResolver.localResolver.overloads(ofGroup: reference) {
+                    topicSectionGroups.append(.init(title: "Overloads", identifiers: overloads.map(\.absoluteString)))
                 }
 
                 taskGroups = topicSectionGroups

Sources/docc/DocCDocumentation.docc/adding-structure-to-your-documentation-pages.md

@@ -118,9 +118,13 @@ backticks (\`\`) .
 - ``Activity``
 - ``CareSchedule``
 - ``FoodGenerator``
-- ``Sloth/Food``
 ```
 
+> Tip: 
+> If you feel that a top-level topic group contains too many links, 
+> it could be an indication that that you can further organize the links into subtopics.
+> For more information, see the <doc:#Incorporate-Hierarchy-in-Your-Navigation> section below.
+
 DocC uses the double backtick format to create symbol links, and to add the 
 symbol's type information and summary. For more information, see 
 <doc:formatting-your-documentation-content>.
@@ -206,6 +210,11 @@ documentation hierarchy.
 
 After you arrange nested symbols in an extension file, use DocC to compile your changes and review them in your browser.
 
+> Note: 
+> If you organize a member symbol into a topic group outside of the type that defines the member, 
+> DocC will still include the member symbol in the default topic group.
+> This ensures that the reader can always find the member symbol somewhere in the sub-hierarchy of the containing type.  
+
 ### Incorporate Hierarchy in Your Navigation
 
 Much like you organize symbols on a landing page or in an extension file, you 
@@ -241,4 +250,4 @@ they can also confuse a reader if you create too many levels of hierarchy.
 Avoid using a collection when a topic group at a higher level can achieve the 
 same result.
 
-<!-- Copyright (c) 2021-2023 Apple Inc and the Swift Project authors. All Rights Reserved. -->
+<!-- Copyright (c) 2021-2024 Apple Inc and the Swift Project authors. All Rights Reserved. -->

Tests/SwiftDocCTests/Catalog Processing/GeneratedCurationWriterTests.swift

@@ -269,6 +269,7 @@ class GeneratedCurationWriterTests: XCTestCase {
         ### Basics
 
         - ``age``
+        - <doc:TopClass-API-Collection>
 
         <!-- Copyright (c) 2021 Apple Inc and the Swift Project authors. All Rights Reserved. -->
         

Tests/SwiftDocCTests/Indexing/NavigatorIndexTests.swift

@@ -617,11 +617,15 @@ Root
             ┃ ┣╸first()
             ┃ ┃ ┣╸Manual curation
             ┃ ┃ ┗╸second()
-            ┃ ┗╸OtherSymbol
+            ┃ ┣╸OtherSymbol
+            ┃ ┃ ┣╸Manual curation
+            ┃ ┃ ┗╸second()
+            ┃ ┃   ┣╸Manual curation
+            ┃ ┃   ┗╸first()
+            ┃ ┣╸Instance Methods
+            ┃ ┗╸second()
             ┃   ┣╸Manual curation
-            ┃   ┗╸second()
-            ┃     ┣╸Manual curation
-            ┃     ┗╸first()
+            ┃   ┗╸first()
             ┗╸OtherSymbol
               ┣╸Manual curation
               ┗╸second()

Tests/SwiftDocCTests/Indexing/RenderIndexTests.swift

@@ -241,6 +241,15 @@ final class RenderIndexTests: XCTestCase {
                           }
                         ]
                       },
+                      {
+                        "title": "Type Aliases",
+                        "type": "groupMarker"
+                      },
+                      {
+                        "path": "/documentation/mixedlanguageframework/foo-c.typealias",
+                        "title": "Foo",
+                        "type": "typealias"
+                      },
                       {
                         "title": "Enumerations",
                         "type": "groupMarker"