Skip to content

Commit

Permalink
Implement
Browse files Browse the repository at this point in the history
  • Loading branch information
@pavel-mikula-sonarsource
pavel-mikula-sonarsource committed Jun 28, 2024
1 parent 2818bfb commit c3bfeb5
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 214 deletions.
82 changes: 63 additions & 19 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/CastShouldNotBeDuplicated.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand All @@ -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);

Expand All @@ -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<Location> 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)
{
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand All @@ -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<CastExpressionSyntax> visited = new(new CSharpSyntaxNodeEqualityComparer<CastExpressionSyntax>());

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<SyntaxNode>(IsScopeNode);
ReportPatternAtMainVariable(context, node.Expression, node.GetLocation(), sameBlock, node.Type, ExtractRepetitiveCastMessage, false);
}
base.VisitCastExpression(node);
}
}
}
184 changes: 7 additions & 177 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.CSharp9.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
};
}

}
}
Loading

0 comments on commit c3bfeb5

Please sign in to comment.