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 S1125 FN: recognize "is" and "is not" keyword with constant pattern #7687

Merged
merged 26 commits into from
Aug 4, 2023
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
607dcf0
First implementation
CristianAmbrosini Jul 25, 2023
7b9dfb9
Add ternary exception
CristianAmbrosini Jul 25, 2023
ff56b70
Fix CheckForNullabilityAndBooleanConstantsReport
CristianAmbrosini Jul 26, 2023
2f773ca
Add code fix
CristianAmbrosini Jul 26, 2023
4ebcf3e
Add link to C#9 issue
CristianAmbrosini Jul 26, 2023
dffbc9f
Fix UTs
CristianAmbrosini Jul 26, 2023
689bb62
Fix code smells
CristianAmbrosini Jul 26, 2023
8252f7f
Merge IsPatternExpression check with IsEqual
CristianAmbrosini Jul 31, 2023
20539ad
Move methods around
CristianAmbrosini Jul 31, 2023
3dc5275
Fix AD0001
CristianAmbrosini Jul 31, 2023
3997b6c
Add more UTs
CristianAmbrosini Jul 31, 2023
87af3f8
Fix quickFixes for "is" and "is not" (one issue still pending)
CristianAmbrosini Jul 31, 2023
58934be
Fix non-defined quick-fixes
CristianAmbrosini Jul 31, 2023
cffb15c
Static local function
CristianAmbrosini Jul 31, 2023
35a7cb6
Remove TBinaryExpression
CristianAmbrosini Aug 1, 2023
8679ce3
Add UTs
CristianAmbrosini Aug 1, 2023
25f48ea
Move to IsTrue/IsFalse to SyntaxNodeExtension
CristianAmbrosini Aug 1, 2023
1678236
Support basic cases on SyntaxNodeExtension methods
CristianAmbrosini Aug 1, 2023
f088cd2
Fix quick-fix
CristianAmbrosini Aug 2, 2023
b9988d5
Remove useless using
CristianAmbrosini Aug 2, 2023
b58a7ce
Remove whitespace
CristianAmbrosini Aug 2, 2023
04bc0c9
Update expected issues location
CristianAmbrosini Aug 2, 2023
fe3996c
Add support for parenthesized expressions
CristianAmbrosini Aug 2, 2023
52ad42e
Remove stuff from GetOperatorToken
CristianAmbrosini Aug 2, 2023
81901f7
Address more comments
CristianAmbrosini Aug 3, 2023
019043e
Add .WithAdditionalAnnotations(Simplifier.Annotation) on code fix
CristianAmbrosini Aug 3, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Ember-MM/Ember%20Media%20Manager/dlgManualEdit.vb#L259",
"region": {
"startLine": 259,
"startColumn": 24,
"startColumn": 22,
"endLine": 259,
"endColumn": 29
}
Expand All @@ -72,7 +72,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Ember-MM/Ember%20Media%20Manager/dlgManualEdit.vb#L376",
"region": {
"startLine": 376,
"startColumn": 22,
"startColumn": 20,
"endLine": 376,
"endColumn": 27
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Ember-MM/EmberAPI/clsAPIDatabase.vb#L112",
"region": {
"startLine": 112,
"startColumn": 64,
"startColumn": 62,
"endLine": 112,
"endColumn": 69
}
Expand Down
8 changes: 4 additions & 4 deletions analyzers/its/expected/Nancy/Nancy--net452-S1125.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/TinyIoc/TinyIoC.cs#L3289",
"region": {
"startLine": 3289,
"startColumn": 76,
"startColumn": 73,
"endLine": 3289,
"endColumn": 81
}
Expand Down Expand Up @@ -111,7 +111,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/ViewEngines/SuperSimpleViewEngine/SuperSimpleViewEngine.cs#L284",
"region": {
"startLine": 284,
"startColumn": 44,
"startColumn": 41,
"endLine": 284,
"endColumn": 49
}
Expand All @@ -124,7 +124,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/ViewEngines/SuperSimpleViewEngine/SuperSimpleViewEngine.cs#L306",
"region": {
"startLine": 306,
"startColumn": 45,
"startColumn": 42,
"endLine": 306,
"endColumn": 50
}
Expand All @@ -137,7 +137,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/ViewEngines/SuperSimpleViewEngine/SuperSimpleViewEngine.cs#L487",
"region": {
"startLine": 487,
"startColumn": 53,
"startColumn": 50,
"endLine": 487,
"endColumn": 58
}
Expand Down
8 changes: 4 additions & 4 deletions analyzers/its/expected/Nancy/Nancy--netstandard2.0-S1125.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/TinyIoc/TinyIoC.cs#L3289",
"region": {
"startLine": 3289,
"startColumn": 76,
"startColumn": 73,
"endLine": 3289,
"endColumn": 81
}
Expand Down Expand Up @@ -111,7 +111,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/ViewEngines/SuperSimpleViewEngine/SuperSimpleViewEngine.cs#L284",
"region": {
"startLine": 284,
"startColumn": 44,
"startColumn": 41,
"endLine": 284,
"endColumn": 49
}
Expand All @@ -124,7 +124,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/ViewEngines/SuperSimpleViewEngine/SuperSimpleViewEngine.cs#L306",
"region": {
"startLine": 306,
"startColumn": 45,
"startColumn": 42,
"endLine": 306,
"endColumn": 50
}
Expand All @@ -137,7 +137,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/Nancy/src/Nancy/ViewEngines/SuperSimpleViewEngine/SuperSimpleViewEngine.cs#L487",
"region": {
"startLine": 487,
"startColumn": 53,
"startColumn": 50,
"endLine": 487,
"endColumn": 58
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka/Configuration/Config.cs#L445",
"region": {
"startLine": 445,
"startColumn": 36,
"startColumn": 33,
"endLine": 445,
"endColumn": 41
}
Expand All @@ -20,7 +20,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka/Dispatch/Mailbox.cs#L385",
"region": {
"startLine": 385,
"startColumn": 80,
"startColumn": 77,
"endLine": 385,
"endColumn": 85
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka.Cluster/ClusterHeartbeat.cs#L663",
"region": {
"startLine": 663,
"startColumn": 48,
"startColumn": 45,
"endLine": 663,
"endColumn": 53
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka.MultiNodeTestRunner.Shared/Sinks/ConsoleMessageSinkActor.cs#L54",
"region": {
"startLine": 54,
"startColumn": 57,
"startColumn": 54,
"endLine": 54,
"endColumn": 62
}
Expand All @@ -20,7 +20,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka.MultiNodeTestRunner.Shared/Sinks/ConsoleMessageSinkActor.cs#L59",
"region": {
"startLine": 59,
"startColumn": 71,
"startColumn": 68,
"endLine": 59,
"endColumn": 76
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka.Remote.TestKit/Player.cs#L510",
"region": {
"startLine": 510,
"startColumn": 113,
"startColumn": 110,
"endLine": 510,
"endColumn": 118
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka.TestKit/EventFilter/Internal/EventFilterApplier.cs#L410",
"region": {
"startLine": 410,
"startColumn": 42,
"startColumn": 39,
"endLine": 410,
"endColumn": 47
}
Expand All @@ -20,7 +20,7 @@
"uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/akka.net/src/core/Akka.TestKit/EventFilter/Internal/EventFilterApplier.cs#L433",
"region": {
"startLine": 433,
"startColumn": 42,
"startColumn": 39,
"endLine": 433,
"endColumn": 47
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,30 @@ private static string GetUnknownType(SyntaxKind kind) =>

#endif

public static bool IsTrue(this SyntaxNode node) =>
node switch
{
{ RawKind: (int)SyntaxKind.TrueLiteralExpression } => true, // true
{ RawKind: (int)SyntaxKind.LogicalNotExpression } => IsFalse(((PrefixUnaryExpressionSyntax)node).Operand), // !false
{ RawKind: (int)SyntaxKindEx.ConstantPattern } => IsTrue(((ConstantPatternSyntaxWrapper)node).Expression), // is true
{ RawKind: (int)SyntaxKindEx.NotPattern } => IsFalse(((UnaryPatternSyntaxWrapper)node).Pattern), // is not false
{ RawKind: (int)SyntaxKind.ParenthesizedExpression } => IsTrue(((ParenthesizedExpressionSyntax)node).Expression), // (true)
{ RawKind: (int)SyntaxKindEx.ParenthesizedPattern } => IsTrue(((ParenthesizedPatternSyntaxWrapper)node).Pattern), // is (true)
_ => false,
};

public static bool IsFalse(this SyntaxNode node) =>
node switch
{
{ RawKind: (int)SyntaxKind.FalseLiteralExpression } => true, // false
{ RawKind: (int)SyntaxKind.LogicalNotExpression } => IsTrue(((PrefixUnaryExpressionSyntax)node).Operand), // !true
{ RawKind: (int)SyntaxKindEx.ConstantPattern } => IsFalse(((ConstantPatternSyntaxWrapper)node).Expression), // is false
{ RawKind: (int)SyntaxKindEx.NotPattern } => IsTrue(((UnaryPatternSyntaxWrapper)node).Pattern), // is not true
{ RawKind: (int)SyntaxKind.ParenthesizedExpression } => IsFalse(((ParenthesizedExpressionSyntax)node).Expression), // (false)
{ RawKind: (int)SyntaxKindEx.ParenthesizedPattern } => IsFalse(((ParenthesizedPatternSyntaxWrapper)node).Pattern), // is (false)
_ => false,
};

private readonly record struct PathPosition(int Index, int TupleLength);

private sealed class ControlFlowGraphCache : ControlFlowGraphCacheBase
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,32 +21,48 @@
namespace SonarAnalyzer.Rules.CSharp
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class BooleanLiteralUnnecessary : BooleanLiteralUnnecessaryBase<BinaryExpressionSyntax, SyntaxKind>
public sealed class BooleanLiteralUnnecessary : BooleanLiteralUnnecessaryBase<SyntaxKind>
{
protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;

protected override bool IsBooleanLiteral(SyntaxNode node) => IsTrueLiteralKind(node) || IsFalseLiteralKind(node);

protected override SyntaxNode GetLeftNode(BinaryExpressionSyntax binaryExpression) => binaryExpression.Left;

protected override SyntaxNode GetRightNode(BinaryExpressionSyntax binaryExpression) => binaryExpression.Right;

protected override SyntaxToken GetOperatorToken(BinaryExpressionSyntax binaryExpression) => binaryExpression.OperatorToken;
protected override SyntaxToken? GetOperatorToken(SyntaxNode node) =>
node switch
{
BinaryExpressionSyntax binary => binary.OperatorToken,
_ when IsPatternExpressionSyntaxWrapper.IsInstance(node) => ((IsPatternExpressionSyntaxWrapper)node).IsKeyword,
_ => null,
};

protected override bool IsTrueLiteralKind(SyntaxNode syntaxNode) => syntaxNode.IsKind(SyntaxKind.TrueLiteralExpression);
protected override bool IsTrue(SyntaxNode syntaxNode) => syntaxNode.IsTrue();

protected override bool IsFalseLiteralKind(SyntaxNode syntaxNode) => syntaxNode.IsKind(SyntaxKind.FalseLiteralExpression);
protected override bool IsFalse(SyntaxNode syntaxNode) => syntaxNode.IsFalse();

protected override bool IsInsideTernaryWithThrowExpression(BinaryExpressionSyntax syntaxNode) =>
protected override bool IsInsideTernaryWithThrowExpression(SyntaxNode syntaxNode) =>
syntaxNode.Parent is ConditionalExpressionSyntax conditionalExpression
&& (IsThrowExpression(conditionalExpression.WhenTrue) || IsThrowExpression(conditionalExpression.WhenFalse));

protected override SyntaxNode GetLeftNode(SyntaxNode node) =>
node switch
{
BinaryExpressionSyntax binaryExpression => binaryExpression.Left,
_ when IsPatternExpressionSyntaxWrapper.IsInstance(node) => ((IsPatternExpressionSyntaxWrapper)node).Expression,
_ => null
};

protected override SyntaxNode GetRightNode(SyntaxNode node) =>
node switch
{
BinaryExpressionSyntax binaryExpression => binaryExpression.Right,
_ when IsPatternExpressionSyntaxWrapper.IsInstance(node) => ((IsPatternExpressionSyntaxWrapper)node).Pattern,
_ => null
};

protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterNodeAction(CheckLogicalNot, SyntaxKind.LogicalNotExpression);
context.RegisterNodeAction(CheckAndExpression, SyntaxKind.LogicalAndExpression);
context.RegisterNodeAction(CheckOrExpression, SyntaxKind.LogicalOrExpression);
context.RegisterNodeAction(CheckEquals, SyntaxKind.EqualsExpression);
context.RegisterNodeAction(CheckEquals, SyntaxKind.EqualsExpression, SyntaxKindEx.IsPatternExpression);
context.RegisterNodeAction(CheckNotEquals, SyntaxKind.NotEqualsExpression);
context.RegisterNodeAction(CheckConditional, SyntaxKind.ConditionalExpression);
context.RegisterNodeAction(CheckForLoopCondition, SyntaxKind.ForStatement);
Expand All @@ -67,13 +83,10 @@ private void CheckLogicalNot(SonarSyntaxNodeReportingContext context)
{
var logicalNot = (PrefixUnaryExpressionSyntax)context.Node;
var logicalNotOperand = logicalNot.Operand.RemoveParentheses();
if (IsBooleanLiteral(logicalNotOperand))
if (IsTrue(logicalNotOperand) || IsFalse(logicalNotOperand))
{
context.ReportIssue(Diagnostic.Create(Rule, logicalNot.Operand.GetLocation()));
}

static bool IsBooleanLiteral(SyntaxNode node) =>
node.IsKind(SyntaxKind.TrueLiteralExpression) || node.IsKind(SyntaxKind.FalseLiteralExpression);
}

private void CheckConditional(SonarSyntaxNodeReportingContext context)
Expand All @@ -94,7 +107,7 @@ private void CheckConditional(SonarSyntaxNodeReportingContext context)
{
return;
}
CheckTernaryExpressionBranches(context, conditional.SyntaxTree, whenTrue, whenFalse);
CheckTernaryExpressionBranches(context, whenTrue, whenFalse);
}

private static bool IsThrowExpression(ExpressionSyntax expressionSyntax) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ protected override Task RegisterCodeFixesAsync(SyntaxNode root, SonarCodeFixCont
return Task.CompletedTask;
}

if (IsPatternExpressionSyntaxWrapper.IsInstance(syntaxNode))
{
RegisterPatternExpressionReplacement(context, root, (IsPatternExpressionSyntaxWrapper)syntaxNode);
}

if (syntaxNode is not LiteralExpressionSyntax literal)
{
return Task.CompletedTask;
Expand Down Expand Up @@ -86,6 +91,31 @@ protected override Task RegisterCodeFixesAsync(SyntaxNode root, SonarCodeFixCont
return Task.CompletedTask;
}

private static void RegisterPatternExpressionReplacement(SonarCodeFixContext context, SyntaxNode root, IsPatternExpressionSyntaxWrapper patternExpression)
{
var replacement = patternExpression.Pattern.SyntaxNode.IsTrue()
? patternExpression.Expression
: GetNegatedExpression(patternExpression.Expression);

if (replacement.IsTrue())
{
replacement = CSharpSyntaxHelper.TrueLiteralExpression;
}
else if (replacement.IsFalse())
{
replacement = CSharpSyntaxHelper.FalseLiteralExpression;
}

context.RegisterCodeFix(
Title,
c =>
{
var newRoot = root.ReplaceNode(patternExpression.SyntaxNode, replacement.WithAdditionalAnnotations(Formatter.Annotation));
return Task.FromResult(context.Document.WithSyntaxRoot(newRoot));
},
context.Diagnostics);
}

private static void RegisterForStatementConditionRemoval(SonarCodeFixContext context, SyntaxNode root, ForStatementSyntax forStatement) =>
context.RegisterCodeFix(
Title,
Expand Down
Loading