From c3bfeb55e010a5dc1fc65cf6395f915d3b813bd5 Mon Sep 17 00:00:00 2001 From: Pavel Mikula Date: Thu, 27 Jun 2024 15:31:13 +0200 Subject: [PATCH] Implement --- .../Rules/CastShouldNotBeDuplicated.cs | 82 ++++++-- .../CastShouldNotBeDuplicated.CSharp9.cs | 184 +----------------- .../TestCases/CastShouldNotBeDuplicated.cs | 48 +++-- 3 files changed, 100 insertions(+), 214 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/CastShouldNotBeDuplicated.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/CastShouldNotBeDuplicated.cs index 89b1eff0e53..137cd1ac83d 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/CastShouldNotBeDuplicated.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/CastShouldNotBeDuplicated.cs @@ -18,6 +18,8 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ +using SonarAnalyzer.Common.Walkers; + namespace SonarAnalyzer.Rules.CSharp; [DiagnosticAnalyzer(LanguageNames.CSharp)] @@ -28,6 +30,7 @@ public sealed class CastShouldNotBeDuplicated : SonarDiagnosticAnalyzer private const string ReplaceWithAsAndNullCheckMessage = "Replace this type-check-and-cast sequence with an 'as' and a null check."; private const string RemoveRedundantCastAnotherVariableMessage = "Remove this cast and use the appropriate variable."; private const string RemoveRedundantCastMessage = "Remove this redundant cast."; + private const string ExtractRepetitiveCastMessage = "Extract this repetitive cast into a variable."; private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); @@ -39,51 +42,58 @@ protected override void Initialize(SonarAnalysisContext context) context.RegisterNodeAction(IsPatternExpression, SyntaxKindEx.IsPatternExpression); context.RegisterNodeAction(SwitchExpressionArm, SyntaxKindEx.SwitchExpressionArm); context.RegisterNodeAction(CasePatternSwitchLabel, SyntaxKindEx.CasePatternSwitchLabel); + context.RegisterNodeAction(Block, SyntaxKind.Block); } - private static void CasePatternSwitchLabel(SonarSyntaxNodeReportingContext analysisContext) + private static void CasePatternSwitchLabel(SonarSyntaxNodeReportingContext context) { - var casePatternSwitch = (CasePatternSwitchLabelSyntaxWrapper)analysisContext.Node; + var casePatternSwitch = (CasePatternSwitchLabelSyntaxWrapper)context.Node; if (casePatternSwitch.SyntaxNode.GetFirstNonParenthesizedParent().GetFirstNonParenthesizedParent() is SwitchStatementSyntax parentSwitchStatement) { - ProcessPatternExpression(analysisContext, casePatternSwitch.Pattern, parentSwitchStatement.Expression, parentSwitchStatement); + ProcessPatternExpression(context, casePatternSwitch.Pattern, parentSwitchStatement.Expression, parentSwitchStatement); } } - private static void SwitchExpressionArm(SonarSyntaxNodeReportingContext analysisContext) + private static void SwitchExpressionArm(SonarSyntaxNodeReportingContext context) { - var isSwitchExpression = (SwitchExpressionArmSyntaxWrapper)analysisContext.Node; + var isSwitchExpression = (SwitchExpressionArmSyntaxWrapper)context.Node; var parent = isSwitchExpression.SyntaxNode.GetFirstNonParenthesizedParent(); if (parent.IsKind(SyntaxKindEx.SwitchExpression)) { var switchExpression = (SwitchExpressionSyntaxWrapper)parent; - ProcessPatternExpression(analysisContext, isSwitchExpression.Pattern, switchExpression.GoverningExpression, isSwitchExpression); + ProcessPatternExpression(context, isSwitchExpression.Pattern, switchExpression.GoverningExpression, isSwitchExpression); } } - private static void IsPatternExpression(SonarSyntaxNodeReportingContext analysisContext) + private static void IsPatternExpression(SonarSyntaxNodeReportingContext context) { - var isPatternExpression = (IsPatternExpressionSyntaxWrapper)analysisContext.Node; + var isPatternExpression = (IsPatternExpressionSyntaxWrapper)context.Node; if (isPatternExpression.SyntaxNode.GetFirstNonParenthesizedParent() is IfStatementSyntax parentIfStatement) { - ProcessPatternExpression(analysisContext, isPatternExpression.Pattern, isPatternExpression.Expression, parentIfStatement.Statement); + ProcessPatternExpression(context, isPatternExpression.Pattern, isPatternExpression.Expression, parentIfStatement.Statement); } } - private static void IsExpression(SonarSyntaxNodeReportingContext analysisContext) + private static void IsExpression(SonarSyntaxNodeReportingContext context) { - var isExpression = (BinaryExpressionSyntax)analysisContext.Node; + var isExpression = (BinaryExpressionSyntax)context.Node; if (isExpression.Right is TypeSyntax castType && isExpression.GetFirstNonParenthesizedParent() is IfStatementSyntax parentIfStatement) { - ReportPatternAtMainVariable(analysisContext, isExpression.Left, isExpression.GetLocation(), parentIfStatement.Statement, castType, ReplaceWithAsAndNullCheckMessage); + ReportPatternAtMainVariable(context, isExpression.Left, isExpression.GetLocation(), parentIfStatement.Statement, castType, ReplaceWithAsAndNullCheckMessage, true); } } - private static Location[] DuplicatedCastLocations(SonarSyntaxNodeReportingContext context, SyntaxNode parentStatement, TypeSyntax castType, SyntaxNode typedVariable) + private static void Block(SonarSyntaxNodeReportingContext context) + { + var walker = new RepetitiveCastWalker(context); + walker.SafeVisit(context.Node); + } + + private static IEnumerable DuplicatedCastLocations(SonarSyntaxNodeReportingContext context, SyntaxNode parent, TypeSyntax castType, SyntaxNode variable, bool inspectScopeNodes) { - var typeExpressionSymbol = context.SemanticModel.GetSymbolInfo(typedVariable).Symbol ?? context.SemanticModel.GetDeclaredSymbol(typedVariable); - return typeExpressionSymbol is null ? [] : parentStatement.DescendantNodes().Where(IsDuplicatedCast).Select(x => x.GetLocation()).ToArray(); + var typeExpressionSymbol = context.SemanticModel.GetSymbolInfo(variable).Symbol ?? context.SemanticModel.GetDeclaredSymbol(variable); + return typeExpressionSymbol is null ? [] : parent.DescendantNodes(InspectScopeNode).Where(IsDuplicatedCast).Select(x => x.GetLocation()); bool IsDuplicatedCast(SyntaxNode node) { @@ -101,6 +111,9 @@ bool IsDuplicatedCast(SyntaxNode node) } } + bool InspectScopeNode(SyntaxNode node) => + inspectScopeNodes || node == parent || !IsScopeNode(node); + bool IsDuplicatedCastOnSameSymbol(ExpressionSyntax expression, SyntaxNode type) => type.WithoutTrivia().IsEquivalentTo(castType.WithoutTrivia()) && IsCastOnSameSymbol(expression) @@ -137,7 +150,7 @@ private static void ProcessPatternExpression(SonarSyntaxNodeReportingContext ana : RemoveRedundantCastMessage; foreach (var targetType in targetTypes) { - ReportPatternAtMainVariable(analysisContext, leftVariable, leftVariable.GetLocation(), parentStatement, targetType, mainVarMsg); + ReportPatternAtMainVariable(analysisContext, leftVariable, leftVariable.GetLocation(), parentStatement, targetType, mainVarMsg, true); } foreach (var variableTypePair in rightPartsToCheck) @@ -206,9 +219,10 @@ private static void ReportPatternAtMainVariable(SonarSyntaxNodeReportingContext Location mainLocation, SyntaxNode parentStatement, TypeSyntax castType, - string message) + string message, + bool inspectScopeNodes) { - var duplicatedCastLocations = DuplicatedCastLocations(context, parentStatement, castType, variableExpression); + var duplicatedCastLocations = DuplicatedCastLocations(context, parentStatement, castType, variableExpression, inspectScopeNodes).Where(x => x != mainLocation).ToArray(); if (duplicatedCastLocations.Any()) { context.ReportIssue(Rule, mainLocation, duplicatedCastLocations.ToSecondary(), message); @@ -222,10 +236,40 @@ private static void ReportPatternAtCastLocation(SonarSyntaxNodeReportingContext TypeSyntax castType, string message) { - var duplicatedCastLocations = DuplicatedCastLocations(context, parentStatement, castType, variableExpression); + var duplicatedCastLocations = DuplicatedCastLocations(context, parentStatement, castType, variableExpression, true); foreach (var castLocation in duplicatedCastLocations) { context.ReportIssue(Rule, castLocation, [patternLocation.ToSecondary()], message); } } + + private static bool IsScopeNode(SyntaxNode node) => + node.IsAnyKind(SyntaxKind.Block, SyntaxKind.SwitchSection, SyntaxKindEx.SwitchExpressionArm); + + private sealed class RepetitiveCastWalker : SafeCSharpSyntaxWalker + { + private readonly SonarSyntaxNodeReportingContext context; + private readonly HashSet visited = new(new CSharpSyntaxNodeEqualityComparer()); + + public RepetitiveCastWalker(SonarSyntaxNodeReportingContext context) => + this.context = context; + + public override void Visit(SyntaxNode node) + { + if (!node.IsKind(SyntaxKind.Block) || node == context.Node) // We only inspect within the same block + { + base.Visit(node); + } + } + + public override void VisitCastExpression(CastExpressionSyntax node) + { + if (visited.Add(node)) + { + var sameBlock = node.FirstAncestorOrSelf(IsScopeNode); + ReportPatternAtMainVariable(context, node.Expression, node.GetLocation(), sameBlock, node.Type, ExtractRepetitiveCastMessage, false); + } + base.VisitCastExpression(node); + } + } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.CSharp9.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.CSharp9.cs index 50c2a2200ee..615efd3d8ad 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.CSharp9.cs @@ -15,22 +15,10 @@ class Program public void Foo(Object x, Object y) { - var fruit = x as Fruit; - if (fruit is not Fruit) // Compliant, redundant condition, not related for the current rule - { - } - object o; switch (x) // Noncompliant [switch-st-0] {{Remove this cast and use the appropriate variable.}} // ^ { - case Fruit m: // Secondary [switch-st-1] - // ^^^^^^^ - o = (Fruit)m; // Noncompliant [switch-st-1] {{Remove this redundant cast.}} - break; - case Vegetable t when t != null: // Secondary [switch-st-2] - o = (Vegetable)t; // Noncompliant [switch-st-2] {{Remove this redundant cast.}} - break; case Water u: o = (Water)x; // Secondary [switch-st-0] break; @@ -39,182 +27,24 @@ public void Foo(Object x, Object y) break; } - if ((x, y) is (Fruit f1, Vegetable v1)) // Secondary - // ^^^^^^^^ - { - var ff1 = (Fruit)f1; // Noncompliant - // ^^^^^^^^^ - } - - if ((x, y) is (Fruit f2, Vegetable v2)) // Secondary - { - var ff2 = (Vegetable)v2; // Noncompliant - } - - if ((x, y) is (Fruit f3, Vegetable v3)) // Noncompliant - { - var ff3 = (Fruit)x; // Secondary - } - - if ((x, y) is (Fruit f4, Vegetable v4)) // Noncompliant - { - var ff4 = (Vegetable)y; // Secondary - } - - if ((x,y) is (Fruit f5, Vegetable v5, Vegetable v51)) // Error [CS8502] - { - var ff5 = (Fruit)x; - } - - if (x is Fruit f6) // Secondary - { - var ff6 = (Fruit)f6; // Noncompliant {{Remove this redundant cast.}} - var fff6 = (Vegetable)x; - } - - if (x is Fruit f7) // Noncompliant {{Remove this cast and use the appropriate variable.}} - { - var ff7 = (Fruit)x; // Secondary - var fff7 = (Vegetable)x; - } - - if (x is UnknownFruit f8) // Error [CS0246] - { - var ff8 = (Fruit)x; - } - - if (x is Water f9) - { - var ff9 = (Fruit)x; - } - - x is Fruit f0; // Error [CS0201] - - if (x is not Water) - { - var xWater = (Water)x; - } - else if (x is not Fruit) - { - var xFruit = (Fruit)x; - } - var message = x switch // Noncompliant [switch-expression-1] {{Remove this cast and use the appropriate variable.}} { - Fruit f10 => // Secondary [switch-expression-2] - // ^^^^^^^^^ - ((Fruit)f10).ToString(), // Noncompliant [switch-expression-2] {{Remove this redundant cast.}} - // ^^^^^^^^^^ - Vegetable v11 => // Secondary [switch-expression-3] - ((Vegetable)v11).ToString(), // Noncompliant [switch-expression-3] - (string left, string right) => // Secondary [switch-expression-4, switch-expression-5] - (string) left + (string) right,// Noncompliant [switch-expression-4] - // Noncompliant@-1 [switch-expression-5] Water w12 => ((Water)x).ToString(), // Secondary [switch-expression-1] - Complex { x : Fruit apple } => "apple", _ => "More than 10" }; - - if ((x) is (Fruit f12, Vegetable v12)) // Noncompliant - { - var ff12 = (Vegetable)x; // Secondary - } - - Foo k = null; - if (k is { x : 0 }) - { - } - - if (x is (Water f13)) // Noncompliant - { - var ff13 = (Water)x; // Secondary - } - } - - public void Bar(object x, object y) - { - if (x is not Fruit) - { - var f1 = (Fruit)x; // Compliant - but will throw - } - else - { - var f2 = (Fruit)x; // Compliant - } - - if (x is Fruit { Prop: 1 } tuttyFrutty) // Secondary [property-pattern-1] - // Noncompliant@-1 [property-pattern-2] {{Remove this cast and use the appropriate variable.}} - { - var aRealFruit = (Fruit)tuttyFrutty; // Noncompliant [property-pattern-1] {{Remove this redundant cast.}} - var anotherFruit = (Fruit)x; // Secondary [property-pattern-2] - } - - var foo = new Complex(); - if (foo is {x: Fruit f }) // Secondary - // ^^^^^^^ - { - var something = (Fruit)f; // Noncompliant - // ^^^^^^^^ - } - - if ((x, y) is (1)) // Error[CS0029] - { - } - - if ((x, y) is (R { SomeProperty: { Count: 5 } })) // Error [CS0246] - { - } - } - - public void FooBar(object x) - { - if (x is nuint) // Noncompliant - { - var res = (nuint)x; // Secondary - } - } - - public void Baz(object x, object y) - { - if ((x, y) is ((Fruit a, Fruit b), string v)) // Secondary - // Secondary@-1 - // Secondary@-2 - { - var a1 = (Fruit)a; // Noncompliant - var b1 = (Fruit)b; // Noncompliant - var v1 = (string)v; // Noncompliant - } - - if (x is (Fruit or Vegetable)) // Noncompliant - { - var fruit = (Fruit)x; // Secondary - } - - if (x is (Fruit or Vegetable)) // Noncompliant - { - var vegetable = (Vegetable)x; // Secondary - } } - public void NonExistingType() + public void MultipleCastsDifferentBlocks(object arg) { - if (x is Fruit f) // Error [CS0103] - // Secondary@-1 - { - var ff = (Fruit)f; // Noncompliant {{Remove this redundant cast.}} - } - } + _ = (Fruit)arg; - // See https://github.com/SonarSource/sonar-dotnet/issues/2314 - public void TakeIdentifierIntoAccount(object x) - { - if (x is Fruit) + _ = 42 switch { - Fruit f = new(); - var c = (Fruit)f; - } + 1 => (Fruit)arg, // Compliant, not the same block + 2 => (Fruit)arg, // Compliant, not the same block + _ => null + }; } - } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.cs index dbe5d949509..c8e6b97ec99 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.cs @@ -13,8 +13,6 @@ public void Foo(Object x) if (x is Fruit) // Noncompliant { var f1 = (Fruit)x; -// ^^^^^^^^ Secondary - var f2 = (Fruit)x; // ^^^^^^^^ Secondary } @@ -108,47 +106,49 @@ public void UnknownFoo(object x) public void MultipleCasts_RootBlock(object arg) { - _ = (Fruit)arg; // FN - _ = (Fruit)arg; // Sec-ondary - _ = (Fruit)arg; // Sec-ondary + _ = (Fruit)arg; // Noncompliant {{Extract this repetitive cast into a variable.}} + // ^^^^^^^^^^ + _ = (Fruit)arg; // Secondary + _ = (Fruit/*Comment*/)arg; // Secondary + _ = ( Fruit )arg; // Secondary with extra whitespace } public void MultipleCasts_SameBlock(object arg) { if (true) { - _ = (Fruit)arg; // FN - _ = (Fruit)arg; // Sec-ondary - _ = (Fruit)arg; // Sec-ondary + _ = (Fruit)arg; // Noncompliant {{Extract this repetitive cast into a variable.}} + _ = (Fruit)arg; // Secondary + _ = (Fruit)arg; // Secondary } } public void MultipleCasts_NestedBlock(object arg) { - _ = (Fruit)arg; // FN + _ = (Fruit)arg; // Compliant, not in the same block if (true) { - _ = (Fruit)arg; // Sec-ondary - foreach(var ch in "Lorem ipsum") + _ = (Fruit)arg; // Noncompliant [Outer] {{Extract this repetitive cast into a variable.}} + foreach (var ch in "Lorem ipsum") { - _ = (Fruit)arg; // Sec-ondary - _ = (Fruit)arg; // Sec-ondary + _ = (Fruit)arg; // Noncompliant [Inner] + _ = (Fruit)arg; // Secondary [Inner] } - _ = (Fruit)arg; // Sec-ondary + _ = (Fruit)arg; // Secondary [Outer] } } public void MultipleCasts_Lambda(object arg) { - _ = (Fruit)arg; // FN + _ = (Fruit)arg; // Compliant, not in the same block Action a = () => { - _ = (Fruit)arg; // Sec-ondary - _ = (Fruit)arg; // Sec-ondary + _ = (Fruit)arg; // Noncompliant [Action] + _ = (Fruit)arg; // Secondary [Action] }; Func f = x => { - _ = (Fruit)arg; // Sec-ondary + _ = (Fruit)arg; _ = (Fruit)x; return 0; }; @@ -165,6 +165,18 @@ public void MultipleCastsDifferentBlocks(object arg) { _ = (Fruit)arg; } + + _ = (Fruit)arg; + + switch (42) + { + case 1: + _ = (Fruit)arg; // Compliant, not the same block + break; + case 2: + _ = (Fruit)arg; + break; + } } }