diff --git a/Directory.Build.props b/Directory.Build.props index c302a740..eb0393cf 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -12,7 +12,7 @@ enable en false - + all true true diff --git a/Directory.Packages.props b/Directory.Packages.props index 5950e50b..5978cc30 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -1,27 +1,27 @@ - - true - - - - runtime; build; native; contentfiles; analyzers; buildtransitive - all - - - runtime; build; native; contentfiles; analyzers; buildtransitive - all - - - - - - - - - - - - - - - \ No newline at end of file + + true + + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + + + + + + + + + + + + + diff --git a/NuGet.Config b/NuGet.Config index 85ebb4c5..1f4549ca 100644 --- a/NuGet.Config +++ b/NuGet.Config @@ -1,8 +1,8 @@  - - - - - + + + + + diff --git a/Rules to work on.md b/Rules to work on.md index 21446b6e..dfd03401 100644 --- 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 index 2657d2ba..cd7b3128 100644 --- 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 index 9cec9a01..56364e05 100644 --- 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 Invocations = [OperationKind.Invocation]; /// The diagnostic descriptor. 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, 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[] array, Action action) + { + if (SymbolEqualityComparer.Default.Equals(symbol, compilation.GetTypeByMetadataName("System.Array"))) + return operation.Arguments[1]; + } + else if ( // List and ImmutableList : void ForEach(Action 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 new file mode 100644 index 00000000..4ec35dd5 --- /dev/null +++ b/src/EcoCode.Core/Extensions/OperationExtensions.cs @@ -0,0 +1,27 @@ +namespace EcoCode.Extensions; + +/// Extensions methods for . +public static class OperationExtensions +{ + /// Returns whether the given operation's target matches one of the given operations' target. + /// The operation. + /// The left operation. + /// The right operation. + /// True if the operation's target matches one of the given operations' target, false otherwise. + 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, + }; +} diff --git a/src/EcoCode.Tests/Tests/EC75.DontConcatenateStringsInLoops.Tests.cs b/src/EcoCode.Tests/Tests/EC75.DontConcatenateStringsInLoops.Tests.cs index 69c765e8..9b27b403 100644 --- a/src/EcoCode.Tests/Tests/EC75.DontConcatenateStringsInLoops.Tests.cs +++ b/src/EcoCode.Tests/Tests/EC75.DontConcatenateStringsInLoops.Tests.cs @@ -8,68 +8,302 @@ public sealed class DontConcatenateStringsInLoopsTests [TestMethod] public async Task EmptyCodeAsync() => await VerifyAsync("").ConfigureAwait(false); + private const string WarningCode = """ + + { + string si = i.ToString(); + [|s += si|]; + [|s = s + si|]; + [|s = si + s|]; + s = si + si; + } + """; + + #region Regular loops + [TestMethod] - public async Task DontConcatenateStringsInLoops1Async() => await VerifyAsync(""" - public class Test + public async Task DontConcatenateStringsInLoopsParameterWithAllLoopsAsync() => await VerifyAsync($$""" + using System.Linq; + class Test { - private string s1 = string.Empty; - private static string s2 = string.Empty; + void Run1(string s) + { + for (int i = 0; i < 10; i++){{WarningCode}} + } - public void Run(string s0) + void Run2(string s) { - for (int i = 0; i < 10; i++) - [|s0 += i;|] - for (int i = 0; i < 10; i++) - s0 = i.ToString(); + foreach (int i in Enumerable.Range(0, 10)){{WarningCode}} + } - for (int i = 0; i < 10; i++) - [|s1 += i;|] - for (int i = 0; i < 10; i++) - s1 = i.ToString(); + void Run3(string s) + { + int i = 0; + while (i++ < 10){{WarningCode}} + } - for (int i = 0; i < 10; i++) - [|s2 += i;|] - for (int i = 0; i < 10; i++) - s2 = i.ToString(); + void Run4(string s) + { + int i = 0; + do{{WarningCode}} while (++i < 10); + } + } + """).ConfigureAwait(false); - string s3 = string.Empty; - for (int i = 0; i < 10; i++) - [|s3 += i;|] - for (int i = 0; i < 10; i++) - s3 = i.ToString(); + [TestMethod] + public async Task DontConcatenateStringsInLoopsFieldAsync() => await VerifyAsync($$""" + class Test + { + string s = string.Empty; + void Run() + { + for (int i = 0; i < 10; i++){{WarningCode}} } } """).ConfigureAwait(false); [TestMethod] - public async Task DontConcatenateStringsInLoops2Async() => await VerifyAsync(""" - public class Test + public async Task DontConcatenateStringsInLoopsPropertyAsync() => await VerifyAsync($$""" + class Test + { + string s { get; set; } = string.Empty; + void Run() + { + for (int i = 0; i < 10; i++){{WarningCode}} + } + } + """).ConfigureAwait(false); + + [TestMethod] + public async Task DontConcatenateStringsInLoopsStaticFieldAsync() => await VerifyAsync($$""" + class Test + { + static string s = string.Empty; + void Run() + { + for (int i = 0; i < 10; i++){{WarningCode}} + } + } + """).ConfigureAwait(false); + + [TestMethod] + public async Task DontConcatenateStringsInLoopsStaticPropertyAsync() => await VerifyAsync($$""" + class Test { - private string s1 = string.Empty; - private static string s2 = string.Empty; + static string s { get; set; } = string.Empty; + void Run() + { + for (int i = 0; i < 10; i++){{WarningCode}} + } + } + """).ConfigureAwait(false); - public void Run(string s0) + [TestMethod] + public async Task DontConcatenateStringsInLoopsLocalVariableAsync() => await VerifyAsync($$""" + class Test + { + void Run() { - string s3 = string.Empty; + string s = string.Empty; + for (int i = 0; i < 10; i++){{WarningCode}} + for (int i = 0; i < 10; i++) { - s0 = i.ToString(); - [|s0 += i;|] + string si = i.ToString(); + string s2 = string.Empty; + s2 += si; + s2 = s2 + si; + s2 = si + s2; + s2 = si + si; + } + } + } + """).ConfigureAwait(false); - s1 = i.ToString(); - [|s1 += i;|] + #endregion - s2 = i.ToString(); - [|s2 += i;|] + #region ForEach methods - s3 = i.ToString(); - [|s3 += i;|] + [TestMethod] + [DataRow("List")] + [DataRow("ImmutableList")] + public async Task DontConcatenateStringsInForEachParameterAsync(string type) => await VerifyAsync($$""" + using System.Collections.Generic; + using System.Collections.Immutable; + using System.Threading.Tasks; + public class Test + { + public void Run({{type}} list, string s) + { + list.ForEach(i =>{{WarningCode}}); + list.ForEach(i => [|s += i.ToString()|]); + list.ForEach(i => [|s = s + i.ToString()|]); + list.ForEach(i => [|s = i.ToString() + s|]); + list.ForEach(i => s = i.ToString() + i.ToString()); - string s4; - s4 = i.ToString(); - s4 += i; - } + Parallel.ForEach(list, i =>{{WarningCode}}); + Parallel.ForEach(list, i => [|s += i.ToString()|]); + Parallel.ForEach(list, i => [|s = s + i.ToString()|]); + Parallel.ForEach(list, i => [|s = i.ToString() + s|]); + Parallel.ForEach(list, i => s = i.ToString() + i.ToString()); + } + } + """).ConfigureAwait(false); + + [TestMethod] + [DataRow("List")] + [DataRow("ImmutableList")] + public async Task DontConcatenateStringsInForEachFieldAsync(string type) => await VerifyAsync($$""" + using System.Collections.Generic; + using System.Collections.Immutable; + using System.Threading.Tasks; + public class Test + { + private string s = string.Empty; + public void Run({{type}} list) + { + list.ForEach(i =>{{WarningCode}}); + list.ForEach(i => [|s += i.ToString()|]); + list.ForEach(i => [|s = s + i.ToString()|]); + list.ForEach(i => [|s = i.ToString() + s|]); + list.ForEach(i => s = i.ToString() + i.ToString()); + + Parallel.ForEach(list, i =>{{WarningCode}}); + Parallel.ForEach(list, i => [|s += i.ToString()|]); + Parallel.ForEach(list, i => [|s = s + i.ToString()|]); + Parallel.ForEach(list, i => [|s = i.ToString() + s|]); + Parallel.ForEach(list, i => s = i.ToString() + i.ToString()); } } """).ConfigureAwait(false); + + [TestMethod] + [DataRow("List")] + [DataRow("ImmutableList")] + public async Task DontConcatenateStringsInForEachPropertyAsync(string type) => await VerifyAsync($$""" + using System.Collections.Generic; + using System.Collections.Immutable; + using System.Threading.Tasks; + public class Test + { + private string s { get; set; } = string.Empty; + public void Run({{type}} list) + { + list.ForEach(i =>{{WarningCode}}); + list.ForEach(i => [|s += i.ToString()|]); + list.ForEach(i => [|s = s + i.ToString()|]); + list.ForEach(i => [|s = i.ToString() + s|]); + list.ForEach(i => s = i.ToString() + i.ToString()); + + Parallel.ForEach(list, i =>{{WarningCode}}); + Parallel.ForEach(list, i => [|s += i.ToString()|]); + Parallel.ForEach(list, i => [|s = s + i.ToString()|]); + Parallel.ForEach(list, i => [|s = i.ToString() + s|]); + Parallel.ForEach(list, i => s = i.ToString() + i.ToString()); + } + } + """).ConfigureAwait(false); + + [TestMethod] + [DataRow("List")] + [DataRow("ImmutableList")] + public async Task DontConcatenateStringsInForEachStaticFieldAsync(string type) => await VerifyAsync($$""" + using System.Collections.Generic; + using System.Collections.Immutable; + using System.Threading.Tasks; + public class Test + { + private static string s = string.Empty; + public void Run({{type}} list) + { + list.ForEach(i =>{{WarningCode}}); + list.ForEach(i => [|s += i.ToString()|]); + list.ForEach(i => [|s = s + i.ToString()|]); + list.ForEach(i => [|s = i.ToString() + s|]); + list.ForEach(i => s = i.ToString() + i.ToString()); + + Parallel.ForEach(list, i =>{{WarningCode}}); + Parallel.ForEach(list, i => [|s += i.ToString()|]); + Parallel.ForEach(list, i => [|s = s + i.ToString()|]); + Parallel.ForEach(list, i => [|s = i.ToString() + s|]); + Parallel.ForEach(list, i => s = i.ToString() + i.ToString()); + } + } + """).ConfigureAwait(false); + + [TestMethod] + [DataRow("List")] + [DataRow("ImmutableList")] + public async Task DontConcatenateStringsInForEachStaticPropertyAsync(string type) => await VerifyAsync($$""" + using System.Collections.Generic; + using System.Collections.Immutable; + using System.Threading.Tasks; + public class Test + { + private static string s { get; set; } = string.Empty; + public void Run({{type}} list) + { + list.ForEach(i =>{{WarningCode}}); + list.ForEach(i => [|s += i.ToString()|]); + list.ForEach(i => [|s = s + i.ToString()|]); + list.ForEach(i => [|s = i.ToString() + s|]); + list.ForEach(i => s = i.ToString() + i.ToString()); + + Parallel.ForEach(list, i =>{{WarningCode}}); + Parallel.ForEach(list, i => [|s += i.ToString()|]); + Parallel.ForEach(list, i => [|s = s + i.ToString()|]); + Parallel.ForEach(list, i => [|s = i.ToString() + s|]); + Parallel.ForEach(list, i => s = i.ToString() + i.ToString()); + } + } + """).ConfigureAwait(false); + + [TestMethod] + [DataRow("List")] + [DataRow("ImmutableList")] + public async Task DontConcatenateStringsInForEachLocalVariableAsync(string type) => await VerifyAsync($$""" + using System.Collections.Generic; + using System.Collections.Immutable; + using System.Threading.Tasks; + public class Test + { + public void Run({{type}} list) + { + string s = string.Empty; + list.ForEach(i =>{{WarningCode}}); + list.ForEach(i => [|s += i.ToString()|]); + list.ForEach(i => [|s = s + i.ToString()|]); + list.ForEach(i => [|s = i.ToString() + s|]); + list.ForEach(i => s = i.ToString() + i.ToString()); + + Parallel.ForEach(list, i =>{{WarningCode}}); + Parallel.ForEach(list, i => [|s += i.ToString()|]); + Parallel.ForEach(list, i => [|s = s + i.ToString()|]); + Parallel.ForEach(list, i => [|s = i.ToString() + s|]); + Parallel.ForEach(list, i => s = i.ToString() + i.ToString()); + + list.ForEach(i => + { + string si = i.ToString(); + string s2 = string.Empty; + s2 += si; + s2 = s2 + si; + s2 = si + s2; + s2 = si + si; + }); + + Parallel.ForEach(list, i => + { + string si = i.ToString(); + string s2 = string.Empty; + s2 += si; + s2 = s2 + si; + s2 = si + s2; + s2 = si + si; + }); + } + } + """).ConfigureAwait(false); + + #endregion }