As of Nov 7th the only sonar warnings were were getting were three S3267 false positives like this.
Warning S3267: Loop should be simplified by calling Select(identifiedSet => identifiedSet.Value))
The next PR was submitted Nov 20th that added only a unit test and removed a line from the nuget.config. The pipeline logs now have 200+ S3900 warnings, and every single one is a false positive. This is an example of the warning…
Warning S3900: Refactor this method to add validation of parameter ‘items’ before using it.
This is the function signature that message specifically is complaining about.
public string CreateMultiQuery(List<FlatCandidateToEvaluate> items)
{
_ = items.NullIfEmpty() ?? throw new ArgumentNullException(nameof(items));
var indexInBatch = 0;
var itemQueries = new List<string>(items.Count);
This is definitely getting checked as evidenced by the first statement. The extension method being called is straight forward.
[SuppressMessage("Major Code Smell", "S1168:Empty arrays and collections should be returned instead of null",
Justification = "This needs to return null so that argument checks can be '?? throw new ArgumentNullException(nameof(param))' ")]
public static ICollection NullIfEmpty(this ICollection value)
{
if (value == null)
{
return null;
}
if (value.Count == 0)
{
return null;
}
return value;
}
This null guarding pattern is not uncommon, and was not a problem for the last PR. The S3267 warnings have rightfully stopped showing up, but now there are exponentially more warnings to sift through to find real warnings. Please fix ASAP, and kindly avoid suggesting to turn off the rule as that is not something we have control of (nor would we want to if we could). Thanks!
To give some context why we haven’t addressed it yet:
our Symbolic Execution engine (for dataflow bug detection), based on our in-house control flow graph (CFG) implementation, has some structural limitations. Also, it is not cross-procedural, so it only analyzes the data flow in individual methods.
because of that, we’re currently in the process of migrating to use the Roslyn CFG (which is much better) and writing a new Symbolic Execution engine on top of it (because the old one is tightly coupled with our in-house CFG). You can track progress of the end-to-end work here: MMF-2105, with the mention that once we migrate to the Roslyn CFG, our Symbolic Execution rules will work for C# 9, C# 10 and future language versions out of the box (as the Roslyn CFG normalizes new syntax and provides an abstraction layer for dealing with the program structure). S3900 will be handled in MMF-2401.
Having this said, I’ll look into how we can reduce the noise of the current implementation until we finish the migration.
Thanks for reporting this and sorry for the noise. We’re going to improve this in time. However it will take a while to go cross-procedural (this is not planned yet).
I’d like to understand better what happened here. Was it that there were no S3900 warnings at all before, or after these changes, the existing S3900 warnings were brought up in the new code period? (see details on terminology in Clean As You Code).
They were not there before the PR and branch were created. The PR and branch’s only changes were to a unit test and a non-code file. All of the warnings started happening for code that was not touched as part of this PR.
Thanks for the tip on the attribute but I’m not adding 200+ [ValidatedNotNull] attributes in a codebase just to appease a new sonar warning thats FP-ing. I saw that the extension method fix is in progress and will wait until then.
The fix will only work if the validation extension method has the attribute. You use the extension method to check that the param is null so the ValidatedNotNull would be a bit counterintuitive.
It’s a bit weird that this wasn’t raised before…
In any case, I imagine you don’t have 200+ extension methods, to fix, right?
No, just one or two. But it will still require a user story be written up and pushed through the process (estimation, sprint planning, etc), which is not terrible, but I didnt do anything to cause these to start happening. Why should I have to do anything to make them stop? =/
The other possible weirdness is that I use these extension methods in a few other repos and I dont recall this kind of problem with those CI builds. I have to double check on that but am getting permissions errors at the moment so it will have to wait.
Was there any change in terms of version of msbuild, scanner for .net, sonarqube? I’m trying to understand why this appeared out of a sudden whereas the rule has had this behavior for a long while.