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

Rule request: [no-async-without-await] Functions marked async must contain an await or return statement #5082

Open
2 tasks done
pm-dev opened this issue Jun 26, 2023 · 8 comments
Labels
discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. rule-request Requests for a new rules.

Comments

@pm-dev
Copy link

pm-dev commented Jun 26, 2023

New Issue Checklist

New rule request

Disallow async functions which have no await expression.

  1. Why should this rule be added? Share links to existing discussion about what
    the community thinks about this.
    async functions that don't suspend contradict the Swift Concurrency documentation which specifies they are able to suspend partway through execution.
    Asynchronous functions that don’t use await can cause confusion in that a programmer awaiting such a function may assume it's guaranteed to execute its work in the background. Although such an assumption is a fundamental misunderstanding of Swift Concurrency, there are plenty of examples.

Async functions without await are a common and unintentional result of refactoring.

  1. Provide several examples of what would and wouldn't trigger violations.

Pass:

func f() async {
    await fetch();
}

Fail:

func add(a: Int, b: Int) async {
    return a + b
}
  1. Should the rule be configurable, if so what parameters should be configurable?
    There may be edge cases where async without await is appropriate that I'm not thinking of.

  2. Should the rule be opt-in or enabled by default? Why?
    Enabled by default
    I believe this rule would be fast, is not prone to false positives and is general consensus.

Linters for other languages have this rule:
TS Lint
ES Lint

@SimplyDanny SimplyDanny added the rule-request Requests for a new rules. label Jun 26, 2023
@jshier
Copy link
Contributor

jshier commented Jun 27, 2023

Async functions without awaits are entirely appropriate, as they can be used to run synchronous code in the background. Barring any enclosing actor scope (isolation), your add example will be run on the default executor, making it easy to ensure it will run in the background. You can do the same thing from within an actor by marking the function nonisolated, which will ensure it hops out of the current isolation onto the default executor.

If this is added it should certainly not be enabled by default.

@SimplyDanny SimplyDanny added the discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. label Jun 27, 2023
@pm-dev
Copy link
Author

pm-dev commented Jun 27, 2023

@jshier you're suggesting async can be added to run the function in the background, yet admit asynchronous functions aren't guaranteed to run in the background. This is why I believe this rule is important, to avoid this confusion.

Until Swift 5.9 and custom executors, it's not possible to use Swift Concurrency to guarantee code is run in the background.

@jshier
Copy link
Contributor

jshier commented Jun 27, 2023

If defined outside an actor context, async does guarantee background execution (execution on the default executor, i.e. the concurrency thread pool). If defined in an actor context, async still forces concurrent execution back onto the same context. Both of these are useful behaviors. Even in an actor context where the method will execute on the actor, this warning doesn't seem useful, as that fact should be self evident if you're familiar with Swift concurrency (e.g. unlabeled async functions will execute on their inherited actor context, if any, or the default executor), so I'm not sure what it would be protecting against.

@pm-dev
Copy link
Author

pm-dev commented Jun 27, 2023

Execution on the default executor != execution in the background. Today that may be the case, but the way the cooperative thread pool works is an implementation detail that is not exposed and could change in the future/on more constrained systems. Swift Concurrency provides an abstraction that doesn't have a concept of "background". Currently (Until swift 5.9) if you want to guarantee code is executed off the main thread (i.e. the background) you need to drop down to GCD or Threads.

If this rule is not useful, then why shouldn't every function be marked as async?

@jshier
Copy link
Contributor

jshier commented Jun 27, 2023

Yeah, that's not a distinction Swift concurrency makes, so I think you're pretty off base on user expectations here. Why would anyone expect that when, given your definition, it's not even possible right now?

In any case, I've laid out my reasoning, it's up to the maintainers now.

@pm-dev
Copy link
Author

pm-dev commented Jun 27, 2023

that's not a distinction Swift concurrency makes

I'd argue it does make that distinction by intentionally not documenting whether code is run in the background:

Swift's concurrency design is intentionally vague about the details of how code is actually run.

Source

My response to your original comment only intended to explain why the one example you provided against this rule is invalid. Adding async to a function without awaits is not a way to run synchronous code in the background.

@SVGreg
Copy link

SVGreg commented Jul 5, 2023

Function declaration represents interface, whereas it's content is implementation.

protocol MyAsyncProtocol {
    func doSomething() async
}

class ImpOne: MyAsyncProtocol {
    func doSomething() async {
        await useAsyncFunction() // Is Ok - using await
    }
}

class ImpTwo: MyAsyncProtocol {
    func doSomething() async {
        useSyncFunction() // Is also Ok - not to use await if not needed
    }
}

@pm-dev
Copy link
Author

pm-dev commented Jul 5, 2023

☝️ valid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. rule-request Requests for a new rules.
Projects
None yet
Development

No branches or pull requests

4 participants