From 180453af77928c5740323436a47c6002e4702187 Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 20 May 2025 14:46:33 +0200 Subject: [PATCH 1/2] =?UTF-8?q?Impl=C3=A9mentation=20de=20la=20r=C3=A8gle?= =?UTF-8?q?=20RCS1242:=20Do=20not=20pass=20non-read-only=20struct=20by=20r?= =?UTF-8?q?ead-only=20reference?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .gitignore | 1 + .../GCIACV.NonReadOnlyStruct.Fixer.cs | 58 +++++++ .../Analyzers/GCIACV.NonReadOnlyStruct.cs | 76 +++++++++ src/Creedengo.Core/Models/Rule.cs | 1 + .../Tests/GCIACV.NonReadOnlyStruct.Tests.cs | 161 ++++++++++++++++++ 5 files changed, 297 insertions(+) create mode 100644 src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.Fixer.cs create mode 100644 src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.cs create mode 100644 src/Creedengo.Tests/Tests/GCIACV.NonReadOnlyStruct.Tests.cs diff --git a/.gitignore b/.gitignore index e6d35f75..13844b08 100644 --- a/.gitignore +++ b/.gitignore @@ -486,3 +486,4 @@ $RECYCLE.BIN/ **/Container **/Publish +llm.md 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..fa607919 --- /dev/null +++ b/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.Fixer.cs @@ -0,0 +1,58 @@ +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; + + context.RegisterCodeFix( + CodeAction.Create( + title: "Remove 'in' modifier", + c => RemoveInModifierAsync(context.Document, parameter, c), + equivalenceKey: "Remove 'in' modifier"), + context.Diagnostics); + } + 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(); + } +} diff --git a/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.cs b/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.cs new file mode 100644 index 00000000..9c52dc45 --- /dev/null +++ b/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.cs @@ -0,0 +1,76 @@ +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' 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 parameter has the 'in' keyword + if (!parameter.Modifiers.Any(SyntaxKind.InKeyword)) + continue; + + // Skip if parameter type is null + if (parameter.Type == null) + continue; + + // Get the parameter type symbol + var parameterTypeSymbol = ModelExtensions.GetTypeInfo(semanticModel, parameter.Type).Type; + if (parameterTypeSymbol == null) + continue; + + // Skip reference types (not structs) + if (!parameterTypeSymbol.IsValueType) + continue; + + // Check if we're dealing with a named type symbol for a struct + if (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 0015b636..020a8322 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"; } /// Creates a diagnostic descriptor. 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..2f703f75 --- /dev/null +++ b/src/Creedengo.Tests/Tests/GCIACV.NonReadOnlyStruct.Tests.cs @@ -0,0 +1,161 @@ +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 + { + } + """); +} From 94943830af8c8abafd9c173ce84349bdc12032cc Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 21 May 2025 11:04:55 +0200 Subject: [PATCH 2/2] prise en compte de ref readonly --- .gitignore | 1 - .../GCIACV.NonReadOnlyStruct.Fixer.cs | 75 +++++++++++++++++-- .../Analyzers/GCIACV.NonReadOnlyStruct.cs | 34 +++------ .../Tests/GCIACV.NonReadOnlyStruct.Tests.cs | 68 +++++++++++++++++ 4 files changed, 147 insertions(+), 31 deletions(-) diff --git a/.gitignore b/.gitignore index 13844b08..e6d35f75 100644 --- a/.gitignore +++ b/.gitignore @@ -486,4 +486,3 @@ $RECYCLE.BIN/ **/Container **/Publish -llm.md diff --git a/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.Fixer.cs b/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.Fixer.cs index fa607919..656f4289 100644 --- a/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.Fixer.cs +++ b/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.Fixer.cs @@ -30,14 +30,35 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) if (parameter == null) return; - context.RegisterCodeFix( - CodeAction.Create( - title: "Remove 'in' modifier", - c => RemoveInModifierAsync(context.Document, parameter, c), - equivalenceKey: "Remove 'in' modifier"), - context.Diagnostics); + // 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( + + private static async Task RemoveInModifierAsync( Document document, ParameterSyntax parameter, CancellationToken cancellationToken) @@ -55,4 +76,44 @@ private static async Task RemoveInModifierAsync( 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 index 9c52dc45..1639fd3a 100644 --- a/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.cs +++ b/src/Creedengo.Core/Analyzers/GCIACV.NonReadOnlyStruct.cs @@ -13,7 +13,7 @@ public sealed class NonReadOnlyStruct : DiagnosticAnalyzer message: "Non-read-only struct should not be passed by read-only reference.", category: Rule.Categories.Performance, severity: DiagnosticSeverity.Warning, - description: "Using 'in' parameter modifier for non-readonly struct types can lead to defensive copies, causing performance degradation."); + 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; @@ -31,30 +31,18 @@ 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 + var compilation = context.Compilation; // Analyze each parameter in the method declaration foreach (var parameter in methodDeclaration.ParameterList.Parameters) { - // Check if parameter has the 'in' keyword - if (!parameter.Modifiers.Any(SyntaxKind.InKeyword)) - continue; - - // Skip if parameter type is null - if (parameter.Type == null) - continue; - - // Get the parameter type symbol - var parameterTypeSymbol = ModelExtensions.GetTypeInfo(semanticModel, parameter.Type).Type; - if (parameterTypeSymbol == null) - continue; - - // Skip reference types (not structs) - if (!parameterTypeSymbol.IsValueType) - continue; - - // Check if we're dealing with a named type symbol for a struct - if (parameterTypeSymbol is INamedTypeSymbol namedTypeSymbol) + // 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) diff --git a/src/Creedengo.Tests/Tests/GCIACV.NonReadOnlyStruct.Tests.cs b/src/Creedengo.Tests/Tests/GCIACV.NonReadOnlyStruct.Tests.cs index 2f703f75..49b6b7ab 100644 --- a/src/Creedengo.Tests/Tests/GCIACV.NonReadOnlyStruct.Tests.cs +++ b/src/Creedengo.Tests/Tests/GCIACV.NonReadOnlyStruct.Tests.cs @@ -158,4 +158,72 @@ 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) + { + } + } + """); }