dotnetanalyzers / asyncusageanalyzers

Now superseded by Microsoft/vs-threading

DotNetAnalyzers

This project is for the community to make a common code base of .NET code analyzers using the new VS14 code analyzer functionality. Overview

• Guidelines

asyncusageanalyzers's Issues

New rule proposal: Called methods that are not marked async but return a task should be awaited

Discussed in dotnet/roslyn#20782. CS4014 is only reported if the callee is marked with async, this leaves out a good portion of the problem cases. It would be be helpful if an analyzer expanded on this to report on the cases where the method is not marked with async.

Update AvoidAsyncVoid to test lambda functions

The non-async method for subclass return a ValueTask report AvoidAsyncSuffix

I have a method need implement from the interface, and this implement is not async. So I write the following signature:
public ValueTask<SongInfo> FindAsync()
But it report AvoidAsyncSuffix. It work fine with:
public Task<SongInfo> FindAsync()

I think the ValueTask should be support.

Addition: I see the recent update, but the package in nuget is really out of date. Could you update the nuget package?

Refactor issue for ignoring a rule

I am assuming that the alt + enter suppression is a feature by this analyzer.

When I have a method like this:

/// <summary>
/// some text
/// </summary>
public async void MyMethod()
{
}

And want to supress the warning, it puts the pragma above the documentation:

#pragma warning disable AvoidAsyncVoid // Avoid async void
/// <summary>
/// some text
/// </summary>
public async void MyMethod()
#pragma warning restore AvoidAsyncVoid // Avoid async void
{
}

I think it should be put above the method call:

/// <summary>
/// some text
/// </summary>
#pragma warning disable AvoidAsyncVoid // Avoid async void
public async void MyMethod()
#pragma warning restore AvoidAsyncVoid // Avoid async void
{
}

Define the behavior of AvoidAsyncSuffix for Event-based Asynchronous Pattern

The current implementation of the AvoidAsyncSuffix analyzer only targets the Task-based Asynchronous Pattern (TAP). The Event-based Asynchronous Pattern (EAP) also uses the Async suffix for certain methods, but the signatures are different from the TAP.

The behavior of the AvoidAsyncSuffix analyzer should be fully defined for this case, and tests included to detect regressions.

AvoidAsyncSuffix misfires on ValueTask<T> returning methods

Code fixes from AsyncUsageAnalyzers.CodeFixes don't work in Visual Studio

Code fixes from AsyncUsageAnalyzers.CodeFixes don't work in Visual Studio.

I've created a pull request #48 which fixes it.

Task<T> class Properties shouldn't require `Async` suffix

UseAsyncSuffixAnalyzer enforces the Async suffix on property names. Is that something this rule should enforce -- it seems like that should be the exception to the rule?

Errors:

public class MyClass
{
    Task<string> MyString { get; set; }
}

UseAsyncSuffix should not fire for overriding methods

If the class being derived from is ill designed and doesn't suffix asynchronous methods with Async, there's nothing the derived class can do about it, so this should not be a warning.

New rule proposal: Async methods should have a CancellationToken parameter

This doesn't apply to all asynchronous methods, but I think it might be a useful rule to have in the toolbox (rulebox?) provided it is disabled by default.

I write a lot of methods like this:

public Task<Response> GetResponse(
    string href,
    CancellationToken cancellationToken = default(CancellationToken))
{ 
    //... 
}

...which is a pattern modeled after the TAP guidelines and Mongo's excellent C# driver.

It's easy to forget the CancellationToken parameter, though, so in my own project I have a short unit test that uses reflection to make sure I haven't forgotten something. That made me think that an analyzer rule might be a better tool for the job! 😄

Non-fatal error while installing package

While installing the package with the Package Manager Console, the following is reported:

Executing script file '...\StyleCopAnalyzers\packages\AsyncUsageAnalyzers.1.0.0-alpha001\tools\install.ps1'
Get-ChildItem : Cannot find path '...\StyleCopAnalyzers\packages\AsyncUsageAnalyzers.1.0.0-alpha001\tools\analyzers\C#' because it does not exist.
At ...\StyleCopAnalyzers\packages\AsyncUsageAnalyzers.1.0.0-alpha001\tools\install.ps1:18 char:31
+ foreach ($analyzerFilePath in Get-ChildItem $languageAnalyzersPath -Filter *.dll ...
+                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (...ls\analyzers\C#:String) [Get-ChildItem], ItemNotFoundException
    + FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.GetChildItemCommand

New rule proposal: Propagate cancellation tokens

This diagnostic would be reported when CancellationToken.None is explicitly provided in a call, but another cancellation token is available in the current context (see #36 and #32 for comments describing the current context).

The code fix for this would replace the use of CancellationToken.None with the "best" cancellation token as described in the code fix for #36.

Treat types marked with AsyncMethodBuilderAttribute as asynchronous

See #70 (comment)

This is a subset of #74.

Support Main with async

C# 7.1 introduced async Main methods. Can we leave them out from UseAsyncSuffix?

Update to Roslyn 1.0

The AsyncUsageAnalyzers has to be updated to Roslyn 1.0 to be supported in visual studio 2015

Bug: Analyser not raising warnings for local functions not ending with "Async".

I have C# local functions returning Task / Task<T>, but the analyser doesn't give me a warning when I have incorrectly omitted the "Async" suffix on the function name.

Define the behavior of AvoidAsyncVoid for event handlers

Currently the AvoidAsyncVoid analyzer does not have any special consideration for event handlers. The desired behavior for these cases should be explicitly documented, even if the implementation is kept as-is.

UseConfigureAwait rule inappropriate in many contexts

The default behavior of await is appropriate for app development. In some libraries, it may be appropriate to add .ConfigureAwait(false) at the end of an awaitable expression, but there are plenty of contexts where the default behavior is appropriate.

Yes, I know rules can be disabled. But perhaps the default should be to disable the rule and let users enable it when they know they're working on a shared library project for which no requirement to abide by the SynchronizationContext rules.

Don't provide code fixes for Visual Basic in UseConfigureAwaitCodeFixProvider and DontUseThreadSleepCodeUniversalCodeFixProvider

I think that Visual Basic should be deleted from ExportCodeFixProvider attributes in UseConfigureAwaitCodeFixProvider and DontUseThreadSleepCodeUniversalCodeFixProvider (from PR #50 ). Both code fixes use classes from Microsoft.CodeAnalysis.CSharp and Microsoft.CodeAnalysis.CSharp.Syntax and clearly won't fix Visual Basic code.

Analyzers require mscorlib 2.0.5.0 because of JSON.NET

I'm getting this warning:

MissingAnalyzerReference Analyzer assembly 'packages\StyleCop.Analyzers.1.0.0-beta009\analyzers\dotnet\cs\Newtonsoft.Json.dll' depends on 'mscorlib, Version=2.0.5.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e' but it was not found. Analyzers may not run correctly

Update IncludeCancellationParameter analyzer to check overloads

Currently the analyzer does not check for an overload that takes a cancellation token. For example, the following interface would result in a diagnostic being reported, but it shouldn't.

interface InterfaceName
{
  Task MethodAsync();
  Task MethodAsync(CancellationToken cancellationToken);
}

New rule proposal: Provide CancellationToken in calls

This diagnostic would be reported if a method M is called without providing a CancellationToken, and either M or one of its overloads has a CancellationToken parameter.

The code fix for this would attempt to fill in the CancellationToken as follows:

1. If a local variable or parameter provides a CancellationToken (with consideration for context objects as described in #32), the innermost visible CancellationToken is passed.
2. Otherwise, pass CancellationToken.None.

⚠️ Separate equivalence keys should be used for the code fix for the case where the updated call still calls the same method, and the case where the updated call is to a different overload of the method.

Consider ConfigureAwait() for API calls

As already written here DotNetAnalyzers/Proposals#26 this is tricky, but an important item to try and catch.

Update documentation to include [SuppressMessage] examples

As mentioned in #45, the documentation for rules currently only includes examples for using #pragma warning disable to suppress violations. While [SuppressMessage] works as well, examples need to be created so users are aware of the option.

Support Supression attribute

At the moment there is only support for "pragma" for supression of a rule.

It would be good to be consistent and support SupressionAttribute as well.

Update IncludeCancellationParameter analyzer to cache context lookups

Currently the analyzer checks each parameter type to see if it is a context type containing a CancellationToken property. The results of these lookups should be cached in the Analyzer instance to avoid multiple lookups for the same type.

Use Task.WhenAll or Task.WhenAny instead of awaiting in loops

Again, I'm not sure exactly how to spot this, but too much code does something like:

for (int i = 0; i < 100; i++)
    await SomeTask(i);

Instead of:

var tasks = from i in Enumerable.Range(0,100)
            select SomeTask(i);
await tasks.WhenAll();

Treat methods with awaitable return types as asynchronous

See #70 (comment)

Suppress async void nag in asserts.

Would be nice if this scenario was special cased:

Assert.DoesNotThrow(async () => await foo.BarAsync().ConfigureAwait(false));

Method name analyzers should not run run for override/implementing methods

Currently the UseAsyncSuffix diagnostic is reported for cases where a method overrides a method from a base class and/or implements an interface method. In these cases, the name is set by the base definition, so no naming warnings should be reported at the site of an overriding or implementing method.

New rule proposal: Methods returning a Task should end with Async

Category: Naming
Rule: Methods returning a Task should end with Async
Description: The manner in which asynchronous methods are consumed in code differs substantially from non-asynchronous methods. The Task-based Asynchronous Pattern (TAP) documentation, along with the .NET Framework itself, uses a specific naming convention to identify methods following one particular pattern for asynchronous method calls. This rule ensures other methods following this pattern do the same.

I was originally planning to file this as a new rule proposal for StyleCopAnalyzers. However, I decided to post it here since it likely applies to a wider range of code than StyleCopAnalyzers supports (e.g. VB code).

Warn when passing a task into an parameter with type object.

I've run into this a few interesting times and I can't see any reason why this should be valid. Would be interesting to add.

New rule proposal: Use ConfigureAwait(false)

Category: Usage
Rule: Use ConfigureAwait(false)
Severity: Hidden
Description: This analyzer identifies code which uses await on a Task, and does not use ConfigureAwait(false). By default it uses Hidden severity, which enables Ctrl+. on the line containing the call, but libraries that don't interact with the UI can reset the severity to Warning or Error in order to enforce this rule across an entire project.

Add special rule for async void eventhandlers.

private async void OnChanged(object sender, EventArgs e)
{
    ...
}

What about adding a rule AvoidAsyncVoidInEventhandlers?
The reason for this would be to be able to tune severity separately for them.
Heuristics could be: method with two arguments of type object and assignable to EventArgs or maybe it can be figured out from climbing around the syntax tree that the method is only used for subscribing to an event.

Perhaps there could be another analyzer checking that there is try-catch surrounding all awaits.

Document IncludeCancellationParameter diagnostic

Currently no documentation page exists for this diagnostic.

Await inside usings.

using (var webClient = new WebClient())
{
    return webClient.DownloadStringTaskAsync(address);
}

We talked about this in chat, this can lead to situations where it is undefined if the IDisposable is disposed or not.

ValueTask<long[]> DeleteAsync

Use Task.FromResult to return already computed results

I'm not sure how well to enforce this, because the anti-patterns are very diverse, and subtle.

During coded reviews I've seen code (often in mocks for tests) that use some form of task construction and execution (like Task.Run()) to produce a result when Task.FromResult() would have been much cleaner.

Calling Wait() (or any variation) instead of awaiting a task

Tasks should not block. Instead, they should be awaited so that execution can continue.

Call Task.Yield() instead of Thread.Sleep() to yield control

Thread.Sleep() is not recommended. Instead, call Task.Yield() to yield control to another task.

Proposal: Properties should not be of type Task or Task<T>

Inspired by #21:

Properties should not be of type Task or Task<T>.
Most people perceive properties as near instantaneous (synchronous) operations.
Task and Task<T> are associated with asynchronous operations and should therefore not be used as property types, as this does not match with the expectations of the majority of developers.

New rule proposal: Methods not returning a Task should not end with Async

Category: Naming
Rule: Methods not returning a Task should not end with Async
Description: This rule is the converse of #7.

avoid async void methods

Async methods should not return void (unless mandated by an interface or event handler contract)

We should even suggest a clear "Fire and Forget" idiom to catch and log exceptions in the case of exceptions when async void must be used.