Support `@Metadata` and `@DeprecationSummary` in documentation comments #1107

franklinsch · 2024-11-28T18:15:17Z

Bug/issue #, if applicable: rdar://138395778

Summary

This PR adds the following behavior changes:

(First commit) Support for authoring @Metadata directives in documentation comments. Some notes:

Only the @Available and @CustomMetadata are supported as child directives of @Metadata in documentation comments. DocC ignores other directives and emits a warnings for their uses:
If you author a @Metadata directive both in the symbol's documentation comment and documentation extension file, DocC will pick the one in the documentation extension, and ignore the one in the documentation comment and emit a warning.

(Second commit) Support @DeprecationSummary in doc comments. Adds support for authoring @DeprecationSummary messages in documentation comments. This is useful when you want to provide a formatted documentation summary for an API that's deprecated, but don't want to create a documentation extension file in order to do just that.
(Third commit) Support for @Comment in documentation comments. They are innocuous, so may as well support them.

Dependencies

None.

Testing

In a Swift package, define @Metadata, @DeprecationSummary, and @Comment directives in documentation comments and verify that the rendered output matches your expectations.

Also, author invalid uses of these directives to trigger the diagnostics described above.

Checklist

Added tests
Ran the ./bin/test script and it succeeded
Updated documentation if necessary

franklinsch · 2024-11-28T18:15:29Z

@swift-ci please test

grynspan · 2024-11-29T15:06:18Z

Woo! Do the directives apply recursively? For instance, if I have:

/// @Metadata {
///   @Available(Swift, introduced: 6.1)
/// }
struct S {
  var x
}

Will x be documented as introduced in Swift 6.1, or does it also need the directive?

franklinsch · 2024-11-29T15:13:33Z

Good question. As of this PR, the changes don't get applied recursively. You need to annotate every member. Longer term I think it would make sense for availability to be carried over to child members (like in Swift), but we'd want a way to support annotating an API as unavailable so that you can opt out of the inferred availability. Currently the Available directive only supports introduced and deprecated versions.

grynspan · 2024-11-29T15:18:02Z

I think for our own purposes (Swift Testing) we can just make sure all symbols are explicitly annotated, but let's make sure to track that enhancement somewhere as it's probably not a scalable solution long-term. 🤔

d-ronnqvist · 2024-12-02T15:22:26Z

Sources/SwiftDocC/Model/DocumentationNode.swift

+                        bundle: bundle,
+                        context: context,
+                        problems: &problems
+                    ) as! (Metadata?, [BlockDirective])


Why the need to force cast here?

It was because the method returned any Markup and we needed to force cast the second element of the tuple so that it can be assigned to docCommentDirectives. But with your suggestion of making the markupElements argument inout, this is no longer needed.

d-ronnqvist · 2024-12-02T15:23:03Z

Sources/SwiftDocC/Semantics/DirectiveParser.swift

+import Markdown
+
+/// A utlity type for parsing directives from markup.
+struct DirectiveParser<Directive: AutomaticDirectiveConvertible> {


minor: If this doesn't have any state, should it be an enum with static methods instead?

I typically prefer set these as structs in case we want to add configuration arguments to the initialiser, e.g., to customise the parsing behaviour. In this case it doesn't matter though, because the API is not public and we can switch it from an enum to a struct if/when we need to.

d-ronnqvist · 2024-12-02T15:27:06Z

Sources/SwiftDocC/Model/DocumentationNode.swift

+        bundle: DocumentationBundle,
+        context: DocumentationContext


Why are we adding the bundle and context parameters here?

We need to pass them all the way down to DirectiveParser because the Metadata initialiser needs it.

Franklin and I talked about this some more offline. Ideally the directives wouldn't need the full context because it creates a cyclic dependency. They really only need some mechanism to load data but changing that is way out of scope for this PR.

+1 to this. I think there should be a better way to do it without having to pass the full context and bundle. This does not look correct for initialising a symbol.

d-ronnqvist · 2024-12-02T15:29:52Z

Sources/SwiftDocC/Semantics/DirectiveParser.swift

+    /// and diagnoses subsequent instances.
+    func parseSingleDirective<MarkupElements: Sequence<Markup>>(
+        _ directiveType: Directive.Type,
+        from markupElements: MarkupElements,


Considering how this is called, how do you feel about making the markup inout instead of returning remaining markup?

That's a great suggestion, thanks! Initially I though we'd run into trouble because we sometimes call this function with [any Markup] and sometimes with [any BlockDirective] and I thought that'd be problematic with type erasure and any Markup not conforming to Markup, but we can actually just make this function take inout [any Markup], so I did that.

d-ronnqvist · 2024-12-02T15:30:58Z

Sources/SwiftDocC/Semantics/Metadata/Metadata.swift

+        in context: DocumentationContext,
+        problems: inout [Problem]
+    ) {
+        let invalidDirectives: [(any AutomaticDirectiveConvertible)?] = [


Would it be better to have a list of supported directives rather than the unsupported directives here? I imagine that that would need to be updated less frequently.

I went back and forth on this. If we were to specify the valid ones, we'd still need to somehow get the values of the invalid ones in order to produce a diagnostic. I couldn't find a good way to get an array of the valid ones easily. We could maybe use keyPaths.values and find a way to generically access the relevant properties, but that seems too complex.

I like the idea of having the valid ones, If not we will have to remember to add it to the list, which could be forgotten and lead to errors.

We'd need to list the valid ones as well anyway, unfortunately.

d-ronnqvist · 2024-12-02T15:36:36Z

Sources/SwiftDocC/Semantics/DirectiveParser.swift

+    func parseSingleDirective<MarkupElements: Sequence<Markup>>(
+        _ directiveType: Directive.Type,
+        from markupElements: MarkupElements,


FYI: You can express this more concisely using some

Suggested change

func parseSingleDirective<MarkupElements: Sequence<Markup>>(

_ directiveType: Directive.Type,

from markupElements: MarkupElements,

func parseSingleDirective(

_ directiveType: Directive.Type,

from markupElements: some Sequence<Markup>,

d-ronnqvist · 2024-12-02T15:40:10Z

Tests/SwiftDocCTests/Semantics/MetadataTests.swift

@@ -416,10 +420,7 @@ class MetadataTests: XCTestCase {
        let document = Document(parsing: source, options: [.parseBlockDirectives, .parseSymbolLinks])
        let (bundle, context) = try testBundleAndContext(named: "TestBundle")

-        var analyzer = SemanticAnalyzer(source: nil, context: context, bundle: bundle)
-        _ = analyzer.visit(document)


Why removing the analysis in this test and any problems raised from that?

We were parsing the document twice for some reason, and producing the diagnostics twice. I didn't see a reason for us needing to do that.

d-ronnqvist · 2024-12-02T15:53:05Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+                docComment
+                    .components(
+                        separatedBy: .newlines
+                    )
+                    .enumerated()
+                    .map { arg -> SymbolGraph.LineList.Line in
+                        var (
+                            index,
+                            line
+                        ) = arg


nit: I find this excessive use of newlines hard to read. AFAICT not based on width—because .map { arg -> SymbolGraph.LineList.Line in is wider than .components(separatedBy: .newlines)—so I'm assuming that it's a purely stylistic choice.

Suggested change

docComment

.components(

separatedBy: .newlines

)

.enumerated()

.map { arg -> SymbolGraph.LineList.Line in

var (

index,

line

) = arg

docComment

.components(separatedBy: .newlines)

.enumerated()

.map { index, line in

Yes, I agree. I had used Xcode's multi-line format here but it's a bit excessive here.

d-ronnqvist · 2024-12-02T15:53:42Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+        XCTAssert(problems.isEmpty)
+
+        XCTAssertEqual(
+            try XCTUnwrap(


FYI: This XCTWrap isn't necessary because you can compare an optional value with a non-optional value.

Ah that's right, thanks!

d-ronnqvist · 2024-12-02T15:58:09Z

Sources/docc/DocCDocumentation.docc/Reference Syntax/API Reference Syntax/Metadata.md

@@ -56,6 +56,17 @@ to add additional URLs for a page.
 > Note: Starting with version 6.0, @Redirected is supported as a child directive of @Metadata. In
 previous versions, @Redirected must be used as a top level directive.

+### Usage in documentation comments
+
+If you wish, you can specify `@Metadata` configuration in documentation comments for the following customizations:


I don't think we use "if you wish" in any other user-facing documentation

Suggested change

If you wish, you can specify `@Metadata` configuration in documentation comments for the following customizations:

You can specify the following `@Metadata` configuration in an in-source documentation comment:

Thanks, I agree. I prefer your more concise version.

franklinsch · 2024-12-03T12:06:13Z

@swift-ci please test

d-ronnqvist · 2024-12-04T15:51:26Z

Sources/SwiftDocC/Semantics/Symbol/DeprecationSummary.swift

@@ -24,7 +24,7 @@ import Markdown
 /// }
 /// ```
 ///
-/// You can use the `@DeprecationSummary` directive top-level in both articles and documentation extension files.
+/// You can use the `@DeprecationSummary` directive top-level in articles, documentation extension files, or documentation comments.


FYI: you need to run swift run generate-symbol-graph for this change to be reflected in the published documentation.

Ah yes, thanks. I updated this in the latest commit.

It would also be good to add an "Earlier Versions" aside, like on the @Available documentation to say that before Swift-DocC 6.2 (or 6.1 if we plan on cherry-picking this), @DeprecationSummary isn't supported in documentation comments.

Good idea. Updated, thanks! I also realised that CustomMetadata is actually intentionally hidden from documentation (although I'm not sure why), so I removed its reference in this document.

sofiaromorales · 2024-12-05T10:26:43Z

Sources/SwiftDocC/Model/DocumentationNode.swift

+        bundle: DocumentationBundle,
+        context: DocumentationContext


+1 to this. I think there should be a better way to do it without having to pass the full context and bundle. This does not look correct for initialising a symbol.

sofiaromorales · 2024-12-05T10:57:04Z

Sources/SwiftDocC/Semantics/Metadata/Metadata.swift

+        in context: DocumentationContext,
+        problems: inout [Problem]
+    ) {
+        let invalidDirectives: [(any AutomaticDirectiveConvertible)?] = [


I like the idea of having the valid ones, If not we will have to remember to add it to the list, which could be forgotten and lead to errors.

sofiaromorales · 2024-12-05T10:57:34Z

Sources/SwiftDocC/Semantics/Metadata/Metadata.swift

+            titleHeading,
+        ] + (redirects ?? [])
+          + supportedLanguages
+          + pageImages


why the change on code style here?

Can you clarify what you mean here? This is all new code

d-ronnqvist · 2024-12-09T15:58:57Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

@@ -492,7 +492,7 @@ class SymbolTests: XCTestCase {
        XCTAssertEqual(withRedirectInArticle.redirects, nil)

        XCTAssertEqual(problems.first?.diagnostic.identifier, "org.swift.docc.UnsupportedDocCommentDirective")
-        XCTAssertEqual(problems.first?.diagnostic.range?.lowerBound.line, 3)
+        XCTAssertEqual(problems.first?.diagnostic.range?.lowerBound.line, 15)


This test seems like it's finding another directive in some other symbol. Please update the test to

don't build more content that it needs to

warn about some unsupported directive that's created in this test.

It's referring to the @Redirected directive in the test, but I can see how that's confusing. I had made makeDocumentationNodeForSymbol add an arbitrary offset of 12 to the doc comment's line numbers, but I changed it so that it takes the value as a parameter instead for the specific tests I wrote.

d-ronnqvist · 2024-12-09T16:01:06Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

-                )
-                return .init(text: line, range: range)
-            })
+            let newDocComment = SymbolGraph.LineList(


Is it possible to use makeLineList(docComment:startOffset:url:) here instead?

Yes, thanks!

d-ronnqvist · 2024-12-09T16:08:08Z

Sources/docc/DocCDocumentation.docc/Reference Syntax/API Reference Syntax/Metadata.md

+> Note: Don't specify an `@Metadata` directive in both the symbol's documentation comment and its documentation extension file.
+If you have one in each, DocC will pick the one in the documentation extension and discard the one in the documentation
+comment.
+


It would be good to add an "Earlier Versions" aside, like on the @Available documentation to say that before Swift-DocC 6.2 (or 6.1 if we plan on cherry-picking this), @metadata isn't supported in documentation comments.

d-ronnqvist

LGTM

Adds support for authoring `@Metadata` directives in documentation comments, for convenience. For some children of `@Metadata`, like `@Available`, it's a bit inconvenient to need to create a documentation extension just to specify availability for a symbol. This commit allows you to do that in documentation comments instead. A few notes on the behavior: - Only the `@Available` and `@CustomMetadata` are supported as child directives of `@Metadata` in documentation comments. DocC ignores other directives and emits a warnings for their uses. - If you author a `@Metadata` directive both in the symbol's documentation comment and documentation extension file, DocC will pick the one in the documentation extension, and ignore the one in the documentation comment and emit a warning. On the implementation: - While `AutomaticDirectiveConvertible` provides nice utilities for parsing arguments and child directives, its benefits can't be leveraged when the directive you're parsing in a top-level document, e.g., an article or documentation comment. This commits adds a `DirectiveParser` utility type which takes a list of Markup elements and parses the first one that matches the directive type you're looking for. - Emitting a diagnostic for a directive in a documentation comment at the right source location is complicated. The codebase already has utilities to offset a diagnostic's source range which makes this computation easier, and this commit adds non-mutating variants of those APIs. rdar://138395778

Adds support for authoring `@DeprecationSummary` messages in documentation comments. This is useful when you want to provide a formatted documentation summary for an API that's deprecated, but don't want to create a documentation extension file in order to do just that. It turns out that DocC already supported this, but emitted a warning whenever you did so. This commit removes that warning.

Adds support for `@Comment` directives in documentation comments. They don't hurt anyone, so might as well allow them.

franklinsch · 2024-12-11T12:55:44Z

@swift-ci please test

…ts (swiftlang#1107)

…ts (#1107) (#1127)

franklinsch requested review from d-ronnqvist and sofiaromorales November 28, 2024 18:15

d-ronnqvist reviewed Dec 2, 2024

View reviewed changes

franklinsch mentioned this pull request Dec 3, 2024

Provide diagnostic solution for invalid metadata directives in documentation comments #1111

Open

franklinsch force-pushed the r138395778-metadata-in-doc-comments branch from 7a71608 to f10b1a4 Compare December 3, 2024 12:06

franklinsch requested a review from d-ronnqvist December 3, 2024 12:07

d-ronnqvist reviewed Dec 4, 2024

View reviewed changes

franklinsch force-pushed the r138395778-metadata-in-doc-comments branch from f10b1a4 to 4f75d2f Compare December 4, 2024 16:45

franklinsch requested a review from d-ronnqvist December 4, 2024 16:46

sofiaromorales reviewed Dec 5, 2024

View reviewed changes

franklinsch requested a review from sofiaromorales December 6, 2024 16:06

franklinsch force-pushed the r138395778-metadata-in-doc-comments branch from 4f75d2f to 8b4a0ea Compare December 6, 2024 16:09

d-ronnqvist reviewed Dec 9, 2024

View reviewed changes

grynspan mentioned this pull request Dec 9, 2024

Fix DocC warnings and errors, including those from SPI swiftlang/swift-testing#836

Merged

2 tasks

franklinsch force-pushed the r138395778-metadata-in-doc-comments branch from 8b4a0ea to 3511925 Compare December 10, 2024 09:46

franklinsch requested a review from d-ronnqvist December 10, 2024 09:54

d-ronnqvist approved these changes Dec 10, 2024

View reviewed changes

sofiaromorales approved these changes Dec 10, 2024

View reviewed changes

franklinsch force-pushed the r138395778-metadata-in-doc-comments branch from 3bf6dd7 to 0fc804c Compare December 11, 2024 12:51

franklinsch added 3 commits December 11, 2024 12:53

Add support for @Comment in doc comments

a5c523e

Adds support for `@Comment` directives in documentation comments. They don't hurt anyone, so might as well allow them.

franklinsch force-pushed the r138395778-metadata-in-doc-comments branch from 0fc804c to a5c523e Compare December 11, 2024 12:55

franklinsch merged commit 62fb5d7 into swiftlang:main Dec 11, 2024
2 checks passed

franklinsch deleted the r138395778-metadata-in-doc-comments branch December 11, 2024 13:01

franklinsch added a commit to franklinsch/swift-docc that referenced this pull request Dec 11, 2024

Support @Metadata and @DeprecationSummary in documentation commen…

e785c62

…ts (swiftlang#1107)

franklinsch mentioned this pull request Dec 11, 2024

Support @Metadata and @DeprecationSummary in documentation commen… #1127

Merged

franklinsch added a commit that referenced this pull request Dec 11, 2024

Support @Metadata and @DeprecationSummary in documentation commen…

c23571a

…ts (#1107) (#1127)

	If you wish, you can specify `@Metadata` configuration in documentation comments for the following customizations:
	You can specify the following `@Metadata` configuration in an in-source documentation comment:

Support @Metadata and @DeprecationSummary in documentation comments #1107

Support @Metadata and @DeprecationSummary in documentation comments #1107

Conversation

franklinsch commented Nov 28, 2024

Summary

Dependencies

Testing

Checklist

franklinsch commented Nov 28, 2024

grynspan commented Nov 29, 2024

franklinsch commented Nov 29, 2024

grynspan commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franklinsch commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-ronnqvist left a comment

Choose a reason for hiding this comment

franklinsch commented Dec 11, 2024

Support `@Metadata` and `@DeprecationSummary` in documentation comments #1107

Support `@Metadata` and `@DeprecationSummary` in documentation comments #1107