Improve EC75 - Don't concatenate strings in loops (#68)

green-code-initiative · Jun 8, 2024 · 8404032 · 8404032
1 parent b7bda48
commit 8404032
Show file tree

Hide file tree

Showing 8 changed files with 413 additions and 87 deletions.
diff --git a/Directory.Build.props b/Directory.Build.props
@@ -12,7 +12,7 @@
 		<Nullable>enable</Nullable>
 		<NeutralLanguage>en</NeutralLanguage>
 		<IsPackable>false</IsPackable>
-		
+
 		<AnalysisMode>all</AnalysisMode>
 		<EnableNETAnalyzers>true</EnableNETAnalyzers>
 		<RunAnalyzersDuringBuild>true</RunAnalyzersDuringBuild>

diff --git a/Directory.Packages.props b/Directory.Packages.props
@@ -1,27 +1,27 @@
 <Project>
-  <PropertyGroup>
-    <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
-  </PropertyGroup>
-  <ItemGroup>
-    <PackageVersion Include="coverlet.collector" Version="6.0.2">
-      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
-      <PrivateAssets>all</PrivateAssets>
-    </PackageVersion>
-    <PackageVersion Include="coverlet.msbuild" Version="6.0.2">
-      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
-      <PrivateAssets>all</PrivateAssets>
-    </PackageVersion>
-    <PackageVersion Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.4" />
-    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.9.2" />
-    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.MSTest" Version="1.1.1" />
-    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.MSTest" Version="1.1.1" />
-    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.9.2" />
-    <PackageVersion Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="8.0.0" />
-    <PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.10.0" />
-    <PackageVersion Include="Microsoft.VSSDK.BuildTools" Version="17.10.2179" />
-    <PackageVersion Include="MSTest.TestAdapter" Version="3.4.3" />
-    <PackageVersion Include="MSTest.TestFramework" Version="3.4.3" />
-    <PackageVersion Include="Roslynator.Analyzers" Version="4.12.4" />
-    <PackageVersion Include="Roslynator.CodeAnalysis.Analyzers" Version="4.12.4" />
-  </ItemGroup>
-</Project>
+	<PropertyGroup>
+		<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
+	</PropertyGroup>
+	<ItemGroup>
+		<PackageVersion Include="coverlet.collector" Version="6.0.2">
+			<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
+			<PrivateAssets>all</PrivateAssets>
+		</PackageVersion>
+		<PackageVersion Include="coverlet.msbuild" Version="6.0.2">
+			<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
+			<PrivateAssets>all</PrivateAssets>
+		</PackageVersion>
+		<PackageVersion Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.4" />
+		<PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.9.2" />
+		<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.MSTest" Version="1.1.1" />
+		<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.MSTest" Version="1.1.1" />
+		<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.9.2" />
+		<PackageVersion Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="8.0.0" />
+		<PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.10.0" />
+		<PackageVersion Include="Microsoft.VSSDK.BuildTools" Version="17.10.2179" />
+		<PackageVersion Include="MSTest.TestAdapter" Version="3.4.3" />
+		<PackageVersion Include="MSTest.TestFramework" Version="3.4.3" />
+		<PackageVersion Include="Roslynator.Analyzers" Version="4.12.4" />
+		<PackageVersion Include="Roslynator.CodeAnalysis.Analyzers" Version="4.12.4" />
+	</ItemGroup>
+</Project>
diff --git a/NuGet.Config b/NuGet.Config
@@ -1,8 +1,8 @@
 <?xml version="1.0" encoding="utf-8"?>
 <configuration>
-  <packageSourceMapping>
-    <packageSource key="nuget.org">
-      <package pattern="*" />
-    </packageSource>
-  </packageSourceMapping>
+	<packageSourceMapping>
+		<packageSource key="nuget.org">
+			<package pattern="*" />
+		</packageSource>
+	</packageSourceMapping>
 </configuration>
diff --git a/Rules to work on.md b/Rules to work on.md
@@ -3,7 +3,6 @@ They seem to be good candidates but need reviewing first.
 
 From [Roslynator](https://github.com/dotnet/roslynator):
 + Optimize LINQ method call: [RCS1077](https://josefpihrt.github.io/docs/roslynator/analyzers/RCS1077/)
-+ Use string.Length instead of comparison with empty string: [RCS1156](https://josefpihrt.github.io/docs/roslynator/analyzers/RCS1156/)
 + Use 'is' operator instead of 'as' operator: [RCS1172](https://josefpihrt.github.io/docs/roslynator/analyzers/RCS1172/)
 + Unnecessary assignment: [RCS1179](https://josefpihrt.github.io/docs/roslynator/analyzers/RCS1179/)
 + Use constant instead of field : [RCS1187](https://josefpihrt.github.io/docs/roslynator/analyzers/RCS1187/)
@@ -20,7 +19,6 @@ From [Meziantou Analyzer](https://github.com/meziantou/Meziantou.Analyzer):
 + Optimize Enumerable.Count() usage: [MA0031](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0031.md)
 + Do not use blocking calls in an async method: [MA0042](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0042.md)
 + Do not use finalizer: [MA0055](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0055.md)
-+ Use Where before OrderBy: [MA0063](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0063.md)
 + Default ValueType.Equals or HashCode is used for struct equality: [MA0065](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0065.md)
 + Use 'Cast' instead of 'Select' to cast: [MA0078](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0078.md)
 + Optimize string method usage: [MA0089](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0089.md)
@@ -37,4 +35,3 @@ From [SonarQube](https://www.sonarsource.com/products/sonarqube/):
 Rules that are natively implemented in [Roslyn](https://github.com/dotnet/roslyn), that we could transitively enable:
 + Do not declare static members on generic types: [CA1000](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1000)
 + Mark members as static: [CA1822](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1822)
-+ Use 'Count/Length' property instead of 'Any' method: [CA1860](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1860)
diff --git a/global.json b/global.json
@@ -1,6 +1,6 @@
 {
-  "sdk": {
-    "version": "8.0.205",
-    "rollForward": "latestMajor"
-  }
+    "sdk": {
+        "version": "8.0.206",
+        "rollForward": "latestMajor"
+    }
 }
diff --git a/src/EcoCode.Core/Analyzers/EC75.DontConcatenateStringsInLoops.cs b/src/EcoCode.Core/Analyzers/EC75.DontConcatenateStringsInLoops.cs
@@ -9,6 +9,7 @@ public sealed class DontConcatenateStringsInLoops : DiagnosticAnalyzer
         SyntaxKind.ForEachStatement,
         SyntaxKind.WhileStatement,
         SyntaxKind.DoStatement];
+    private static readonly ImmutableArray<OperationKind> Invocations = [OperationKind.Invocation];
 
     /// <summary>The diagnostic descriptor.</summary>
     public static DiagnosticDescriptor Descriptor { get; } = Rule.CreateDescriptor(
@@ -29,22 +30,89 @@ public override void Initialize(AnalysisContext context)
         context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
         context.EnableConcurrentExecution();
         context.RegisterSyntaxNodeAction(static context => AnalyzeLoopNode(context), SyntaxKinds);
+        context.RegisterOperationAction(static context => AnalyzeForEach(context), Invocations);
     }
 
     private static void AnalyzeLoopNode(SyntaxNodeAnalysisContext context)
     {
         foreach (var loopStatement in context.Node.GetLoopStatements())
         {
-            if (loopStatement is ExpressionStatementSyntax expressionStatement &&
-                expressionStatement.Expression is AssignmentExpressionSyntax assignment &&
-                assignment.IsKind(SyntaxKind.AddAssignmentExpression) &&
-                assignment.Left is IdentifierNameSyntax identifierName &&
-                context.SemanticModel.GetSymbolInfo(identifierName).Symbol is ISymbol symbol &&
-                symbol.IsVariableOfType(SpecialType.System_String) &&
-                symbol.IsDeclaredOutsideLoop(context.Node))
+            if (loopStatement is not ExpressionStatementSyntax { Expression: AssignmentExpressionSyntax assignment } ||
+                assignment.Left is not IdentifierNameSyntax identifierName ||
+                context.SemanticModel.GetSymbolInfo(identifierName).Symbol is not ISymbol symbol ||
+                !symbol.IsVariableOfType(SpecialType.System_String)) // The assigned symbol must be a string
             {
-                context.ReportDiagnostic(Diagnostic.Create(Descriptor, assignment.Parent!.GetLocation()));
+                continue;
             }
+
+            // AddAssignmentExpression corresponds to : a += b. We know we can warn at this point
+            // SimpleAssignmentExpression corresponds to : a = b. In this case, check that
+            // the right term is an addition and the assigned symbol is one of the operands
+            bool report = assignment.IsKind(SyntaxKind.AddAssignmentExpression) ||
+                assignment.IsKind(SyntaxKind.SimpleAssignmentExpression) &&
+                assignment.Right is BinaryExpressionSyntax binExpr &&
+                binExpr.IsKind(SyntaxKind.AddExpression) &&
+                (SymbolEqualityComparer.Default.Equals(symbol, context.SemanticModel.GetSymbolInfo(binExpr.Left).Symbol) ||
+                 SymbolEqualityComparer.Default.Equals(symbol, context.SemanticModel.GetSymbolInfo(binExpr.Right).Symbol));
+
+            if (report && symbol.IsDeclaredOutsideLoop(context.Node)) // Check IsDeclaredOutsideLoop last as it requires more work
+                context.ReportDiagnostic(Diagnostic.Create(Descriptor, assignment.GetLocation()));
+        }
+    }
+
+    private static void AnalyzeForEach(OperationAnalysisContext context)
+    {
+        if (context.Operation is not IInvocationOperation { TargetMethod.Name: "ForEach" } operation ||
+            GetBodyDelegateOperation(operation, context.Compilation)?.Value is not IDelegateCreationOperation { Target: { } body })
+        {
+            return;
+        }
+
+        foreach (var op in body.Descendants())
+        {
+            var target = default(IOperation);
+            if (op is ICompoundAssignmentOperation compoundAssign) // a += b
+            {
+                if (compoundAssign.OperatorKind is BinaryOperatorKind.Add && compoundAssign.Target.Type?.SpecialType is SpecialType.System_String)
+                    target = compoundAssign.Target;
+            }
+            else if (op is ISimpleAssignmentOperation simpleAssign && // a = b
+                simpleAssign.Target.Type?.SpecialType is SpecialType.System_String &&
+                simpleAssign.Value is IBinaryOperation { OperatorKind: BinaryOperatorKind.Add } binOp &&
+                simpleAssign.MatchesAnyOperand(binOp.LeftOperand, binOp.RightOperand))
+            {
+                target = simpleAssign.Target;
+            }
+            if (target is null) continue;
+
+            if (target is not ILocalReferenceOperation localRef ||
+                localRef.Local.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is { } declaringSyntax &&
+                !body.Syntax.Span.Contains(declaringSyntax.Span)) // Local variable is a capture, declared outside of the method body
+            {
+                context.ReportDiagnostic(Diagnostic.Create(Descriptor, op.Syntax.GetLocation()));
+            }
+        }
+
+        static IArgumentOperation? GetBodyDelegateOperation(IInvocationOperation operation, Compilation compilation)
+        {
+            var symbol = operation.TargetMethod.ContainingType.OriginalDefinition;
+            if (operation.TargetMethod.ContainingType.IsStatic) // Parallel.ForEach<T>, the delegate to analyze is always called 'body'
+            {
+                if (SymbolEqualityComparer.Default.Equals(symbol, compilation.GetTypeByMetadataName("System.Threading.Tasks.Parallel")))
+                    return operation.Arguments.FirstOrDefault(a => a.Parameter?.Name == "body");
+            }
+            else if (operation.TargetMethod.IsStatic) // Array : static void ForEach<T>(T[] array, Action<T> action)
+            {
+                if (SymbolEqualityComparer.Default.Equals(symbol, compilation.GetTypeByMetadataName("System.Array")))
+                    return operation.Arguments[1];
+            }
+            else if ( // List<T> and ImmutableList<T> : void ForEach(Action<T> action)
+                SymbolEqualityComparer.Default.Equals(symbol, compilation.GetTypeByMetadataName("System.Collections.Generic.List`1")) ||
+                SymbolEqualityComparer.Default.Equals(symbol, compilation.GetTypeByMetadataName("System.Collections.Immutable.ImmutableList`1")))
+            {
+                return operation.Arguments[0];
+            }
+            return null;
         }
     }
 }
diff --git a/src/EcoCode.Core/Extensions/OperationExtensions.cs b/src/EcoCode.Core/Extensions/OperationExtensions.cs
@@ -0,0 +1,27 @@
+namespace EcoCode.Extensions;
+
+/// <summary>Extensions methods for <see cref="IOperation"/>.</summary>
+public static class OperationExtensions
+{
+    /// <summary>Returns whether the given operation's target matches one of the given operations' target.</summary>
+    /// <param name="op">The operation.</param>
+    /// <param name="left">The left operation.</param>
+    /// <param name="right">The right operation.</param>
+    /// <returns>True if the operation's target matches one of the given operations' target, false otherwise.</returns>
+    public static bool MatchesAnyOperand(this ISimpleAssignmentOperation op, IOperation left, IOperation right) => op.Target switch
+    {
+        IFieldReferenceOperation fieldOp =>
+            left is IFieldReferenceOperation fieldLeft && SymbolEqualityComparer.Default.Equals(fieldOp.Field, fieldLeft.Field) ||
+            right is IFieldReferenceOperation fieldRight && SymbolEqualityComparer.Default.Equals(fieldOp.Field, fieldRight.Field),
+        IPropertyReferenceOperation propOp =>
+            left is IPropertyReferenceOperation propLeft && SymbolEqualityComparer.Default.Equals(propOp.Property, propLeft.Property) ||
+            right is IPropertyReferenceOperation propRight && SymbolEqualityComparer.Default.Equals(propOp.Property, propRight.Property),
+        IParameterReferenceOperation paramOp =>
+            left is IParameterReferenceOperation paramLeft && SymbolEqualityComparer.Default.Equals(paramOp.Parameter, paramLeft.Parameter) ||
+            right is IParameterReferenceOperation paramRight && SymbolEqualityComparer.Default.Equals(paramOp.Parameter, paramRight.Parameter),
+        ILocalReferenceOperation localOp =>
+            left is ILocalReferenceOperation localLeft && SymbolEqualityComparer.Default.Equals(localOp.Local, localLeft.Local) ||
+            right is ILocalReferenceOperation localRight && SymbolEqualityComparer.Default.Equals(localOp.Local, localRight.Local),
+        _ => false,
+    };
+}