diff --git a/build/targets/compiler/Compiler.props b/build/targets/compiler/Compiler.props index 7f9eb03..e0cf891 100644 --- a/build/targets/compiler/Compiler.props +++ b/build/targets/compiler/Compiler.props @@ -11,4 +11,13 @@ runtime; build; native; contentfiles; analyzers + + + + <_Parameter1>false + + + <_Parameter1>false + + diff --git a/docs/rules/ECS0009.md b/docs/rules/ECS0009.md new file mode 100644 index 0000000..559ff63 --- /dev/null +++ b/docs/rules/ECS0009.md @@ -0,0 +1,73 @@ +# ECS0009: Minimize boxing and unboxing + +This rule is described in detail in [Effective C#: 50 Specific Ways to Improve your C#](https://www.oreilly.com/library/view/effective-c-50/9780134579290/). + +## Cause + +Value types can be converted to `System.Object` or any interface reference. Those conversions may happen implicitly, complicating the task of finding them. The boxing and unboxing operations make copies where you might not expect. That causes bugs.Boxing and unboxing operations can degrade performance and cause subtle bugs through those copies. These operations occur when a value type is converted to a reference type and vice versa. Be on the lookout for any constructs that convert value types to either `System.Object` or interface types: placing values in collections, calling methods defined in `System.Object`, and casts to `System.Object`. + +## Rule description + +This rule detects scenarios where boxing and unboxing occur implicitly or explicitly. It aims to help developers identify and minimize these operations to improve performance and avoid potential issues. + +## How to fix violations + +To fix violations, consider using generics, value type collections, or other means to avoid converting value types to reference types. + +## When to suppress warnings + +Suppress warnings if boxing or unboxing is necessary and there is no performance-critical impact, or if the code is optimized for readability and maintainability rather than performance. + +## Example of a violation + +### Description + +Assigning a value type to a reference type or passing a value type to a method that expects a reference type causes boxing. + +### Code + +```csharp +int i = 5; +object o = i; // boxing +``` + +Boxing may also occur in compiler-generated code implicitly. + +```csharp +var attendees = new List(); +var p = new Person { Name = "Old Name" }; +attendees.Add(p); + +// Try to change the name +var p2 = attendees[0]; +p2.Name = "New Name"; // Boxing occurs here + +// Writes "Old Name": +Console.WriteLine(attendees[0].ToString()); +``` + +## Example of how to fix + +### Description + +Use collections or methods that avoid boxing and unboxing operations. + +### Code + +```csharp +int i = 5; +int j = i; // No boxing +``` + +For the `Person` value type, create an immutable value type. + +```csharp +public struct Person +{ + public string Name { get; } + + public Person(string name) => Name = name; + + public override string ToString() => Name; +} +``` \ No newline at end of file diff --git a/src/EffectiveCSharp.Analyzers/AnalyzerReleases.Unshipped.md b/src/EffectiveCSharp.Analyzers/AnalyzerReleases.Unshipped.md index 4b0114a..96717c9 100644 --- a/src/EffectiveCSharp.Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/EffectiveCSharp.Analyzers/AnalyzerReleases.Unshipped.md @@ -5,5 +5,6 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- ECS0002 | Maintainability | Info | PreferReadonlyOverConstAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/10c2d53afd688efe5a59097f76cb4edf33f6a474/docs/ECS0002.md) -ECS0006 | Refactoring | Warning | AvoidStringlyTypedApisAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/main/docs/ECS0006.md) +ECS0006 | Refactoring | Info | AvoidStringlyTypedApisAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers6213cba8473dac61d6132e205550884eae1c94bf/docs/ECS0006.md) +ECS0009 | Performance | Info | MinimizeBoxingUnboxingAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/6213cba8473dac61d6132e205550884eae1c94bf/docs/ECS0009.md) ECS1000 | Performance | Info | SpanAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/d00a4cc9f61e7d5b392894aad859e46c43a5611c/docs/ECS1000.md) \ No newline at end of file diff --git a/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisAnalyzer.cs b/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisAnalyzer.cs index 15ed2fe..2eac5bb 100644 --- a/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisAnalyzer.cs +++ b/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisAnalyzer.cs @@ -21,11 +21,10 @@ public class AvoidStringlyTypedApisAnalyzer : DiagnosticAnalyzer Title, MessageFormat, Category, - DiagnosticSeverity.Warning, + DiagnosticSeverity.Info, isEnabledByDefault: true, description: Description, - helpLinkUri: - $"https://github.com/rjmurillo/EffectiveCSharp.Analyzers{ThisAssembly.GitCommitId}/docs/{DiagnosticId}.md"); + helpLinkUri: $"https://github.com/rjmurillo/EffectiveCSharp.Analyzers{ThisAssembly.GitCommitId}/docs/{DiagnosticId}.md"); /// public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); diff --git a/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisCodeFixProvider.cs b/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisCodeFixProvider.cs index d097007..f59705e 100644 --- a/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisCodeFixProvider.cs +++ b/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisCodeFixProvider.cs @@ -20,7 +20,7 @@ public class AvoidStringlyTypedApisCodeFixProvider : CodeFixProvider public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) { SyntaxNode? root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); - Diagnostic diagnostic = context.Diagnostics.First(); + Diagnostic diagnostic = context.Diagnostics[0]; TextSpan diagnosticSpan = diagnostic.Location.SourceSpan; SyntaxNode? node = root?.FindNode(diagnosticSpan); @@ -33,7 +33,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) // Check if the node is an ArgumentSyntax containing a LiteralExpressionSyntax ArgumentSyntax { Expression: LiteralExpressionSyntax argLiteralNode } => argLiteralNode, - _ => null + _ => null, }; if (literalExpression != null) @@ -49,7 +49,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) private static async Task UseNameofOperatorAsync(Document document, LiteralExpressionSyntax? literalExpression, CancellationToken cancellationToken) { - string literalValue = literalExpression?.Token.ValueText; + string? literalValue = literalExpression?.Token.ValueText; // Walk up the syntax tree to find the containing class or method TypeDeclarationSyntax? containingClass = literalExpression?.FirstAncestorOrSelf(); @@ -65,7 +65,7 @@ private static async Task UseNameofOperatorAsync(Document document, Li { IEnumerable memberNames = containingTypeSymbol.GetMembers().Select(member => member.Name); - if (memberNames.Contains(literalValue)) + if (memberNames.Contains(literalValue, StringComparer.Ordinal)) { nameofExpressionText = $"nameof({literalValue})"; } @@ -80,7 +80,7 @@ private static async Task UseNameofOperatorAsync(Document document, Li { IEnumerable parameterNames = methodSymbol.Parameters.Select(parameter => parameter.Name); - if (parameterNames.Contains(literalValue)) + if (parameterNames.Contains(literalValue, StringComparer.Ordinal)) { nameofExpressionText = $"nameof({literalValue})"; } diff --git a/src/EffectiveCSharp.Analyzers/Common/DiagnosticExtensions.cs b/src/EffectiveCSharp.Analyzers/Common/DiagnosticExtensions.cs index 7fb5c93..479d127 100644 --- a/src/EffectiveCSharp.Analyzers/Common/DiagnosticExtensions.cs +++ b/src/EffectiveCSharp.Analyzers/Common/DiagnosticExtensions.cs @@ -2,12 +2,14 @@ internal static class DiagnosticExtensions { + [DebuggerStepThrough] internal static Diagnostic CreateDiagnostic( this SyntaxNode node, DiagnosticDescriptor rule, params object?[]? messageArgs) => node.CreateDiagnostic(rule, properties: null, messageArgs); + [DebuggerStepThrough] internal static Diagnostic CreateDiagnostic( this SyntaxNode node, DiagnosticDescriptor rule, @@ -15,6 +17,7 @@ internal static Diagnostic CreateDiagnostic( params object?[]? messageArgs) => node.CreateDiagnostic(rule, additionalLocations: ImmutableArray.Empty, properties, messageArgs); + [DebuggerStepThrough] internal static Diagnostic CreateDiagnostic( this SyntaxNode node, DiagnosticDescriptor rule, @@ -29,6 +32,7 @@ internal static Diagnostic CreateDiagnostic( properties: properties, messageArgs: messageArgs); + [DebuggerStepThrough] internal static Diagnostic CreateDiagnostic( this Location location, DiagnosticDescriptor rule, @@ -46,6 +50,7 @@ internal static Diagnostic CreateDiagnostic( params object?[]? messageArgs) => location.CreateDiagnostic(rule, ImmutableArray.Empty, properties, messageArgs); + [DebuggerStepThrough] internal static Diagnostic CreateDiagnostic( this Location location, DiagnosticDescriptor rule, diff --git a/src/EffectiveCSharp.Analyzers/Common/IOperationExtensions.cs b/src/EffectiveCSharp.Analyzers/Common/IOperationExtensions.cs new file mode 100644 index 0000000..4c24185 --- /dev/null +++ b/src/EffectiveCSharp.Analyzers/Common/IOperationExtensions.cs @@ -0,0 +1,28 @@ +namespace EffectiveCSharp.Analyzers.Common; + +internal static class IOperationExtensions +{ + /// + /// Determines if a given operation involves boxing through type conversion. + /// + /// The operation to check. + /// True if the operation is a boxing conversion, otherwise false. + internal static bool IsBoxingOperation(this IOperation? operation) + => operation is IConversionOperation { Operand.Type.IsValueType: true, Type.IsReferenceType: true }; + + /// + /// Determines if a given operation involves unboxing through type conversion. + /// + /// The operation to check. + /// True if the operation is an unboxing conversion, otherwise false. + internal static bool IsUnboxingOperation(this IOperation? operation) + => operation is IConversionOperation { Operand.Type.IsReferenceType: true, Type.IsValueType: true }; + + /// + /// Determines if a given operation involves boxing or unboxing through type conversion. + /// + /// The operation to check. + /// True if the operation is a boxing or unboxing conversion, otherwise false. + internal static bool IsBoxingOrUnboxingOperation(this IOperation? operation) + => operation.IsBoxingOperation() || operation.IsUnboxingOperation(); +} diff --git a/src/EffectiveCSharp.Analyzers/DiagnosticIds.cs b/src/EffectiveCSharp.Analyzers/DiagnosticIds.cs index a1b62d3..7fc9554 100644 --- a/src/EffectiveCSharp.Analyzers/DiagnosticIds.cs +++ b/src/EffectiveCSharp.Analyzers/DiagnosticIds.cs @@ -4,5 +4,7 @@ internal static class DiagnosticIds { internal const string PreferReadonlyOverConst = "ECS0002"; internal const string AvoidStringlyTypedApis = "ECS0006"; + internal const string MinimizeBoxingUnboxing = "ECS0009"; + internal const string BeAwareOfValueTypeCopyInReferenceTypes = "ECS0009"; internal const string UseSpanInstead = "ECS1000"; } diff --git a/src/EffectiveCSharp.Analyzers/GlobalUsings.cs b/src/EffectiveCSharp.Analyzers/GlobalUsings.cs index bafd3f4..10e9b57 100644 --- a/src/EffectiveCSharp.Analyzers/GlobalUsings.cs +++ b/src/EffectiveCSharp.Analyzers/GlobalUsings.cs @@ -8,4 +8,5 @@ global using Microsoft.CodeAnalysis.CSharp; global using Microsoft.CodeAnalysis.CSharp.Syntax; global using Microsoft.CodeAnalysis.Diagnostics; +global using Microsoft.CodeAnalysis.Operations; global using Microsoft.CodeAnalysis.Text; diff --git a/src/EffectiveCSharp.Analyzers/MinimizeBoxingUnboxingAnalyzer.cs b/src/EffectiveCSharp.Analyzers/MinimizeBoxingUnboxingAnalyzer.cs new file mode 100644 index 0000000..506f8ea --- /dev/null +++ b/src/EffectiveCSharp.Analyzers/MinimizeBoxingUnboxingAnalyzer.cs @@ -0,0 +1,122 @@ +namespace EffectiveCSharp.Analyzers; + +/// +/// A for Effective C# Item #9 - Minimize boxing and unboxing. +/// +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class MinimizeBoxingUnboxingAnalyzer : DiagnosticAnalyzer +{ + private const string Id = DiagnosticIds.MinimizeBoxingUnboxing; + + private static readonly DiagnosticDescriptor Rule = new( + id: Id, + title: "Minimize boxing and unboxing", + messageFormat: "Consider using an alternative implementation to avoid boxing and unboxing", + category: "Performance", + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + helpLinkUri: $"https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/{ThisAssembly.GitCommitId}/docs/{Id}.md"); + + /// + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + /// + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterCompilationStartAction(compilationStartAnalysisContext => + { + INamedTypeSymbol? dictionarySymbol = compilationStartAnalysisContext.Compilation.GetTypeByMetadataName("System.Collections.Generic.Dictionary`2"); + INamedTypeSymbol? listSymbol = compilationStartAnalysisContext.Compilation.GetTypeByMetadataName("System.Collections.Generic.List`1"); + + compilationStartAnalysisContext.RegisterOperationAction(AnalyzeOperation, OperationKind.Conversion); + compilationStartAnalysisContext.RegisterSyntaxNodeAction( + syntaxNodeContext => AnalyzeNode(syntaxNodeContext, dictionarySymbol, listSymbol), + SyntaxKind.ElementAccessExpression); + }); + } + + private static void AnalyzeNode(SyntaxNodeAnalysisContext context, INamedTypeSymbol? dictionarySymbol, INamedTypeSymbol? listSymbol) + { + if (context.Node is not ElementAccessExpressionSyntax elementAccess) + { + return; + } + + // Get the type of the accessed object + TypeInfo typeInfo = context.SemanticModel.GetTypeInfo(elementAccess.Expression, context.CancellationToken); + + if (typeInfo.Type is not INamedTypeSymbol { IsGenericType: true } namedType) + { + return; + } + + INamedTypeSymbol baseType = namedType.ConstructedFrom; + if (SymbolEqualityComparer.Default.Equals(baseType, dictionarySymbol)) + { + ITypeSymbol keyType = namedType.TypeArguments[0]; // The TKey in Dictionary + if (ReportDiagnosticOnValueType(keyType)) + { + return; + } + + ITypeSymbol valueType = namedType.TypeArguments[1]; // The TValue in Dictionary + if (ReportDiagnosticOnValueType(valueType)) + { + return; + } + } + else if (SymbolEqualityComparer.Default.Equals(baseType, listSymbol)) + { + ITypeSymbol elementType = namedType.TypeArguments[0]; // The T in List + if (ReportDiagnosticOnValueType(elementType)) + { + return; + } + } + else + { + Debug.Fail($"Unrecognized constructed from named type '{baseType}'."); + } + + return; + + bool ReportDiagnosticOnValueType(ITypeSymbol? typeSymbol) + { + // Check if the struct is read/write; if so, there can be bad things that happen to warn + if (typeSymbol is not { IsValueType: true, IsReadOnly: false }) + { + return false; + } + + // Create and report a diagnostic if the element is accessed directly + Diagnostic diagnostic = elementAccess.GetLocation().CreateDiagnostic(Rule, typeSymbol.Name); + context.ReportDiagnostic(diagnostic); + + return true; + } + } + + private static void AnalyzeOperation(OperationAnalysisContext context) + { + if (context.Operation is IConversionOperation conversionOperation) + { + AnalyzeConversionOperation(conversionOperation, context); + } + else + { + throw new NotSupportedException($"Unsupported operation kind: {context.Operation.Kind}"); + } + } + + private static void AnalyzeConversionOperation(IConversionOperation conversionOperation, OperationAnalysisContext context) + { + if (conversionOperation.IsBoxingOrUnboxingOperation()) + { + Diagnostic diagnostic = conversionOperation.Syntax.GetLocation().CreateDiagnostic(Rule); + context.ReportDiagnostic(diagnostic); + } + } +} diff --git a/src/EffectiveCSharp.Analyzers/SpanAnalyzer.cs b/src/EffectiveCSharp.Analyzers/SpanAnalyzer.cs index ab93d7f..f203ee5 100644 --- a/src/EffectiveCSharp.Analyzers/SpanAnalyzer.cs +++ b/src/EffectiveCSharp.Analyzers/SpanAnalyzer.cs @@ -14,8 +14,8 @@ public class SpanAnalyzer : DiagnosticAnalyzer messageFormat: "Consider using Span instead of array for better performance", category: "Performance", defaultSeverity: DiagnosticSeverity.Info, - helpLinkUri: $"https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/{ThisAssembly.GitCommitId}/docs/{Id}.md", - isEnabledByDefault: true); + isEnabledByDefault: true, + helpLinkUri: $"https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/{ThisAssembly.GitCommitId}/docs/{Id}.md"); /// public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); diff --git a/tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs0009Benchmarks.cs b/tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs0009Benchmarks.cs new file mode 100644 index 0000000..71e7d7a --- /dev/null +++ b/tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs0009Benchmarks.cs @@ -0,0 +1,106 @@ +namespace EffectiveCSharp.Analyzers.Benchmarks; + +[InProcess] +[MemoryDiagnoser] +public class Ecs0009Benchmarks +{ + private static CompilationWithAnalyzers? BaselineCompilation { get; set; } + + private static CompilationWithAnalyzers? TestCompilation { get; set; } + + [IterationSetup] + [SuppressMessage("Usage", "VSTHRD002:Avoid problematic synchronous waits", Justification = "Async setup not supported in BenchmarkDotNet.See https://github.com/dotnet/BenchmarkDotNet/issues/2442.")] + public static void SetupCompilation() + { + List<(string Name, string Content)> sources = []; + for (int index = 0; index < Constants.NumberOfCodeFiles; index++) + { + string name = $"TypeName{index}"; + sources.Add((name, $$""" + +using System; + +internal class {{name}} +{ + public void Method() + { + int i = 1; + object o = i; // boxing + + i = (int)o; // unboxing + + int firstNumber = 4; + int secondNumber = 2; + int thirdNumber = 6; + + Method( + "A few numbers: {0}, {1}, {2}", + firstNumber, + secondNumber, + thirdNumber); + + // Using the Person in a collection + var attendees = new List(); + var p = new Person { Name = "Old Name" }; + attendees.Add(p); + + // Try to change the name + var p2 = attendees[0]; + p2.Name = "New Name"; + + // Writes "Old Name": + Console.WriteLine(attendees[0].ToString()); + } + + void Method(params object?[]? arg) { } +} + +public struct Person +{ + public string Name { get; set; } + public override string ToString() => Name; +} + +""")); + } + + (BaselineCompilation, TestCompilation) = + BenchmarkCSharpCompilationFactory + .CreateAsync(sources.ToArray()) + .GetAwaiter() + .GetResult(); + } + + [Benchmark] + public async Task Ecs0009WithDiagnostics() + { + ImmutableArray diagnostics = + (await TestCompilation! + .GetAnalysisResultAsync(CancellationToken.None) + .ConfigureAwait(false)) + .AssertValidAnalysisResult() + .GetAllDiagnostics(); + + // We have 4 instances in our test sample + if (diagnostics.Length != Constants.NumberOfCodeFiles * 5) + { + throw new InvalidOperationException($"Expected '{Constants.NumberOfCodeFiles:N0}' analyzer diagnostics but found '{diagnostics.Length:N0}'"); + } + } + + [Benchmark(Baseline = true)] + public async Task Ecs0009Baseline() + { + ImmutableArray diagnostics = + (await BaselineCompilation! + .GetAnalysisResultAsync(CancellationToken.None) + .ConfigureAwait(false)) + .AssertValidAnalysisResult() + .GetAllDiagnostics(); + + if (diagnostics.Length != 0) + { + throw new InvalidOperationException($"Expected no analyzer diagnostics but found '{diagnostics.Length}'"); + } + } +} diff --git a/tests/EffectiveCSharp.Analyzers.Tests/AvoidBoxingUnboxingTests.cs b/tests/EffectiveCSharp.Analyzers.Tests/AvoidBoxingUnboxingTests.cs new file mode 100644 index 0000000..cecf835 --- /dev/null +++ b/tests/EffectiveCSharp.Analyzers.Tests/AvoidBoxingUnboxingTests.cs @@ -0,0 +1,385 @@ +using Verifier = EffectiveCSharp.Analyzers.Tests.Helpers.AnalyzerVerifier; + +namespace EffectiveCSharp.Analyzers.Tests; + +#pragma warning disable SA1204 // Static members are grouped with their Theory +#pragma warning disable SA1001 // The harness has literal code as a string, which can be weirdly formatted +#pragma warning disable SA1113 +#pragma warning disable S125 // There's code in comments as examples +#pragma warning disable MA0051 // Some test methods are "too long" +#pragma warning disable MA0007 // There are multiple types of tests defined in theory data +#pragma warning disable IDE0028 // We cannot simply object creation on TheoryData because we need to convert from object[] to string, the way it is now is cleaner + +public class AvoidBoxingUnboxingTests +{ + public static TheoryData TestData() + { + TheoryData data = new() + { + // This should fire + """ + int i = 5; + object o = {|ECS0009:i|}; // boxing + """, + """ + int i = 5; + object o = {|ECS0009:(object)i|}; // boxing + """, + + // This should fire for method call with value type defined in System.Object + """ + int i = 5; + Console.WriteLine(i); // boxing + """, + + // This should not fire because it's suppressed + """ + #pragma warning disable ECS0009 // Minimize boxing and unboxing + int i = 5; + object o = i; // boxing + #pragma warning restore ECS0009 // Minimize boxing and unboxing + """, + """ + var dt = new DateTimeOffset(2021, 1, 1, 0, 0, 0, TimeSpan.Zero); + """, + }; + + return data.WithReferenceAssemblyGroups(); + } + + [Theory] + [MemberData(nameof(TestData))] + public async Task Analyzer(string referenceAssemblyGroup, string source) + { + await Verifier.VerifyAnalyzerAsync( + $$""" + internal class MyClass + { + void Method() + { + {{source}} + } + } + """, + referenceAssemblyGroup); + } + + public static TheoryData TestData2() + { + TheoryData data = new() + { + // This should fire + // Equal to + // Method("a few number...", new object[3]{ (object)firstNumber... }); + """ + void Foo() + { + int firstNumber = 4; + int secondNumber = 2; + int thirdNumber = 6; + + Method( + "A few numbers: {0}, {1}, {2}", + {|ECS0009:firstNumber|}, + {|ECS0009:secondNumber|}, + {|ECS0009:thirdNumber|} + ); + } + """, + + // This should not fire because it's suppressed + """ + void Foo() + { + int firstNumber = 4; + int secondNumber = 2; + int thirdNumber = 6; + + Method( + "A few numbers: {0}, {1}, {2}", + #pragma warning disable ECS0009 // Minimize boxing and unboxing + firstNumber, + secondNumber, + thirdNumber + #pragma warning restore ECS0009 // Minimize boxing and unboxing + ); + } + """, + + // This should not fire because the string interpolation does not box + """ + void Foo() + { + int firstNumber = 4; + int secondNumber = 2; + int thirdNumber = 6; + + Method( + $"A few numbers: {firstNumber}, {secondNumber}, {thirdNumber}" + ); + } + """, + """ + private const int MyNumber = 42; + + void Foo() + { + for(var i = 0; i <= MyNumber; i++) + { + Method("Bar"); + } + } + """, + + // Regression test: we are too aggressive when assigning ctor params to read-only properties + """ + public int Arg { get; } + + public MyClass(int arg) + { + Arg = arg; + } + """, + }; + + return data.WithReferenceAssemblyGroups(); + } + + [Theory] + [MemberData(nameof(TestData2))] + public async Task AnalyzerParams(string referenceAssemblyGroup, string source) + { + await Verifier.VerifyAnalyzerAsync( + $$""" + internal class MyClass + { + {{source}} + + void Method(string arg) { } + + void Method(params object?[]? arg) { } + } + """, + referenceAssemblyGroup); + } + + [Fact] + public async Task AnalyzerRecordType() + { + await Verifier.VerifyAnalyzerAsync( + """ +using System; + +namespace MyNamespace; + +public record MyRecord(bool Flag); + +public class MyClass +{ + public void Method(MyRecord rec) + { + if (rec.Flag) + { + Console.WriteLine("Flag is true"); + } + } +} +""", + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task Return_Boxing_Detected() + { + await Verifier.VerifyAnalyzerAsync( + """ + public class TestClass + { + public object ReturnBoxing() + { + int i = 42; + return {|ECS0009:i|}; + } + } + """, + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task Argument_Boxing_Detected() + { + await Verifier.VerifyAnalyzerAsync( + """ + public class TestClass + { + public void TakeObject(object obj) {} + + public void Method() + { + int i = 42; + TakeObject({|ECS0009:i|}); + } + } + """, + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task SimpleAssignment_Boxing_Detected() + { + await Verifier.VerifyAnalyzerAsync( + """ + public class TestClass + { + public void AssignExample() + { + int i = 42; + object boxed = {|ECS0009:i|}; + } + } + """, + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task ArrayElementReference_Unboxing_Detected() + { + await Verifier.VerifyAnalyzerAsync( + """ + using System.Collections.Generic; + + public class TestClass + { + public void ArrayAccessExample() + { + List list = new List(); + int i = 42; + list.Add({|ECS0009:i|}); // Boxing operation + int value = {|ECS0009:(int)list[0]|}; // Unboxing operation + } + } + """, + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task Assignment_With_Unboxing_Operation_Detected() + { + await Verifier.VerifyAnalyzerAsync( + """ + public class TestClass + { + public void TestMethod() + { + object obj = {|ECS0009:42|}; // Boxing operation expected here + int value = {|ECS0009:(int)obj|}; // Unboxing operation expected here + } + } + """, + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task Assignment_With_Boxing_When_ValueType_As_Interface() + { + await Verifier.VerifyAnalyzerAsync( + """ + public interface ITestInterface + { + void TestMethod(); + } + + public struct TestStruct : ITestInterface + { + public int Value; + public void TestMethod() { } + } + + public class TestClass + { + public void TestMethod() + { + TestStruct myStruct = new TestStruct { Value = 42 }; + ITestInterface myInterface = {|ECS0009:myStruct|}; // Expected to trigger boxing warning + } + } + """, + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task Implicit_boxing_in_List() + { + // We need to ensure the detection of boxing in cases where a value type + // is being assigned or accessed in a way that results in copy semantics + // + // In this case, we are copying the value type when we bring it in and out + // the reference type List. + await Verifier.VerifyAnalyzerAsync( + """ + internal class Program + { + static void Main() + { + + // Using the Person in a collection + var attendees = new List(); + var p = new Person { Name = "Old Name" }; + attendees.Add(p); + + // Try to change the name + var p2 = {|ECS0009:attendees[0]|}; + p2.Name = "New Name"; + + // Writes "Old Name" because we pulled a copy of the struct + Console.WriteLine({|ECS0009:attendees[0]|}.ToString()); + } + } + + public struct Person + { + public string Name { get; set; } + public override string ToString() => Name; + } + """ + , ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task Implicit_boxing_in_Dictionary_As_Value() + { + // We need to ensure the detection of boxing in cases where a value type + // is being assigned or accessed in a way that results in copy semantics + // + // In this case, we are copying the value type when we bring it in and out + // the reference type List. + await Verifier.VerifyAnalyzerAsync( + """ + internal class Program + { + static void Main() + { + + // Using the Person in a collection + var attendees = new Dictionary(); + var p = new Person { Name = "Old Name" }; + attendees.Add(1, p); + + // Try to change the name + var p2 = {|ECS0009:attendees[1]|}; + p2.Name = "New Name"; + + // Writes "Old Name" because we pulled a copy of the struct + Console.WriteLine({|ECS0009:attendees[1]|}.ToString()); + } + } + + public struct Person + { + public string Name { get; set; } + public override string ToString() => Name; + } + """ + , ReferenceAssemblyCatalog.Net80); + } +} diff --git a/tests/EffectiveCSharp.Analyzers.Tests/AvoidStringlyTypedApisTests.cs b/tests/EffectiveCSharp.Analyzers.Tests/AvoidStringlyTypedApisTests.cs index 59cfc32..ea45775 100644 --- a/tests/EffectiveCSharp.Analyzers.Tests/AvoidStringlyTypedApisTests.cs +++ b/tests/EffectiveCSharp.Analyzers.Tests/AvoidStringlyTypedApisTests.cs @@ -3,29 +3,30 @@ namespace EffectiveCSharp.Analyzers.Tests; +#pragma warning disable IDE0028 // We cannot simply object creation on TheoryData because we need to convert from object[] to string, the way it is now is cleaner + public class AvoidStringlyTypedApisTests { - public static IEnumerable TestData() + public static TheoryData TestData() { - return new object[][] + TheoryData data = new() { // This should not fire because it's using nameof - ["""nameof(thisCantBeNull)"""], + "nameof(thisCantBeNull)", // This should fire because it's referring to a member name using a string literal - [""" + """ {|ECS0006:"thisCantBeNull"|} - """], + """, // This should not fire because it's suppressed - [ - """ - #pragma warning disable ECS0006 - "thisCantBeNull" - #pragma warning restore ECS0006 - """ - ], - }.WithReferenceAssemblyGroups(); + """ + #pragma warning disable ECS0006 + "thisCantBeNull" + #pragma warning restore ECS0006 + """, + }; + return data.WithReferenceAssemblyGroups(); } [Theory] @@ -54,35 +55,35 @@ public static void ExceptionMessage(object thisCantBeNull) [Fact] public async Task CodeFix() { - string testCode = """ - public class MyClass - { - public static void ExceptionMessage(object thisCantBeNull) - { - if (thisCantBeNull == null) + const string testCode = """ + public class MyClass { - throw new ArgumentNullException( - {|ECS0006:"thisCantBeNull"|}, - "We told you this cant be null"); + public static void ExceptionMessage(object thisCantBeNull) + { + if (thisCantBeNull == null) + { + throw new ArgumentNullException( + {|ECS0006:"thisCantBeNull"|}, + "We told you this cant be null"); + } + } } - } - } - """; + """; - string fixedCode = """ - public class MyClass - { - public static void ExceptionMessage(object thisCantBeNull) - { - if (thisCantBeNull == null) + const string fixedCode = """ + public class MyClass { - throw new ArgumentNullException( - nameof(thisCantBeNull), - "We told you this cant be null"); + public static void ExceptionMessage(object thisCantBeNull) + { + if (thisCantBeNull == null) + { + throw new ArgumentNullException( + nameof(thisCantBeNull), + "We told you this cant be null"); + } + } } - } - } - """; + """; await CodeFixVerifier.VerifyCodeFixAsync(testCode, fixedCode, ReferenceAssemblyCatalog.Net80); } diff --git a/tests/EffectiveCSharp.Analyzers.Tests/BoxingTests.cs b/tests/EffectiveCSharp.Analyzers.Tests/BoxingTests.cs new file mode 100644 index 0000000..457eb21 --- /dev/null +++ b/tests/EffectiveCSharp.Analyzers.Tests/BoxingTests.cs @@ -0,0 +1,122 @@ +using Verifier = EffectiveCSharp.Analyzers.Tests.Helpers.AnalyzerVerifier; + +namespace EffectiveCSharp.Analyzers.Tests; + +#pragma warning disable MA0048 // There are multiple types of tests here, keeping them in the same file +#pragma warning disable SA1649 +#pragma warning disable SA1402 +#pragma warning disable SA1001 // The harness has literal code as a string, which can be weirdly formatted +#pragma warning disable SA1113 +#pragma warning disable SA1115 + +public class HeapAllocationTests +{ + [Fact] + public async Task Value_Type_Member_of_a_Reference_Type() + { + await Verifier.VerifyAnalyzerAsync( + @" +public class Container +{ + public int ValueField; +} +" + , + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task Boxing() + { + await Verifier.VerifyAnalyzerAsync( + @" +public class Container +{ + public void BoxingExample() + { + int i = 42; + object boxed = {|ECS0009:i|}; + } +} +" + , + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task Struct_with_Value_Type_on_Heap() + { + await Verifier.VerifyAnalyzerAsync( + @" +public class Container +{ + public MyStruct StructField; + + public struct MyStruct + { + public int ValueField; + } +} +" + , + ReferenceAssemblyCatalog.Net80); + } +} + +public class StackAllocationTests +{ + [Fact] + public async Task Value_Type_as_Local_Variable() + { + await Verifier.VerifyAnalyzerAsync( + @" +public class Container +{ + public void LocalVariableExample() + { + int i = 42; + } +} +" + , + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task Value_Type_as_Parameter() + { + await Verifier.VerifyAnalyzerAsync( + @" +public class Container +{ + public void LocalVariableExample(int parameterValue) + { + } +} +" + , + ReferenceAssemblyCatalog.Net80); + } + + [Fact] + public async Task Ref_Struct() + { + await Verifier.VerifyAnalyzerAsync( + @" +public class Container +{ + public void RefStructExample() + { + MyRefStruct localRefStruct; + } +} + +public ref struct MyRefStruct +{ + public int ValueField; +} +" + , + ReferenceAssemblyCatalog.Net80); + } +} diff --git a/tests/EffectiveCSharp.Analyzers.Tests/CompositeAnalyzer.cs b/tests/EffectiveCSharp.Analyzers.Tests/CompositeAnalyzer.cs new file mode 100644 index 0000000..b11c63d --- /dev/null +++ b/tests/EffectiveCSharp.Analyzers.Tests/CompositeAnalyzer.cs @@ -0,0 +1,51 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; + +namespace EffectiveCSharp.Analyzers.Tests; + +/// +/// A "meta" analyzer that aggregates all the individual analyzers into a single one. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class CompositeAnalyzer : DiagnosticAnalyzer +{ + private readonly ImmutableArray _analyzers; + private readonly ImmutableArray _supportedDiagnostics; + + /// Initializes a new instance of the class. + public CompositeAnalyzer() + { + _analyzers = [.. DiagnosticAnalyzers()]; + _supportedDiagnostics = [.. _analyzers.SelectMany(diagnosticAnalyzer => diagnosticAnalyzer.SupportedDiagnostics)]; + } + + /// + public override ImmutableArray SupportedDiagnostics => _supportedDiagnostics; + + /// + [System.Diagnostics.CodeAnalysis.SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1026:Enable concurrent execution", Justification = "Delegated off to children")] + [System.Diagnostics.CodeAnalysis.SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1025:Configure generated code analysis", Justification = "Delegated off to children")] + public override void Initialize(AnalysisContext context) + { + foreach (DiagnosticAnalyzer analyzer in _analyzers) + { + analyzer.Initialize(context); + } + } + + private static IEnumerable DiagnosticAnalyzers() + { + Type diagnosticAnalyzerType = typeof(DiagnosticAnalyzer); + IEnumerable diagnosticAnalyzerTypes = typeof(PreferReadonlyOverConstAnalyzer) + .Assembly + .GetTypes() + .Where(type => diagnosticAnalyzerType.IsAssignableFrom(type) && !type.IsAbstract && type.IsClass) + ; + + return diagnosticAnalyzerTypes + .Select(type => (DiagnosticAnalyzer?)Activator.CreateInstance(type)) + .Where(analyzer => analyzer != null) + .Cast() + ; + } +} diff --git a/tests/EffectiveCSharp.Analyzers.Tests/Helpers/ReferenceAssemblyCatalog.cs b/tests/EffectiveCSharp.Analyzers.Tests/Helpers/ReferenceAssemblyCatalog.cs index 2fbc360..8f4ffeb 100644 --- a/tests/EffectiveCSharp.Analyzers.Tests/Helpers/ReferenceAssemblyCatalog.cs +++ b/tests/EffectiveCSharp.Analyzers.Tests/Helpers/ReferenceAssemblyCatalog.cs @@ -12,10 +12,51 @@ /// internal static class ReferenceAssemblyCatalog { - public static string Net80 => nameof(Net80); + // We want to test the supported versions of .NET + // References + // https://learn.microsoft.com/en-us/lifecycle/products/microsoft-net-framework + // https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-framework + // https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core + // + // As of July 2024, the supported versions are: + // .NET Framework: + // 4.8.1 and 4.8 + // 4.7.2, 4.7.1, and 4.7 + // 4.6.2 (ends Jan 12, 2027 because of Windows) + // 3.5 SP1 (ends Jan 9, 2029 because of Windows) + // NOTE: 4.5.2, 4.6, 4.6.1 retired on April 26, 2022 + // .NET (formally .NET Core) + // 8 LTS (ends Nov 10, 2026) + // 6 LTS (ends Nov 12, 2024) + // NOTE: 7, 5, Core 3.1, 3.0, 2.2, 2.1, 2.0, 1.1, and 1.0 are no longer supported + public static string Net48 => nameof(ReferenceAssemblies.NetFramework.Net48); + + public static string Net472 => nameof(ReferenceAssemblies.NetFramework.Net472); + + public static string Net471 => nameof(ReferenceAssemblies.NetFramework.Net471); + + public static string Net47 => nameof(ReferenceAssemblies.NetFramework.Net47); + + public static string Net462 => nameof(ReferenceAssemblies.NetFramework.Net462); + + public static string Net35 => nameof(ReferenceAssemblies.NetFramework.Net35); + + public static string Net60 => nameof(ReferenceAssemblies.Net.Net60); + + public static string Net80 => nameof(ReferenceAssemblies.Net.Net80); + + public static string Net90 => nameof(ReferenceAssemblies.Net.Net90); public static IReadOnlyDictionary Catalog { get; } = new Dictionary(StringComparer.Ordinal) { + { Net48, ReferenceAssemblies.NetFramework.Net48.Default }, + { Net472, ReferenceAssemblies.NetFramework.Net472.Default }, + { Net471, ReferenceAssemblies.NetFramework.Net471.Default }, + { Net47, ReferenceAssemblies.NetFramework.Net47.Default }, + { Net462, ReferenceAssemblies.NetFramework.Net462.Default }, + { Net35, ReferenceAssemblies.NetFramework.Net35.Default }, + { Net60, ReferenceAssemblies.Net.Net60 }, { Net80, ReferenceAssemblies.Net.Net80 }, + { Net90, ReferenceAssemblies.Net.Net90 }, }; } diff --git a/tests/EffectiveCSharp.Analyzers.Tests/Helpers/Test.cs b/tests/EffectiveCSharp.Analyzers.Tests/Helpers/Test.cs index 0fd7223..0f512e6 100644 --- a/tests/EffectiveCSharp.Analyzers.Tests/Helpers/Test.cs +++ b/tests/EffectiveCSharp.Analyzers.Tests/Helpers/Test.cs @@ -19,10 +19,11 @@ public Test() """ global using System; global using System.Collections.Generic; - global using System.Threading.Tasks; """; TestState.Sources.Add(globalUsings); FixedState.Sources.Add(globalUsings); + + MarkupOptions = MarkupOptions.UseFirstDescriptor; } } diff --git a/tests/EffectiveCSharp.Analyzers.Tests/Helpers/TestDataExtensions.cs b/tests/EffectiveCSharp.Analyzers.Tests/Helpers/TestDataExtensions.cs index 22f87d5..a7c79f0 100644 --- a/tests/EffectiveCSharp.Analyzers.Tests/Helpers/TestDataExtensions.cs +++ b/tests/EffectiveCSharp.Analyzers.Tests/Helpers/TestDataExtensions.cs @@ -2,11 +2,21 @@ internal static class TestDataExtensions { - public static IEnumerable WithReferenceAssemblyGroups(this IEnumerable data) + public static TheoryData WithReferenceAssemblyGroups(this TheoryData data) { - foreach (object[] item in data) + TheoryData retVal = []; + + foreach (object[]? theoryDataItem in data) { - yield return item.Prepend(ReferenceAssemblyCatalog.Net80).ToArray(); + foreach (object entry in theoryDataItem) + { + foreach (string referenceAssembly in ReferenceAssemblyCatalog.Catalog.Keys) + { + retVal.Add(referenceAssembly, (string)entry); + } + } } + + return retVal; } } diff --git a/tests/EffectiveCSharp.Analyzers.Tests/PreferReadonlyOverConstTests.cs b/tests/EffectiveCSharp.Analyzers.Tests/PreferReadonlyOverConstTests.cs index e3b69ba..62724fa 100644 --- a/tests/EffectiveCSharp.Analyzers.Tests/PreferReadonlyOverConstTests.cs +++ b/tests/EffectiveCSharp.Analyzers.Tests/PreferReadonlyOverConstTests.cs @@ -3,27 +3,29 @@ namespace EffectiveCSharp.Analyzers.Tests; +#pragma warning disable IDE0028 // We cannot simply object creation on TheoryData because we need to convert from object[] to string, the way it is now is cleaner + public class PreferReadonlyOverConstTests { - public static IEnumerable TestData() + public static TheoryData TestData() { - return new object[][] + TheoryData data = new() { // This should not fire because it's a readonly field - ["""public static readonly int StartValue = 5;"""], + "public static readonly int StartValue = 5;", // This should fire because a const - ["""{|ECS0002:public const int EndValue = 10;|}"""], + "{|ECS0002:public const int EndValue = 10;|}", // This should not fire because it's suppressed - [ - """ - #pragma warning disable ECS0002 - public const int EndValue = 10; - #pragma warning restore ECS0002 - """ - ], - }.WithReferenceAssemblyGroups(); + """ + #pragma warning disable ECS0002 + public const int EndValue = 10; + #pragma warning restore ECS0002 + """, + }; + + return data.WithReferenceAssemblyGroups(); } [Theory] @@ -43,19 +45,19 @@ public class UsefulValues [Fact] public async Task CodeFix() { - string testCode = """ - class Program - { - {|ECS0002:private const int StartValue = 5;|} - } - """; - - string fixedCode = """ - class Program - { - private static readonly int StartValue = 5; - } - """; + const string testCode = """ + class Program + { + {|ECS0002:private const int StartValue = 5;|} + } + """; + + const string fixedCode = """ + class Program + { + private static readonly int StartValue = 5; + } + """; await CodeFixVerifier.VerifyCodeFixAsync(testCode, fixedCode, ReferenceAssemblyCatalog.Net80); } diff --git a/tests/EffectiveCSharp.Analyzers.Tests/SpanAnalyzerTests.cs b/tests/EffectiveCSharp.Analyzers.Tests/SpanAnalyzerTests.cs index 1bafc8e..aa9379c 100644 --- a/tests/EffectiveCSharp.Analyzers.Tests/SpanAnalyzerTests.cs +++ b/tests/EffectiveCSharp.Analyzers.Tests/SpanAnalyzerTests.cs @@ -3,28 +3,40 @@ namespace EffectiveCSharp.Analyzers.Tests; +#pragma warning disable IDE0028 // We cannot simply object creation on TheoryData because we need to convert from object[] to string, the way it is now is cleaner + public class SpanAnalyzerTests { - public static IEnumerable TestData() + public static TheoryData TestData() { - return new object[][] + TheoryData data = new() { // This should fire - ["""var arr = {|ECS1000:new int[10]|};"""], + "var arr = {|ECS1000:new int[10]|};", // This should not fire because it's wrapped by a Span - ["""var arr = new Span(new int[10]);"""], + """ + #if NET6_0_OR_GREATER + var arr = new Span(new int[10]); + #endif + """, // This should not fire because it's wrapped by a ReadOnlySpan - ["""var arr = new ReadOnlySpan(new int[10]);"""], + """ + #if NET6_0_OR_GREATER + var arr = new ReadOnlySpan(new int[10]); + #endif + """, // This should not fire because it's suppressed - [""" - #pragma warning disable ECS1000 // Use Span for performance - var arr = new int[10]; - #pragma warning restore ECS1000 // Use Span for performance - """], - }.WithReferenceAssemblyGroups(); + """ + #pragma warning disable ECS1000 // Use Span for performance + var arr = new int[10]; + #pragma warning restore ECS1000 // Use Span for performance + """, + }; + + return data.WithReferenceAssemblyGroups(); } [Theory] @@ -47,7 +59,7 @@ void Method() [Fact(Skip = "Reporting an analyzer failure when the unit test code above shows it is correct")] public async Task CodeFix() { - string testCode = """ + const string testCode = """ class Program { void Method() @@ -58,16 +70,16 @@ void Method() } """; - string fixedCode = """ - class Program - { - void Method() - { - var arr = new Span(new int[10]); - var val = arr.Slice(5, 1)[0]; - } - } - """; + const string fixedCode = """ + class Program + { + void Method() + { + var arr = new Span(new int[10]); + var val = arr.Slice(5, 1)[0]; + } + } + """; await CodeFixVerifier.VerifyCodeFixAsync(testCode, fixedCode, ReferenceAssemblyCatalog.Net80); }