From ad776a212f951d11944f25dc1edcd8f4616ed88a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20R=C3=B6nnqvist?= Date: Thu, 24 Oct 2024 16:48:01 +0200 Subject: [PATCH] Refine logic for when curation removes a symbol from automatic curation rdar://133338975 --- .../Infrastructure/DocumentationContext.swift | 73 ++++++++- ...erarchyBasedLinkResolver+Breadcrumbs.swift | 64 ++++++++ .../Topic Graph/AutomaticCuration.swift | 2 +- .../Topic Graph/TopicGraph.swift | 8 +- .../AutomaticCurationTests.swift | 148 ++++++++++++++++++ .../SymbolBreadcrumbTests.swift | 96 ++++++++++++ 6 files changed, 381 insertions(+), 10 deletions(-) create mode 100644 Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchyBasedLinkResolver+Breadcrumbs.swift create mode 100644 Tests/SwiftDocCTests/Infrastructure/SymbolBreadcrumbTests.swift diff --git a/Sources/SwiftDocC/Infrastructure/DocumentationContext.swift b/Sources/SwiftDocC/Infrastructure/DocumentationContext.swift index 42f77f1714..8ea14f2f6a 100644 --- a/Sources/SwiftDocC/Infrastructure/DocumentationContext.swift +++ b/Sources/SwiftDocC/Infrastructure/DocumentationContext.swift @@ -2387,7 +2387,7 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate { /// /// - 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]?.isManuallyCuratedInContainer` 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, @@ -2448,7 +2448,7 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate { 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.isManuallyCuratedInContainer else { return } // Check that the symbol doesn't already have parent's that aren't either language representation's hierarchical parent. @@ -2574,9 +2574,72 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate { 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]?.isManuallyCuratedInContainer == false else { + // Descendant is already marked to be removed from automatic curation. + return + } + + guard let (canonicalContainer, counterpartContainer) = linkResolver.localResolver.nearestContainers(ofSymbol: descendant) else { + // Any curation of a non-symbol removes it from automatic curation + topicGraph.nodes[descendant]?.isManuallyCuratedInContainer = true + return + } + + // For symbols we only stop automatic curation if they are curated within their canonical container's sub-hierarchy. + // + // For example, curating a member under an API collection within the container does remove the member from automatic curation: + // ┆ + // ├─SomeClass + // │ └─API Collection + // │ └─SomeClass/someMethod() + // + // Similarly, curating a member under another type in the same container does remove the member from automatic curation: + // ┆ + // ├─SomeClass + // │ └─SomeClass/SomeInnerClass + // │ └─SomeClass/someMethod() + // + // However, curating a member outside under another container doesn't remove it from automatic curation: + // ┆ + // ├─Other Container + // │ └─SomeClass/someMethod() + // ├─SomeClass + + // To determine if `container` exist 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) + 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 + } + + // An inner function to be able to `return` once an answer has been found. + func isCuratedWithinCanonicalContainerSubHierarchy() -> Bool { + if nearestSymbolContainer == canonicalContainer || nearestSymbolContainer == counterpartContainer { + return true + } + + for language in nearestSymbolContainer.sourceLanguages { + guard let breadcrumbs = linkResolver.localResolver.breadcrumbs(of: nearestSymbolContainer, in: language), + breadcrumbs.contains(where: { $0 == canonicalContainer || $0 == counterpartContainer }) + else { continue } + + return true + } + + return false + } + + + if isCuratedWithinCanonicalContainerSubHierarchy() { + topicGraph.nodes[descendant]?.isManuallyCuratedInContainer = true + } } ) } diff --git a/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchyBasedLinkResolver+Breadcrumbs.swift b/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchyBasedLinkResolver+Breadcrumbs.swift new file mode 100644 index 0000000000..86e606b125 --- /dev/null +++ b/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchyBasedLinkResolver+Breadcrumbs.swift @@ -0,0 +1,64 @@ +/* + 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 { + func breadcrumbs(of reference: ResolvedTopicReference, in sourceLanguage: SourceLanguage) -> [ResolvedTopicReference]? { + guard let nodeID = resolvedReferenceMap[reference] else { return nil } + + var node = pathHierarchy.lookup[nodeID]! // Only the path hierarchy can create its IDs and a created ID always matches a node + + func matchesRequestedLanguage(_ node: PathHierarchy.Node) -> Bool { + guard let symbol = node.symbol, + let language = SourceLanguage(knownLanguageIdentifier: symbol.identifier.interfaceLanguage) + else { + return false + } + return language == sourceLanguage + } + + if !matchesRequestedLanguage(node) { + guard let counterpart = node.counterpart, matchesRequestedLanguage(counterpart) else { + // Neither this symbol, nor its counterpart matched the requested language + return nil + } + // Traverse from the counterpart instead because it matches the requested language + node = counterpart + } + + // Traverse up the hierarchy and gather each reference + return sequence(first: node, next: \.parent) + // The hierarchy traversal happened from the starting point up, but the callers of `breadcrumbs(of:in:)` + // expect paths going from the root page, excluding the starting point itself (already dropped above). + // To match the caller's expectations we remove the starting point and then flip the paths. + .dropFirst().reversed() + .compactMap { + $0.identifier.flatMap { resolvedReferenceMap[$0] } + } + } + + func nearestContainers(ofSymbol reference: ResolvedTopicReference) -> (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)) + } +} diff --git a/Sources/SwiftDocC/Infrastructure/Topic Graph/AutomaticCuration.swift b/Sources/SwiftDocC/Infrastructure/Topic Graph/AutomaticCuration.swift index b71727f5f4..fc68f56688 100644 --- a/Sources/SwiftDocC/Infrastructure/Topic Graph/AutomaticCuration.swift +++ b/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.isManuallyCuratedInContainer else { return } diff --git a/Sources/SwiftDocC/Infrastructure/Topic Graph/TopicGraph.swift b/Sources/SwiftDocC/Infrastructure/Topic Graph/TopicGraph.swift index 1efb09bf22..75639abeb2 100644 --- a/Sources/SwiftDocC/Infrastructure/Topic Graph/TopicGraph.swift +++ b/Sources/SwiftDocC/Infrastructure/Topic Graph/TopicGraph.swift @@ -87,8 +87,8 @@ 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 has been manually organized into a topic section in its canonical container page's hierarchy. + var isManuallyCuratedInContainer: Bool = false /// If true, this topic is a generated "overload group" symbol page. var isOverloadGroup: Bool = false @@ -101,7 +101,7 @@ struct TopicGraph { self.isResolvable = isResolvable self.isVirtual = isVirtual self.isEmptyExtension = isEmptyExtension - self.isManuallyCurated = isManuallyCurated + self.isManuallyCuratedInContainer = isManuallyCurated } func withReference(_ reference: ResolvedTopicReference) -> Node { @@ -307,7 +307,7 @@ struct TopicGraph { return nil } return edges[groupReference, default: []].filter({ childReference in - nodes[childReference]?.isManuallyCurated == false + nodes[childReference]?.isManuallyCuratedInContainer == false }) } diff --git a/Tests/SwiftDocCTests/Infrastructure/AutomaticCurationTests.swift b/Tests/SwiftDocCTests/Infrastructure/AutomaticCurationTests.swift index 5adc4864ef..53f336d640 100644 --- a/Tests/SwiftDocCTests/Infrastructure/AutomaticCurationTests.swift +++ b/Tests/SwiftDocCTests/Infrastructure/AutomaticCurationTests.swift @@ -832,6 +832,154 @@ class AutomaticCurationTests: XCTestCase { ]) } + func testCurationUnderModuleStopsAutomaticCuration() throws { + let catalog = Folder(name: "Something.docc", content: [ + JSONFile(name: "Something.symbols.json", content: makeSymbolGraph(moduleName: "Something", symbols: [ + makeSymbol(id: "first-symbol-id", kind: .class, pathComponents: ["FirstClass"]), + makeSymbol(id: "second-symbol-id", kind: .class, pathComponents: ["SecondClass"]), + ])), + + TextFile(name: "ModuleExtension.md", utf8Content: """ + # ``Something`` + + ## Topics + - ``FirstClass`` + """), + ]) + let (_, context) = try loadBundle(catalog: catalog) + XCTAssert(context.problems.isEmpty, "Unexpected problems: \(context.problems.map(\.diagnostic.summary))") + + let moduleReference = try XCTUnwrap(context.soleRootModuleReference) + + let firstNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("FirstClass")]) + XCTAssertTrue(firstNode.isManuallyCuratedInContainer, "This symbol is manually curated under its module") + let secondNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("SecondClass")]) + XCTAssertFalse(secondNode.isManuallyCuratedInContainer, "This symbol is never manually curated") + } + + func testCurationUnderAPICollectionStopsAutomaticCuration() throws { + let catalog = Folder(name: "Something.docc", content: [ + JSONFile(name: "Something.symbols.json", content: makeSymbolGraph(moduleName: "Something", symbols: [ + makeSymbol(id: "first-symbol-id", kind: .class, pathComponents: ["FirstClass"]), + makeSymbol(id: "second-symbol-id", kind: .class, pathComponents: ["SecondClass"]), + ])), + + TextFile(name: "API Collection.md", utf8Content: """ + # Some API collection + + ## Topics + - ``FirstClass`` + """), + + TextFile(name: "ModuleExtension.md", utf8Content: """ + # ``Something`` + + ## Topics + - + """), + ]) + let (_, context) = try loadBundle(catalog: catalog) + XCTAssert(context.problems.isEmpty, "Unexpected problems: \(context.problems.map(\.diagnostic.summary))") + + let moduleReference = try XCTUnwrap(context.soleRootModuleReference) + + let firstNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("FirstClass")]) + XCTAssertTrue(firstNode.isManuallyCuratedInContainer, "This symbol is manually curated under an API collection under its module") + let secondNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("SecondClass")]) + XCTAssertFalse(secondNode.isManuallyCuratedInContainer, "This symbol is never manually curated") + + let apiCollectionNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("API-Collection")]) + XCTAssertTrue(apiCollectionNode.isManuallyCuratedInContainer, "Any curation of non-symbols stops automatic curation") + } + + func testCurationUnderSiblingDoesStopAutomaticCuration() throws { + let catalog = Folder(name: "Something.docc", content: [ + JSONFile(name: "Something.symbols.json", content: makeSymbolGraph(moduleName: "Something", symbols: [ + makeSymbol(id: "first-symbol-id", kind: .class, pathComponents: ["FirstClass"]), + makeSymbol(id: "second-symbol-id", kind: .class, pathComponents: ["SecondClass"]), + ])), + + TextFile(name: "SymbolExtension.md", utf8Content: """ + # ``Something/FirstClass`` + + ## Topics + - ``SecondClass`` + """), + ]) + let (_, context) = try loadBundle(catalog: catalog) + XCTAssert(context.problems.isEmpty, "Unexpected problems: \(context.problems.map(\.diagnostic.summary))") + + let moduleReference = try XCTUnwrap(context.soleRootModuleReference) + + let firstNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("FirstClass")]) + XCTAssertFalse(firstNode.isManuallyCuratedInContainer, "This symbol is never manually curated") + let secondNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("SecondClass")]) + XCTAssertTrue(secondNode.isManuallyCuratedInContainer, "Sibling symbol exist in sub-hierarchy of canonical container") + } + + func testCurationOfMemberUnderModuleDoesNotStopAutomaticCuration() throws { + let catalog = Folder(name: "Something.docc", content: [ + JSONFile(name: "Something.symbols.json", content: makeSymbolGraph(moduleName: "Something", symbols: [ + makeSymbol(id: "first-symbol-id", kind: .class, pathComponents: ["FirstClass"]), + makeSymbol(id: "first-member-symbol-id", kind: .func, pathComponents: ["FirstClass", "firstMember"]), + ], relationships: [ + .init(source: "first-member-symbol-id", target: "first-symbol-id", kind: .memberOf, targetFallback: nil), + ])), + + TextFile(name: "SymbolExtension.md", utf8Content: """ + # ``Something`` + + ## Topics + - ``FirstClass/firstMember`` + """), + ]) + let (_, context) = try loadBundle(catalog: catalog) + XCTAssert(context.problems.isEmpty, "Unexpected problems: \(context.problems.map(\.diagnostic.summary))") + + let moduleReference = try XCTUnwrap(context.soleRootModuleReference) + + let firstNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("FirstClass")]) + XCTAssertFalse(firstNode.isManuallyCuratedInContainer, "This symbol is never manually curated") + let memberNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("FirstClass/firstMember")]) + XCTAssertFalse(memberNode.isManuallyCuratedInContainer, "Curation of member outside its canonical container's hierarchy doesn't stop automatic curation") + } + + func testCurationOfMemberDeeperInHierarchyDoesStopAutomaticCuration() throws { + let outerContainerID = "outer-container-symbol-id" + let innerContainerID = "inner-container-symbol-id" + let memberID = "some-member-symbol-id" + + let catalog = Folder(name: "Something.docc", content: [ + JSONFile(name: "Something.symbols.json", content: makeSymbolGraph(moduleName: "Something", symbols: [ + makeSymbol(id: outerContainerID, kind: .class, pathComponents: ["OuterClass"]), + makeSymbol(id: innerContainerID, kind: .class, pathComponents: ["OuterClass", "InnerClass"]), + makeSymbol(id: memberID, kind: .func, pathComponents: ["OuterClass", "someMember"]), + ], relationships: [ + .init(source: innerContainerID, target: outerContainerID, kind: .memberOf, targetFallback: nil), + .init(source: memberID, target: outerContainerID, kind: .memberOf, targetFallback: nil), + ])), + + TextFile(name: "SymbolExtension.md", utf8Content: """ + # ``OuterClass/InnerClass`` + + ## Topics + - ``OuterClass/someMember`` + """), + ]) + let (_, context) = try loadBundle(catalog: catalog) + XCTAssert(context.problems.isEmpty, "Unexpected problems: \(context.problems.map(\.diagnostic.summary))") + + let moduleReference = try XCTUnwrap(context.soleRootModuleReference) + + let outerNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("OuterClass")]) + XCTAssertFalse(outerNode.isManuallyCuratedInContainer, "This symbol is never manually curated") + let innerNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("OuterClass/InnerClass")]) + XCTAssertFalse(innerNode.isManuallyCuratedInContainer, "This symbol is never manually curated") + + let memberNode = try XCTUnwrap(context.topicGraph.nodes[moduleReference.appendingPath("OuterClass/someMember")]) + XCTAssertTrue(memberNode.isManuallyCuratedInContainer) + } + func testAutomaticallyCuratedSymbolTopicsAreMergedWithManuallyCuratedTopics() throws { for kind in availableNonExtensionSymbolKinds { let containerID = "some-container-id" diff --git a/Tests/SwiftDocCTests/Infrastructure/SymbolBreadcrumbTests.swift b/Tests/SwiftDocCTests/Infrastructure/SymbolBreadcrumbTests.swift new file mode 100644 index 0000000000..f8e2b62ae2 --- /dev/null +++ b/Tests/SwiftDocCTests/Infrastructure/SymbolBreadcrumbTests.swift @@ -0,0 +1,96 @@ +/* + 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 XCTest +import SymbolKit +@testable import SwiftDocC +import SwiftDocCTestUtilities +import Markdown + +class SymbolBreadcrumbTests: XCTestCase { + func testLanguageSpecificBreadcrumbs() throws { + let (_, context) = try testBundleAndContext(named: "GeometricalShapes") + let resolver = try XCTUnwrap(context.linkResolver.localResolver) + let moduleReference = try XCTUnwrap(context.soleRootModuleReference) + + // typedef struct { + // CGPoint center; + // CGFloat radius; + // } TLACircle NS_SWIFT_NAME(Circle); + do { + let reference = moduleReference.appendingPath("Circle/center") + + XCTAssertEqual(resolver.breadcrumbs(of: reference, in: .swift)?.map(\.path), [ + "/documentation/GeometricalShapes", + "/documentation/GeometricalShapes/Circle", + "/documentation/GeometricalShapes/Circle/center", + ]) + XCTAssertEqual(resolver.breadcrumbs(of: reference, in: .objectiveC)?.map(\.path), [ + "/documentation/GeometricalShapes", + "/documentation/GeometricalShapes/Circle", // named TLACircle in Objective-C + "/documentation/GeometricalShapes/Circle/center", + ]) + } + + // extern const TLACircle TLACircleZero NS_SWIFT_NAME(Circle.zero); + do { + let reference = moduleReference.appendingPath("Circle/zero") + + XCTAssertEqual(resolver.breadcrumbs(of: reference, in: .swift)?.map(\.path), [ + "/documentation/GeometricalShapes", + "/documentation/GeometricalShapes/Circle", + "/documentation/GeometricalShapes/Circle/zero", + ]) + XCTAssertEqual(resolver.breadcrumbs(of: reference, in: .objectiveC)?.map(\.path), [ + "/documentation/GeometricalShapes", + "/documentation/GeometricalShapes/Circle/zero", // named TLACircleZero in Objective-C + ]) + } + + // BOOL TLACircleIntersects(TLACircle circle, TLACircle otherCircle) NS_SWIFT_NAME(Circle.intersects(self:_:)); + do { + let reference = moduleReference.appendingPath("Circle/intersects(_:)") + + XCTAssertEqual(resolver.breadcrumbs(of: reference, in: .swift)?.map(\.path), [ + "/documentation/GeometricalShapes", + "/documentation/GeometricalShapes/Circle", + "/documentation/GeometricalShapes/Circle/intersects(_:)", + ]) + XCTAssertEqual(resolver.breadcrumbs(of: reference, in: .objectiveC)?.map(\.path), [ + "/documentation/GeometricalShapes", + "/documentation/GeometricalShapes/Circle/intersects(_:)", // named TLACircleIntersects in Objective-C + ]) + } + + // TLACircle TLACircleMake(CGPoint center, CGFloat radius) NS_SWIFT_UNAVAILABLE("Use 'Circle.init(center:radius:)' instead."); + do { + let reference = moduleReference.appendingPath("TLACircleMake") + + XCTAssertEqual(resolver.breadcrumbs(of: reference, in: .swift)?.map(\.path), nil) + XCTAssertEqual(resolver.breadcrumbs(of: reference, in: .objectiveC)?.map(\.path), [ + "/documentation/GeometricalShapes", + "/documentation/GeometricalShapes/TLACircleMake", + ]) + } + + do { + let reference = moduleReference.appendingPath("Circle/init(center:radius:)") + + XCTAssertEqual(resolver.breadcrumbs(of: reference, in: .swift)?.map(\.path), [ + "/documentation/GeometricalShapes", + "/documentation/GeometricalShapes/Circle", + "/documentation/GeometricalShapes/Circle/init(center:radius:)", + ]) + XCTAssertEqual(resolver.breadcrumbs(of: reference, in: .objectiveC)?.map(\.path), nil) + } + + + } +}