Analyzer suggestion: Report improvability of the async/await pattern

[CollinAlpert]

Describe the problem you are trying to solve

What I see often in code written by developers who are not completely familiar with how async/await works is the following pattern:

public async Task RunAsync() {
  await DoSomethingAsync();
}

or

public async Task<int> RunAsync() {
  if(new Random().Next() % 2 == 0) {
    return await GetValueAsync();
  }
  
  return await GetOtherValueAsync();
}

These methods can be optimized to return the Tasks directly without a need for awaiting them:

public Task RunAsync() {
  return DoSomethingAsync();
}

and

public Task<int> RunAsync() {
  if(new Random().Next() % 2 == 0) {
    return GetValueAsync();
  }
  
  return GetOtherValueAsync();
}

respectively. This prevents the compiler from creating an unnecessary state machine and can delegate awaiting to the caller.

I am proposing an Analyzer with the "Info" severity which analyzes methods, local functions and lambdas and can report when a Task can be returned directly instead of awaiting it.

Describe suggestions on how to achieve the rule

I have a pretty solid Analyzer + CodeFix implemented already. It targets methods, local functions and lambdas, checks if the return type is Task or Task<T> and then acts accordingly.
For a Task-returning method, an await expression must either immediately be followed by a return; or must be the last statement in the body.
For a Task<T>-returning method, an await expression must be immediately returned.

If the body contains one await expression which does not match these rules, the diagnostic is not reported.

Additional context

The Analyzer should not report an await within a try or using block, since Tasks need to be awaited in these scenarios.

[Youssef1313]

FYI @stephentoub

[CollinAlpert]

I found some false positives in the last few days and fixed them. I'd love to get some feedback and ideally contribute this towards the .NET 7 release. Since I already have the implementation ready, I could open a PR fairly quickly. Here is a more detailed overview of what the Analyzer does and does not report:

What it does report:

1. Async methods with a single await or return await expression (this includes expression-bodied methods).
2. Async methods which have a single await or return await expression at the end of the method body.
3. Async methods which have multiple (nested) return await expressions.
4. Async methods which have multiple (nested) await expressions, either when they are the last statement in the method or if the method immediately returns after the await.

The same goes for lambdas and local functions. Also, a .ConfigureAwait() call is removed when the CodeFix is applied.

What it does not report:

1. Non-async methods.
2. Task-returning methods without an await expression.
3. async void methods.
4. Async method where not every return statement is an await expression.
5. Async method with an await expression in a using block or statement.
6. Async method with an await expression in a try block.
7. Async method with multiple await expressions which do not immediately precede the method's return.
8. Async methods with an await expression in a loop.
9. Async methods which have a covariant return type. E.g.:

public async Task<IEnumerable<int>> GetValuesAsync() {
    return await GetListAsync();
}

public Task<List<int>> GetListAsync() {
    return Task.FromResult(new List<int>() { 1, 2, 3 });
}

The same goes for lambdas and local functions.

[mavasani]

Tagging @jmarolf who has investigated adding/porting async/threading related analyzer rules from MS.VS.Threading analyzers

[CollinAlpert]

@jmarolf I see you added this as a To Do. Does this mean it is approved and I can open a PR?

[stephentoub]

While there are perf benefits of just returning a task at the end of a method rather than awaiting it, there are diagnostic/maintainability reasons why folks might not want to do that, e.g. exceptions will be missing that method from the stack trace.

[stephentoub]

Also, it does change semantics, e.g. if someone is calling .ContinueWith off of the returned task, and if an exception happens in the body of what was the async method, previously that ContinueWith would have been invoked for the faulted task, but with the "fixed" code, the exception will propagate synchronously from the method and the ContinueWith will never be invoked.

In other words, there are very subtle issues the proposed fixer could cause.

[CollinAlpert]

These are all valid points. Do you see any way (theoretically) in which the analyzer/fixer could work around these problems? Or are these issues blockers?

[stephentoub]

Do you see any way (theoretically) in which the analyzer/fixer could work around these problems?

To determine whether there's an observable difference around exceptions escaping synchronously or asynchronously, you could teach the analyzer to look at all callers and determine whether every call site directly awaits the returned task. If every use is a direct await, then the difference between them isn't observable (other than diagnostics). That of course requires you to be able to find every call site, which logically means this would be limited to non-public members (and non-internals in an assembly with InternalsVisibleTo).

For diagnostics, there's really nothing an analyzer could do if it wanted to avoid false positives: it depends on the needs / intent of the developer.