SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
796 stars 228 forks source link

Fix S3236 FP: CallerArgumentExpressionAttribute should be overridable when using Nullable<> #9568

Open rjgotten opened 3 months ago

rjgotten commented 3 months ago

Description

S3236 is raised in cases where the user passes a caller expression explicitly, and the rule advises against doing this. However, there are cases wrt the expanded set of 'throw helpers' introduced by Microsoft in later .NET versions where passing a caller expression name explicitly is exactly the right thing to do, such as when having to check for negative value on nullable numeric properties.

Cases like ArgumentOutOfRangeException.ThrowIfNegative when used with nullables must use some form of guarded access to the underlying value to pass to the throw helper, because Nullable<> does not implement the requisite interface and the throw helper is not overloaded to understand Nullable<INumberBase>.

But in this case, the actual parameter that should be reported as in error, should still be the actual nullable numeric property - and not whatever inbetween value is used.

Repro steps

public class Test
{
  private readonly int _amount;
  public int? Amount
  {
     get => _amount;
     init
     {
       if (value is int val)
         // The exception actually *SHOULD* be reported for 'value', not for 'val' or for 'value.Value'
         ArgumentOutOfRangeException.ThrowIfNegative(val, nameof(value));

       _amount = value;
     }
  }

Expected behavior

S3236 is not raised in situations that could be identified as explicitly passing the related parameter name to a throw helper.

Actual behavior

S3236 is raised in situations that could be identified as explicitly passing the related parameter name to a throw helper.

Known workarounds

None. Suppress the rule.

Related information

sebastien-marichal commented 3 months ago

Hello @rjgotten,

This is indeed quite inconvenient; I confirm this to be a false positive.

I think it is difficult to decide whether it makes sense or not to override caller information arguments. We would need to add a very specific exception for this scenario:

I am not sure if there could be scenarios where it would produce false negatives.