Consider not emitting CS1998 for interface implementations / method overrides

[stephentoub]

CS1998 warns about async methods that lack any awaits. There's a balance to be struck here, as there are valid scenarios for this to happen, especially when you're forced to have a Task-returning method due to external requirements, e.g. you're overriding a Task-returning method or you're implementing an interface method. I end up seeing a non-trivial amount of code that gets this warning in such cases, and then rather than suppressing it, they add in an unnecessary await, e.g. await Task.FromResult(result), which doesn't make the method any "safer" but does make it more expensive.

I suggest we consider not emitting CS1998 when the async method in question is an override or is part of implementing an interface.

cc: @jcouv

[CyrusNajmabadi]

Perhaps a different warning id that can be suppressed independently?

[stephentoub]

Perhaps a different warning id that can be suppressed independently?

If we could make it a suggestion/ info by default, sure. My concern is the default warning is leading some folks to the wrong default outcome.

[jcouv]

Why wouldn't the user remove the async in such case and just implement as a normal Task-returning method (ie. return Task.FromResult(...) without await) to satisfy the interface/override?
Maybe an example would be useful.
We could adjust the diagnostic message to make it clear that is a valid option.

[stephentoub]

Why wouldn't the user remove the async in such case and just implement as a normal Task-returning method (ie. return Task.FromResult(...) without await) to satisfy the interface/override? Maybe an example would be useful. We could adjust the diagnostic message to make it clear that is a valid option.

Just using Task.FromResult is often "incorrect", e.g. if I have:

public override Task<Stream> GetStreamAsync()
{
    return Task.FromResult<Stream>(new FileStream(...));
}

that fails to catch any exceptions from the FileStream ctor and store them into the returned task, so it really need to be more like:

public override Task<Stream> GetStreamAsync()
{
    try
    {
        return Task.FromResult<Stream>(new FileStream(...));
    }
    catch (Exception e)
    {
        return Task.FromException<Stream>(e);
    }
}

which is a whole lot more (and requiring of more thought) than:

public override async Task<Stream> GetStreamAsync() => new FileStream(...);

I've been seeing this repeatedly in various codebases, where folks get the warning and then think the answer is to use an await, when it's not.

[CyrusNajmabadi]

This has often felt like a warning that was more useful early on when people were learning about async/await. I find it mostly a nuisance now.

[stephentoub]

This has often felt like a warning that was more useful early on when people were learning about async/await. I find it mostly a nuisance now.

Maybe the answer is just downgrade the warning to a suggestion in general?

[333fred]

After discussion, and especially with runtime async, we will be removing this warning entirely.