Remove S4457 from SonarWay

[MxNbrt]

S4457 tells me that my async code should be splitted into synchronous data validation and asynchronous data processing. E.g the compliant solution looks like this:

public static Task SkipLinesAsync(this TextReader reader, int linesToSkip)
{
    if (reader == null) { throw new ArgumentNullException(nameof(reader)); }
    if (linesToSkip < 0) { throw new ArgumentOutOfRangeException(nameof(linesToSkip)); }

    return reader.SkipLinesInternalAsync(linesToSkip);
}

My problem here is that last line which returns the Task directly instead of awaiting it. This conflicts with Microsofts recommendation
"Prefer async/await over directly returning Task" for async code which we followed everywhere in our code

So the correct solution according to Microsoft would be to make the whole method async and return await reader.SkipLinesInternalAsync(linesToSkip). After i got confused, I did what every developer would do and ended up at this stackoverflow answer which also claimed that the rule is "overly cautious".

So now I am not quite sure which coding style to follow and whether the rule S4457 is even reasonable.
Can you please clearify what is the purpose of S4457 and which code style would be best to follow?

Thank you very much

[rjgotten]

The rule is overly cautious.
All code up to the first await is always executed synchronously, i.e. any ArgumentException or derivative that is thrown before the first await will execute synchronously. This isn't just an implementation detail. Several parts of and places in e.g. ASP.NET actually rely on this behavior. (Notably the hosted services feature is such a place.)

If the criterium is fail-fast, then the correct rule implementation would be to not allow ArgumentException passed the first await - only before it.

From a debugging perspective, this pattern where you wrap calls to an async method with a synchronous method to perform parameter validation and have argument exceptions thrown without having to observe tasks, is actually more than overly cautious. It's downright counter-productive to the intent of clarity towards consuming developers as well.
Reason being; if an exception occurs within the async portion of the method, then the synchronous method -- which will in 99.99% of cases be the actual public method wheras the async one will be either a mangled local function or a private method -- will not be part of the stack trace. I.e. there will be a discoverability problem for which call actually failed.

(I guess you could say the synchronous wrapper is actually... exceptionally bad.)

[andrei-epure-sonarsource]

Thank you for opening this issue, @MxNbrt . Your feedback helps us improve our products. And thank you, @rjgotten , for bringing the additional details.

We will look at it as soon as possible and come up with an answer.

[gregory-paidis-sonarsource]

Hey @MxNbrt ,

Can you please clearify what is the purpose of S4457 and which code style would be best to follow?

Here is an example:

DoWorkAsync(null);          // fire-and-forget

// async-await used,
// treated as a special method for the async state machine,
// re-written by the compiler.
static async Task DoWorkAsync(int? maybeValue)
{
    if (!maybeValue.HasValue)
    {
        throw new ArgumentNullException(nameof(maybeValue));
    }

    await Task.Delay(42); // Actual work happens here
}

In this snippet, the "fire-and-forget" line will not throw an exception.

This is because exceptions raised in asynchronous methods are placed on the returned task.

If said task is not awaited, the exception is also not propagated to the call site, which could lead to potential bugs.

The correct approach would be something like the following:

DoWorkAsync(null);          // fire-and-forget

// no async-await used, treated as a normal method
static Task DoWorkAsync(int? maybeValue)
{
    if (!maybeValue.HasValue)
    {
        throw new ArgumentNullException(nameof(maybeValue));
    }

    return Task.Delay(42); // Actual work happens here
}

In this case, the fire-and-forget method call will raise as expected.

"Prefer async/await over directly returning Task" for async code which we followed everywhere in our code

Notice that the await DoWorkAsync(null); method call raises in both circumstances, so if your codebase is using "async/await all the way down" as you mentioned, as well as not exposing any public APIs that a third-party application might use (without using async/await), you can safely ignore this rule or mark it as "won't fix".

I hope the snippets provided above clarify the utility of this rule.

[rjgotten]

The 'but fire-and-forget tasks' argument is, respectfully, bullshit.

The whole point of fire-and-forget is that you kick something off, and then forget about it.
If you need to care about the arguments and need to care that the task actually completes successfully, then you are using the wrong pattern.

Meanwhile, to pander to a very specific corner case that maybe makes up 1% of the total case, this style rule enforces work on 99% of the other cases. To needlessly restructure code to comply with the rule's silly wishes; to explicitly mark each individual occurrence of a rule violation as 'won't fix' in a central Sonarqube instance; or to add suppressions to each individual occurrence in code. It's a rule that, when activated, has in practicality a 99% false positive ratio needing workarounds.
And rules that have a 99% false positive ratio aren't very good rules.

Moreover, the proposed fix allowing ArgumentException and derivatives to be thrown synchronously, relies on an internal implementation detail of the async/await state machine. The fact that code runs synchronously until the first await statement is an artifact of Microsoft's current implementation. And that implementation is afaik not guaranteed to remain. There have been extensive rewrites to the state machine generating code in the past; both for general performance reasons; as well as for added features such as when support for the lightweight ValueTask type was baked in.

In fact, it's been possible for years for users to write their own async method builder and to tell the runtime to use that. Unity has libraries like Uni.AsyncRx that do this so you can tune performance characteristics to be more in line with what you'd expect from a game engine. And there are more than likely other examples of this, esp. in performance-sensitive context.

In other words: your rule's proposed style improvement hinges on an internal implementation detail you cannot guarantee.
The only way you can guarantee receiving an exception back, is to actually await the task.

S4457 is wrong; stupid and backwards and if you have no plans to remove it; then at the very least it should be disabled by default and should contain a big fat comment warning about the caveats.

---

Microsoft actually has a style rule that handles unawaited - i.e. fires-and-forget - tasks, btw. It warns users when they execute an async method without either awaiting it; or assigning its result anywhere. Explicit fire-and-forget tasks can satisfy the rule by assigning the result to a discard, e.g.

_ = DoFireAndForgetAsync(someData);

[MxNbrt]

Hello @gregory-paidis-sonarsource

Thanks for clarifying, what your thoughts were when implementing the rule. Nonetheless i have to agree to @rjgotten and think that the fire-and-forget pattern should itself be a rule that is being violated here. The fire-and-forget pattern is bad and should only be used in a very small percentage of use cases.

Thats why you should get rid of S4457 or at least disable it as a default.

Thanks and have a nice day

[gregory-paidis-sonarsource]

Hello again @MxNbrt and @rjgotten,

After some testing, as well as internal discussion with the team, we decided to remove this rule from the SonarWay profile.