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

S1226: rule doesn't consider null checking as a read #2603

Closed
Elanis opened this issue Aug 26, 2019 · 6 comments
Closed

S1226: rule doesn't consider null checking as a read #2603

Elanis opened this issue Aug 26, 2019 · 6 comments
Assignees
Labels
Area: C# C# rules related issues. Type: False Positive Rule IS triggered when it shouldn't be.

Comments

@Elanis
Copy link

Elanis commented Aug 26, 2019

Hello,

This issue is about rule S1226: Method parameters, caught exceptions and foreach variables' initial values should not be ignored.

When we run analysis on our projects with this kind of code:

public ReturnObject Foo(MyParamObject bar) {
    if (bar == null)
         bar = new MyParamObject(); // Sonar raise a S1226 bug here
    }

    // ...
}

Sonar detect it as a bug, but the variable is read (null checking). Is it a false positive ?

Sonarqube version: 7.9.1.27448
Sonar C# version: 7.16 (build 8981)
SonarScanner for Team Foundation Server version: 4.7.2
SonarScanner-Msbuild version: 4.6.2

Thanks :)

@Corniel
Copy link
Contributor

Corniel commented Aug 27, 2019

I think Sonar is ride here. You should do something like:

public ReturnObject Foo(MyParamObject bar) {
    var b = bar ?? new MyParamObject();
    // ...
}

Just as suggested, you should not reuse the parameter, but instead introduce a new variable.

@Elanis
Copy link
Author

Elanis commented Aug 29, 2019

The rule description says

While it is technically correct to assign to parameters from within method bodies, doing so before the parameter value is read is likely a bug. Instead, initial values of parameters, caught exceptions, and foreach parameters should be, if not treated as final, then at least read before reassignment.

I read this as "Read the parameter at least one time, then you can reuse/overwrite it". Am I wrong ?

@neolution-ch2
Copy link

#2555 is related to this

@Corniel
Copy link
Contributor

Corniel commented Aug 30, 2019

@Elanis : I assume (always tricky) that the implementation triggers the rule when a reassign is done, no matter if is validated. The description is therefore confusing/not matching the behaviour. I would argue though, that you should never reassign a parameter, but introduce a new one, if you want to change the value before proceeding, just as I suggested.

But let us wait what the Sonar guys/girls think about this.

@andrei-epure-sonarsource andrei-epure-sonarsource added this to the Support milestone Sep 26, 2019
@costin-zaharia-sonarsource
Copy link
Member

It is indeed a false positive. I'm closing this issue as a duplicate of #2555.

@costin-zaharia-sonarsource costin-zaharia-sonarsource added Area: C# C# rules related issues. Area: Rules Type: False Positive Rule IS triggered when it shouldn't be. labels Nov 6, 2019
@Elanis
Copy link
Author

Elanis commented Nov 6, 2019

Thanks for your answer :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Type: False Positive Rule IS triggered when it shouldn't be.
Projects
None yet
Development

No branches or pull requests

5 participants