Fix S2583 FP: IsNullOrEmpty / IsNullOrWhiteSpace confused with ==null

BrophyJeff commented 1 year ago

Description

Downstream code analysis after (value==null), string.IsNullOrEmpty(value), and string.IsNullOrWhiteSpace(null) make it clear that the methods are being treated as equivalent to null-checks, when they are not.

Repro steps

 public void SampleOne(string value)
 {
    if (value == null)
       throw new ArgumentNullException("value", "Value cannot be null.");
    if (string.IsNullOrEmpty(value)) // BUG: Change this condition so that it does not always evaluate to 'false'.
       throw new ArgumentException("Value cannot be empty.", "value");
    if (string.IsNullOrWhiteSpace(value))
       throw new ArgumentException("Value cannot be whitespace.", "value");
 }

 public void SampleTwo(string value)
 {
    if (string.IsNullOrWhiteSpace(value))
    {
       if (value == null)   // BUG: Change this condition so that it does not always evaluate to 'true'
          throw new ArgumentNullException("value", "Value cannot be null.");
       else if (string.IsNullOrEmpty(value))
          throw new ArgumentException("Value cannot be empty.", "value");
       else
          throw new ArgumentException("Value cannot be whitespace.", "value");
    }
 }

Expected behavior

A code branch where IsNullOrEmpty/IsNullOrWhiteSpace == false can ensure that a value is NOT null, but a branch whereIsNullOrEmpty/IsNullOrWhiteSpace == true does NOT mean that the value is null.

Actual behavior

No warning should be issued in either of the above sample cases.

Known workarounds

Replace string.IsNullOrEmpty() with explicit: value.Length == 0. Replace string.IsNullOrWhiteSpace() with explicit: value.Trim().Length == 0.

Related information

.NET framework 4.7.2 Unknown SQ version, since it's a build step managed by someone else.

mary-georgiou-sonarsource commented 1 year ago

Hell @BrophyJeff and thank you for reporting this.

I cannot reproduce this FP - I'll need the analyzer version to move further.

How do you analyze your project?

BrophyJeff commented 1 year ago

Analysis runs as part of our build pipeline. I don't know the specific version, sorry. --Jeff;

From: Mary Georgiou @.> Sent: Thursday, March 30, 2023 4:51 AM To: SonarSource/sonar-dotnet @.> Cc: Brophy, Jeff @.>; Mention @.> Subject: [EXT] Re: [SonarSource/sonar-dotnet] Fix S2583 FP: IsNullOrEmpty / IsNullOrWhiteSpace confused with ==null (Issue #6984)

Hell @BrophyJeffhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FBrophyJeff&data=05%7C01%7Cjeff.brophy%40finastra.com%7Ccc383d1c4fd44490b26f08db3114f9cc%7C0b9b90da3fe1457ab340f1b67e1024fb%7C0%7C0%7C638157738379780487%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=k1kfmgMbgtm%2FwkLltkHZFbLzrhCMlXknpsDlVFG2Ptc%3D&reserved=0 and thank you for reporting this.

I cannot reproduce this FP - I'll need the analyzer version to move further.

How do you analyze your project?

- Reply to this email directly, view it on GitHubhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FSonarSource%2Fsonar-dotnet%2Fissues%2F6984%23issuecomment-1490168876&data=05%7C01%7Cjeff.brophy%40finastra.com%7Ccc383d1c4fd44490b26f08db3114f9cc%7C0b9b90da3fe1457ab340f1b67e1024fb%7C0%7C0%7C638157738379936724%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=LzFw%2FUvwUpkJBAQvaoWIX601yZbBpdmmlYT5saa6JdY%3D&reserved=0, or unsubscribehttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FATWGT6AISX5SKL6T7YKA4VDW6VXQXANCNFSM6AAAAAAWFVEUOA&data=05%7C01%7Cjeff.brophy%40finastra.com%7Ccc383d1c4fd44490b26f08db3114f9cc%7C0b9b90da3fe1457ab340f1b67e1024fb%7C0%7C0%7C638157738379936724%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=m3up5Od8nJk8SiOG4XRPNt8hs%2FXt0QjDknJqW38IGgo%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.**@.>>

"FINASTRA" is the trade name of the FINASTRA group of companies. This email and any attachments have been scanned for known viruses using multiple scanners. This email message is intended for the named recipient only. It may be privileged and/or confidential. If you are not the named recipient of this email please notify us immediately and do not copy it or use it for any purpose, nor disclose its contents to any other person. This email does not constitute the commencement of legal relations between you and FINASTRA. Please refer to the executed contract between you and the relevant member of the FINASTRA group for the identity of the contracting party with which you are dealing.

mary-georgiou-sonarsource commented 1 year ago

Hello, @BrophyJeff - Is there a way to get the analysis logs somehow - or ask the administrator to extract them for you? Thanks a lot!

BrophyJeff commented 1 year ago

Looking at the output I can see, it does not appear our usage of SonarQube is writing any analysis logs. I did find the SonarQube version: 4.35.0

Build and analysis typically takes place on a virtual container which is discarded after the build completes. The SonarQube report is uploaded to a different site, where I can view it.

There is mention of "report metadata" written to a folder within the container. This doesn't sound like an analysis log. There is also a link to "more about the report processing" which I do not have permissions to view.

The original issue was pretty easy to replicate, so if you're not seeing it now it seems likely it was fixed.

int Test(string value) { if (value == null) return 0; if (string.IsNullOrEmpty(value)) // scan error: Expression is always False because (value != true) due to previous condition. return 1; if (string.IsNullOrWhiteSpace(value)) // scan error: Expression is always False because (value != true) due to previous condition. return 2; return 3; }

Workaround is to not use the Null-Or functions:

int Test(string value) { if (value == null) return 0; if (value == string.Empty) return 1; if (value.Trim() == string.Empty) return 2; return 3; }

--Jeff;

From: Mary Georgiou @.> Sent: Tuesday, April 11, 2023 6:30 AM To: SonarSource/sonar-dotnet @.> Cc: Brophy, Jeff @.>; Mention @.> Subject: [EXT] Re: [SonarSource/sonar-dotnet] Fix S2583 FP: IsNullOrEmpty / IsNullOrWhiteSpace confused with ==null (Issue #6984)

Hello, @BrophyJeffhttps://github.com/BrophyJeff - Is there a way to get the analysis logs somehow - or ask the administrator to extract them for you? Thanks a lot!

- Reply to this email directly, view it on GitHubhttps://github.com/SonarSource/sonar-dotnet/issues/6984#issuecomment-1503355892, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ATWGT6DIMA35ZBNPKPA6NV3XAVMFNANCNFSM6AAAAAAWFVEUOA. You are receiving this because you were mentioned.Message ID: @.***>

"FINASTRA" is the trade name of the FINASTRA group of companies. This email and any attachments have been scanned for known viruses using multiple scanners. This email message is intended for the named recipient only. It may be privileged and/or confidential. If you are not the named recipient of this email please notify us immediately and do not copy it or use it for any purpose, nor disclose its contents to any other person. This email does not constitute the commencement of legal relations between you and FINASTRA. Please refer to the executed contract between you and the relevant member of the FINASTRA group for the identity of the contracting party with which you are dealing.

mary-georgiou-sonarsource commented 1 year ago

Thanks a lot, @BrophyJeff!

I still cannot replicate this. In any case, if indeed the SQ version you are using is 4.35.0 then I strongly suggest you update it as this is a super old version.

I will close this issue for now - if something new comes up please feel free to re-open it.

Thanks a lot! Best regards Mary

SonarSource / sonar-dotnet