Why do you believe it’s a false-positive/false-negative? I have made recommended changes
Are you using
SonarQube Cloud? yes
SonarQube Server / Community Build - which version? no
SonarQube for IDE - which IDE/version? yes (for Rider, 10.12.0.79769)
in connected mode with SonarQube Server / Community Build or SonarQube Cloud? (tried both)
How can we reproduce the problem? Give us a self-contained snippet of code (formatted text, no screenshots)
var singleBars = (ISingleBars)rebargrp;
if (singleBars == null) {
AddRuntimeMessage(GH_RuntimeMessageLevel.Warning, "Rebar group is null.");
return;
}
var positions = singleBars.Positions;
if (positions == null) {
AddRuntimeMessage(GH_RuntimeMessageLevel.Warning, "Positions are null for the rebar group.");
return;
}
foreach (var pos in positions) {
...
}
The problem is on the positions inside the foreach. I have tried a lot of other ways of doing that that picked up from different posts. like putting the property in a variable so SonarQube is seeing that I made the check for that variable, not use complex pattern matching in switch statements (that is why I am also checking singleBars)
I am not sure what is the problem, but I don’t think it is related on this particular piece of code, as it looks correct to me. Could it be related to a comment set on the singleBars.Positions property API that warns thing might be null? Which take precedence?
/// <summary>The positions for the <see cref="P:Oasys.AdSec.Reinforcement.Groups.ISingleBars.BarBundle" />s.</summary>
/// <exception cref="!:System.ArgumentNullException">If Positions is set to null</exception>
IList<IPoint> Positions { get; set; }
Although I think your snippit should not report anything, you might want to refactor, using pattern matching anyway:
if (rebargrp is not ISingleBars singleBars)
{
AddRuntimeMessage(GH_RuntimeMessageLevel.Warning, "Rebar group is null.");
return;
}
if (singleBars.Positions is not { } positions) {
AddRuntimeMessage(GH_RuntimeMessageLevel.Warning, "Positions are null for the rebar group.");
return;
}
foreach (var pos in positions) {
// ...
}
Hey @spsarras thanks for your report.
This looks like a clear FP to me. However, I cannot reproduce it locally. Could you please do one of the following:
Check the issue in SonarQube Cloud and give us a screenshot of the issue. The newly added Execution Flows should give us a hint at what is going wrong here.
Create a fully self-contained reproducer that I can analyze without making assumptions about the rest of the solution.
For reference, this is my attempt at reproducing it (SharpLab link):
void Method(object rebargrp)
{
var singleBars = (ISingleBars)rebargrp;
if (singleBars == null) {
AddRuntimeMessage(GH_RuntimeMessageLevel.Warning, "Rebar group is null.");
return;
}
var positions = singleBars.Positions;
if (positions == null) {
AddRuntimeMessage(GH_RuntimeMessageLevel.Warning, "Positions are null for the rebar group.");
return;
}
foreach (var pos in positions) {
Console.Write(pos);
}
}
void AddRuntimeMessage(object warning, string v)
{
throw new NotImplementedException();
}
internal static class GH_RuntimeMessageLevel
{
public static object Warning { get; }
}
internal interface ISingleBars
{
IEnumerable<object> Positions { get; }
}
I could do you one better! This is an open source project, so you should be able to find the report on our dashboard here: SonarQube Cloud
This appears (correctly) on the main branch: SonarQube Cloud
This is the commit I’ve added the ignore. AdSecGh-64 spike testing component cherry by psarras · Pull Request #105 · arup-group/AdSec-Grasshopper · GitHub and as you can see the previous commit, also had a failed SonarQube and I was using a switch case, where it guarantees that rebargrp is of that type. I did also try to do further (unnecessary) checks as above, to try and convince SonarQube to stop complaining Unfortunately, I would need to revert to demonstrate you that sonarQube on the dashboard was actually complaining with the following solution.
Ideally this should be enough:
// loop through rebar groups in flattened section
foreach (var rebargrp in flat.ReinforcementGroups) {
switch (rebargrp) {
case ISingleBars singleBars:
var positions = singleBars.Positions;
if (positions == null) {
AddRuntimeMessage(GH_RuntimeMessageLevel.Warning, "Positions are null for the rebar group.");
return;
}
foreach (var pos in positions) {}
}
}
let me know if I can provide more files. I can perhaps set a new branch that shows the problem on the daskboard for you.
The difference? There is a Try/Catch on the first one.
I think it still is a FP, I don’t understand why…but the reason I changed to a switch statement was to remove the try/Catch so I can fix the actual FP in my case but it still seems to misbehave if you have a try catch around a switch?
I managed to reproduce it and identify the underlying issue. Thanks for sharing all the details. I created an internal ticket to track the issue and it will be fixed in a future hardening sprint.
For reference, here is my reproducers:
void Method(IEnumerable<object> rebargrps)
{
foreach (var rebargrp in rebargrps)
{
try // first try if not a link group type
{
var snglBrs = (ISingleBars)rebargrp;
foreach (var pos in snglBrs.Positions) // FP
{
Console.Write(pos);
}
}
catch (Exception)
{
}
}
}
Unfortunately, I would need to revert to demonstrate you that sonarQube on the dashboard was actually complaining with the following solution.
