Resolve dependency symbol reference in declarations and relationships (#826)

* Resolve dependency symbol reference in declarations and relationships

rdar://116085974

* Use the entities relativePresentationURL as its render reference's URL

* Synchronize access to 'context.externallyResolvedLinks' while resolving links

* Ensure that the temp container directory exist when merging archives

* Remove test assertion that's testing an incorrect behavior

Author: d-ronnqvist

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

@@ -1546,7 +1546,8 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
         in symbols: [String: UnifiedSymbolGraph.Symbol],
         relationships: [UnifiedSymbolGraph.Selector: Set<SymbolGraph.Relationship>]
     ) throws {
-        guard let symbolResolver = globalExternalSymbolResolver else {
+        if globalExternalSymbolResolver == nil, linkResolver.externalResolvers.isEmpty {
+            // Context has no mechanism for resolving external symbol links. No reason to gather any symbols to resolve.
             return
         }
 
@@ -1576,9 +1577,21 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
 
         // TODO: When the symbol graph includes the precise identifiers for conditional availability, those symbols should also be resolved (rdar://63768609).
 
+        func resolveSymbol(symbolID: String) -> (ResolvedTopicReference, LinkResolver.ExternalEntity)? {
+            if let globalResult = globalExternalSymbolResolver?.symbolReferenceAndEntity(withPreciseIdentifier: symbolID) {
+                return globalResult
+            }
+            for externalResolver in linkResolver.externalResolvers.values {
+                if let result = externalResolver.symbolReferenceAndEntity(symbolID: symbolID) {
+                    return result
+                }
+            }
+            return nil
+        }
+        
         // Resolve all the collected symbol identifiers and add them do the topic graph.
         for symbolID in symbolsToResolve {
-            if let (reference, entity) = symbolResolver.symbolReferenceAndEntity(withPreciseIdentifier: symbolID) {
+            if let (reference, entity) = resolveSymbol(symbolID: symbolID) {
                 externalCache.add(entity, reference: reference, symbolID: symbolID)
             } else {
                 diagnosticEngine.emit(Problem(diagnostic: Diagnostic(source: nil, severity: .warning, range: nil, identifier: "org.swift.docc.ReferenceSymbolNotFound", summary: "Symbol with identifier \(symbolID.singleQuoted) was referenced in the combined symbol graph but couldn't be found in the symbol graph or externally."), possibleSolutions: []))

Sources/SwiftDocC/Infrastructure/Link Resolution/ExternalPathHierarchyResolver.swift

@@ -78,11 +78,9 @@ final class ExternalPathHierarchyResolver {
     }
 
     /// Returns the external entity for a symbol's unique identifier or `nil` if that symbol isn't known in this external context.
-    func entity(symbolID usr: String) -> LinkResolver.ExternalEntity? {
-        // TODO: Resolve external symbols by USR (rdar://116085974) (There is nothing calling this function)
-        // This function has an optional return value since it's not easy to check what module a symbol belongs to based on its identifier.
+    func symbolReferenceAndEntity(symbolID usr: String) -> (ResolvedTopicReference, LinkResolver.ExternalEntity)? {
         guard let reference = symbols[usr] else { return nil }
-        return entity(reference)
+        return (reference, entity(reference))
     }
 
     /// Returns the external entity for a reference that was successfully resolved by this external resolver.
@@ -163,14 +161,14 @@ final class ExternalPathHierarchyResolver {
         }
     }
 
-    convenience init(dependencyArchive: URL) throws {
+    convenience init(dependencyArchive: URL, fileManager: FileManagerProtocol) throws {
         // ???: Should it be the callers responsibility to pass both these URLs?
         let linkHierarchyFile = dependencyArchive.appendingPathComponent("link-hierarchy.json")
         let entityURL = dependencyArchive.appendingPathComponent("linkable-entities.json")
 
         self.init(
-            linkInformation: try JSONDecoder().decode(SerializableLinkResolutionInformation.self, from: Data(contentsOf: linkHierarchyFile)),
-            entityInformation: try JSONDecoder().decode([LinkDestinationSummary].self, from: Data(contentsOf: entityURL))
+            linkInformation: try JSONDecoder().decode(SerializableLinkResolutionInformation.self, from: fileManager.contents(of: linkHierarchyFile)),
+            entityInformation: try JSONDecoder().decode([LinkDestinationSummary].self, from: fileManager.contents(of: entityURL))
         )
     }
 }
@@ -209,7 +207,7 @@ private extension LinkDestinationSummary {
             identifier: .init(referenceURL.absoluteString),
             titleVariants: titleVariants,
             abstractVariants: abstractVariants,
-            url: referenceURL.absoluteString,
+            url: relativePresentationURL.absoluteString,
             kind: kind,
             required: false,
             role: role,

Sources/SwiftDocC/Infrastructure/Link Resolution/LinkResolver.swift

@@ -17,6 +17,7 @@ public class LinkResolver {
     @_spi(ExternalLinks) // This needs to be public SPI so that the ConvertAction can set it.
     public var dependencyArchives: [URL] = []
 
+    var fileManager: FileManagerProtocol = FileManager.default
     /// The link resolver to use to resolve links in the local bundle
     var localResolver: PathHierarchyBasedLinkResolver!
     /// A fallback resolver to use when the local resolver fails to resolve a link.
@@ -29,7 +30,7 @@ public class LinkResolver {
     /// Create link resolvers for all documentation archive dependencies.
     func loadExternalResolvers() throws {
         let resolvers = try dependencyArchives.compactMap {
-            try ExternalPathHierarchyResolver(dependencyArchive: $0)
+            try ExternalPathHierarchyResolver(dependencyArchive: $0, fileManager: fileManager)
         }
         for resolver in resolvers {
             for moduleNode in resolver.pathHierarchy.modules {
@@ -84,22 +85,16 @@ public class LinkResolver {
     ///   - context: The documentation context to resolve the link in.
     /// - Returns: The result of resolving the reference.
     func resolve(_ unresolvedReference: UnresolvedTopicReference, in parent: ResolvedTopicReference, fromSymbolLink isCurrentlyResolvingSymbolLink: Bool, context: DocumentationContext) -> TopicReferenceResolutionResult {
-        // Check if the unresolved reference is external
-        if let bundleID = unresolvedReference.bundleIdentifier,
-           !context.registeredBundles.contains(where: { bundle in
-               bundle.identifier == bundleID || urlReadablePath(bundle.displayName) == bundleID
-           }) {
-            if context.externalDocumentationSources[bundleID] != nil,
-               let resolvedExternalReference = context.externallyResolvedLinks[unresolvedReference.topicURL] {
-                // Return the successful or failed externally resolved reference.
-                return resolvedExternalReference
-            } else if !context.registeredBundles.contains(where: { $0.identifier == bundleID }) {
-                return .failure(unresolvedReference, TopicReferenceResolutionErrorInfo("No external resolver registered for \(bundleID.singleQuoted)."))
-            }
+        // Check if this is an external link that has previously been resolved, either successfully or not.
+        if let previousExternalResult = contextExternalLinksLock.sync({ context.externallyResolvedLinks[unresolvedReference.topicURL] }) {
+            return previousExternalResult
         }
 
-        if let previousExternalResult = context.externallyResolvedLinks[unresolvedReference.topicURL] {
-            return previousExternalResult
+        // Check if this is a link to an external documentation source that should have previously been resolved in `DocumentationContext.preResolveExternalLinks(...)`
+        if let bundleID = unresolvedReference.bundleIdentifier,
+           !context.registeredBundles.contains(where: { $0.identifier == bundleID || urlReadablePath($0.displayName) == bundleID })
+        {
+            return .failure(unresolvedReference, TopicReferenceResolutionErrorInfo("No external resolver registered for \(bundleID.singleQuoted)."))
         }
 
         do {
@@ -108,9 +103,11 @@ public class LinkResolver {
             // Check if there's a known external resolver for this module.
             if case .moduleNotFound(_, let remainingPathComponents, _) = error, let resolver = externalResolvers[remainingPathComponents.first!.full] {
                 let result = resolver.resolve(unresolvedReference, fromSymbolLink: isCurrentlyResolvingSymbolLink)
-                context.externallyResolvedLinks[unresolvedReference.topicURL] = result
-                if case .success(let resolved) = result {
-                    context.externalCache[resolved] = resolver.entity(resolved)
+                contextExternalLinksLock.sync {
+                    context.externallyResolvedLinks[unresolvedReference.topicURL] = result
+                    if case .success(let resolved) = result {
+                        context.externalCache[resolved] = resolver.entity(resolved)
+                    }
                 }
                 return result
             }
@@ -125,6 +122,15 @@ public class LinkResolver {
             fatalError("Only SymbolPathTree.Error errors are raised from the symbol link resolution code above.")
         }
     }
+    
+    /// The context resolves links concurrently for different pages.
+    ///
+    /// Since the link resolver may both read and write to the context to cache externally resolved references, it needs to synchronize those accesses to avoid data races.
+    ///
+    /// > Note:
+    /// > We don't generally want to require a lock around these properties because the context will also render pages in parallel and during rendering the external content
+    /// > is only read, so requiring synchronization would add unnecessary overhead during rendering.
+    private var contextExternalLinksLock = Lock()
 }
 
 // MARK: Fallback resolver

Sources/SwiftDocC/Utility/FileManagerProtocol.swift

@@ -68,6 +68,17 @@ public protocol FileManagerProtocol {
     /// - Throws: If the file couldn't be created with the specified contents.
     func createFile(at: URL, contents: Data) throws
 
+    /// Returns the data content of a file at the given URL.
+    ///
+    /// - Parameters:
+    ///   - url: The location to create the file
+    ///
+    /// - Note: This method doesn't exist on ``FileManager``.
+    ///         There is a similar looking method but it doesn't provide information about potential errors.
+    ///
+    /// - Throws: If the file couldn't be read.
+    func contents(of url: URL) throws -> Data
+    
     /// Creates a file with the given contents at the given url with the specified
     /// writing options.
     ///
@@ -93,6 +104,10 @@ extension FileManagerProtocol {
 /// most of the methods are already implemented in Foundation.
 @_spi(FileManagerProtocol)
 extension FileManager: FileManagerProtocol {
+    // This method doesn't exist on `FileManager`. There is a similar looking method but it doesn't provide information about potential errors.
+    public func contents(of url: URL) throws -> Data {
+        return try Data(contentsOf: url)
+    }
 
     // This method doesn't exist on `FileManager`. There is a similar looking method but it doesn't provide information about potential errors.
     public func createFile(at location: URL, contents: Data) throws {

Sources/SwiftDocCTestUtilities/FilesAndFolders.swift

@@ -93,7 +93,7 @@ public struct InfoPlist: File, DataRepresentable {
     /// The information that the Into.plist file contains.
     public let content: Content
 
-    public init(displayName: String, identifier: String? = nil, versionString: String = "1.0") {
+    public init(displayName: String? = nil, identifier: String? = nil, versionString: String = "1.0") {
         self.content = Content(
             displayName: displayName,
             identifier: identifier,
@@ -102,11 +102,11 @@ public struct InfoPlist: File, DataRepresentable {
     }
 
     public struct Content: Codable, Equatable {
-        public let displayName: String
+        public let displayName: String?
         public let identifier: String?
         public let versionString: String?
 
-        fileprivate init(displayName: String, identifier: String?, versionString: String) {
+        fileprivate init(displayName: String?, identifier: String?, versionString: String) {
             self.displayName = displayName
             self.identifier = identifier
             self.versionString = versionString

Sources/SwiftDocCTestUtilities/TestFileSystem.swift

@@ -121,11 +121,15 @@ public class TestFileSystem: FileManagerProtocol, DocumentationWorkspaceDataProv
         defer { filesLock.unlock() }
 
         guard let file = files[url.path] else {
-            throw Errors.invalidPath(url.path)
+            throw CocoaError.error(.fileReadNoSuchFile)
         }
         return file
     }
 
+    public func contents(of url: URL) throws -> Data {
+        try contentsOfURL(url)
+    }
+    
     func filesIn(folder: Folder, at: URL) throws -> [String: Data] {
         filesLock.lock()
         defer { filesLock.unlock() }
@@ -223,7 +227,7 @@ public class TestFileSystem: FileManagerProtocol, DocumentationWorkspaceDataProv
             // If it's not the root folder, check if parents exist
             if createIntermediates == false {
                 guard files.keys.contains(parent.path) else {
-                    throw Errors.invalidPath(path)
+                    throw CocoaError.error(.fileReadNoSuchFile)
                 }
             } else {
                 // Create missing parent directories

Sources/SwiftDocCUtilities/Action/Actions/Action+MoveOutput.swift

@@ -24,6 +24,8 @@ extension Action {
 
         if let template = template {
             // If a template directory has been provided, create the temporary build folder with its contents
+            // Ensure that the container exists
+            try? fileManager.createDirectory(at: targetURL.deletingLastPathComponent(), withIntermediateDirectories: false, attributes: nil)
             try fileManager.copyItem(at: template, to: targetURL)
         } else {
             // Otherwise, create an empty directory