Make ResolvedTopicReference immutable and copy-on-write

ethan-kusters · franklinsch · ethan-kusters · commit e76ef474d807 · 2021-12-09T12:15:37.000-08:00
Co-authored-by: Franklin Schrans &lt;fschrans@apple.com&gt;
diff --git a/Sources/SwiftDocC/Infrastructure/DocumentationContext.swift b/Sources/SwiftDocC/Infrastructure/DocumentationContext.swift
@@ -454,7 +454,7 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
             externallyResolvedLinks[pair.url] = pair.resolved
             if case .success(let resolvedReference) = pair.resolved,
                 pair.url.absoluteString != resolvedReference.absoluteString,
-                let url = resolvedReference.url.flatMap(ValidatedURL.init) {
+                let url = ValidatedURL(resolvedReference.url) {
                 // If the resolved reference has a different URL than the link cache both URLs
                 // so we can resolve both unresolved and resolved references.
                 externallyResolvedLinks[url] = pair.resolved
@@ -1110,7 +1110,7 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
                 }
                 
                 // If it's an existing module, update the interface languages
-                moduleReferences[moduleName]?.sourceLanguages.formUnion(moduleInterfaceLanguages)
+                moduleReferences[moduleName] = moduleReferences[moduleName]?.addingSourceLanguages(moduleInterfaceLanguages)
                 
                 // Import the symbol graph symbols
                 let moduleReference: ResolvedTopicReference
diff --git a/Sources/SwiftDocC/Model/Identifier.swift b/Sources/SwiftDocC/Model/Identifier.swift
@@ -66,11 +66,24 @@ public enum TopicReferenceResolutionResult: Hashable, CustomStringConvertible {
     }
 }
 
-/**
- A reference to a piece of documentation which has been verified to exist.
- 
- A `ResolvedTopicReference` refers to some piece of documentation, such as an article or symbol. Once an `UnresolvedTopicReference` has been resolved to this type, it should be guaranteed that the content backing the documentation is available (i.e. there is a file on disk or data in memory ready to be recalled at any time).
- */
+/// A reference to a piece of documentation which has been verified to exist.
+///
+/// A `ResolvedTopicReference` refers to some piece of documentation, such as an article or symbol.
+/// Once an `UnresolvedTopicReference` has been resolved to this type, it should be guaranteed
+/// that the content backing the documentation is available
+/// (i.e. there is a file on disk or data in memory ready to be
+/// recalled at any time).
+///
+/// ## Implementation Details
+///
+/// `ResolvedTopicReference` is effectively a wrapper around Foundation's `URL` and,
+/// because of this, it exposes an API very similar to `URL` and does not allow direct modification
+/// of its properties. This immutability brings performance benefits and communicates with
+/// user's of the API that doing something like adding a path component
+/// is a potentially expensive operation, just as it is on `URL`.
+///
+/// > Important: This type has copy-on-write semantics and wraps an underlying class to store
+/// > its data.
 public struct ResolvedTopicReference: Hashable, Codable, Equatable, CustomStringConvertible {
     typealias ReferenceBundleIdentifier = String
     typealias ReferenceKey = String
@@ -92,30 +105,34 @@ public struct ResolvedTopicReference: Hashable, Codable, Equatable, CustomString
         return url?.scheme?.lowercased() == ResolvedTopicReference.urlScheme
     }
     
+    /// The storage for the resolved topic reference's state.
+    let _storage: Storage
+    
     /// The identifier of the bundle that owns this documentation topic.
     public var bundleIdentifier: String {
-        didSet { updateURL() }
+        return _storage.bundleIdentifier
     }
     
     /// The absolute path from the bundle to this topic, delimited by `/`.
     public var path: String {
-        didSet { updateURL() }
+        return _storage.path
     }
     
     /// A URL fragment referring to a resource in the topic.
     public var fragment: String? {
-        didSet { updateURL() }
+        return _storage.fragment
     }
     
     /// The source language for which this topic is relevant.
     public var sourceLanguage: SourceLanguage {
         // Return Swift by default to maintain backwards-compatibility.
-        get { sourceLanguages.contains(.swift) ? .swift : sourceLanguages.first! }
-        set { sourceLanguages.insert(newValue) }
+        return sourceLanguages.contains(.swift) ? .swift : sourceLanguages.first!
     }
     
     /// The source languages for which this topic is relevant.
-    public var sourceLanguages: Set<SourceLanguage>
+    public var sourceLanguages: Set<SourceLanguage> {
+        return _storage.sourceLanguages
+    }
     
     /// - Note: The `path` parameter is escaped to a path readable string.
     public init(bundleIdentifier: String, path: String, fragment: String? = nil, sourceLanguage: SourceLanguage) {
@@ -132,20 +149,20 @@ public struct ResolvedTopicReference: Hashable, Codable, Equatable, CustomString
             return
         }
         
-        // Create a new reference
-        self.bundleIdentifier = bundleIdentifier
-        self.path = urlReadablePath(path)
-        self.fragment = fragment.map { urlReadableFragment($0) }
-        self.sourceLanguages = sourceLanguages
-        updateURL()
+        _storage = Storage(
+            bundleIdentifier: bundleIdentifier,
+            path: urlReadablePath,
+            fragment: urlReadableFragment,
+            sourceLanguages: sourceLanguages
+        )
 
         // Cache the reference
         Self.sharedPool.sync { $0[bundleIdentifier, default: [:]][key] = self }
     }
     
     private static func cacheKey(
-        path: String,
-        fragment: String?,
+        urlReadablePath path: String,
+        urlReadableFragment fragment: String?,
         sourceLanguages: Set<SourceLanguage>
     ) -> String {
         let sourceLanguagesString = sourceLanguages.map(\.id).sorted().joined(separator: "-")
@@ -158,28 +175,19 @@ public struct ResolvedTopicReference: Hashable, Codable, Equatable, CustomString
     }
     
     /// The topic URL as you would write in a link.
-    private (set) public var url: URL! = nil
-    
-    private mutating func updateURL() {
-        var components = URLComponents()
-        components.scheme = ResolvedTopicReference.urlScheme
-        components.host = bundleIdentifier
-        components.path = path
-        components.fragment = fragment
-        url = components.url!
-        pathComponents = url.pathComponents
-        absoluteString = url.absoluteString
+    public var url: URL {
+        return _storage.url
     }
     
     /// A list of the reference path components.
-    /// > Note: This value is updated inside `updateURL()` to avoid
-    /// accessing the property on `URL`.
-    private(set) var pathComponents = [String]()
+    var pathComponents: [String] {
+        return _storage.pathComponents
+    }
     
     /// A string representation of `url`.
-    /// > Note: This value is updated inside `updateURL()` to avoid
-    /// accessing the property on `URL`.
-    private(set) var absoluteString = ""
+    var absoluteString: String {
+        return _storage.absoluteString
+    }
     
     enum CodingKeys: CodingKey {
         case url, interfaceLanguage
@@ -284,6 +292,25 @@ public struct ResolvedTopicReference: Hashable, Codable, Equatable, CustomString
         return newReference
     }
     
+    /// Returns a topic reference based on the current one that includes the given source languages.
+    ///
+    /// If the current topic reference already includes the given source languages, this returns
+    /// the original topic reference.
+    public func addingSourceLanguages(_ sourceLanguages: Set<SourceLanguage>) -> ResolvedTopicReference {
+        let combinedSourceLanguages = self.sourceLanguages.union(sourceLanguages)
+        
+        guard combinedSourceLanguages != self.sourceLanguages else {
+            return self
+        }
+        
+        return ResolvedTopicReference(
+            bundleIdentifier: bundleIdentifier,
+            urlReadablePath: path,
+            urlReadableFragment: fragment,
+            sourceLanguages: combinedSourceLanguages
+        )
+    }
+    
     /// The last path component of this topic reference.
     public var lastPathComponent: String {
         // There is always at least one component, so we can unwrap `last`.
@@ -322,15 +349,48 @@ public struct ResolvedTopicReference: Hashable, Codable, Equatable, CustomString
     // which languages they are available in.
     
     public func hash(into hasher: inout Hasher) {
-        hasher.combine(bundleIdentifier)
-        hasher.combine(path)
-        hasher.combine(fragment)
+        hasher.combine(_storage.identifierPathAndFragment)
     }
     
     public static func == (lhs: ResolvedTopicReference, rhs: ResolvedTopicReference) -> Bool {
-        return lhs.bundleIdentifier == rhs.bundleIdentifier
-            && lhs.path == rhs.path
-            && lhs.fragment == rhs.fragment
+        return lhs._storage.identifierPathAndFragment == rhs._storage.identifierPathAndFragment
+    }
+    
+    /// Storage for a resolved topic reference's state.
+    ///
+    /// This is a reference type which allows ``ResolvedTopicReference`` to have copy-on-write behavior.
+    class Storage {
+        let bundleIdentifier: String
+        let path: String
+        let fragment: String?
+        let sourceLanguages: Set<SourceLanguage>
+        let url: URL
+        let pathComponents: [String]
+        let absoluteString: String
+        
+        let identifierPathAndFragment: String
+        
+        init(
+            bundleIdentifier: String,
+            path: String,
+            fragment: String? = nil,
+            sourceLanguages: Set<SourceLanguage>
+        ) {
+            self.bundleIdentifier = bundleIdentifier
+            self.path = path
+            self.fragment = fragment
+            self.sourceLanguages = sourceLanguages
+            self.identifierPathAndFragment = "\(bundleIdentifier)\(path)\(fragment ?? "")"
+            
+            var components = URLComponents()
+            components.scheme = ResolvedTopicReference.urlScheme
+            components.host = bundleIdentifier
+            components.path = path
+            components.fragment = fragment
+            url = components.url!
+            pathComponents = url.pathComponents
+            absoluteString = url.absoluteString
+        }
     }
 }
 
diff --git a/Sources/SwiftDocC/Model/Rendering/RenderNodeTranslator.swift b/Sources/SwiftDocC/Model/Rendering/RenderNodeTranslator.swift
@@ -804,7 +804,7 @@ public struct RenderNodeTranslator: SemanticVisitor {
                 title: group.title,
                 abstract: nil,
                 discussion: nil,
-                identifiers: group.references.compactMap(\.url?.absoluteString),
+                identifiers: group.references.map(\.url.absoluteString),
                 generated: true
             )
         }
@@ -892,10 +892,8 @@ public struct RenderNodeTranslator: SemanticVisitor {
     public mutating func visitSymbol(_ symbol: Symbol) -> RenderTree? {
         let documentationNode = try! context.entity(with: identifier)
         
-        var identifier = identifier
+        let identifier = identifier.addingSourceLanguages(documentationNode.availableSourceLanguages)
         
-        // Add the source languages declared in the documentation node.
-        identifier.sourceLanguages = identifier.sourceLanguages.union(documentationNode.availableSourceLanguages)
         var node = RenderNode(identifier: identifier, kind: .symbol)
         var contentCompiler = RenderContentCompiler(context: context, bundle: bundle, identifier: identifier)
 
diff --git a/Tests/SwiftDocCTests/Infrastructure/ResolvedTopicReferenceTests.swift b/Tests/SwiftDocCTests/Infrastructure/ResolvedTopicReferenceTests.swift
@@ -15,21 +15,27 @@ import XCTest
 
 class ResolvedTopicReferenceTests: XCTestCase {
     func testReferenceURL() {
-        var reference = ResolvedTopicReference(bundleIdentifier: "bundleID", path: "/path/sub-path", fragment: "fragment", sourceLanguage: .swift)
-        XCTAssertEqual(reference.absoluteString, "doc://bundleID/path/sub-path#fragment")
+        let firstTopicReference = ResolvedTopicReference(
+            bundleIdentifier: "bundleID",
+            path: "/path/sub-path",
+            fragment: "fragment",
+            sourceLanguage: .swift
+        )
+        XCTAssertEqual(firstTopicReference.absoluteString, "doc://bundleID/path/sub-path#fragment")
         
-        reference.bundleIdentifier = "new-bundleID"
-        XCTAssertEqual(reference.absoluteString, "doc://new-bundleID/path/sub-path#fragment")
+        let secondTopicReference = ResolvedTopicReference(
+            bundleIdentifier: "new-bundleID",
+            path: "/new-path/sub-path",
+            fragment: firstTopicReference.fragment,
+            sourceLanguage: firstTopicReference.sourceLanguage
+        )
+        XCTAssertEqual(secondTopicReference.absoluteString, "doc://new-bundleID/new-path/sub-path#fragment")
         
-        reference.path = "/new-path/sub-path"
-        XCTAssertEqual(reference.absoluteString, "doc://new-bundleID/new-path/sub-path#fragment")
-        
-        reference.fragment = nil
-        XCTAssertEqual(reference.absoluteString, "doc://new-bundleID/new-path/sub-path")
+        let thirdTopicReference = secondTopicReference.withFragment(nil)
+        XCTAssertEqual(thirdTopicReference.absoluteString, "doc://new-bundleID/new-path/sub-path")
         
         // Changing the language does not change the url
-        reference.sourceLanguage = .metal
-        XCTAssertEqual(reference.absoluteString, "doc://new-bundleID/new-path/sub-path")
+        XCTAssertEqual(thirdTopicReference.addingSourceLanguages([.metal]).absoluteString, "doc://new-bundleID/new-path/sub-path")
     }
     
     func testAppendingReferenceWithEmptyPath() {
diff --git a/Tests/SwiftDocCTests/Infrastructure/Symbol Link Resolution/AbsoluteSymbolLinkTests.swift b/Tests/SwiftDocCTests/Infrastructure/Symbol Link Resolution/AbsoluteSymbolLinkTests.swift
@@ -524,9 +524,7 @@ class AbsoluteSymbolLinkTests: XCTestCase {
         XCTAssertEqual(expectedDescriptions.count, context.symbolIndex.count)
         
         let validatedSymbolLinkDescriptions = context.symbolIndex.values
-            .map(\.reference)
-            .compactMap(\.url)
-            .map(\.absoluteString)
+            .map(\.reference.url.absoluteString)
             .sorted()
             .compactMap(AbsoluteSymbolLink.init(string:))
             .map(\.description)
@@ -850,9 +848,7 @@ class AbsoluteSymbolLinkTests: XCTestCase {
         XCTAssertEqual(expectedDescriptions.count, context.symbolIndex.count)
         
         let validatedSymbolLinkDescriptions = context.symbolIndex.values
-            .map(\.reference)
-            .compactMap(\.url)
-            .map(\.absoluteString)
+            .map(\.reference.url.absoluteString)
             .sorted()
             .compactMap(AbsoluteSymbolLink.init(string:))
             .map(\.description)
@@ -872,10 +868,7 @@ class AbsoluteSymbolLinkTests: XCTestCase {
         defer { try? FileManager.default.removeItem(at: url) }
         
         let bundlePathComponents = context.symbolIndex.values
-            .map(\.reference)
-            .compactMap(\.url)
-            .map(\.pathComponents)
-            .flatMap { $0 }
+            .flatMap(\.reference.pathComponents)
         
         
         bundlePathComponents.forEach { component in
diff --git a/Tests/SwiftDocCTests/Model/IdentifierTests.swift b/Tests/SwiftDocCTests/Model/IdentifierTests.swift
@@ -121,4 +121,14 @@ class IdentifierTests: XCTestCase {
         ref1 = ref1.removingLastPathComponent()
         XCTAssertEqual(ref1.absoluteString, "doc://bundle/MyClass")
     }
+    
+    func testResolvedTopicReferenceDoesNotCopyStorageIfNotModified() {
+         let reference1 = ResolvedTopicReference(bundleIdentifier: "bundle", path: "/", sourceLanguage: .swift)
+         let reference2 = reference1
+
+         XCTAssertEqual(
+             ObjectIdentifier(reference1._storage),
+             ObjectIdentifier(reference2._storage)
+         )
+     }
 }
diff --git a/Tests/SwiftDocCTests/Model/SemaToRenderNodeTests.swift b/Tests/SwiftDocCTests/Model/SemaToRenderNodeTests.swift
@@ -1240,10 +1240,10 @@ class SemaToRenderNodeTests: XCTestCase {
                     reference: reference,
                     kind: .collection,
                     sourceLanguage: .swift,
-                    name: .conceptual(title: "Title for \(reference.url!.path)"),
+                    name: .conceptual(title: "Title for \(reference.url.path)"),
                     markup: Document(
                         Paragraph(
-                            Text("Abstract for \(reference.url!.path)")
+                            Text("Abstract for \(reference.url.path)")
                         )
                     ),
                     semantic: nil
