Replace string with nameof in ArgumentException is too aggressive

[Isaks (Isak Savo)]

The following code will raise S2302 - Replace the string 'changes' with nameof(changes)

private void DoStuff(int changes)
{
    if (changes < 0)
        throw new ArgumentException("Too few changes specified");    
}

The fact that the argument name changes happens to match the word changes in the exception message is not a bug but rather the result of a sensible variable names.

Of course, for the second parameter to ArgumentException, then nameof should always be used (sonar correctly catches this with the same rule).

My suggestion is to only worry about the paramName argument to ArgumentException and its derived types. Anything else has a very high risk of false positives

[Dmitry_Golovinov]

Good morning, @Isaks !

Based on your feedback we’ve created the following issue in our repo Fix S2302 FP: Rule should be less sensitive for message of Argument exceptions · Issue #4454 · SonarSource/sonar-dotnet (github.com). Let me know if you have a feedback to share.

[Dmitry_Golovinov]

We have to admit, that even by fixing the issue, I’ve posted above, your code will still raise the issue.
However, the following code won’t raise an issue:

private void DoStuff(int changes)
{
    if (changes < 0)
        throw new ArgumentException("Too few changes specified", nameof(changes));    
}

[Isaks (Isak Savo)]

That’s great news, thanks.

Just so I understand, the following is still non-compliant:

throw new ArgumentException("too few changes specified")

but this is fine and will not produce a warning:

throw new ArgumentException("too few changes specified", nameof(changes));

?

That’s ok with me (even if I’d prefer it to only validate the paramName argument)

What about other types of exception, not derived from ArgumentException (and therefore won’t have an paramName argument.) Will the following raise an error?

throw new InvalidOperationException("Too few changes specified");

[Dmitry_Golovinov]

@Isaks

Just so I understand, the following is still non-compliant:

throw new ArgumentException("too few changes specified")

but this is fine and will not produce a warning:

throw new ArgumentException("too few changes specified", nameof(changes));

?

Yes, that’s right.

Regarding InvalidOperationException and co. There are no plans to change the rule behaviour for any other exceptions, that do not take the parameter name as an argument and it is still advised to use nameof there.