dotnet / roslyn-analyzers

MIT License
1.6k stars 468 forks source link

CA1031 should treat `return Task.FromException(...)` as `throw` #3748

Open Pretasoc opened 4 years ago

Pretasoc commented 4 years ago

Analyzer package

Microsoft.CodeQuality.Analyzers

Package Version

v3.0.0 (Latest)

Diagnostic ID

Example: CA1031

Repro steps

Consider the following snippet:

public Task FooAsync()
{
    try
    {
        DoStuff()
        return Task.CompletedTask;
    }
    catch(Exception ex)
    {
        return Task.FromException(ex);
    }

}

Expected behavior

The code compiles without a warning. In my understanding returning a failed task from a method, with an awaitable result is the equivalent on rethrowing an exception in a non awaitable method.

Actual behavior

A CA1031 is raised on the catch(Exception ex) line.

jinek commented 3 years ago

In general a stack can not be recovered by simple copying of the exception reference.

This is the idea of CA1031: our code should stay transparent for unexpected exceptions, thus allowing the code which "is expecting" to do the job.

For example, exceptions thrown in DoStuff will not stop the debugger anymore (no more possible to see the stack, evaluate debugging stuff etc).

Another example, is if somebody calls FooAsync(); without even checking the returned value. In this case all exception there are just lost. (just to mention most of the exception even does not belong to DoStuff method. They are NullReference, OutOfMemory, TaskScheduler initialization exception etc)