Fix a crash when configuring a @DisplayName for a non-symbol (#783)

d-ronnqvist · mayaepps · web-flow · commit 2e354274df56 · 2024-01-05T10:37:39.000+01:00
* Preserve all other parsed metadata when article has @DisplayName rdar://114654594 * Hide metadata initializer that only exist to workaround #468 * Update year in header comments * Apply suggestions from code review Co-authored-by: mayaepps <53411851+mayaepps@users.noreply.github.com> * Clarify test assertion descriptions --------- Co-authored-by: mayaepps <53411851+mayaepps@users.noreply.github.com>
diff --git a/Sources/SwiftDocC/Infrastructure/External Data/OutOfProcessReferenceResolver.swift b/Sources/SwiftDocC/Infrastructure/External Data/OutOfProcessReferenceResolver.swift
@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2021-2023 Apple Inc. and the Swift project authors
+ Copyright (c) 2021-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
@@ -276,7 +276,7 @@ public class OutOfProcessReferenceResolver: ExternalReferenceResolver, FallbackR
         // with a render node.
         
         if let topicImages = resolvedInformation.topicImages, !topicImages.isEmpty {
-            let metadata = node.metadata ?? Metadata(originalMarkup: BlockDirective(name: "Metadata", children: []), documentationExtension: nil, technologyRoot: nil, displayName: nil, titleHeading: nil)
+            let metadata = node.metadata ?? Metadata._make(originalMarkup: BlockDirective(name: "Metadata", children: []))
             
             metadata.pageImages = topicImages.map { topicImage in
                 let purpose: PageImage.Purpose
diff --git a/Sources/SwiftDocC/Semantics/Article/Article.swift b/Sources/SwiftDocC/Semantics/Article/Article.swift
@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2021-2022 Apple Inc. and the Swift project authors
+ Copyright (c) 2021-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
@@ -217,8 +217,8 @@ public final class Article: Semantic, MarkupConvertible, Abstracted, Redirected,
             
             problems.append(Problem(diagnostic: diagnostic, possibleSolutions: solutions))
             
-            // Remove the display name customization from the article's metadata.
-            optionalMetadata = Metadata(originalMarkup: metadata.originalMarkup, documentationExtension: metadata.documentationOptions, technologyRoot: metadata.technologyRoot, displayName: nil, titleHeading: metadata.titleHeading)
+            metadata.displayName = nil
+            optionalMetadata = metadata
         }
         
         self.init(
diff --git a/Sources/SwiftDocC/Semantics/Metadata/Metadata.swift b/Sources/SwiftDocC/Semantics/Metadata/Metadata.swift
@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2021-2023 Apple Inc. and the Swift project authors
+ Copyright (c) 2021-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
@@ -89,21 +89,6 @@ public final class Metadata: Semantic, AutomaticDirectiveConvertible {
         "titleHeading"          : \Metadata._titleHeading,
     ]
     
-    /// Creates a metadata object with a given markup, documentation extension, and technology root.
-    /// - Parameters:
-    ///   - originalMarkup: The original markup for this metadata directive.
-    ///   - documentationExtension: Optional configuration that describes how this documentation extension file merges or overrides the in-source documentation.
-    ///   - technologyRoot: Optional configuration to make this page root-level documentation.
-    ///   - displayName: Optional configuration to customize this page's symbol's display name.
-    ///   - titleHeading: Optional configuration to customize the text of this page's title heading.
-    init(originalMarkup: BlockDirective, documentationExtension: DocumentationExtension?, technologyRoot: TechnologyRoot?, displayName: DisplayName?, titleHeading: TitleHeading?) {
-        self.originalMarkup = originalMarkup
-        self.documentationOptions = documentationExtension
-        self.technologyRoot = technologyRoot
-        self.displayName = displayName
-        self.titleHeading = titleHeading
-    }
-    
     @available(*, deprecated, message: "Do not call directly. Required for 'AutomaticDirectiveConvertible'.")
     init(originalMarkup: BlockDirective) {
         self.originalMarkup = originalMarkup
@@ -203,5 +188,76 @@ public final class Metadata: Semantic, AutomaticDirectiveConvertible {
         
         return true
     }
+    
+    // MARK: Private API for OutOfProcessReferenceResolver
+    
+    /// Don't use this outside of ``OutOfProcessReferenceResolver/entity(with:)`` .
+    ///
+    /// Directives aren't meant to be created from non-markup but the out-of-process resolver needs to create a ``Metadata`` to hold the ``PageImage``
+    /// values that it creates to associate topic images with external pages. This is because DocC renders external content in the local context. (rdar://78718811)
+    /// https://github.com/apple/swift-docc/issues/468
+    ///
+    /// This is intentionally defined as an underscore prefixed static function instead of an initializer to make it less likely that it's used in other places.
+    static func _make(
+        originalMarkup: BlockDirective,
+        documentationOptions: DocumentationExtension? = nil,
+        technologyRoot: TechnologyRoot? = nil,
+        displayName: DisplayName? = nil,
+        pageImages: [PageImage] = [],
+        customMetadata: [CustomMetadata] = [],
+        callToAction: CallToAction? = nil,
+        availability: [Metadata.Availability] = [],
+        pageKind: Metadata.PageKind? = nil,
+        supportedLanguages: [SupportedLanguage] = [],
+        _pageColor: PageColor? = nil,
+        titleHeading: TitleHeading? = nil
+    ) -> Metadata {
+        // FIXME: https://github.com/apple/swift-docc/issues/468
+        return Metadata(
+            originalMarkup: originalMarkup,
+            documentationOptions: documentationOptions,
+            technologyRoot: technologyRoot,
+            displayName: displayName,
+            pageImages: pageImages,
+            customMetadata: customMetadata,
+            callToAction: callToAction,
+            availability: availability,
+            pageKind: pageKind,
+            supportedLanguages: supportedLanguages,
+            _pageColor: _pageColor,
+            titleHeading: titleHeading
+        )
+    }
+    
+    // This initializer only exists to be called by `_make` above.
+    private init(
+        originalMarkup: BlockDirective,
+        documentationOptions: DocumentationExtension?,
+        technologyRoot: TechnologyRoot?,
+        displayName: DisplayName?,
+        pageImages: [PageImage],
+        customMetadata: [CustomMetadata],
+        callToAction: CallToAction?,
+        availability: [Metadata.Availability],
+        pageKind: Metadata.PageKind?,
+        supportedLanguages: [SupportedLanguage],
+        _pageColor: PageColor?,
+        titleHeading: TitleHeading?
+    ) {
+        self.originalMarkup = originalMarkup
+        self.documentationOptions = documentationOptions
+        self.technologyRoot = technologyRoot
+        self.displayName = displayName
+        self.callToAction = callToAction
+        self._pageColor = _pageColor
+        self.pageKind = pageKind
+        self.titleHeading = titleHeading
+        // Non-optional child directives need to be set after `super.init()`.
+        super.init()
+        self.customMetadata = customMetadata
+        self.pageImages = pageImages
+        self.availability = availability
+        self.supportedLanguages = supportedLanguages
+    }
 }
 
diff --git a/Tests/SwiftDocCTests/Infrastructure/TestExternalReferenceResolvers.swift b/Tests/SwiftDocCTests/Infrastructure/TestExternalReferenceResolvers.swift
@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2023 Apple Inc. and the Swift project authors
+ Copyright (c) 2023-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
@@ -159,7 +159,7 @@ class TestMultiResultExternalReferenceResolver: ExternalReferenceResolver, Fallb
         // This is a workaround for how external content is processed. See details in OutOfProcessReferenceResolver.addImagesAndCacheMediaReferences(to:from:)
         
         if let topicImages = entityInfo.topicImages {
-            let metadata = node.metadata ?? Metadata(originalMarkup: BlockDirective(name: "Metadata", children: []), documentationExtension: nil, technologyRoot: nil, displayName: nil, titleHeading: nil)
+            let metadata = node.metadata ?? Metadata._make(originalMarkup: BlockDirective(name: "Metadata", children: []))
             
             metadata.pageImages = topicImages.map { topicImage, alt in
                 let purpose: PageImage.Purpose
diff --git a/Tests/SwiftDocCTests/Semantics/ArticleTests.swift b/Tests/SwiftDocCTests/Semantics/ArticleTests.swift
@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2021-2023 Apple Inc. and the Swift project authors
+ Copyright (c) 2021-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
@@ -208,4 +208,45 @@ class ArticleTests: XCTestCase {
         
         XCTAssertEqual(article?.options[.local]?.automaticSeeAlsoEnabled, false)
     }
+    
+    func testDisplayNameDirectiveIsRemoved() throws {
+        let source = """
+        # Root
+        
+        @Metadata {
+          @TechnologyRoot
+          @PageColor(purple)
+          @DisplayName("Example")
+        }
+        
+        Adding @DisplayName to an article will result in a warning.
+        """
+        let document = Document(parsing: source, options: [.parseBlockDirectives])
+        let (bundle, context) = try testBundleAndContext()
+        var problems = [Problem]()
+        let article = Article(from: document, source: nil, for: bundle, in: context, problems: &problems)
+        
+        XCTAssertEqual(problems.map(\.diagnostic.summary), [
+            "A 'DisplayName' directive is only supported in documentation extension files. To customize the display name of an article, change the content of the level-1 heading."
+        ])
+        
+        let semantic = try XCTUnwrap(article)
+        XCTAssertNotNil(semantic.metadata, "Article should have a metadata container since the markup has a @Metadata directive")
+        XCTAssertNotNil(semantic.metadata?.technologyRoot, "Article should have a technology root configuration since the markup has a @TechnologyRoot directive")
+        XCTAssertNotNil(semantic.metadata?.pageColor, "Article should have a page color configuration since the markup has a @PageColor directive")
+        
+        XCTAssertNil(semantic.metadata?.displayName, "Articles shouldn't have a display name metadata configuration, even though the markup has a @DisplayName directive. Article names are specified by the level-1 header instead of a metadata directive.")
+        
+        // Non-optional child directives should be initialized.
+        XCTAssertEqual(semantic.metadata?.pageImages, [])
+        XCTAssertEqual(semantic.metadata?.customMetadata, [])
+        XCTAssertEqual(semantic.metadata?.availability, [])
+        XCTAssertEqual(semantic.metadata?.supportedLanguages, [])
+        
+        // Optional child directives should default to nil
+        XCTAssertNil(semantic.metadata?.documentationOptions)
+        XCTAssertNil(semantic.metadata?.callToAction)
+        XCTAssertNil(semantic.metadata?.pageKind)
+        XCTAssertNil(semantic.metadata?.titleHeading)
+    }
 }
