Only curate external non-symbols in different lang (#1269)

* Minor fixes to grammar and documentation

* Only curate external non-symbols in different lang

PR #757 enabled DocC to include manually curated non-symbol nodes in the
topics section, irrespective of the node language. This allows curating
articles across documentation in different languages, even though the
article's language is considered to be "Swift". However, the logic was
too permissive, and allowed curating non-symbol nodes within the same
bundle but across different languages. This is incorrect, since the
target page should be annotated with the `SupportedLanguage` directive
to enable curating it in a different language. A bespoke mechanism to
curate an internal reference correctly is not necessary. This patch
tightens the logic to only allow curating external references in a
different language than the source page language, and removes the
existing test for the undesired behaviour of including local references
to cross-language symbols.

The changes in PR #757 were not propagated to the navigator, resulting
in the references being present in the topics section but not in the
sidebar. This patch also introduces the curation logic in the navigator
to maintain parity with the topics section. The navigator tests for
external render nodes have been modified accordingly to reflect this
change.

rdar://155522179

Author: snprajwal

Sources/SwiftDocC/Indexing/Navigator/AvailabilityIndex+Ext.swift

@@ -83,7 +83,7 @@ public struct InterfaceLanguage: Hashable, CustomStringConvertible, Codable, Equ
     /// > ``from(string:)`` function.
     public let id: String
 
-    /// A mask to use to identify the interface language..
+    /// A mask to use to identify the interface language.
     public let mask: ID
 
 

Sources/SwiftDocC/Indexing/Navigator/NavigatorIndex.swift

@@ -219,12 +219,12 @@ public class NavigatorIndex {
     }
 
     /**
-     Initialize an `NavigatorIndex` from a given path with an empty tree.
+     Initialize a `NavigatorIndex` from a given path with an empty tree.
 
      - Parameter url: The URL pointing to the path from which the index should be read.
      - Parameter bundleIdentifier: The name of the bundle the index is referring to.
 
-     - Note: Don't exposed this initializer as it's used **ONLY** for building an index.
+     - Note: Don't expose this initializer as it's used **ONLY** for building an index.
      */
     fileprivate init(withEmptyTree url: URL, bundleIdentifier: String) throws {
         self.url = url
@@ -364,14 +364,14 @@ public class NavigatorIndex {
     Read a tree on disk from a given path.
     The read is atomically performed, which means it reads all the content of the file from the disk and process the tree from loaded data.
     The queue is used to load the data for a given timeout period, after that, the queue is used to schedule another read after a given delay.
-    This approach ensures that the used  queue doesn't stall while loading the content from the disk keeping the used queue responsive.
+    This approach ensures that the used queue doesn't stall while loading the content from the disk keeping the used queue responsive.
 
     - Parameters:
-       - timeout: The amount of time we can load a batch of items from data, once the timeout time pass,
+       - timeout: The duration for which we can load a batch of items from data. Once the timeout duration passes,
                   the reading process will reschedule asynchronously using the given queue.
-       - delay: The delay to wait before schedule the next read. Default: 0.01 seconds.
+       - delay: The duration to wait for before scheduling the next read. Default: 0.01 seconds.
        - queue: The queue to use.
-       - broadcast: The callback to update get updates of the current process.
+       - broadcast: The callback to receive updates on the status of the current process.
 
     - Note: Do not access the navigator tree root node or the map from identifier to node from a different thread than the one the queue is using while the read is performed,
      this may cause data inconsistencies. For that please use the broadcast callback that notifies which items have been loaded.
@@ -455,6 +455,17 @@ extension NavigatorIndex {
             self.fragment = fragment
             self.languageIdentifier = languageIdentifier
         }
+
+        /// Compare an identifier with another one, ignoring the identifier language.
+        ///
+        /// Used when curating cross-language references in multi-language frameworks.
+        ///
+        /// - Parameter other: The other identifier to compare with.
+        func isEquivalentIgnoringLanguage(to other: Identifier) -> Bool {
+            return self.bundleIdentifier == other.bundleIdentifier &&
+                   self.path == other.path &&
+                   self.fragment == other.fragment
+        }
     }
 
     /**
@@ -884,7 +895,7 @@ extension NavigatorIndex {
         ///   - emitJSONRepresentation: Whether or not a JSON representation of the index should
         ///     be written to disk.
         ///
-        ///     Defaults to `false`.
+        ///     Defaults to `true`.
         ///
         ///   - emitLMDBRepresentation: Whether or not an LMDB representation of the index should
         ///     written to disk.
@@ -917,7 +928,7 @@ extension NavigatorIndex {
                     let (nodeID, parent) = nodesMultiCurated[index]
                     let placeholders = identifierToChildren[nodeID]!
                     for reference in placeholders {
-                        if let child = identifierToNode[reference] {
+                        if let child = identifierToNode[reference] ?? externalNonSymbolNode(for: reference) {
                             parent.add(child: child)
                             pendingUncuratedReferences.remove(reference)
                             if !multiCurated.keys.contains(reference) && reference.fragment == nil {
@@ -938,7 +949,7 @@ extension NavigatorIndex {
             for (nodeIdentifier, placeholders) in identifierToChildren {
                 for reference in placeholders {
                     let parent = identifierToNode[nodeIdentifier]!
-                    if let child = identifierToNode[reference] {
+                    if let child = identifierToNode[reference] ?? externalNonSymbolNode(for: reference) {
                         let needsCopy = multiCurated[reference] != nil
                         parent.add(child: (needsCopy) ? child.copy() : child)
                         pendingUncuratedReferences.remove(reference)
@@ -969,14 +980,11 @@ extension NavigatorIndex {
                 // page types as symbol nodes on the assumption that an unknown page type is a
                 // symbol kind added in a future version of Swift-DocC.
                 // Finally, don't add external references to the root; if they are not referenced within the navigation tree, they should be dropped altogether.
-                if let node = identifierToNode[nodeID], PageType(rawValue: node.item.pageType)?.isSymbolKind == false , !node.item.isExternal {
+                if let node = identifierToNode[nodeID], PageType(rawValue: node.item.pageType)?.isSymbolKind == false, !node.item.isExternal {
 
                     // If an uncurated page has been curated in another language, don't add it to the top-level.
                     if curatedReferences.contains(where: { curatedNodeID in
-                        // Compare all the identifier's properties for equality, except for its language.
-                        curatedNodeID.bundleIdentifier == nodeID.bundleIdentifier
-                            && curatedNodeID.path == nodeID.path
-                            && curatedNodeID.fragment == nodeID.fragment
+                        curatedNodeID.isEquivalentIgnoringLanguage(to: nodeID)
                     }) {
                         continue
                     }
@@ -1256,7 +1264,22 @@ extension NavigatorIndex {
             problem = Problem(diagnostic: diagnostic, possibleSolutions: [])
             problems.append(problem)
         }
-        
+
+        /// Find an external node for the reference that is not of a symbol kind. The source language
+        /// of the reference is ignored during this lookup since the reference assumes the target node
+        /// to be of the same language as the page that it is curated in. This may or may not be true
+        /// since non-symbol kinds (articles, tutorials, etc.) are not tied to a language.
+        // This is a workaround for https://github.com/swiftlang/swift-docc/issues/240.
+        // FIXME: This should ideally be solved by making the article language-agnostic rather
+        // than accomodating the "Swift" language and special-casing for non-symbol nodes.
+        func externalNonSymbolNode(for reference: NavigatorIndex.Identifier) -> NavigatorTree.Node? {
+            identifierToNode
+            .first { identifier, node in
+                identifier.isEquivalentIgnoringLanguage(to: reference)
+                && PageType.init(rawValue: node.item.pageType)?.isSymbolKind == false
+                && node.item.isExternal
+            }?.value
+        }
 
         /// Build the index using the render nodes files in the provided documentation archive.
         /// - Returns: A list containing all the errors encountered during indexing.

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

@@ -2756,6 +2756,11 @@ public class DocumentationContext {
         ).isSymbol
     }
 
+    /// Returns whether the given reference resolves to an external entity.
+    func isExternal(reference: ResolvedTopicReference) -> Bool {
+        externalCache[reference] != nil
+    }
+
     // MARK: - Relationship queries
 
     /// Fetch the child nodes of a documentation node with the given `reference`, optionally filtering to only children of the given `kind`.

Sources/SwiftDocC/Model/Rendering/RenderNodeTranslator.swift

@@ -1054,10 +1054,13 @@ public struct RenderNodeTranslator: SemanticVisitor {
                     return true
                 }
 
-                guard context.isSymbol(reference: reference) else {
-                    // If the reference corresponds to any kind except Symbol
-                    // (e.g., Article, Tutorial, SampleCode...), allow the topic
-                    // to appear independently of the source language it belongs to.
+                // If this is a reference to a non-symbol kind (article, tutorial, sample code, etc.),
+                // and is external to the bundle, then curate the topic irrespective of the source
+                // language of the page or reference, since non-symbol kinds are not tied to a language.
+                // This is a workaround for https://github.com/swiftlang/swift-docc/issues/240.
+                // FIXME: This should ideally be solved by making the article language-agnostic rather
+                // than accomodating the "Swift" language and special-casing for non-symbol nodes.
+                if !context.isSymbol(reference: reference) && context.isExternal(reference: reference) {
                     return true
                 }
 

Tests/SwiftDocCTests/Indexing/ExternalRenderNodeTests.swift

@@ -275,10 +275,10 @@ class ExternalRenderNodeTests: XCTestCase {
 
         // Verify that the curated external links are part of the index.
         let swiftExternalNodes = try XCTUnwrap(externalTopLevelNodes(for: .swift))
-        XCTAssertEqual(swiftExternalNodes.count, 2)
+        XCTAssertEqual(swiftExternalNodes.count, 3)
 
         let objcExternalNodes = try XCTUnwrap(externalTopLevelNodes(for: .objectiveC))
-        XCTAssertEqual(objcExternalNodes.count, 2)
+        XCTAssertEqual(objcExternalNodes.count, 3)
 
         let swiftArticleExternalNode = try XCTUnwrap(swiftExternalNodes.first(where: { $0.path == "/path/to/external/swiftarticle" }))
         let swiftSymbolExternalNode = try XCTUnwrap(swiftExternalNodes.first(where: { $0.path == "/path/to/external/swiftsymbol" }))
@@ -300,6 +300,19 @@ class ExternalRenderNodeTests: XCTestCase {
         XCTAssertEqual(objcSymbolExternalNode.title, "- (void) ObjCSymbol")
         XCTAssertEqual(objcSymbolExternalNode.isBeta, false)
         XCTAssertEqual(objcSymbolExternalNode.type, "func")
+
+        // External articles curated in the Topics section appear in all language variants. This is a workaround for https://github.com/swiftlang/swift-docc/issues/240.
+        // FIXME: This should ideally be solved by making the article language-agnostic rather than accomodating the "Swift" language and special-casing for non-symbols.
+        let swiftArticleInObjcTree = try XCTUnwrap(objcExternalNodes.first(where: { $0.path == "/path/to/external/swiftarticle" }))
+        let objcArticleInSwiftTree = try XCTUnwrap(swiftExternalNodes.first(where: { $0.path == "/path/to/external/objcarticle" }))
+
+        XCTAssertEqual(swiftArticleInObjcTree.title, "SwiftArticle")
+        XCTAssertEqual(swiftArticleInObjcTree.isBeta, false)
+        XCTAssertEqual(swiftArticleInObjcTree.type, "article")
+
+        XCTAssertEqual(objcArticleInSwiftTree.title, "ObjCArticle")
+        XCTAssertEqual(objcArticleInSwiftTree.isBeta, true)
+        XCTAssertEqual(objcArticleInSwiftTree.type, "article")
     }
 
     func testNavigatorWithExternalNodesWithNavigatorTitle() async throws {
@@ -440,11 +453,11 @@ class ExternalRenderNodeTests: XCTestCase {
         let swiftExternalNodes = (renderIndex.interfaceLanguages[SourceLanguage.swift.id]?.first?.children?.filter(\.isExternal) ?? []).sorted(by: \.title)
         let objcExternalNodes  = (renderIndex.interfaceLanguages[SourceLanguage.objectiveC.id]?.first?.children?.filter(\.isExternal) ?? []).sorted(by: \.title)
         XCTAssertEqual(swiftExternalNodes.count, 1)
-        XCTAssertEqual(objcExternalNodes.count, 1)
+        XCTAssertEqual(objcExternalNodes.count, 2)
         XCTAssertEqual(swiftExternalNodes.map(\.title), ["SwiftArticle"])
-        XCTAssertEqual(objcExternalNodes.map(\.title), ["- (void) ObjCSymbol"])
+        XCTAssertEqual(objcExternalNodes.map(\.title), ["- (void) ObjCSymbol", "SwiftArticle"])
         XCTAssertEqual(swiftExternalNodes.map(\.type), ["article"])
-        XCTAssertEqual(objcExternalNodes.map(\.type), ["func"])
+        XCTAssertEqual(objcExternalNodes.map(\.type), ["func", "article"])
     }
 
     func testExternalRenderNodeVariantRepresentationWhenIsBeta() throws {

Tests/SwiftDocCTests/Model/SemaToRenderNodeMultiLanguageTests.swift

@@ -878,88 +878,6 @@ class SemaToRenderNodeMixedLanguageTests: XCTestCase {
             defaultLanguage: .swift
         )
     }
-    
-    func testArticlesAreIncludedInAllVariantsTopicsSection() async throws {
-        let outputConsumer = try await renderNodeConsumer(
-            for: "MixedLanguageFramework",
-            configureBundle: { bundleURL in
-                try """
-                # ObjCArticle
-
-                @Metadata {
-                    @SupportedLanguage(objc)
-                }
-
-                This article has Objective-C as the source language.
-                
-                ## Topics
-                """.write(to: bundleURL.appendingPathComponent("ObjCArticle.md"), atomically: true, encoding: .utf8)
-                try """
-                # SwiftArticle
-                
-                @Metadata {
-                    @SupportedLanguage(swift)
-                }
-
-                This article has Swift as the source language.
-                """.write(to: bundleURL.appendingPathComponent("SwiftArticle.md"), atomically: true, encoding: .utf8)
-                try """
-                # ``MixedLanguageFramework``
-                
-                This symbol has a Swift and Objective-C variant.
-
-                ## Topics
-                
-                - <doc:ObjCArticle>
-                - <doc:SwiftArticle>
-                - ``_MixedLanguageFrameworkVersionNumber``
-                - ``SwiftOnlyStruct``
-                
-                """.write(to: bundleURL.appendingPathComponent("MixedLanguageFramework.md"), atomically: true, encoding: .utf8)
-            }
-        )
-        assertIsAvailableInLanguages(
-            try outputConsumer.renderNode(
-                withTitle: "ObjCArticle"
-            ),
-            languages: ["occ"],
-            defaultLanguage: .objectiveC
-        )
-        assertIsAvailableInLanguages(
-            try outputConsumer.renderNode(
-                withTitle: "_MixedLanguageFrameworkVersionNumber"
-            ),
-            languages: ["occ"],
-            defaultLanguage: .objectiveC
-        )
-        
-        let renderNode = try outputConsumer.renderNode(withIdentifier: "MixedLanguageFramework")
-        
-        // Topic identifiers in the Swift variant of the `MixedLanguageFramework` symbol
-        let swiftTopicIDs = renderNode.topicSections.flatMap(\.identifiers)
-
-        let data = try renderNode.encodeToJSON()
-        let variantRenderNode = try RenderNodeVariantOverridesApplier()
-            .applyVariantOverrides(in: data, for: [.interfaceLanguage("occ")])
-        let objCRenderNode = try RenderJSONDecoder.makeDecoder().decode(RenderNode.self, from: variantRenderNode)
-        // Topic identifiers in the ObjC variant of the `MixedLanguageFramework` symbol
-        let objCTopicIDs = objCRenderNode.topicSections.flatMap(\.identifiers)
-
-
-        // Verify that articles are included in the Topics section of both symbol
-        // variants regardless of their perceived language.
-        XCTAssertTrue(swiftTopicIDs.contains("doc://org.swift.MixedLanguageFramework/documentation/MixedLanguageFramework/ObjCArticle"))
-        XCTAssertTrue(swiftTopicIDs.contains("doc://org.swift.MixedLanguageFramework/documentation/MixedLanguageFramework/SwiftArticle"))
-        XCTAssertTrue(objCTopicIDs.contains("doc://org.swift.MixedLanguageFramework/documentation/MixedLanguageFramework/SwiftArticle"))
-        XCTAssertTrue(objCTopicIDs.contains("doc://org.swift.MixedLanguageFramework/documentation/MixedLanguageFramework/ObjCArticle"))
-        
-        // Verify that language specific symbols are dropped from the Topics section in the
-        // variants for languages where the symbol isn't available.
-        XCTAssertTrue(swiftTopicIDs.contains("doc://org.swift.MixedLanguageFramework/documentation/MixedLanguageFramework/SwiftOnlyStruct"))
-        XCTAssertFalse(swiftTopicIDs.contains("doc://org.swift.MixedLanguageFramework/documentation/MixedLanguageFramework/_MixedLanguageFrameworkVersionNumber"))
-        XCTAssertTrue(objCTopicIDs.contains("doc://org.swift.MixedLanguageFramework/documentation/MixedLanguageFramework/_MixedLanguageFrameworkVersionNumber"))
-        XCTAssertFalse(objCTopicIDs.contains("doc://org.swift.MixedLanguageFramework/documentation/MixedLanguageFramework/SwiftOnlyStruct"))
-    }
 
     func testAutomaticSeeAlsoSectionElementLimit() async throws {
         let (bundle, context) = try await loadBundle(catalog: