Bug 225134 – Validate Swift async imports

• Component: WebKit API
• Status: RESOLVED FIXED

[James Savage]

<rdar://problem/73620237>

[James Savage]

Created attachment 427230 [details]
Patch

[Sam Weinig]

Comment on attachment 427230 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427230&action=review

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.h:74
> +#ifdef NS_SWIFT_ASYNC_NAME

Given how common this idiom is, what do you think about having a WK_SWIFT_ASYNC_NAME() in WKFoundation.h that does nothing if NS_SWIFT_ASYNC_NAME is not defined? Or would that break the importer?

[James Savage]

Comment on attachment 427230 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427230&action=review

>> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.h:74
>> +#ifdef NS_SWIFT_ASYNC_NAME
> 
> Given how common this idiom is, what do you think about having a WK_SWIFT_ASYNC_NAME() in WKFoundation.h that does nothing if NS_SWIFT_ASYNC_NAME is not defined? Or would that break the importer?

I debated adding a WK_ macro. My major hesitation was around whether we'd be able to remove it, or if it then becomes "API". Perhaps this could be mitigated by naming it something more like __WK_SWIFT_ASYNC() instead? This is really just staging code while we wait for trunk's min SDK to get raised high enough that this is unconditionally available, and then my plan was to delete all the #ifdefs.

I also looked into post-processing it, which we could do with either unifdef or some of the more custom scripts we use for things like TBA availability. The existing support didn't seem to have logic for checking if something is available in the SDK already, which is what we'd want to key this off of.

With all this said, I fully admit that this approach is aesthetically… not great. So quite happy to consider other options.

[Maciej Stachowiak]

Comment on attachment 427230 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427230&action=review

I'm not familiar enough with Swift to give this a + or - but I left some comments on things that seemed odd to me.

> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.h:75
> +    NS_SWIFT_ASYNC_NAME(fromHTML(request:options:))

Is it really right to call this method fromHTML? That sounds like the name of something that would be a conversion operator from HTML to something else, but that's not what this is (and likewise for other related methods).

> Source/WebKit/UIProcess/API/Cocoa/WKContentRuleListStore.h:50
> +    NS_SWIFT_ASYNC_NAME(availableIdentifiers())

Seems sensible to avoid the "get" (redundant for an async method) but I'm not sure "Identifiers" is as descriptive as "ContentRuleListIdentifiers".

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:90
> +    NS_SWIFT_ASYNC_NAME(download(_:respondTo:))

What does "respondTo:" correspond to from the original parameters? Seems odd to not mention authentication in this authentication-related delegate name.

> Source/WebKit/UIProcess/API/Cocoa/WKNavigationDelegate.h:82
> +    NS_SWIFT_ASYNC(3)

What does the 3 mean here? does it mean, same name but third parameter is the async callback?

> Source/WebKit/UIProcess/API/Cocoa/WKNavigationDelegate.h:172
> +    NS_SWIFT_ASYNC_NAME(webView(_:respondTo:))

Same thing with the mysteriousness of not mentioning authentication here.

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegate.h:104
> +    NS_SWIFT_DISABLE_ASYNC

Why no async for this method?

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegate.h:124
> +    NS_SWIFT_DISABLE_ASYNC

Why no async for this method?

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegate.h:146
> +    NS_SWIFT_DISABLE_ASYNC

Why no async for this method?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:410
> +    NS_SWIFT_ASYNC_NAME(snapshot(configuration:))

Is this using snapshot as a verb or a noun?

[James Savage]

Comment on attachment 427230 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427230&action=review

>> Source/WebKit/UIProcess/API/Cocoa/NSAttributedString.h:75
>> +    NS_SWIFT_ASYNC_NAME(fromHTML(request:options:))
> 
> Is it really right to call this method fromHTML? That sounds like the name of something that would be a conversion operator from HTML to something else, but that's not what this is (and likewise for other related methods).

I think so. We could do something more drastic like name this `byLoadingRequest(_:options:)`. The shipping name is already `NSAttributedString.fromHTML(request:options:completionHandler:)`, so the only change here is to drop the initial argument label. My thought was that this would make the call more concise without really sacrificing clarity, since in a typical call you'd probably be writing `NSAttributedString.fromHTML(request: someRequest, …)`, which spells out request twice anyways. Since Swift can handle overloads.

>> Source/WebKit/UIProcess/API/Cocoa/WKContentRuleListStore.h:50
>> +    NS_SWIFT_ASYNC_NAME(availableIdentifiers())
> 
> Seems sensible to avoid the "get" (redundant for an async method) but I'm not sure "Identifiers" is as descriptive as "ContentRuleListIdentifiers".

Since the call will be on some store, I'd expect it to look something like `contentRuleListStore.availableIdentifiers()` (or `store.availableIdentifiers()`). In either case, since the class seems so focused on content rule lists already, do we really need to repeat that in the method name itself? My feeling was that it was redundant, unless we need to reserve room to add different top-level identifier types here in the future.

>> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:90
>> +    NS_SWIFT_ASYNC_NAME(download(_:respondTo:))
> 
> What does "respondTo:" correspond to from the original parameters? Seems odd to not mention authentication in this authentication-related delegate name.

So this is the most compressed form of the method name. An actual adopter of this API will see something like…

func download(_ download: WKDownload, respondTo challenge: NSURLAuthenticationChallenge) async -> (NSURLSessionAuthChallengeDisposition, NSURLCredential?) {
    …
}

So the context of authentication challenge comes from the internal argument names, and type names. We can also shorten this without fear of some other "respondsTo:" method coming along, since Swift<->ObjC support understands how to translate Swift overloads to the correct ObjC selector. Even if we added another `download(_:respondsTo:)` "WKOtherChallenge", that would be a very different method implementation and be disambiguated by the compiler.

On the caller side: as this is a delegate we don't really expect many callers to call it from Swift, but even if we did this still follows Swift API contentions of not repeating words that are already present in the type information. So you would expect to see `delegate.download(self, respondsTo: authenticationChallenge)` rather than `delegate.download(self, respondsToAuthenticationChallenge: authenticationChallenge)` anyways.

>> Source/WebKit/UIProcess/API/Cocoa/WKNavigationDelegate.h:82
>> +    NS_SWIFT_ASYNC(3)
> 
> What does the 3 mean here? does it mean, same name but third parameter is the async callback?

3 identifiers the completion block parameter using a 1-based index, so we're telling the compiler that `decisionHandler` is actually a completion handler for the purposes of Swift's async feature.
The importer only considers `completion` and `completionHandler` for this, so this was skipped over.

>> Source/WebKit/UIProcess/API/Cocoa/WKNavigationDelegate.h:172
>> +    NS_SWIFT_ASYNC_NAME(webView(_:respondTo:))
> 
> Same thing with the mysteriousness of not mentioning authentication here.

See above.

>> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegate.h:104
>> +    NS_SWIFT_DISABLE_ASYNC
> 
> Why no async for this method?

Thinking about this a bit more, I think removing this may be a good idea because, for ObjC derived methods only, clients get to pick whether they implement the completion handler or async version (but they can only pick one).

My rational here was following the guidance to disable async for methods that bridge to UI frameworks that will still be taking a completion block. For example, if you were building a UIAlertController to display this dialog, you wouldn't be working with async APIs, you'd be constructing an object with blocks still and then calling present(alertController) with that alert. So by making this async we actually make it harder for clients to correctly interface with that construct since they'll need to create a block from the current async context instead of just using the perfectly good one we'd otherwise give them. In code this would look something like…

func webView(_ webView: WKWebView, runJavaScriptAlertPanelWithMessage message: String, initiatedByFrame frame: WKFrameInfo) async {
    return await withUnsafeContinuation { continuation in
        let alert = UIAlertController(…)
        alert.addAction(UIAlertAction(title: "OK", handler: { _ in
            continuation.resume()
        })
        viewController.present(alert, animated: true)
    }
}

The `return await withUnsafeContinuation { … }` dance could be avoided by just using the existing completion handler based callback, and (at least for this example) not much is gained by adopting async here.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:410
>> +    NS_SWIFT_ASYNC_NAME(snapshot(configuration:))
> 
> Is this using snapshot as a verb or a noun?

I was thinking of it as a verb, but I do see the ambiguity now that you mention it. My first though was to remove "take" the same way we are supposed to remove other verbs like "get" or "fetch" for async methods, but here I guess snapshot may be too broad since a PDF is technically a form of snapshotting too. Happy to add this back.

[James Savage]

Created attachment 428024 [details]
Patch

[EWS]

Committed r277215 (237486@main): <https://commits.webkit.org/237486@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428024 [details].