Centralize directive parsing and declaration logic #362

ethan-kusters · 2022-08-14T17:37:55Z

Bug/issue #: rdar://99065717

Summary

Introduces a new AutomaticDirectiveConvertible protocol along with
several property wrappers to allow for declaring new markup directives
with built-in parsing and diagnostics. The general architecture is based
on ArgumentParser.

Here’s an example directive:

public final class Intro: Semantic, AutomaticDirectiveConvertible {
    public let originalMarkup: BlockDirective
    
    /// The title of the containing ``Tutorial``.
    @DirectiveArgumentWrapped
    public private(set) var title: String
    
    /// An optional video, displayed as a modal.
    @ChildDirective
    public private(set) var video: VideoMedia? = nil
    
    /// An optional standout image.
    @ChildDirective
    public private(set) var image: ImageMedia? = nil
    
    /// The child markup content.
    @ChildMarkup(numberOfParagraphs: .zeroOrMore)
    public private(set) var content: MarkupContainer
    
    static var keyPaths: [String : AnyKeyPath] = [
        "title"     : \Intro._title,
	"video"     : \Intro._video,
	"image"     : \Intro._image,
	"content"   : \Intro._content,
    ]
    
    @available(*, deprecated, message: "Do not call directly. Required for 'AutomaticDirectiveConvertible'.")
    init(originalMarkup: BlockDirective) {
	self.originalMarkup = originalMarkup
    }
}

This has some immediate benefits of reducing code duplication but
longer term the main goal is to create a single source of truth for
what Directives are allowed where and what arguments they expect.

The next step here will be to expose this information to DocC’s own
conceptual documentation so that directive documentation can be
auto-generated based on this metadata. Longer term, this information
can be used to power other integrations like code completion strategies.

Testing

The existing fairly extensive directive tests for each individually converted directive continue to pass. I've added some addition tests focused on the directive mirror as well.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

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

ethan-kusters · 2022-08-14T17:38:49Z

@swift-ci please test

ethan-kusters · 2022-08-14T18:02:25Z

I opened a PR on my fork to show how this new infrastructure would be used to create the new directives described here: https://forums.swift.org/t/supporting-more-dynamic-content-in-swift-docc-reference-documentation/59527.

Create new directives for dynamic conceptual docs ethan-kusters/swift-docc#1

@available

Introduces a new `AutomaticDirectiveConvertible` protocol along with several property wrappers to allow for declaring new markup directives with built-in parsing and diagnostics. The general architecture is based on `ArgumentParser`. Here’s an example directive: public final class Intro: Semantic, AutomaticDirectiveConvertible { public let originalMarkup: BlockDirective /// The title of the containing ``Tutorial``. @DirectiveArgumentWrapped public private(set) var title: String /// An optional video, displayed as a modal. @ChildDirective public private(set) var video: VideoMedia? = nil /// An optional standout image. @ChildDirective public private(set) var image: ImageMedia? = nil /// The child markup content. @ChildMarkup(numberOfParagraphs: .zeroOrMore) public private(set) var content: MarkupContainer static var keyPaths: [String : AnyKeyPath] = [ "title" : \Intro._title, "video" : \Intro._video, "image" : \Intro._image, "content" : \Intro._content ] init(originalMarkup: BlockDirective, title: String, image: ImageMedia?, video: VideoMedia?, content: MarkupContainer) { self.originalMarkup = originalMarkup super.init() self.content = content self.title = title self.image = image self.video = video } @available(*, deprecated, message: "Do not call directly. Required for 'AutomaticDirectiveConvertible'.") init(originalMarkup: BlockDirective) { self.originalMarkup = originalMarkup } } This has some immediate benefits of reducing code duplication but longer term the main goal is to create a single source of truth for what Directives are allowed where and what arguments they expect. The next step here will be to expose this information to DocC’s own conceptual documentation so that directive documentation can be auto-generated based on this metadata. Longer term, this information can be used to power other integrations like code completion strategies. Resolves rdar://99065717

ethan-kusters · 2022-08-24T04:05:37Z

@swift-ci please test

franklinsch

Nice work, this cleans things up significantly! It would be good to have an article that describes at a high-level what needs to be done to add a new directive to DocC, either as part of this PR or a future one.

franklinsch · 2022-08-24T09:50:14Z

Sources/SwiftDocC/Semantics/DirectiveInfrastructure/AutomaticDirectiveConvertible.swift

+        precondition(directive.name == Self.directiveName)
+        self.init(originalMarkup: directive)
+
+        let reflectedDirective = DirectiveIndex.shared.reflection(of: type(of: self))


Did you find any performance impact in using Mirror?

I haven't run performance tests yet, but I don't expect any because of the DirectiveIndex that caches each mirror creation so we only need to make one per directive.

Seeing a slight performance hit but nothing too significant. My guess is this is caused by the lock on the DirectiveIndex. I'm planning on making the DirectiveIndex read-only in a future PR (the one that adds docs support) to remove the need for the lock so I'd prefer to land this as-is and make sure we address any performance hit then.

$ swift run make-test-bundle --output ~/Downloads/test-bundle.docc --sizeFactor 10 $ swift run --package-path bin/benchmark benchmark measure-commits main 473e21a82e14f475f5c20e4d1aa87ae85619b4d8 --repetitions 5 --docc-arguments convert ~/Downloads/test-bundle.docc/TestFramework/Docs.docc $ swift run --package-path bin/benchmark benchmark diff benchmark-main.json benchmark-473e21a82e14f475f5c20e4d1aa87ae85619b4d8.json ┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ Metric │ Change │ main │ 473e21a82e14f475f5c20e4d1aa87ae85619b4d8 │ ├──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ Duration for 'bundle-registration' │ no change¹,² │ 4.056 sec │ 4.141 sec │ │ Duration for 'convert-total-time' │ +2.321%³,⁴ │ 5.738 sec │ 5.871 sec │ │ Duration for 'documentation-processing' │ no change⁵ │ 1.516 sec │ 1.563 sec │ │ Duration for 'finalize-navigation-index' │ no change⁶ │ 0.089 sec │ 0.088 sec │ │ Peak memory footprint │ no change⁷,⁸ │ 428.9 MB │ 432.6 MB │ │ Data subdirectory size │ no change │ 111.7 MB │ 111.7 MB │ │ Index subdirectory size │ no change │ 994 KB │ 994 KB │ │ Total DocC archive size │ no change │ 117.8 MB │ 117.8 MB │ │ Topic Anchor Checksum │ no change │ c8f03d4e81da8e3b6d7f │ c8f03d4e81da8e3b6d7fb7f63c771147 │ │ Topic Graph Checksum │ no change │ edce22bce4f8de475a5e │ edce22bce4f8de475a5e58a2e45dcdc0 │ └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

Ok, sounds good. Should we consider landing the performance fix as part of this PR though, otherwise we'll incur a regression?

franklinsch · 2022-08-24T09:52:14Z

Sources/SwiftDocC/Semantics/DirectiveInfrastructure/ChildDirectiveWrapper.swift

+    }
+}
+
+protocol OptionallyWrappedCollectionWrappedDirectiveConvertible: OptionallyWrapped {}


franklinsch · 2022-08-24T10:40:26Z

Sources/SwiftDocC/Semantics/DirectiveInfrastructure/AutomaticDirectiveConvertible.swift

+        problems: inout [Problem]
+    ) -> Bool
+
+    static var keyPaths: [String : AnyKeyPath] { get }


I would be good to document why this is required

franklinsch · 2022-08-24T10:41:10Z

Sources/SwiftDocC/Semantics/DirectiveInfrastructure/DirectiveArgumentWrapper.swift

+}
+
+@propertyWrapper
+public struct DirectiveArgumentWrapped<Value>: _DirectiveArgumentProtocol {


Does this API and the ones below need to be public? If so, we're missing documentation for them.

They unfortunately do because you can't attach an internal property wrapper to a public property. This would actually be a good use case for the no documentation attribute @QuietMisdreavus is working on!

Curious on your thoughts here- I don't really think these should be considered truly public API but it also makes them much less user-friendly in the library itself if they're all underscored. Any suggestions? I suppose documenting them as-such is a reasonable option. 😃

Hmm I see, so you want the projected value of the property wrapper to be public, but not the property itself? It feels like we should just document what this API does for clients, with a note that we're not expecting them to use it.

ethan-kusters · 2022-08-24T15:33:42Z

Nice work, this cleans things up significantly! It would be good to have an article that describes at a high-level what needs to be done to add a new directive to DocC, either as part of this PR or a future one.

Agreed! Planning on adding that as part of a future PR that will also add auto-generated user-facing docs for any defined directives.

franklinsch · 2022-08-24T17:52:01Z

Planning on adding that as part of a future PR that will also add auto-generated user-facing docs for any defined directives.

👀👀👀👀👀 that sounds really neat!

ethan-kusters · 2022-08-25T05:25:20Z

@swift-ci please test

I missed your comments regarding the performance regression: #362 (comment), which should be addressed before merging the PR.

ethan-kusters · 2022-08-26T00:35:33Z

77260cf looks to have resolved the regression.

$ swift run --package-path bin/make-test-bundle  make-test-bundle --output ~/Downloads/test-bundle.docc --sizeFactor 10
$ swift run --package-path bin/benchmark benchmark measure-commits origin/main 77260cf50803d5bfc59213924a5201aef2963428 --repetitions 5 --docc-arguments convert ~/Downloads/test-bundle.docc/TestFramework/Docs.docc
$ swift run --package-path bin/benchmark benchmark diff benchmark-origin/main.json benchmark-77260cf50803d5bfc59213924a5201aef2963428.json

┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Metric                                   │ Change          │ main                 │ 77260cf50803d5bfc59213924a5201aef2963428 │
├──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Duration for 'bundle-registration'       │ no change¹,²    │ 3.555 sec            │ 3.532 sec                                │
│ Duration for 'convert-total-time'        │ no change³,⁴    │ 4.644 sec            │ 4.637 sec                                │
│ Duration for 'documentation-processing'  │ no change⁵,⁶    │ 0.997 sec            │ 1.015 sec                                │
│ Duration for 'finalize-navigation-index' │ -1.291%⁷,⁸      │ 0.045 sec            │ 0.045 sec                                │
│ Peak memory footprint                    │ no change⁹,¹⁰   │ 434.3 MB             │ 430.8 MB                                 │
│ Data subdirectory size                   │ no change       │ 111.7 MB             │ 111.7 MB                                 │
│ Index subdirectory size                  │ no change       │ 994 KB               │ 994 KB                                   │
│ Total DocC archive size                  │ no change       │ 117.8 MB             │ 117.8 MB                                 │
│ Topic Anchor Checksum                    │ no change       │ c8f03d4e81da8e3b6d7f │ c8f03d4e81da8e3b6d7fb7f63c771147         │
│ Topic Graph Checksum                     │ no change       │ edce22bce4f8de475a5e │ edce22bce4f8de475a5e58a2e45dcdc0         │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

ethan-kusters · 2022-08-26T00:35:37Z

@swift-ci please test

franklinsch

🎉 Super excited about this!

ethan-kusters · 2022-08-26T22:10:22Z

@swift-ci please test

ethan-kusters requested review from d-ronnqvist, QuietMisdreavus and franklinsch August 14, 2022 17:37

ethan-kusters force-pushed the directive-updates branch from 2a24e61 to 925cf4e Compare August 14, 2022 17:38

ethan-kusters force-pushed the directive-updates branch from 925cf4e to f6395ab Compare August 24, 2022 04:04

ethan-kusters force-pushed the directive-updates branch from f6395ab to 473e21a Compare August 24, 2022 04:05

franklinsch reviewed Aug 24, 2022

View reviewed changes

ethan-kusters added 2 commits August 24, 2022 22:17

Clean up documentation for public API

3c68109

Document automatic directive convertible

1592b8d

franklinsch previously approved these changes Aug 25, 2022

View reviewed changes

Pre-populate directive index to remove need for lock

77260cf

franklinsch approved these changes Aug 26, 2022

View reviewed changes

Merge branch 'main' into directive-updates

e4fb80f

ethan-kusters merged commit d991842 into swiftlang:main Aug 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize directive parsing and declaration logic #362

Centralize directive parsing and declaration logic #362

ethan-kusters commented Aug 14, 2022 •

edited

Loading

ethan-kusters commented Aug 14, 2022

ethan-kusters commented Aug 14, 2022

ethan-kusters commented Aug 24, 2022

franklinsch left a comment

franklinsch Aug 24, 2022

ethan-kusters Aug 24, 2022

ethan-kusters Aug 24, 2022

franklinsch Aug 25, 2022

franklinsch Aug 24, 2022

franklinsch Aug 24, 2022

franklinsch Aug 24, 2022

ethan-kusters Aug 24, 2022

franklinsch Aug 24, 2022

ethan-kusters commented Aug 24, 2022

franklinsch commented Aug 24, 2022

ethan-kusters commented Aug 25, 2022

ethan-kusters commented Aug 26, 2022

ethan-kusters commented Aug 26, 2022

franklinsch left a comment

ethan-kusters commented Aug 26, 2022

Centralize directive parsing and declaration logic #362

Centralize directive parsing and declaration logic #362

Conversation

ethan-kusters commented Aug 14, 2022 • edited Loading

Summary

Testing

Checklist

ethan-kusters commented Aug 14, 2022

ethan-kusters commented Aug 14, 2022

ethan-kusters commented Aug 24, 2022

franklinsch left a 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

ethan-kusters commented Aug 24, 2022

franklinsch commented Aug 24, 2022

ethan-kusters commented Aug 25, 2022

ethan-kusters commented Aug 26, 2022

ethan-kusters commented Aug 26, 2022

franklinsch left a comment

Choose a reason for hiding this comment

ethan-kusters commented Aug 26, 2022

ethan-kusters commented Aug 14, 2022 •

edited

Loading