New rule proposal: Async methods should have a CancellationToken parameter

[nbarbettini]

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! :smile:

[sharwell]

I fully agree, with one additional note - this warning should only be reported if there is no overload of the method which takes a CancellationToken.

This type of rule is the reason this project exists. Moving straight to up for grabs. :+1:

[nbarbettini]

Sweet. :+1:

I'm assuming StyleCop.Analyzers gets most of the attention these days, and this one is on the back-burner?

[sharwell]

Well it's not intentionally that way, but StyleCop.Analyzers does seem to get more attention than the other projects I started.

[pdelvo]

What should happen if a method parameter has the token in it, e.g. a SyntaxNodeAnalysisContext parameter?

[sharwell]

We could get clever. If the method takes an argument, and the static type of that argument exposes a CancellationToken property, then don't report a warning?

If it's slow, we could further restrict it to the case where it's a single argument and the name ends with Context.

[nbarbettini]

Composition is an interesting edge case. If we wanted to keep things simple, that might be a good scenario to leave the rule disabled.

[pdelvo]

I dont think that this is too slow. We only have to check it if the method is async and does not have any CancellationToken parameter. We could also cache per Compilation if a specific type symbol has a CancellationToken property in it

[sharwell]

During my implementation, I've made a few exclusions:

1. Private methods. In the event a cancellation token is required due to a change in the implementation of a private method, it is straightforward to add the parameter without introducing breaking API changes.
2. Overriding/implementing methods: Only the original/base definition for a method is checked.
3. Unit test methods. Test frameworks generally do not allow test entry points to include a cancellation token, so checking these would result in a large number of false positives.