New rule reliability/maintainability: Do await incomplete Tasks before returning and implicitly running other code

[TimLovellSmith]

Approximate match on rule categories to put it in:

Usage: CA1801, CA1806, CA1816, CA2200-CA2209, CA2211-CA2260 Reliability: CA9998-CA9999, CA2000-CA2021 MicrosoftCodeAnalysisCorrectness: RS1000-RS1999 MicrosoftCodeAnalysisDesign: RS1000-RS1999 RoslynDiagnosticsMaintainability: RS0000-RS0999

Suggested rule name: await asynchronous methods when returning a Task directly. Rationale: 'goes-out-of-scope' logic like 'using statement' may run sooner than you expect.

Describe the problem you are trying to solve

The following code is 'clearly a bug waiting to happen' to the eyes of some C# experts.

public Task GetHealthStatusAsync(CancellationToken cancellationToken = default)
{
    using IDisposable disposable = new System.IO.FileStream(); // implicit cleanup at end of scope
    return GetCdsProfileAsync(FromInstrumentationKey(_testIKey), cancellationToken); // return without await
} 

What we want to prevent, is people writing code with signature Task, that calls a Task method, without doing await, but just returning that Task directly, and then having automatic cleanup run because the 'synchronous' scope ends.

i.e. the using statement would dispose the object created even though the chained async call won't be completed yet.

This bug will be non-deterministic based on whether the chained async call actually completes immediately or not.

Describe suggestions on how to achieve the rule

Step 1: filter based on method signature. Rule only analyzes methods that return type Task or ValueTask, but do not use the 'async' keyword to implement an async state machine. Step 2: filter based on method implementation. Rule only fires for methods that a) return a Task object which isn't obviously going to complete immediately. Ones that are obviously always going to return a completed task include, 'Task.CompletedTask, Task.FromResult, Task.FromException', or have previously been provably completed using e.g. task.Wait(), task.Result. b) have code that will run implicitly after the return statement. Because of using statements, finally clauses, or anything similar.

Recommended (automatable) fix:

Convert method to do async/await instead.

Additional context

With kudos to Paul Harrington for giving a talk mentioning this antipattern.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-threading-tasks See info in area-owners.md if you want to be subscribed.

Issue Details

---

Approximate match on rule categories to put it in: Usage: CA1801, CA1806, CA1816, CA2200-CA2209, CA2211-CA2260 Reliability: CA9998-CA9999, CA2000-CA2021 MicrosoftCodeAnalysisCorrectness: RS1000-RS1999 MicrosoftCodeAnalysisDesign: RS1000-RS1999 RoslynDiagnosticsMaintainability: RS0000-RS0999 Suggested rule name: await asynchronous methods when returning a Task directly. Rationale: 'goes-out-of-scope' logic like 'using statement' may run sooner than you expect. ### Describe the problem you are trying to solve The following code is 'clearly a bug waiting to happen' to the eyes of some C# experts. ``` public Task GetHealthStatusAsync(CancellationToken cancellationToken = default) { using IDisposable disposable = new System.IO.FileStream(); // implicit cleanup at end of scope return GetCdsProfileAsync(FromInstrumentationKey(_testIKey), cancellationToken); // return without await } ``` What we want to prevent, is people writing code with signature Task, that calls a Task method, *without* doing await, but just returning that Task directly, and then having automatic cleanup run because the 'synchronous' scope ends. i.e. the using statement would dispose the object created even though the chained async call won't be completed yet. This bug will be non-deterministic based on whether the chained async call actually completes immediately or not. ### Describe suggestions on how to achieve the rule Step 1: filter based on method signature. Rule only analyzes methods that return type Task or ValueTask, but do not use the 'async' keyword to implement an async state machine. Step 2: filter based on method implementation. Rule only fires for methods that a) return a Task object which isn't obviously going to complete immediately. Ones that are obviously always going to return a completed task include, 'Task.CompletedTask, Task.FromResult, Task.FromException', or have previously been provably completed using e.g. task.Wait(), task.Result. b) have code that will run *implicitly after the return statement*. Because of using statements, finally clauses, or anything similar. Recommended (automatable) fix: Convert method to do async/await instead. ### Additional context With kudos to Paul Harrington for giving a talk mentioning this antipattern.

Author:	TimLovellSmith
Assignees:	-
Labels:	`area-System.Threading.Tasks`, `untriaged`
Milestone:	-

[stephentoub]

Thanks. How does this differ from https://github.com/dotnet/runtime/issues/78394?

[TimLovellSmith]

@stephentoub Looks like its the same proposal to me, just described differently, thanks for deduplicating!

[stephentoub]

Duplicate of #78394