S3900 warnings and extension methods

[Bosch-Eli-Black (Eli Black (Bosch))]

S3900: “Arguments of public methods should be validated against null” currently fires for the this parameter in C# extension methods:

public void LogCustomError(this ILogger logger, string errorMessage)
{
   // Triggers S3900 for `logger`
   logger.LogError(errorMessage);
}

This seems questionable, I think, because if someone calls an extension method on a null variable, then we’d actually expect to get a NullReferenceException:

ILogger myLogger = null;

// We'd expect this to throw a NullReferenceException, even though LogCustomError() is an extension method.
myLogger.LogCustomError("encountered error");

Microsoft has CA1062, which does the same thing as S3900, and there’s a way to disable the CA1062 warning for extension methods: CA1062: Validate arguments of public methods (code analysis) - .NET | Microsoft Learn

I’d like to request that SonarQube also have some way to disable S3900 warnings for extension methods

(Alternatively, perhaps the best way for us to handle this is to activate CA1062 warnings and disable S3900 warnings, since they’re essentially duplicates?)

[Colin]

Hey there.

As noted in this post:

How to Report a False-positive / False-negative

Hey SonarSource Community! False-positives happen, as do false-negatives, and we’re eager to fix them. We are thrilled when our users report problems, so we can make our products better. What is a false-positive (FP)? A false-positive is when an issue is raised unexpectedly on code that should not trigger an issue, or where the suggested action doesn’t make any sense for the code. What is a false-negative (FN)? A false-negative is when an issue should be raised on a piece of code, but isn’t…

Please share this information:

Which product(s) you’re using

• SonarQube
  • If so, which version of SonarQube?
• SonarCloud
• SonarLint
• If yes, with which IDE and which version?
  • If you’re using Connected Mode, tell us with which product (if it’s SonarQube, see the above notes on providing version details)

[Bosch-Eli-Black (Eli Black (Bosch))]

Sorry. Details below

• SonarQube
  • Enterprise Edition Version 8.9.9 (build 56886)
• SonarCloud
  • No
• SonarLint
  • No

[Corniel (Corniel Nobel)]

This seems questionable, I think, because if someone calls an extension method on a null variable, then we’d actually expect to get a NullReferenceException (…)

Do we? I would strongly disagree. Extension methods are (nice) syntactic sugar. I would argue that you what to guard and throw an ArgumentNullException instead.

[costin.zaharia]

Hi @Bosch-Eli-Black,

Thanks for your feedback.

I my opinion it’s better to handle the “this” parameter in a similar way to be able to “fail fast”. You could also manually check for null and throw a NullReferenceException.