[Pitch] @completionHandlerAsync attribute

(To clarify, I'm not suggesting we should use string literals)

It's probably possible to express a getter without a string literal. Also, if we type check it, we should be able to infer that it's a getter because we know value is var or func.

(One interesting fact about @available(renamed:) is that it doesn't type check the new name. i.e. @available(*, deprecated, renamed: "foobar()") is not an error and emits fix-it even without actual foobar declaration.)

I guess another reason of using a string literal was to have a common logic to handle objc __attribute__((swift_name("..."))) attribute and/or apinotes. But I'm not sure. Perhaps @jrose knows the history?

#selector uses getter:/setter: prefix. But @completionHandlerAsync(getter:valueName) is not ideal as it's confusable with a attribute argument label...

@derivative(of: ) uses postfix .get/.set?

Related discussion for syntax of referencing getter/setters:
https://github.com/apple/swift-evolution/blob/main/proposals/0021-generalized-naming.md
https://forums.swift.org/t/proposal-expose-getter-setters-in-the-same-way-as-regular-methods/501
https://forums.swift.org/t/review-se-0044-import-as-member/1805/5

1 Like

Another syntax that I just remembered is “@dynamicReplacement(for: hello(_:))”.

Fairly rarely used but another point for trying to make it work without string literals.

I’ll check in an hour or how getters are expressed there (if at all possible!).

1 Like

Yes, it does today. It is handled here as parseQualifiedDeclName.

1 Like

The handling in @derivative seems the most reasonable to me, though it also allows a base type which @completionHandlerAsync shouldn't (?). I could be missing something, but it seems a little weird to allow it in the @derivative can anyway, it looks like it enforces the two functions to have the same parents as well.

@dynamicReplacement doesn't handle accessors from what I can see.

Yeah, doesn't seem to do so -- it can replace an entire property, but can't specify just a getter.

The method signatures of asynchronous vs synchronous functions will often use a predictable pattern (same name, inputs, and outputs). Considering that, I’d like to see the name parameter to @completionHandlerAsync be optional (when possible).

Example:

@completionHandlerAsync
func processImplicit(data: Data, completion: @escaping (ProcessedData) -> Void) { ... }
func processImplicit(data: Data) async -> ProcessedData { ... }

I’m not a big fan of this being implicit for what it’s worth. It invites being lazy about it, doesn’t it?

We found that often functions get renamed because “getThing” is nicer as “await x.thing” etc. Someone has to manually audit those alternatives anyway in normal code, and in imported code it is done automatically by the importer so it being a bit more verbose does not really matter.

The spelling you propose is also a bit backwards… we’d like to say this method has an alternative, not that it is one — but this spelling makes is sound like it IS the “completion handler async”. In some internal discussions this did come up too and was proposed to be solved by “hasAsyncAlternative” which IMHO sounded pretty inelegant.

Just a personal thought though.

The hope is that including the name adds a moment of "should this really be the name?". As an example, let's say that the callback-based function was func whenReady(data: Data, completion: @escaping (ProcessedData) -> Void) { ... } - perhaps that would now be better as var data: ProcessedData { get async { ... } }.

I believe this was in reply to Raybo, but note that they were just using the originally proposed name :laughing:. Note that the spelling is indeed correct there, ie. the attributed function is completion handler async and has the alternative spelled in the parameter. Part of what makes this name hard is that the name is of the alternative and the index is of the attributed function. Perhaps we need a name where it then makes sense to name both parameters, ie. @something(alternative: <func>, completionHandlerIndex: <index>).

2 Likes

Right sorry, I guess I was voicing the general opinion against implicitly inferring the alternative. It also ends up very confusing (to me at least, as one datapoint), what is the preferred alternative and what is the old one if just the existing name were adopted, and we made the alternative implicit even.

To me, it's a valuable moment when one has to write that alternative pointed at -- it may be a good chance to realize the alternative API could be a bit nicer, say, using an async property getter or similar. I've seen a bunch of APIs would could benefit from such treatment.

The "having an alternative API that is more favorable" problem also shows up in other places. Returning to an old hobbyhorse: Migrating higher order function names to comply with API guidelines - #27 by masters3d

It seems to me that one of the main use case is migration where we want folks to move to asynchronous versions of an older synchronous API. I see this as the same problem as "deprecate but never delete" older API.

Can this feature be more engineered to be more general than just async/sync API?

Thanks,
Chéyo

3 Likes

What happens to this very specialized new language feature when everyone is done migrating? Why cannot this be an extension of the existing @available mini-language, which is already used for this kind of redirection, via deprecated?

2 Likes

I don’t remember for sure, but my best guess is this is an accident of incremental development: first the “renamed” in Clang’s availability attribute got copied over to Swift verbatim, and later support got added to actually use the Swift name of a Clang-renamed declaration. By then, though, it was already too late to make it not accept arbitrary strings. We could type-check it regardless, though, deprecating the use of renamed in Swift to reference anything but a Swift declaration in scope. We could even accept both quoted and non-quoted syntax for a while if we wanted.

3 Likes

Why not reuse the existing @available mechanism?
E.g. @available(*, async: "processImplicit(data:)")

6 Likes

@completionHandlerAsync isn't supposed to suggest deprecation, just that the referenced function should have preference in an async context. It's more similar to @masters3d use case, but as mentioned, not a "always use instead of " but rather "when in an async context, use instead of ".

Regarding using @available/a more general purpose attribute: I was originally against this since it would be awkward to add the completion handler index, but after thinking about it a little more, I'm now not sure we actually need it. In general if the async function parameters match the callback function parameters (minus the completion handler), it's already easy to infer the completion handler index. If the parameters don't match up then having the index probably isn't going to be all that useful anyway - refactorings wouldn't be able to replace the call since they can't match up the parameters and compiler warnings don't need it regardless.

Downside of using @available is that I don't think it entirely makes sense on the callback function, ie. @available usually says something about the attributed function, but in this case it would be saying "there is this other function available that could be used instead". Though if we don't need the index, it could make sense to place the attribute on the async function instead.

1 Like

I actually think we should just use the normal @available(deprecated:renamed:) feature for this:

  @available(macOS, deprecated: 12.0, renamed: "process(data:)")
  func process(data: Data, completion: (ProcessedData) -> Void) { ... }
  func process(data: Data) async -> ProcessedData { ... }

The compiler/SourceKit could detect that process(data:) is an async version of process(data:completion:) by applying translation heuristics similar to ClangImporter's (possibly with a little extra logic to support a single Result parameter), and then tailor the diagnostics and fix-its it creates to match.

Since this would just be a change to the diagnostics used for existing syntax, it would not require an evolution proposal and it would be backwards-compatible with earlier compilers, which would simply output generic deprecation diagnostics.

If people strongly want a softer option that doesn't emit deprecation warnings unless the call is in an async context, we could co-opt this syntax to do that:

  @available(*, renamed: "process(data:)")      // No `deprecated` means suggest replacement only in async contexts
  func process(data: Data, completion: (ProcessedData) -> Void) { ... }
  func process(data: Data) async -> ProcessedData { ... }

This syntax is accepted by current compilers but doesn't seem to do anything.

2 Likes

It feels odd to me to use renamed - it implies that the functions have the same semantics, which isn't really true here. I can understand the appeal in that there wouldn't have to be any evolution proposal though.

renamed currently isn't typechecked at all either. We could allow non-quoted and typecheck that, but that would - if I understand correctly - still require a proposal.

I was going to write that maybe it doesn't matter if it's a completion handler or not, ie. we'd want to warn if we know there's an async alternative to a function being used in an async context. But I imagine it would be more common to want to use the non-async version (of a non-completion handler) in an async context - so limiting it to completion handlers only seems like a good idea.

And in that case I'm not a huge fan of heuristics - the idea of the initial pitch was to avoid that, since we'd always have the possibility the heuristic being off (resulting in either erroneous or missing diagnostics/similar for the refactorings).

This reminds me though - another aspect to this is that even for completion handlers, it may be that we'd like a way to silence the warning. Wrapping it in parentheses/a no-op function perhaps?

To add to the above - I probably sound more against the idea than I intend to be. I do think it's a decent idea in order to avoid the evolution proposal (and not adding another fairly-limited attribute to the language). Just wanted to make sure we're aware of the trade-offs.

I think finding ways to circumvent the Swift Evolution process while designing new features, when all user-facing features are supposed to be proposed and reviewed whether they introduce new syntax or not, shouldn't be a goal!

Agree that "renamed" doesn't totally fit the bill here. @available(/* ... */, async: "foo") seems like a pretty intuitive spelling to me.

3 Likes

This is an excellent idea. +1
I love that it is a general solution instead of something specific to async.

I've been thinking about this a fair bit over the last couple weeks and am definitely leaning towards Becca's solution.

A large part of the reason for this pitch initially was for the completionHandlerIndex argument that would be useful for eg. refactorings. This didn't make a lot of sense to put on @available. But through the discussion in this thread I now realise that this isn't actually required - in the cases that a call could be refactored, it's simple enough to determine the index by looking at the parameters of the matched functions.

So from there, @available looks very reasonable solution.

The next question is whether we should introduce a new parameter to @available purely for this purpose. If renamed: required deprecated, I think a new parameter would be necessary (since it isn't necessarily deprecated). As Becca mentioned though, that's not the case.

Another argument for a separate parameter would be that it could be more specific in what it allows as its argument. But I don't think adding another inconsistent function reference is all that helpful. A better idea there is probably what Jordan mentioned, ie. typecheck the renamed: argument and deprecate allowing a reference to anything but a Swift declaration in scope.

So with all that said, I don't think anything new needs to be added for this purpose. Thanks for the input everyone, much appreciated!