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.

Sign up for GitHub

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

Jump to bottom

Fix S1125 FN: recognize "is" and "is not" keyword with constant pattern #7687

Merged
merged 26 commits into from
Aug 4, 2023
Merged

Fix S1125 FN: recognize "is" and "is not" keyword with constant pattern #7687

merged 26 commits into from
Aug 4, 2023

Conversation

cristian-ambrosini-sonarsource
Copy link
Contributor

@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource commented Jul 26, 2023 •
edited
Loading

Fixes #2619
Fixes #7688

martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, removing GetLeftNode and GetRightNode from the base class was a mistake. Try to fix this first. Call me if you think you have good reasons or problems doing so to discuss it. I might be wrong with that request.

analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs Outdated Show resolved Hide resolved
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like how GetRightNode` returns a negate or a non-negated expression. This feels wrong. I made a suggestion to move the logic to another place which (hopefully) fit's better and resolves some complications 🤞. We can also have a chat about it.

analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs
private static bool CheckForNullability(SyntaxNode left, SyntaxNode right, SemanticModel model) =>
right is null // Avoids DeclarationPattern or RecursivePattern
|| model.GetTypeInfo(left).Type.IsNullableBoolean()
|| model.GetTypeInfo(right).Type.IsNullableBoolean();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this misses some cases with implicit conversion to bool. But that is another issue and doesn't need to be fixed. See e.g. sharplab

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can add an issue/reproducer later

analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs Outdated Show resolved Hide resolved
@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource changed the title Fix S1125 FN: recognize "is" keyword with constant pattern Fix S1125 FN: recognize "is" and "is not" keyword with constant pattern Aug 1, 2023
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Aug 2, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be easier to return the pattern for "GetRightNode".

analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessary.cs Outdated Show resolved Hide resolved
@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource mentioned this pull request Aug 3, 2023
Merged
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

There is one unresolved issue from previous rounds. Otherwise LGTM.

analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/BooleanLiteralUnnecessaryBase.cs Outdated Show resolved Hide resolved
martin-strecker-sonarsource
martin-strecker-sonarsource approved these changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. "Simplifier" might come in handy here as well but optional if you have time.

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs Show resolved Hide resolved
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 3, 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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 3, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

94.8% 94.8% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.16.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@pavel-mikula-sonarsource pavel-mikula-sonarsource merged commit 8f6674f into master Aug 4, 2023
@pavel-mikula-sonarsource pavel-mikula-sonarsource deleted the cristian/fix-S1125-FN branch August 4, 2023 07:55
This was referenced Oct 11, 2023
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-strecker-sonarsource martin-strecker-sonarsource martin-strecker-sonarsource approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@cristian-ambrosini-sonarsource @martin-strecker-sonarsource @pavel-mikula-sonarsource @CristianAmbrosini