Fix S3928 FP: False positive for argument validation of local functions

[markusschaber]

Description

When local functions verify the parameters of the enclosing function (as it is sometimes necessary to prevent S4457 from triggering), S3928 triggers a false positive.

Repro steps

namespace FalsePositive_S3928_S4457;

public class Repro
{
    // This method triggers S4457 "Split this method into two, one handling
    // parameters check and the other handling the asynchronous code.
    public static async Task<string> ReproS4457Async(string argument)
    {
        if (string.IsNullOrWhiteSpace(argument))
            throw new ArgumentException("Bad", nameof(argument));

        if (!await SomeLongRunningOperationAsync(argument))
            throw new ArgumentException($"Cannot process {argument}", nameof(argument));

        var intermediate = await DoSomeProcessingAsync(argument);
        var result = await DoSomeOtherProcessingAsync(intermediate);
        return result;
    }

    // This method is now refactored to fulfil the recommendation of S4457. We chose
    // a local function, as the logic should not be exposed anywhere else.
    public static Task<string> ReproS3928Async(string argument)
    {
        if (string.IsNullOrWhiteSpace(argument))
            throw new ArgumentException("Bad", nameof(argument));

        return ProcessAfterSimpleVerificationAsync();

        async Task<string> ProcessAfterSimpleVerificationAsync()
        {
            // This now triggers S3928 on the line throwing the ArgumentException
            if (!await SomeLongRunningOperationAsync(argument))
                throw new ArgumentException($"Cannot process {argument}", nameof(argument));

            var intermediate = await DoSomeProcessingAsync(argument);
            var result = await DoSomeOtherProcessingAsync(intermediate);
            return result;
        }
    }

    private static Task<string> DoSomeOtherProcessingAsync(string intermediate) => throw new NotImplementedException();

    private static Task<string> DoSomeProcessingAsync(string argument) => throw new NotImplementedException();

    private static Task<bool> SomeLongRunningOperationAsync(string argument) => throw new NotImplementedException();

    public record struct SomeType;
}

Expected behavior

Throwing argument exceptions after verification should be allowed, especially when another analyzer rule forces the user to split the method into two parts.

Actual behavior

I get S3928 triggered on the line

Known workarounds

Extract into a "real" private method, but this exposes the logic to other methods of the class, which should be avoided (encapsulation principle).

Related information

• SonarLint for Rider Version 9.0.0.75308
• Jetbrains Rider 2023.2.1
• dotnet sdk 7.0.400
• Windows 11 (X64)

[rjgotten]

S3928 is doing the correct thing here. You shouldn't use an ArgumentException because the value in question was not a parameter to the local function. The local function which will appear as a stack frame in the stack trace and put those debugging the application on the wrong foot.

Nominally, you could resolve this problem for utility functions that check and throw an argument by hiding them from the stack trace. This is why the [StackTraceHidden] attribute exists. But that hides the entire method from the stack trace, including where any other exceptions occur within the method itself or any other functionality it calls into. So going that route is a no-go for methods that implement more general purpose logic.

The proper solution is: disable the S4457 rule and don't follow its advice. It's a bad rule and shouldn't be used. And causes more trouble than it's worth.

Splitting argument handling and throwing exceptions synchronously runs directly counter to Microsoft's own best practices wrt async functions and exception handling. E.g. the practices guide published by David Fowl at https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#prefer-asyncawait-over-directly-returning-task

[markusschaber]

Okay, I'll discuss this in the team and our QA people internally, and see whether we can disable S4457.

[markusschaber]

Should I file an issue to deprecate / remove S4457 from sonar-dotnet?

There's other open issues with this analyzer: #6368 and #6369 - those would get partially obsolete in case S4457 is removed.

[rjgotten]

The rule was, iirc, recently removed from 'the sonar way' profile. So deprecating it formally is just a skip away, but maybe a bit too soon for the comfort of some.

[gregory-paidis-sonarsource]

Hey there @markusschaber! @rjgotten did most of my job here, but just to add a bit more context. :)

The rule has indeed been removed from the SonarWay profile. You can think of this as a "soft" deprecation. It can either mean that it is very context-specific, so it is up to the users to enable it on their own, or that it is very noisy (raises when it should not) and we have to refactor the logic to make it behave in a more helpful manner.

I am guessing that you are running a custom profile and not the SonarWay one. This is perfectly fine, but unfortunately this means that the removal of the rule from the default profile did not affect you. I would also suggest that it is safe to remove S4457 from your profile.

For more context, you can see this discussion a while back.

Let me know if this covers your query.

Cheers, Greg

[markusschaber]

Yes, it covers my query. We actually use 2 different in-house profiles, one for "generic" C# code, and the other with some very specific additional rules for software using a specific in-house framework. I brought this item to the attention of the guys in charge (who'd been out of office last week), and just a few hours ago I got the news that the rule S4457 has been disabled in both profiles. I now checked here to give feedback. Thanks very much :-)