Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support @Metadata and @DeprecationSummary in documentation comments #1107

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ public extension Diagnostic {
mutating func offsetWithRange(_ docRange: SymbolGraph.LineList.SourceRange) {
// If there is no location information in the source diagnostic, the diagnostic might be removed for safety reasons.
range?.offsetWithRange(docRange)

}

/// Returns the diagnostic with its range offset by the given documentation comment range.
func withRangeOffset(by docRange: SymbolGraph.LineList.SourceRange) -> Self {
var diagnostic = self
diagnostic.range?.offsetWithRange(docRange)
return diagnostic
}
}
7 changes: 7 additions & 0 deletions Sources/SwiftDocC/Infrastructure/Diagnostics/Problem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ extension Problem {
}
}
}

/// Returns the diagnostic with its range offset by the given documentation comment range.
func withRangeOffset(by docRange: SymbolGraph.LineList.SourceRange) -> Self {
var problem = self
problem.offsetWithRange(docRange)
return problem
}
}

extension Sequence<Problem> {
Expand Down
16 changes: 13 additions & 3 deletions Sources/SwiftDocC/Infrastructure/DocumentationContext.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1048,15 +1048,21 @@ public class DocumentationContext {
/// A lookup of resolved references based on the reference's absolute string.
private(set) var referenceIndex = [String: ResolvedTopicReference]()

private func nodeWithInitializedContent(reference: ResolvedTopicReference, match foundDocumentationExtension: DocumentationContext.SemanticResult<Article>?) -> DocumentationNode {
private func nodeWithInitializedContent(
reference: ResolvedTopicReference,
match foundDocumentationExtension: DocumentationContext.SemanticResult<Article>?,
bundle: DocumentationBundle
) -> DocumentationNode {
guard var updatedNode = documentationCache[reference] else {
fatalError("A topic reference that has already been resolved should always exist in the cache.")
}

// Pull a matched article out of the cache and attach content to the symbol
updatedNode.initializeSymbolContent(
documentationExtension: foundDocumentationExtension?.value,
engine: diagnosticEngine
engine: diagnosticEngine,
bundle: bundle,
context: self
)

// After merging the documentation extension into the symbol, warn about deprecation summary for non-deprecated symbols.
Expand Down Expand Up @@ -1399,7 +1405,11 @@ public class DocumentationContext {
Array(documentationCache.symbolReferences).concurrentMap { finalReference in
// Match the symbol's documentation extension and initialize the node content.
let match = uncuratedDocumentationExtensions[finalReference]
let updatedNode = nodeWithInitializedContent(reference: finalReference, match: match)
let updatedNode = nodeWithInitializedContent(
reference: finalReference,
match: match,
bundle: bundle
)

return ((
node: updatedNode,
Expand Down
97 changes: 87 additions & 10 deletions Sources/SwiftDocC/Model/DocumentationNode.swift
Original file line number Diff line number Diff line change
Expand Up @@ -335,12 +335,19 @@ public struct DocumentationNode {
/// - Parameters:
/// - article: An optional documentation extension article.
/// - engine: A diagnostics engine.
mutating func initializeSymbolContent(documentationExtension: Article?, engine: DiagnosticEngine) {
mutating func initializeSymbolContent(
documentationExtension: Article?,
engine: DiagnosticEngine,
bundle: DocumentationBundle,
context: DocumentationContext
Comment on lines +341 to +342
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding the bundle and context parameters here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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.

) {
precondition(unifiedSymbol != nil && symbol != nil, "You can only call initializeSymbolContent() on a symbol node.")

let (markup, docChunks) = Self.contentFrom(
let (markup, docChunks, metadataFromDocumentationComment) = Self.contentFrom(
documentedSymbol: unifiedSymbol?.documentedSymbol,
documentationExtension: documentationExtension,
bundle: bundle,
context: context,
engine: engine
)

Expand Down Expand Up @@ -469,7 +476,27 @@ public struct DocumentationNode {
}

options = documentationExtension?.options[.local]
self.metadata = documentationExtension?.metadata

if documentationExtension?.metadata != nil && metadataFromDocumentationComment != nil {
var problem = Problem(
diagnostic: Diagnostic(
source: unifiedSymbol?.documentedSymbol?.docComment?.url,
severity: .warning,
range: metadataFromDocumentationComment?.originalMarkup.range,
identifier: "org.swift.docc.DuplicateMetadata",
summary: "Redeclaration of '@Metadata' for this symbol; this directive will be skipped",
explanation: "A '@Metadata' directive is already declared in this symbol's documentation extension file"
)
)

if let range = unifiedSymbol?.documentedSymbol?.docComment?.lines.first?.range {
problem.offsetWithRange(range)
}

engine.emit(problem)
}

self.metadata = documentationExtension?.metadata ?? metadataFromDocumentationComment

updateAnchorSections()
}
Expand All @@ -483,11 +510,19 @@ public struct DocumentationNode {
static func contentFrom(
documentedSymbol: SymbolGraph.Symbol?,
documentationExtension: Article?,
bundle: DocumentationBundle? = nil,
context: DocumentationContext? = nil,
engine: DiagnosticEngine
) -> (markup: Markup, docChunks: [DocumentationChunk]) {
) -> (
markup: Markup,
docChunks: [DocumentationChunk],
metadata: Metadata?
) {
let markup: Markup
var documentationChunks: [DocumentationChunk]

var metadata: Metadata?

// We should ignore the symbol's documentation comment if it wasn't provided
// or if the documentation extension was set to override.
let ignoreDocComment = documentedSymbol?.docComment == nil
Expand All @@ -512,7 +547,36 @@ public struct DocumentationNode {
let docCommentMarkup = Document(parsing: docCommentString, source: docCommentLocation?.url, options: documentOptions)
let offset = symbol.docComment?.lines.first?.range

let docCommentDirectives = docCommentMarkup.children.compactMap({ $0 as? BlockDirective })
var docCommentMarkupElements = Array(docCommentMarkup.children)

var problems = [Problem]()

if let bundle, let context {
metadata = DirectiveParser()
.parseSingleDirective(
Metadata.self,
from: &docCommentMarkupElements,
parentType: Symbol.self,
source: docCommentLocation?.url,
bundle: bundle,
context: context,
problems: &problems
)

metadata?.validateForUseInDocumentationComment(
symbolSource: symbol.docComment?.url,
problems: &problems
)
}

if let offset {
problems = problems.map { $0.withRangeOffset(by: offset) }
}

engine.emit(problems)

let docCommentDirectives = docCommentMarkupElements.compactMap { $0 as? BlockDirective }

if !docCommentDirectives.isEmpty {
let location = symbol.mixins.getValueIfPresent(
for: SymbolGraph.Symbol.Location.self
Expand All @@ -529,9 +593,7 @@ public struct DocumentationNode {
continue
}

// Renderable directives are processed like any other piece of structured markdown (tables, lists, etc.)
// and so are inherently supported in doc comments.
guard DirectiveIndex.shared.renderableDirectives[directive.name] == nil else {
guard !directive.isSupportedInDocumentationComment else {
continue
}

Expand Down Expand Up @@ -579,7 +641,7 @@ public struct DocumentationNode {
documentationChunks = [DocumentationChunk(source: .sourceCode(location: nil, offset: nil), markup: markup)]
}

return (markup: markup, docChunks: documentationChunks)
return (markup: markup, docChunks: documentationChunks, metadata: metadata)
}

/// Returns a documentation node kind for the given symbol kind.
Expand Down Expand Up @@ -667,7 +729,7 @@ public struct DocumentationNode {
// Prefer content sections coming from an article (documentation extension file)
var deprecated: DeprecatedSection?

let (markup, docChunks) = Self.contentFrom(documentedSymbol: symbol, documentationExtension: article, engine: engine)
let (markup, docChunks, _) = Self.contentFrom(documentedSymbol: symbol, documentationExtension: article, engine: engine)
self.markup = markup
self.docChunks = docChunks

Expand Down Expand Up @@ -784,3 +846,18 @@ public struct DocumentationNode {
/// These tags contain information about the symbol's return values, potential errors, and parameters.
public var tags: Tags = (returns: [], throws: [], parameters: [])
}

private let directivesSupportedInDocumentationComments = [
Comment.directiveName,
Metadata.directiveName,
DeprecationSummary.directiveName,
]
// Renderable directives are processed like any other piece of structured markdown (tables, lists, etc.)
// and so are inherently supported in doc comments.
+ DirectiveIndex.shared.renderableDirectives.keys

private extension BlockDirective {
var isSupportedInDocumentationComment: Bool {
directivesSupportedInDocumentationComments.contains(name)
}
}
23 changes: 10 additions & 13 deletions Sources/SwiftDocC/Semantics/Article/Article.swift
Original file line number Diff line number Diff line change
Expand Up @@ -129,19 +129,16 @@ public final class Article: Semantic, MarkupConvertible, Abstracted, Redirected,
return Redirect(from: childDirective, source: source, for: bundle, in: context, problems: &problems)
}

let metadata: [Metadata]
(metadata, remainder) = remainder.categorize { child -> Metadata? in
guard let childDirective = child as? BlockDirective, childDirective.name == Metadata.directiveName else {
return nil
}
return Metadata(from: childDirective, source: source, for: bundle, in: context)
}

for extraMetadata in metadata.dropFirst() {
problems.append(Problem(diagnostic: Diagnostic(source: source, severity: .warning, range: extraMetadata.originalMarkup.range, identifier: "org.swift.docc.HasAtMostOne<\(Article.self), \(Metadata.self)>.DuplicateChildren", summary: "Duplicate \(Metadata.directiveName.singleQuoted) child directive", explanation: nil, notes: []), possibleSolutions: []))
}

var optionalMetadata = metadata.first
var optionalMetadata = DirectiveParser()
.parseSingleDirective(
Metadata.self,
from: &remainder,
parentType: Article.self,
source: source,
bundle: bundle,
context: context,
problems: &problems
)

// Append any redirects found in the metadata to the redirects
// found in the main content.
Expand Down
68 changes: 68 additions & 0 deletions Sources/SwiftDocC/Semantics/DirectiveParser.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
This source file is part of the Swift.org open source project

Copyright (c) 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
See https://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import Foundation
import Markdown

/// A utlity type for parsing directives from markup.
struct DirectiveParser<Directive: AutomaticDirectiveConvertible> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// Returns a directive of the given type if found in the given sequence of markup elements and the remaining markup.
///
/// If there are multiple instances of the same directive type, this functions returns the first instance
/// and diagnoses subsequent instances.
func parseSingleDirective(
_ directiveType: Directive.Type,
from markupElements: inout [any Markup],
parentType: Semantic.Type,
source: URL?,
bundle: DocumentationBundle,
context: DocumentationContext,
problems: inout [Problem]
) -> Directive? {
let (directiveElements, remainder) = markupElements.categorize { markup -> Directive? in
guard let childDirective = markup as? BlockDirective,
childDirective.name == Directive.directiveName
else {
return nil
}
return Directive(
from: childDirective,
source: source,
for: bundle,
in: context,
problems: &problems
)
}

let directive = directiveElements.first

for extraDirective in directiveElements.dropFirst() {
problems.append(
Problem(
diagnostic: Diagnostic(
source: source,
severity: .warning,
range: extraDirective.originalMarkup.range,
identifier: "org.swift.docc.HasAtMostOne<\(parentType), \(Directive.self)>.DuplicateChildren",
summary: "Duplicate \(Metadata.directiveName.singleQuoted) child directive",
explanation: nil,
notes: []
),
possibleSolutions: []
)
)
}

markupElements = remainder

return directive
}
}
49 changes: 48 additions & 1 deletion Sources/SwiftDocC/Semantics/Metadata/Metadata.swift
Original file line number Diff line number Diff line change
Expand Up @@ -192,5 +192,52 @@ public final class Metadata: Semantic, AutomaticDirectiveConvertible {

return true
}

/// Validates the use of this Metadata directive in a documentation comment.
///
/// Some configuration options of Metadata are invalid in documentation comments. This function
/// emits warnings for illegal uses and sets their values to `nil`.
func validateForUseInDocumentationComment(
symbolSource: URL?,
problems: inout [Problem]
) {
let invalidDirectives: [(any AutomaticDirectiveConvertible)?] = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

documentationOptions,
technologyRoot,
displayName,
callToAction,
pageKind,
_pageColor,
titleHeading,
] + (redirects ?? [])
+ supportedLanguages
+ pageImages
Comment on lines +211 to +214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the change on code style here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


let namesAndRanges = invalidDirectives
.compactMap { $0 }
.map { (type(of: $0).directiveName, $0.originalMarkup.range) }
Comment on lines +217 to +218
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very minor: This is needlessly iterating over the elements twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted for iterating twice just to make the code a bit nicer so that we don't have to unwrap $0 in the map. Although this could be written as:

.map { invalidDirective in invalidDirective.map { (type(of: $0).directiveName, $0.originalMarkup.range) } }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was more thinking that they could be both done in the compactMap:

Suggested change
.compactMap { $0 }
.map { (type(of: $0).directiveName, $0.originalMarkup.range) }
.compactMap { directive in
guard let directive else { return nil }
return (type(of: directive).directiveName, directive.originalMarkup.range)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I meant to use compactMap above. The code is equivalent, but the compactMap then map IMO is more readable/fluent but I don't have a strong opinion.


problems.append(
contentsOf: namesAndRanges.map { (name, range) in
Problem(
diagnostic: Diagnostic(
source: symbolSource,
severity: .warning,
range: range,
identifier: "org.swift.docc.\(Metadata.directiveName).Invalid\(name)InDocumentationComment",
summary: "Invalid use of \(name.singleQuoted) directive in documentation comment; configuration will be ignored",
explanation: "Specify this configuration in a documentation extension file"

// TODO: It would be nice to offer a solution here that removes the directive for you (#1111, rdar://140846407)
)
)
}
)

documentationOptions = nil
technologyRoot = nil
displayName = nil
pageKind = nil
_pageColor = nil
}
}

4 changes: 3 additions & 1 deletion Sources/SwiftDocC/Semantics/Symbol/DeprecationSummary.swift
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// > Earlier versions: Before Swift-DocC 6.1, `@DeprecationSummary` was not supported in documentation comments.
///
/// > Tip:
/// > If you are writing a custom deprecation summary message for an API or documentation page that isn't already deprecated,
Expand Down
Loading