diff --git a/src/Meziantou.Analyzer/Internals/HashSetExtensions.cs b/src/Meziantou.Analyzer/Internals/HashSetExtensions.cs new file mode 100644 index 000000000..9c0bfb38d --- /dev/null +++ b/src/Meziantou.Analyzer/Internals/HashSetExtensions.cs @@ -0,0 +1,13 @@ +using System.Collections.Generic; + +namespace Meziantou.Analyzer.Internals; +internal static class HashSetExtensions +{ + public static void AddIfNotNull(this HashSet hashSet, T? item) + { + if (item is null) + return; + + _ = hashSet.Add(item); + } +} diff --git a/src/Meziantou.Analyzer/Internals/SymbolExtensions.cs b/src/Meziantou.Analyzer/Internals/SymbolExtensions.cs index 0437ccfc1..0bf3f4a08 100644 --- a/src/Meziantou.Analyzer/Internals/SymbolExtensions.cs +++ b/src/Meziantou.Analyzer/Internals/SymbolExtensions.cs @@ -7,7 +7,7 @@ namespace Meziantou.Analyzer; internal static class SymbolExtensions { - public static bool IsEqualTo(this ISymbol? symbol, ISymbol? expectedType) + public static bool IsEqualTo(this ISymbol? symbol, [NotNullWhen(true)] ISymbol? expectedType) { if (symbol is null || expectedType is null) return false; diff --git a/src/Meziantou.Analyzer/Rules/OptimizeStringBuilderUsageAnalyzer.cs b/src/Meziantou.Analyzer/Rules/OptimizeStringBuilderUsageAnalyzer.cs index d941fc60a..91885047c 100644 --- a/src/Meziantou.Analyzer/Rules/OptimizeStringBuilderUsageAnalyzer.cs +++ b/src/Meziantou.Analyzer/Rules/OptimizeStringBuilderUsageAnalyzer.cs @@ -1,4 +1,5 @@ -using System.Collections.Immutable; +using System.Collections.Generic; +using System.Collections.Immutable; using System.Diagnostics.CodeAnalysis; using System.Text; using Meziantou.Analyzer.Internals; @@ -28,63 +29,138 @@ public override void Initialize(AnalysisContext context) context.EnableConcurrentExecution(); context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); - context.RegisterOperationAction(Analyze, OperationKind.Invocation); + context.RegisterCompilationStartAction(compilationStartContext => + { + var analyzerContext = new AnalyzerContext(compilationStartContext.Compilation); + if (!analyzerContext.IsValid) + return; + + compilationStartContext.RegisterOperationAction(analyzerContext.Analyze, OperationKind.Invocation); + }); } - private static void Analyze(OperationAnalysisContext context) + private sealed class AnalyzerContext { - var operation = (IInvocationOperation)context.Operation; - if (operation.Arguments.Length == 0) - return; - - var stringBuilderSymbol = context.Compilation.GetBestTypeByMetadataName("System.Text.StringBuilder"); - if (stringBuilderSymbol is null) - return; + private readonly ITypeSymbol? _stringBuilderSymbol; + private readonly HashSet _appendOverloadTypes = new(SymbolEqualityComparer.Default); + private readonly bool _hasAppendJoin; - var method = operation.TargetMethod; - if (!method.ContainingType.IsEqualTo(stringBuilderSymbol)) - return; - - if (string.Equals(method.Name, nameof(StringBuilder.Append), System.StringComparison.Ordinal)) + public AnalyzerContext(Compilation compilation) { - if (method.Parameters.Length == 0 || !method.Parameters[0].Type.IsString()) + _stringBuilderSymbol = compilation.GetBestTypeByMetadataName("System.Text.StringBuilder"); + if (_stringBuilderSymbol is null) return; - if (!IsOptimizable(context, operation, method.Name, operation.Arguments[0], stringBuilderSymbol)) - return; + _hasAppendJoin = _stringBuilderSymbol.GetMembers("AppendJoin").Length > 0; + + _appendOverloadTypes.AddIfNotNull(_stringBuilderSymbol); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_Int16)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_Int32)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_Int64)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_UInt16)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_UInt32)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_UInt64)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_Boolean)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_Byte)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_SByte)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_Single)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_Double)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_Decimal)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_String)); + _appendOverloadTypes.AddIfNotNull(compilation.GetSpecialType(SpecialType.System_Char)); + _appendOverloadTypes.AddIfNotNull(compilation.CreateArrayTypeSymbol(compilation.GetSpecialType(SpecialType.System_Char))); + _appendOverloadTypes.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.ReadOnlySpan`1")?.Construct(compilation.GetSpecialType(SpecialType.System_Char))); + _appendOverloadTypes.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.ReadOnlyMemory`1")?.Construct(compilation.GetSpecialType(SpecialType.System_Char))); } - else if (string.Equals(method.Name, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) + + public bool IsValid => _stringBuilderSymbol is not null; + + public void Analyze(OperationAnalysisContext context) { - if (method.Parameters.Length == 0 || !method.Parameters[0].Type.IsString()) + var operation = (IInvocationOperation)context.Operation; + if (operation.Arguments.Length == 0) return; - if (!IsOptimizable(context, operation, method.Name, operation.Arguments[0], stringBuilderSymbol)) + var method = operation.TargetMethod; + if (!method.ContainingType.IsEqualTo(_stringBuilderSymbol)) return; - } - else if (string.Equals(method.Name, nameof(StringBuilder.Insert), System.StringComparison.Ordinal)) - { - if (method.Parameters.Length == 2 && method.Parameters[1].Type.IsString()) + + if (string.Equals(method.Name, nameof(StringBuilder.Append), System.StringComparison.Ordinal)) { - if (!IsOptimizable(context, operation, method.Name, operation.Arguments[1], stringBuilderSymbol)) + if (method.Parameters.Length == 0 || !method.Parameters[0].Type.IsString()) + return; + + if (!IsOptimizable(context, operation, method.Name, operation.Arguments[0])) return; } - } - } + else if (string.Equals(method.Name, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) + { + if (method.Parameters.Length == 0 || !method.Parameters[0].Type.IsString()) + return; - private static ImmutableDictionary CreateProperties(OptimizeStringBuilderUsageData data) - { - return ImmutableDictionary.Create().Add("Data", data.ToString()); - } + if (!IsOptimizable(context, operation, method.Name, operation.Arguments[0])) + return; + } + else if (string.Equals(method.Name, nameof(StringBuilder.Insert), System.StringComparison.Ordinal)) + { + if (method.Parameters.Length == 2 && method.Parameters[1].Type.IsString()) + { + if (!IsOptimizable(context, operation, method.Name, operation.Arguments[1])) + return; + } + } + } - private static bool IsOptimizable(OperationAnalysisContext context, IOperation operation, string methodName, IArgumentOperation argument, ITypeSymbol stringBuilderSymbol) - { - if (argument.ConstantValue.HasValue) - return false; + private static ImmutableDictionary CreateProperties(OptimizeStringBuilderUsageData data) + { + return ImmutableDictionary.Create().Add("Data", data.ToString()); + } - var value = argument.Value; - if (value is IInterpolatedStringOperation) + private bool IsOptimizable(OperationAnalysisContext context, IOperation operation, string methodName, IArgumentOperation argument) { - if (TryGetConstStringValue(value, out var constValue)) + if (argument.ConstantValue.HasValue) + return false; + + var value = argument.Value; + if (value is IInterpolatedStringOperation) + { + if (TryGetConstStringValue(value, out var constValue)) + { + if (constValue.Length == 0) + { + if (string.Equals(methodName, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) + { + var properties = CreateProperties(OptimizeStringBuilderUsageData.RemoveArgument); + context.ReportDiagnostic(Rule, properties, operation, "Remove the useless argument"); + } + else + { + var properties = CreateProperties(OptimizeStringBuilderUsageData.RemoveMethod); + context.ReportDiagnostic(Rule, properties, operation, "Remove this no-op call"); + } + + return true; + } + else if (constValue.Length == 1) + { + var properties = CreateProperties(OptimizeStringBuilderUsageData.ReplaceWithChar) + .Add("ConstantValue", constValue); + context.ReportDiagnostic(Rule, properties, argument, $"Replace {methodName}(string) with {methodName}(char)"); + return true; + } + return false; + } + else + { + if (string.Equals(methodName, nameof(StringBuilder.Insert), System.StringComparison.Ordinal)) + return false; + + var properties = CreateProperties(OptimizeStringBuilderUsageData.SplitStringInterpolation); + context.ReportDiagnostic(Rule, properties, operation, $"Replace string interpolation with multiple {methodName} calls or use an interpolated string"); + return true; + } + } + else if (TryGetConstStringValue(value, out var constValue)) { if (constValue.Length == 0) { @@ -103,167 +179,115 @@ private static bool IsOptimizable(OperationAnalysisContext context, IOperation o } else if (constValue.Length == 1) { - var properties = CreateProperties(OptimizeStringBuilderUsageData.ReplaceWithChar) - .Add("ConstantValue", constValue); - context.ReportDiagnostic(Rule, properties, argument, $"Replace {methodName}(string) with {methodName}(char)"); - return true; + if (string.Equals(methodName, nameof(StringBuilder.Append), System.StringComparison.Ordinal) || string.Equals(methodName, nameof(StringBuilder.Insert), System.StringComparison.Ordinal)) + { + var properties = CreateProperties(OptimizeStringBuilderUsageData.ReplaceWithChar) + .Add("ConstantValue", constValue); + context.ReportDiagnostic(Rule, properties, argument, $"Replace {methodName}(string) with {methodName}(char)"); + return true; + } } - return false; - } - else - { - if (string.Equals(methodName, nameof(StringBuilder.Insert), System.StringComparison.Ordinal)) - return false; - var properties = CreateProperties(OptimizeStringBuilderUsageData.SplitStringInterpolation); - context.ReportDiagnostic(Rule, properties, operation, $"Replace string interpolation with multiple {methodName} calls"); - return true; + return false; } - } - else if (TryGetConstStringValue(value, out var constValue)) - { - if (constValue.Length == 0) + else if (value is IBinaryOperation binaryOperation) { - if (string.Equals(methodName, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) + if (string.Equals(methodName, nameof(StringBuilder.Append), System.StringComparison.Ordinal) || string.Equals(methodName, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) { - var properties = CreateProperties(OptimizeStringBuilderUsageData.RemoveArgument); - context.ReportDiagnostic(Rule, properties, operation, "Remove the useless argument"); - } - else - { - var properties = CreateProperties(OptimizeStringBuilderUsageData.RemoveMethod); - context.ReportDiagnostic(Rule, properties, operation, "Remove this no-op call"); - } + if (binaryOperation.OperatorKind == BinaryOperatorKind.Add && value.Type.IsString()) + { + if (IsConstString(binaryOperation.LeftOperand) && IsConstString(binaryOperation.RightOperand)) + return false; - return true; - } - else if (constValue.Length == 1) - { - if (string.Equals(methodName, nameof(StringBuilder.Append), System.StringComparison.Ordinal) || string.Equals(methodName, nameof(StringBuilder.Insert), System.StringComparison.Ordinal)) - { - var properties = CreateProperties(OptimizeStringBuilderUsageData.ReplaceWithChar) - .Add("ConstantValue", constValue); - context.ReportDiagnostic(Rule, properties, argument, $"Replace {methodName}(string) with {methodName}(char)"); - return true; + var properties = CreateProperties(OptimizeStringBuilderUsageData.SplitAddOperator); + context.ReportDiagnostic(Rule, properties, operation, $"Replace the string concatenation by multiple {methodName} calls"); + return true; + } } } - - return false; - } - else if (value is IBinaryOperation binaryOperation) - { - if (string.Equals(methodName, nameof(StringBuilder.Append), System.StringComparison.Ordinal) || string.Equals(methodName, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) + else if (value is IInvocationOperation invocationOperation) { - if (binaryOperation.OperatorKind == BinaryOperatorKind.Add && value.Type.IsString()) + var targetMethod = invocationOperation.TargetMethod; + if (string.Equals(targetMethod.Name, "ToString", System.StringComparison.Ordinal)) { - if (IsConstString(binaryOperation.LeftOperand) && IsConstString(binaryOperation.RightOperand)) - return false; + if (targetMethod.Parameters.Length == 0 && targetMethod.ReturnType.IsString()) + { + if (invocationOperation.Instance?.Type is not null && !_appendOverloadTypes.Contains(invocationOperation.Instance.Type)) + return false; - var properties = CreateProperties(OptimizeStringBuilderUsageData.SplitAddOperator); - context.ReportDiagnostic(Rule, properties, operation, $"Replace the string concatenation by multiple {methodName} calls"); - return true; + var properties = CreateProperties(OptimizeStringBuilderUsageData.RemoveToString); + if (string.Equals(methodName, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) + { + context.ReportDiagnostic(Rule, properties, operation, "Replace with Append().AppendLine()"); + } + else + { + context.ReportDiagnostic(Rule, properties, operation, "Remove the ToString call"); + } + + return true; + } } - } - } - else if (value is IInvocationOperation invocationOperation) - { - var targetMethod = invocationOperation.TargetMethod; - if (string.Equals(targetMethod.Name, "ToString", System.StringComparison.Ordinal)) - { - if (targetMethod.Parameters.Length == 0 && targetMethod.ReturnType.IsString()) + else if (methodName != "Insert" && string.Equals(targetMethod.Name, nameof(string.Format), System.StringComparison.Ordinal) && targetMethod.ContainingType.IsString() && targetMethod.IsStatic) { - if (invocationOperation.Instance?.Type?.IsValueType == true && !IsPrimitive(invocationOperation.Instance.Type)) - return false; - - var properties = CreateProperties(OptimizeStringBuilderUsageData.RemoveToString); + var properties = CreateProperties(OptimizeStringBuilderUsageData.ReplaceStringFormatWithAppendFormat); if (string.Equals(methodName, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) { - context.ReportDiagnostic(Rule, properties, operation, "Replace with Append().AppendLine()"); + context.ReportDiagnostic(Rule, properties, operation, "Replace with AppendFormat().AppendLine()"); } else { - context.ReportDiagnostic(Rule, properties, operation, "Remove the ToString call"); + context.ReportDiagnostic(Rule, properties, operation, "Replace with AppendFormat()"); } return true; } - } - else if (methodName != "Insert" && string.Equals(targetMethod.Name, nameof(string.Format), System.StringComparison.Ordinal) && targetMethod.ContainingType.IsString() && targetMethod.IsStatic) - { - var properties = CreateProperties(OptimizeStringBuilderUsageData.ReplaceStringFormatWithAppendFormat); - if (string.Equals(methodName, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) - { - context.ReportDiagnostic(Rule, properties, operation, "Replace with AppendFormat().AppendLine()"); - } - else + else if (methodName != "Insert" && string.Equals(targetMethod.Name, nameof(string.Join), System.StringComparison.Ordinal) && targetMethod.ContainingType.IsString() && targetMethod.IsStatic) { - context.ReportDiagnostic(Rule, properties, operation, "Replace with AppendFormat()"); - } - - return true; - } - else if (methodName != "Insert" && string.Equals(targetMethod.Name, nameof(string.Join), System.StringComparison.Ordinal) && targetMethod.ContainingType.IsString() && targetMethod.IsStatic) - { - // Check if StringBuilder.AppendJoin exists - if (stringBuilderSymbol.GetMembers("AppendJoin").Length > 0) - { - var properties = CreateProperties(OptimizeStringBuilderUsageData.ReplaceStringJoinWithAppendJoin); - if (string.Equals(methodName, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) - { - context.ReportDiagnostic(Rule, properties, operation, "Replace with AppendJoin().AppendLine()"); - } - else + // Check if StringBuilder.AppendJoin exists + if (_hasAppendJoin) { - context.ReportDiagnostic(Rule, properties, operation, "Replace with AppendJoin()"); - } + var properties = CreateProperties(OptimizeStringBuilderUsageData.ReplaceStringJoinWithAppendJoin); + if (string.Equals(methodName, nameof(StringBuilder.AppendLine), System.StringComparison.Ordinal)) + { + context.ReportDiagnostic(Rule, properties, operation, "Replace with AppendJoin().AppendLine()"); + } + else + { + context.ReportDiagnostic(Rule, properties, operation, "Replace with AppendJoin()"); + } + return true; + } + } + else if (string.Equals(targetMethod.Name, nameof(string.Substring), System.StringComparison.Ordinal) && targetMethod.ContainingType.IsString()) + { + var properties = CreateProperties(OptimizeStringBuilderUsageData.ReplaceSubstring); + context.ReportDiagnostic(Rule, properties, operation, $"Use {methodName}(string, int, int) or {methodName}(ReadOnlySpan) instead of Substring"); return true; } } - else if (string.Equals(targetMethod.Name, nameof(string.Substring), System.StringComparison.Ordinal) && targetMethod.ContainingType.IsString()) - { - var properties = CreateProperties(OptimizeStringBuilderUsageData.ReplaceSubstring); - context.ReportDiagnostic(Rule, properties, operation, $"Use {methodName}(string, int, int) instead of Substring"); - return true; - } - } - - return false; - } - private static bool IsPrimitive(ITypeSymbol symbol) - { - return symbol.SpecialType == SpecialType.System_Boolean - || symbol.SpecialType == SpecialType.System_Byte - || symbol.SpecialType == SpecialType.System_Char - || symbol.SpecialType == SpecialType.System_Decimal - || symbol.SpecialType == SpecialType.System_Double - || symbol.SpecialType == SpecialType.System_Int16 - || symbol.SpecialType == SpecialType.System_Int32 - || symbol.SpecialType == SpecialType.System_Int64 - || symbol.SpecialType == SpecialType.System_SByte - || symbol.SpecialType == SpecialType.System_UInt16 - || symbol.SpecialType == SpecialType.System_UInt32 - || symbol.SpecialType == SpecialType.System_UInt64; - } - - private static bool IsConstString(IOperation operation) - { - return TryGetConstStringValue(operation, out _); - } + return false; + } - private static bool TryGetConstStringValue(IOperation operation, [NotNullWhen(true)] out string? value) - { - var sb = ObjectPool.SharedStringBuilderPool.Get(); - if (OptimizeStringBuilderUsageAnalyzerCommon.TryGetConstStringValue(operation, sb)) + private static bool IsConstString(IOperation operation) { - value = sb.ToString(); - ObjectPool.SharedStringBuilderPool.Return(sb); - return true; + return TryGetConstStringValue(operation, out _); } - value = default; - return false; - } + private static bool TryGetConstStringValue(IOperation operation, [NotNullWhen(true)] out string? value) + { + var sb = ObjectPool.SharedStringBuilderPool.Get(); + if (OptimizeStringBuilderUsageAnalyzerCommon.TryGetConstStringValue(operation, sb)) + { + value = sb.ToString(); + ObjectPool.SharedStringBuilderPool.Return(sb); + return true; + } + value = default; + return false; + } + } } diff --git a/tests/Meziantou.Analyzer.Test/Rules/OptimizeStringBuilderUsageAnalyzerTests.cs b/tests/Meziantou.Analyzer.Test/Rules/OptimizeStringBuilderUsageAnalyzerTests.cs index f994c2508..7074d5ce6 100755 --- a/tests/Meziantou.Analyzer.Test/Rules/OptimizeStringBuilderUsageAnalyzerTests.cs +++ b/tests/Meziantou.Analyzer.Test/Rules/OptimizeStringBuilderUsageAnalyzerTests.cs @@ -91,6 +91,31 @@ void A() .ValidateAsync(); } +#if CSHARP10_OR_GREATER + [Theory] + [InlineData(@"""abc""")] + [InlineData(@"$""abc""")] + [InlineData(@"$""{0}abc""")] + + public async Task AppendLine_Net8_NoDiagnostic(string text) + { + await CreateProjectBuilder() + .WithSourceCode($$""" + using System.Text; + class Test + { + void A() + { + new StringBuilder().AppendLine({{text}}); + } + } + """) + .WithTargetFramework(TargetFramework.Net8_0) + .WithLanguageVersion(Microsoft.CodeAnalysis.CSharp.LanguageVersion.CSharp10) + .ValidateAsync(); + } +#endif + [Theory] [InlineData(@"$""a{1}""")] [InlineData(@"""a"" + 10")] @@ -670,4 +695,44 @@ void A() }") .ValidateAsync(); } + + [Theory] + [InlineData("System.ReadOnlySpan")] + [InlineData("System.ReadOnlyMemory")] + [InlineData("bool")] + [InlineData("byte")] + [InlineData("char")] + [InlineData("char[]")] + [InlineData("decimal")] + [InlineData("double")] + [InlineData("short")] + [InlineData("int")] + [InlineData("long")] + [InlineData("sbyte")] + [InlineData("float")] + [InlineData("string")] + [InlineData("System.Text.StringBuilder")] + [InlineData("ushort")] + [InlineData("uint")] + [InlineData("ulong")] + public async Task AppendLine_ValueToString_Report(string dataType) + { + await CreateProjectBuilder() + .WithSourceCode($$"""[||]new System.Text.StringBuilder().AppendLine(default({{dataType}}).ToString());""") + .WithOutputKind(Microsoft.CodeAnalysis.OutputKind.ConsoleApplication) + .WithTargetFramework(TargetFramework.Net8_0) + .ValidateAsync(); + } + + [Theory] + [InlineData("object")] + [InlineData("System.ReadOnlySpan")] + public async Task AppendLine_ValueToString_NoReport(string dataType) + { + await CreateProjectBuilder() + .WithSourceCode($$"""new System.Text.StringBuilder().AppendLine(default({{dataType}}).ToString());""") + .WithOutputKind(Microsoft.CodeAnalysis.OutputKind.ConsoleApplication) + .WithTargetFramework(TargetFramework.Net8_0) + .ValidateAsync(); + } }