diff --git a/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.Fixer.cs b/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.Fixer.cs new file mode 100644 index 00000000..656f4289 --- /dev/null +++ b/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.Fixer.cs @@ -0,0 +1,119 @@ +namespace Creedengo.Core.Analyzers; + +/// GCIACV fixer: Do not pass non-read-only struct by read-only reference. +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(NonReadOnlyStructFixer)), Shared] +public sealed class NonReadOnlyStructFixer : CodeFixProvider +{ + /// + public override ImmutableArray FixableDiagnosticIds => _fixableDiagnosticIds; + private static readonly ImmutableArray _fixableDiagnosticIds = [NonReadOnlyStruct.Descriptor.Id]; + + /// + [ExcludeFromCodeCoverage] + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + /// + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + if (context.Diagnostics.Length == 0) return; + + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root == null) return; + + var diagnostic = context.Diagnostics.First(); + var nodeSpan = diagnostic.Location.SourceSpan; + var parameter = root.FindToken(nodeSpan.Start) + .Parent? + .AncestorsAndSelf() + .OfType() + .FirstOrDefault(); + + if (parameter == null) return; + + // Check if it's an 'in' parameter or 'ref readonly' parameter + if (parameter.Modifiers.Any(SyntaxKind.InKeyword)) + { + context.RegisterCodeFix( + CodeAction.Create( + title: "Remove 'in' modifier", + c => RemoveInModifierAsync(context.Document, parameter, c), + equivalenceKey: "Remove 'in' modifier"), + diagnostic); + } + else if (parameter.Modifiers.Any(SyntaxKind.RefKeyword) && parameter.Modifiers.Any(SyntaxKind.ReadOnlyKeyword)) + { + context.RegisterCodeFix( + CodeAction.Create( + title: "Remove 'readonly' modifier (keep 'ref')", + c => RemoveReadOnlyModifierAsync(context.Document, parameter, c), + equivalenceKey: "Remove 'readonly' modifier"), + diagnostic); + + context.RegisterCodeFix( + CodeAction.Create( + title: "Remove 'ref readonly' modifiers", + c => RemoveRefReadOnlyModifiersAsync(context.Document, parameter, c), + equivalenceKey: "Remove 'ref readonly' modifiers"), + diagnostic); + } + } + + private static async Task RemoveInModifierAsync( + Document document, + ParameterSyntax parameter, + CancellationToken cancellationToken) + { + var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); + + // Get the 'in' modifier + var inModifier = parameter.Modifiers.First(m => m.IsKind(SyntaxKind.InKeyword)); + + // Create new parameter without the 'in' modifier + var newParameter = parameter.WithModifiers(parameter.Modifiers.Remove(inModifier)); + + // Replace the parameter + editor.ReplaceNode(parameter, newParameter); + + return editor.GetChangedDocument(); + } + + private static async Task RemoveReadOnlyModifierAsync( + Document document, + ParameterSyntax parameter, + CancellationToken cancellationToken) + { + var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); + + // Get the 'readonly' modifier + var readOnlyModifier = parameter.Modifiers.First(m => m.IsKind(SyntaxKind.ReadOnlyKeyword)); + + // Create new parameter without the 'readonly' modifier + var newParameter = parameter.WithModifiers(parameter.Modifiers.Remove(readOnlyModifier)); + + // Replace the parameter + editor.ReplaceNode(parameter, newParameter); + + return editor.GetChangedDocument(); + } + + private static async Task RemoveRefReadOnlyModifiersAsync( + Document document, + ParameterSyntax parameter, + CancellationToken cancellationToken) + { + var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); + + // Get the 'ref' and 'readonly' modifiers + var refModifier = parameter.Modifiers.First(m => m.IsKind(SyntaxKind.RefKeyword)); + var readOnlyModifier = parameter.Modifiers.First(m => m.IsKind(SyntaxKind.ReadOnlyKeyword)); + + // Create new parameter without both modifiers + var newParameter = parameter.WithModifiers( + parameter.Modifiers.Remove(refModifier).Remove(readOnlyModifier)); + + // Replace the parameter + editor.ReplaceNode(parameter, newParameter); + + return editor.GetChangedDocument(); + } +} diff --git a/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.cs b/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.cs new file mode 100644 index 00000000..1639fd3a --- /dev/null +++ b/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.cs @@ -0,0 +1,64 @@ +namespace Creedengo.Core.Analyzers; + +/// GCIACV: Do not pass non-read-only struct by read-only reference. +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class NonReadOnlyStruct : DiagnosticAnalyzer +{ + private static readonly ImmutableArray MethodDeclarations = [SyntaxKind.MethodDeclaration]; + + /// The diagnostic descriptor. + public static DiagnosticDescriptor Descriptor { get; } = Rule.CreateDescriptor( + id: Rule.Ids.GCIACV_NonReadOnlyStruct, + title: "Do not pass non-read-only struct by read-only reference", + message: "Non-read-only struct should not be passed by read-only reference.", + category: Rule.Categories.Performance, + severity: DiagnosticSeverity.Warning, + description: "Using 'in' or 'ref readonly' parameter modifier for non-readonly struct types can lead to defensive copies, causing performance degradation."); + + /// + public override ImmutableArray SupportedDiagnostics => _supportedDiagnostics; + private static readonly ImmutableArray _supportedDiagnostics = [Descriptor]; + + /// + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterSyntaxNodeAction(static context => AnalyzeSyntaxNode(context), MethodDeclarations); + } + + private static void AnalyzeSyntaxNode(SyntaxNodeAnalysisContext context) + { + var methodDeclaration = (MethodDeclarationSyntax)context.Node; + var semanticModel = context.SemanticModel; + var compilation = context.Compilation; // Analyze each parameter in the method declaration + foreach (var parameter in methodDeclaration.ParameterList.Parameters) + { + // Check if the parameter has either 'in' keyword or 'ref readonly' modifiers + bool isReadOnlyReference = parameter.Modifiers.Any(SyntaxKind.InKeyword) || + (parameter.Modifiers.Any(SyntaxKind.RefKeyword) && parameter.Modifiers.Any(SyntaxKind.ReadOnlyKeyword)); + + if (isReadOnlyReference && + parameter.Type != null && + ModelExtensions.GetTypeInfo(semanticModel, parameter.Type).Type is { } parameterTypeSymbol && + parameterTypeSymbol.IsValueType && + parameterTypeSymbol is INamedTypeSymbol namedTypeSymbol) + { + // For nested struct types or any other struct, check if it's readonly + if (!namedTypeSymbol.IsReadOnly) + { + // Report diagnostic on the type identifier part of the parameter + var diagnosticLocation = parameter.Type.GetLocation(); + if (diagnosticLocation == null) + diagnosticLocation = parameter.GetLocation(); + + context.ReportDiagnostic( + Diagnostic.Create( + Descriptor, + diagnosticLocation, + parameter.Identifier.ValueText)); + } + } + } + } +} diff --git a/src/Creedengo.Core/Models/Rule.cs b/src/Creedengo.Core/Models/Rule.cs index c0887968..88873b0d 100644 --- a/src/Creedengo.Core/Models/Rule.cs +++ b/src/Creedengo.Core/Models/Rule.cs @@ -27,6 +27,7 @@ public static class Ids public const string GCI91_UseWhereBeforeOrderBy = "GCI91"; public const string GCI92_UseStringEmptyLength = "GCI92"; public const string GCI93_ReturnTaskDirectly = "GCI93"; + public const string GCIACV_NonReadOnlyStruct = "GCIACV"; public const string GCI95_UseIsOperatorInsteadOfAsOperator = "GCI95"; } diff --git a/src/Creedengo.Tests/Tests/GCIACV.NonReadOnlyStruct.Tests.cs b/src/Creedengo.Tests/Tests/GCIACV.NonReadOnlyStruct.Tests.cs new file mode 100644 index 00000000..49b6b7ab --- /dev/null +++ b/src/Creedengo.Tests/Tests/GCIACV.NonReadOnlyStruct.Tests.cs @@ -0,0 +1,229 @@ +namespace Creedengo.Tests.Tests; + +[TestClass] +public sealed class NonReadOnlyStructTests +{ + private static readonly CodeFixerDlg VerifyAsync = TestRunner.VerifyAsync; + + [TestMethod] + public Task EmptyCodeAsync() => VerifyAsync(""); + + [TestMethod] + public Task NoWarningOnClass() => VerifyAsync(""" + public class Test + { + public void Method(in Test test) + { + } + } + """); + + [TestMethod] + public Task NoWarningOnClassReference() => VerifyAsync(""" + public class Test + { + public void Method(in string test) + { + } + } + """); + + [TestMethod] + public Task NoWarningOnReadOnlyStruct() => VerifyAsync(""" + public readonly struct ReadOnlyTest + { + public void Method(in ReadOnlyTest test) + { + } + } + """); + + [TestMethod] + public Task NoWarningOnParameterWithoutInModifier() => VerifyAsync(""" + public struct Test + { + public void Method(Test test) + { + } + } + """); + + [TestMethod] + public Task WarningOnNonReadOnlyStructWithInModifier() => VerifyAsync(""" + public struct Test + { + public void Method(in [|Test|] test) + { + } + } + """, """ + public struct Test + { + public void Method(Test test) + { + } + } + """); + + [TestMethod] + public Task WarningOnMultipleParametersWithInModifier() => VerifyAsync(""" + public struct Test + { + public void Method(in [|Test|] test1, int value, in [|Test|] test2) + { + } + } + """, """ + public struct Test + { + public void Method(Test test1, int value, Test test2) + { + } + } + """); + + [TestMethod] + public Task WarningOnNestedNonReadOnlyStructs() => VerifyAsync(""" + public struct OuterStruct + { + public struct InnerStruct + { + } + + public void Method(in [|InnerStruct|] inner) + { + } + } + """, """ + public struct OuterStruct + { + public struct InnerStruct + { + } + + public void Method(InnerStruct inner) + { + } + } + """); + + [TestMethod] + public Task WarningOnStructWithComplexMethodBody() => VerifyAsync(""" + public struct Test + { + private int _value; + + public void Method(in [|Test|] test) + { + var value = test._value; + System.Console.WriteLine(value); + System.Console.WriteLine("Processing completed"); + } + } + """, """ + public struct Test + { + private int _value; + + public void Method(Test test) + { + var value = test._value; + System.Console.WriteLine(value); + System.Console.WriteLine("Processing completed"); + } + } + """); + + [TestMethod] + public Task WarningOnStructInExtensionMethod() => VerifyAsync(""" + public static class Extensions + { + public static void Extend(this in [|TestStruct|] test) + { + } + } + + public struct TestStruct + { + } + """, """ + public static class Extensions + { + public static void Extend(this TestStruct test) + { + } + } + + public struct TestStruct + { + } + """); + + [TestMethod] + public Task NoWarningOnRefReadOnlyWithReadOnlyStruct() => VerifyAsync(""" + public readonly struct ReadOnlyTest + { + public void Method(ref readonly ReadOnlyTest test) + { + } + } + """); + + [TestMethod] + public Task WarningOnRefReadOnlyWithNonReadOnlyStruct() => VerifyAsync(""" + public struct Test + { + public void Method(ref readonly [|Test|] test) + { + } + } + """, """ + public struct Test + { + public void Method(ref Test test) + { + } + } + """); + [TestMethod] + public Task WarningOnRefReadOnlyWithNonReadOnlyStructRemoveReadOnly() => VerifyAsync(""" + public struct Test + { + public void Method(ref readonly [|Test|] test) + { + } + } + """, """ + public struct Test + { + public void Method(ref Test test) + { + } + } + """); + [TestMethod] + public Task WarningOnRefReadOnlyWithNonReadOnlyStructRemoveBoth() => VerifyAsync(""" + public struct Test + { + public void Method(ref readonly [|Test|] test) + { + } + } + """, """ + public struct Test + { + public void Method(ref Test test) + { + } + } + """); + [TestMethod] + public Task WarningOnMixedModifiersWithRefReadOnlyAndIn() => VerifyAsync(""" + public struct Test + { + public void Method(ref readonly [|Test|] test1, int value, in [|Test|] test2) + { + } + } + """); +}