Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix S3900 and S2259 FN: Support method dereference #7011

Merged
merged 3 commits into from
Mar 31, 2023

Conversation

pavel-mikula-sonarsource
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource commented Mar 30, 2023

Part of #6997

There will be a rebase conflict with #7004 refactoring.

Copy link
Contributor

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
One question about the test cases.


public void WithDelegate(object asParameter, object asVariable)
{
if(asParameter == null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Educational] How is checking for null sets the null state for these variables? Shouldn't S2259 only raise an issue if it's certain that the variable null when it's being dereferenced? Shouldn't it be like this:

if(asParameter == null)
{
    SomeAction(asParameter.ToString);       // Noncompliant
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if(asParameter=null) {
  // Here, we're certain that asParametr has Null
} else {
  // Here, we're certain that asParamet has NotNull. Even when it's empty.
}
// Here, we will arrive twice: Once asParametr=Null, once asParametr=NotNull

More user-likely scenario is

if(arg !=null) {
  DoSomething(arg.ToString)
} else {
  // And here, we're sure arg==Null. Even if the `else` is not present, such state is possible
}
arg.ToString(); // Noncompliant, this should have been in the "if" branch. We arrive twice here.

@sonarcloud
Copy link

sonarcloud bot commented Mar 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Mar 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants