Update DocumentationContext initializer to be async #1244
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug/issue #, if applicable:
Summary
Today we use GCD in
DocumentationContext.register(_:)
to concurrently load articles, decode json, and do other work.Because
DispatchGroup.wait()
waits synchronously,register(_:)
can also be a synchronous call.This is a 200 line change with 2000 lines of unavoidable modifications in tests.
Assuming that we want to start using Swift Concurrency in this code someday, we need to make this an asynchronous call instead.
We can mark an API as
async
without making any awaiting calls in its implementation but all callers need toawait
when calling this API (and the callers needs to be markedasync
which means that their callers need toawait
etc.). See also What Color is Your Function?.Because
DocumentationContext.register(_:)
is used by the deprecated publicDocumentationContext.init(dataProvider:diagnosticEngine:configuration:)
initializer, we would need to make that initializerasync
if we maderegister(_:)
asynchronous but that would be a source breaking change because callers who depend on Swift DocC would need to add anawait
keyword in their code (and potentially mark their calling codeasync
as well).However, we can mark the new
DocumentationContext.init(bundle:dataProvider:diagnosticEngine:configuration:)
initializer as asynchronous without breaking library consumers because it is still only accessible within the package.Why do this now?
After 6.2 is released we will remove the deprecated
DocumentationContext
initializer.However, we don't have any other API for library consumers to create a documentation context, so we need to add some public replacement.
If we promote the new initializer from package access to public access while it is still a synchronous call, we add another synchronous caller to
register(_:)
and therefore delay when we can start adopting Swift Concurrency by at least another release cycle (because we'd need to deprecate the new initializer, and introduce an asynchronous replacement, and wait until we can remove the new deprecated initializer before we can make anything it calls asynchronous).However, if we make the new public replacement API for creating documentation contexts asynchronous from the start, we have nothing technically preventing us from adopting Swift Concurrency in this code after 6.2 is released.
What "shape" should this new API have?
It's not really important how this new API for creating documentation context "looks". For our ability to start adopting Swift Concurrency it only matters that it's asynchronous.
Three possible alternatives could be:
An initializer (like in this PR)
An static method on the context type itself (or another type)
An (consuming) method on another type itself (like a hypothetical "builder")
The "shape" of this public API doesn't meaningfully impact the internal implementation. For example, we could have a private builder type internal to the initializer if we wanted to separate the context creation code from the rest of the context code.
Also, needing to deprecate one "shape" of asynchronous API to replace it with another "shape" of asynchronous API doesn't affect our ability to adopt Swift Concurrency.
More details about these change
The key change in this PR is adding the
async
keyword to the package accessDocumentationContext
initializer hereTwo of the three places that calls this initializer (except for tests) is
ConvertAction.perform(logHandle:)
andEmitGeneratedCurationAction.perform(logHandle:)
and which are already asynchronous.The ConvertAction code needed one small update to a signpost call to support awaiting the context creation.
The only other place that calls this initializer is the
ConvertService.process(_:completion:)
.Despite the completion handler parameter, this is actually a synchronous API which is possibly unexpected for the caller.
However, the fact that it has a completion handler means that we can easily make it non-blocking for the caller.
I did this my introducing an
async
alternative and having the completion handler version of the API call the asynchronous API inside a Task.The internal implementation of
process(...)
made heavy use ofResult
type to no real benefit but negative impacts on its readability—requiring the reader to jump between 3 private helper functions that unwrapped an optional, decoded a codable value, and encoded a codable value—and the ability to use asynchronous calls.By inlining these simple one-liners and abandoning the use of
Result
types in this code, the code got half as long and all the basic message processing code can be read linearly in one place with only the business logic in a separate private helper function.All the other ~2000 lines modified are all in unit tests. These changes are unavoidable no matter the "shape" of the asynchronous context creation API.
Dependencies
None
Testing
Nothing in particular. This isn't a user-facing change.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded