Skip to content

Commit 50c9904

Browse files
d-ronnqvistanferbui
d-ronnqvist
and
anferbui
authored
Refine logic for when curation removes a symbol from automatic curation (#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]>
1 parent be887df commit 50c9904

24 files changed

+693
-96
lines changed

‎Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

Lines changed: 95 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2409,7 +2409,7 @@ public class DocumentationContext {
24092409
///
24102410
/// - Parameter automaticallyCurated: A list of automatic curation records.
24112411
func removeUnneededAutomaticCuration(_ automaticallyCurated: [AutoCuratedSymbolRecord]) {
2412-
// It might look like it would be correct to check `topicGraph.nodes[symbol]?.isManuallyCurated` here,
2412+
// It might look like it would be correct to check `topicGraph.nodes[symbol]?.shouldAutoCurateInCanonicalLocation` here,
24132413
// but that would incorrectly remove the only parent if the manual curation and the automatic curation was the same.
24142414
//
24152415
// 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 {
24702470
linkResolver.localResolver.traverseSymbolAndParents { reference, parentReference, counterpartParentReference in
24712471
guard let topicGraphNode = topicGraph.nodeWithReference(reference),
24722472
// Check that the node isn't already manually curated
2473-
!topicGraphNode.isManuallyCurated
2473+
topicGraphNode.shouldAutoCurateInCanonicalLocation
24742474
else { return }
24752475

24762476
// 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 {
25952595
for reference in references {
25962596
try crawler.crawlChildren(
25972597
of: reference,
2598-
relateNodes: {
2599-
self.topicGraph.unsafelyAddEdge(source: $0, target: $1)
2600-
self.topicGraph.nodes[$1]?.isManuallyCurated = true
2598+
relateNodes: { container, descendant in
2599+
topicGraph.unsafelyAddEdge(source: container, target: descendant)
2600+
2601+
guard topicGraph.nodes[descendant]?.shouldAutoCurateInCanonicalLocation == true else {
2602+
// Descendant is already marked to be removed from automatic curation.
2603+
return
2604+
}
2605+
2606+
// An inner function called below
2607+
func stopAutoCuratingDescendant() {
2608+
topicGraph.nodes[descendant]?.shouldAutoCurateInCanonicalLocation = false
2609+
}
2610+
2611+
guard let (canonicalContainer, counterpartContainer) = linkResolver.localResolver.nearestContainers(ofSymbol: descendant) else {
2612+
// Any curation of a non-symbol removes it from automatic curation
2613+
stopAutoCuratingDescendant()
2614+
return
2615+
}
2616+
2617+
// For symbols we only stop automatic curation if they are curated within their canonical container's sub-hierarchy
2618+
// or if a top-level symbol is curated under another top-level symbol (more on that below).
2619+
//
2620+
// For example, curating a member under an API collection within the container removes the member from automatic curation:
2621+
// ┆
2622+
// ├─SomeClass
2623+
// │ └─API Collection
2624+
// │ └─SomeClass/someMethod() ◀︎━━ won't auto curate
2625+
//
2626+
// However, curating a member outside under another container _doesn't_ remove it from automatic curation:
2627+
// ┆
2628+
// ├─Other Container
2629+
// │ └─SomeClass/someMethod() ◀︎━━ will still auto curate under `SomeClass`
2630+
// ├─SomeClass
2631+
//
2632+
// The same applies if the authored curation location is a another member of the canonical container:
2633+
// ┆
2634+
// ├─SomeClass
2635+
// │ └─SomeClass/SomeInnerClass
2636+
// │ └─SomeClass/someMethod() ◀︎━━ will still auto curate under `SomeClass`
2637+
//
2638+
// Top-level symbols curated under other top-level is an exception to this rule.
2639+
// ┆
2640+
// ├─SomeClass
2641+
// │ └─OtherTopLevelClass ◀︎━━ won't auto curate because it's top-level.
2642+
//
2643+
// The reason for this exception is to allow developers to group top-level types under one-another without requiring an API collection.
2644+
// For example, in DocC one could curate `DiagnosticConsumer`, `DiagnosticFormattingOptions`, and `Diagnostic` under `DiagnosticEngine`,
2645+
// treating the `DiagnosticEngine` as the top-level topic for all diagnostic related types.
2646+
2647+
2648+
// To determine if `container` exists in the curated symbol's canonical container's sub-hierarchy,
2649+
// first find its nearest container symbol (in case `container` is a series of API collections).
2650+
//
2651+
// If the `container` is a symbol, this returns the container.
2652+
guard let nearestSymbolContainer = topicGraph.reverseEdgesGraph
2653+
.breadthFirstSearch(from: container)
2654+
.first(where: { topicGraph.nodes[$0]?.kind.isSymbol == true })
2655+
else {
2656+
// The container doesn't exist in the same module as the curated symbol.
2657+
// Continue to automatically curate the descendant under its canonical container.
2658+
return
2659+
}
2660+
2661+
if nearestSymbolContainer == canonicalContainer || nearestSymbolContainer == counterpartContainer {
2662+
// The descendant is curated in its canonical container (in either language representation)
2663+
stopAutoCuratingDescendant()
2664+
return
2665+
}
2666+
2667+
// An inner function called below
2668+
func isModule(_ reference: ResolvedTopicReference) -> Bool {
2669+
topicGraph.nodes[reference]?.kind == .module
2670+
}
2671+
2672+
if isModule(canonicalContainer) || counterpartContainer.map(isModule) == true {
2673+
guard let curationLocationContainers = linkResolver.localResolver.nearestContainers(ofSymbol: nearestSymbolContainer) else {
2674+
assertionFailure("""
2675+
Unexpectedly didn't find any canonical containers for symbol \(nearestSymbolContainer.absoluteString.singleQuoted).
2676+
Every non-module symbol should have a canonical container.
2677+
""")
2678+
return
2679+
}
2680+
2681+
if canonicalContainer == curationLocationContainers.main ||
2682+
canonicalContainer == curationLocationContainers.counterpart ||
2683+
counterpartContainer == curationLocationContainers.main ||
2684+
counterpartContainer == curationLocationContainers.counterpart && counterpartContainer != nil
2685+
{
2686+
// The descendant is a top-level symbol, curated under another top-level symbol in the same module
2687+
stopAutoCuratingDescendant()
2688+
return
2689+
}
2690+
}
26012691
}
26022692
)
26032693
}

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

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import SymbolKit
1313

1414
extension PathHierarchyBasedLinkResolver {
1515

16-
///
1716
/// Finds the canonical path, also called "breadcrumbs", to the given symbol in the path hierarchy.
1817
/// 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`.
1918
///
@@ -59,4 +58,23 @@ extension PathHierarchyBasedLinkResolver {
5958
$0.identifier.flatMap { resolvedReferenceMap[$0] }
6059
}
6160
}
61+
62+
/// Returns the nearest canonical containers for the different language representations of a given symbol.
63+
/// - Parameter reference: The symbol reference to find the canonical containers for.
64+
/// - 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.
65+
func nearestContainers(ofSymbol reference: ResolvedTopicReference) -> (main: ResolvedTopicReference, counterpart: ResolvedTopicReference?)? {
66+
guard let nodeID = resolvedReferenceMap[reference] else { return nil }
67+
68+
let node = pathHierarchy.lookup[nodeID]! // Only the path hierarchy can create its IDs and a created ID always matches a node
69+
guard node.symbol != nil else { return nil }
70+
71+
func containerReference(_ node: PathHierarchy.Node) -> ResolvedTopicReference? {
72+
guard let containerID = node.parent?.identifier else { return nil }
73+
return resolvedReferenceMap[containerID]
74+
}
75+
76+
guard let main = containerReference(node) else { return nil }
77+
78+
return (main, node.counterpart.flatMap(containerReference))
79+
}
6280
}

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
/*
2+
This source file is part of the Swift.org open source project
3+
4+
Copyright (c) 2024 Apple Inc. and the Swift project authors
5+
Licensed under Apache License v2.0 with Runtime Library Exception
6+
7+
See https://swift.org/LICENSE.txt for license information
8+
See https://swift.org/CONTRIBUTORS.txt for Swift project authors
9+
*/
10+
11+
import Foundation
12+
import SymbolKit
13+
14+
extension PathHierarchyBasedLinkResolver {
15+
16+
/// Returns the references for the overloaded symbols that belong to the given overload group.
17+
/// - Parameter reference: The reference of an overload group.
18+
/// - Returns: The references for overloaded symbols in the given group, or `nil` if the `reference` is not an overload group reference.
19+
func overloads(ofGroup reference: ResolvedTopicReference) -> [ResolvedTopicReference]? {
20+
guard let groupNodeID = resolvedReferenceMap[reference] else { return nil }
21+
let groupNode = pathHierarchy.lookup[groupNodeID]! // Only the path hierarchy can create its IDs and a created ID always matches a node
22+
23+
guard let groupSymbol = groupNode.symbol, groupSymbol.isOverloadGroup else {
24+
return nil
25+
}
26+
assert(groupNode.languages == [.swift], "Only Swift supports overload groups. The implementation makes assumptions based on this.")
27+
28+
let elementsWithSameName = groupNode.parent?.children[groupNode.name]?.storage ?? []
29+
30+
let groupSymbolKindID = groupSymbol.kind.identifier
31+
return elementsWithSameName.compactMap {
32+
let id = $0.node.identifier
33+
guard id != groupNodeID, // Skip the overload group itself
34+
$0.node.symbol?.kind.identifier == groupSymbolKindID // Only symbols of the same kind as the group are overloads
35+
else {
36+
return nil
37+
}
38+
39+
assert(
40+
// The PathHierarchy doesn't track overloads (and I don't think it should) but we can check that the filtered elements
41+
// have the behaviors that's expected of overloaded symbols as a proxy to verify that no unexpected values are returned.
42+
$0.node.specialBehaviors == [.disfavorInLinkCollision, .excludeFromAutomaticCuration],
43+
"""
44+
Node behaviors \($0.node.specialBehaviors) for \($0.node.symbol?.identifier.precise ?? "<non-symbol>") doesn't match an \
45+
overloaded symbol's behaviors (\(PathHierarchy.Node.SpecialBehaviors(arrayLiteral: [.disfavorInLinkCollision, .excludeFromAutomaticCuration])))
46+
"""
47+
)
48+
49+
return resolvedReferenceMap[$0.node.identifier]
50+
}
51+
}
52+
}

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,6 @@ struct SymbolGraphRelationshipsBuilder {
476476
else {
477477
return
478478
}
479-
overloadGroupTopicGraphNode.isOverloadGroup = true
480479
context.topicGraph.addEdge(from: overloadGroupTopicGraphNode, to: overloadTopicGraphNode)
481480
}
482481
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ public struct AutomaticCuration {
9090
.reduce(into: AutomaticCuration.groups) { groupsIndex, reference in
9191
guard let topicNode = context.topicGraph.nodeWithReference(reference),
9292
!topicNode.isEmptyExtension,
93-
!topicNode.isManuallyCurated
93+
topicNode.shouldAutoCurateInCanonicalLocation
9494
else {
9595
return
9696
}
@@ -105,7 +105,7 @@ public struct AutomaticCuration {
105105
// If this symbol is an overload group and all its overloaded children were manually
106106
// curated elsewhere, skip it so it doesn't clutter the curation hierarchy with a
107107
// duplicate symbol.
108-
if let overloads = context.topicGraph.overloads(of: reference), overloads.isEmpty {
108+
if let overloads = context.linkResolver.localResolver.overloads(ofGroup: reference), overloads.isEmpty {
109109
return
110110
}
111111

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

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -87,21 +87,18 @@ struct TopicGraph {
8787
/// If true, the topic has been removed from the hierarchy due to being an extension whose children have been curated elsewhere.
8888
let isEmptyExtension: Bool
8989

90-
/// If true, the topic has been manually organized into a topic section on some other page.
91-
var isManuallyCurated: Bool = false
90+
/// If true, the topic should automatically organize into a topic section in its canonical container page's hierarchy for each language representation.
91+
var shouldAutoCurateInCanonicalLocation: Bool = true
9292

93-
/// If true, this topic is a generated "overload group" symbol page.
94-
var isOverloadGroup: Bool = false
95-
96-
init(reference: ResolvedTopicReference, kind: DocumentationNode.Kind, source: ContentLocation, title: String, isResolvable: Bool = true, isVirtual: Bool = false, isEmptyExtension: Bool = false, isManuallyCurated: Bool = false) {
93+
init(reference: ResolvedTopicReference, kind: DocumentationNode.Kind, source: ContentLocation, title: String, isResolvable: Bool = true, isVirtual: Bool = false, isEmptyExtension: Bool = false, shouldAutoCurateInCanonicalLocation: Bool = true) {
9794
self.reference = reference
9895
self.kind = kind
9996
self.source = source
10097
self.title = title
10198
self.isResolvable = isResolvable
10299
self.isVirtual = isVirtual
103100
self.isEmptyExtension = isEmptyExtension
104-
self.isManuallyCurated = isManuallyCurated
101+
self.shouldAutoCurateInCanonicalLocation = shouldAutoCurateInCanonicalLocation
105102
}
106103

107104
func withReference(_ reference: ResolvedTopicReference) -> Node {
@@ -301,16 +298,6 @@ struct TopicGraph {
301298
DirectedGraph(edges: reverseEdges)
302299
}
303300

304-
/// Returns the children of this node that reference it as their overload group.
305-
func overloads(of groupReference: ResolvedTopicReference) -> [ResolvedTopicReference]? {
306-
guard nodes[groupReference]?.isOverloadGroup == true else {
307-
return nil
308-
}
309-
return edges[groupReference, default: []].filter({ childReference in
310-
nodes[childReference]?.isManuallyCurated == false
311-
})
312-
}
313-
314301
/// Returns true if a node exists with the given reference and it's set as linkable.
315302
func isLinkable(_ reference: ResolvedTopicReference) -> Bool {
316303
// Sections (represented by the node path + fragment with the section name)

‎Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -336,8 +336,8 @@ public extension DocumentationNode {
336336
default:
337337
var topicSectionGroups: [LinkDestinationSummary.TaskGroup] = renderNode.topicSections.map { group in .init(title: group.title, identifiers: group.identifiers) }
338338

339-
if let overloadChildren = context.topicGraph.overloads(of: self.reference), !overloadChildren.isEmpty {
340-
topicSectionGroups.append(.init(title: "Overloads", identifiers: overloadChildren.map(\.absoluteString)))
339+
if let overloads = context.linkResolver.localResolver.overloads(ofGroup: reference) {
340+
topicSectionGroups.append(.init(title: "Overloads", identifiers: overloads.map(\.absoluteString)))
341341
}
342342

343343
taskGroups = topicSectionGroups

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

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,9 +118,13 @@ backticks (\`\`) .
118118
- ``Activity``
119119
- ``CareSchedule``
120120
- ``FoodGenerator``
121-
- ``Sloth/Food``
122121
```
123122

123+
> Tip:
124+
> If you feel that a top-level topic group contains too many links,
125+
> it could be an indication that that you can further organize the links into subtopics.
126+
> For more information, see the <doc:#Incorporate-Hierarchy-in-Your-Navigation> section below.
127+
124128
DocC uses the double backtick format to create symbol links, and to add the
125129
symbol's type information and summary. For more information, see
126130
<doc:formatting-your-documentation-content>.
@@ -206,6 +210,11 @@ documentation hierarchy.
206210

207211
After you arrange nested symbols in an extension file, use DocC to compile your changes and review them in your browser.
208212

213+
> Note:
214+
> If you organize a member symbol into a topic group outside of the type that defines the member,
215+
> DocC will still include the member symbol in the default topic group.
216+
> This ensures that the reader can always find the member symbol somewhere in the sub-hierarchy of the containing type.
217+
209218
### Incorporate Hierarchy in Your Navigation
210219

211220
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.
241250
Avoid using a collection when a topic group at a higher level can achieve the
242251
same result.
243252

244-
<!-- Copyright (c) 2021-2023 Apple Inc and the Swift Project authors. All Rights Reserved. -->
253+
<!-- Copyright (c) 2021-2024 Apple Inc and the Swift Project authors. All Rights Reserved. -->

‎Tests/SwiftDocCTests/Catalog Processing/GeneratedCurationWriterTests.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,7 @@ class GeneratedCurationWriterTests: XCTestCase {
269269
### Basics
270270
271271
- ``age``
272+
- <doc:TopClass-API-Collection>
272273
273274
<!-- Copyright (c) 2021 Apple Inc and the Swift Project authors. All Rights Reserved. -->
274275

‎Tests/SwiftDocCTests/Indexing/NavigatorIndexTests.swift

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -617,11 +617,15 @@ Root
617617
┃ ┣╸first()
618618
┃ ┃ ┣╸Manual curation
619619
┃ ┃ ┗╸second()
620-
┃ ┗╸OtherSymbol
620+
┃ ┣╸OtherSymbol
621+
┃ ┃ ┣╸Manual curation
622+
┃ ┃ ┗╸second()
623+
┃ ┃ ┣╸Manual curation
624+
┃ ┃ ┗╸first()
625+
┃ ┣╸Instance Methods
626+
┃ ┗╸second()
621627
┃ ┣╸Manual curation
622-
┃ ┗╸second()
623-
┃ ┣╸Manual curation
624-
┃ ┗╸first()
628+
┃ ┗╸first()
625629
┗╸OtherSymbol
626630
┣╸Manual curation
627631
┗╸second()

‎Tests/SwiftDocCTests/Indexing/RenderIndexTests.swift

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,15 @@ final class RenderIndexTests: XCTestCase {
241241
}
242242
]
243243
},
244+
{
245+
"title": "Type Aliases",
246+
"type": "groupMarker"
247+
},
248+
{
249+
"path": "/documentation/mixedlanguageframework/foo-c.typealias",
250+
"title": "Foo",
251+
"type": "typealias"
252+
},
244253
{
245254
"title": "Enumerations",
246255
"type": "groupMarker"

0 commit comments

Comments
 (0)