From 8e266cc931deb0fc5b58fc99ec7a66e15853aae3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9rald=20Barr=C3=A9?= Date: Sat, 17 Oct 2026 23:07:03 -0400 Subject: [PATCH] MA0038: fix the analyzer crash on invalid code and the code fixes that break the code --- docs/Rules/MA0038.md | 4 + .../Rules/MakeMethodStaticFixer.cs | 98 +++++-- .../Rules/MakeMethodStaticAnalyzer.cs | 132 ++++++--- .../Rules/MakeMethodStaticAnalyzerTests.cs | 267 ++++++++++++++++++ 4 files changed, 441 insertions(+), 60 deletions(-) diff --git a/docs/Rules/MA0038.md b/docs/Rules/MA0038.md index 094f36ab5..a7d60161d 100644 --- a/docs/Rules/MA0038.md +++ b/docs/Rules/MA0038.md @@ -8,3 +8,7 @@ Sources: [MakeMethodStaticAnalyzer.cs](https://github.com/meziantou/Meziantou.An If the method only uses static or const members, you should consider making the method static. +The rule does not report the members that are accessed through an instance other than `this` or `base` (`instance.Method()`, `instance?.Method()`), or in an object initializer or a property pattern, as a static member cannot be accessed this way. The code fix replaces `this.Method()` and `base.Method()` with `Method()`. + +The rule does not report the members that use a parameter of the primary constructor, which is captured by the instance, nor the members that the compiler binds by name on an instance (`GetEnumerator`, `MoveNext`, `Current`, `Dispose`, `DisposeAsync`, `GetAwaiter`, `GetResult`, `IsCompleted`, `Deconstruct`, `Add`, `GetPinnableReference`, `Slice`, `Length`, `Count`, ...). The code fix removes the `readonly` modifier of the members of a struct, as a static member cannot be `readonly`. + diff --git a/src/Meziantou.Analyzer.CodeFixers/Rules/MakeMethodStaticFixer.cs b/src/Meziantou.Analyzer.CodeFixers/Rules/MakeMethodStaticFixer.cs index 938c7fca4..3c4e40af7 100644 --- a/src/Meziantou.Analyzer.CodeFixers/Rules/MakeMethodStaticFixer.cs +++ b/src/Meziantou.Analyzer.CodeFixers/Rules/MakeMethodStaticFixer.cs @@ -1,4 +1,6 @@ +using Microsoft.CodeAnalysis.FindSymbols; using Microsoft.CodeAnalysis.Formatting; +using Microsoft.CodeAnalysis.Simplification; namespace Meziantou.Analyzer.Rules; @@ -16,31 +18,93 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) { var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); var nodeToFix = root?.FindNode(context.Span, getInnermostNodeForTie: true); - if (nodeToFix is MethodDeclarationSyntax or PropertyDeclarationSyntax) - { - var title = "Add static modifier"; - var codeAction = CodeAction.Create( - title, - ct => AddStaticModifier(context.Document, nodeToFix, ct), - equivalenceKey: title); + if (nodeToFix is not (MethodDeclarationSyntax or PropertyDeclarationSyntax)) + return; - context.RegisterCodeFix(codeAction, context.Diagnostics); - } + var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + if (semanticModel?.GetDeclaredSymbol(nodeToFix, context.CancellationToken) is not { } symbol) + return; + + var title = "Add static modifier"; + var codeAction = CodeAction.Create( + title, + ct => AddStaticModifier(context.Document, nodeToFix, symbol, ct), + equivalenceKey: title); + + context.RegisterCodeFix(codeAction, context.Diagnostics); } - private static async Task AddStaticModifier(Document document, SyntaxNode nodeToFix, CancellationToken cancellationToken) + private static async Task AddStaticModifier(Document document, SyntaxNode nodeToFix, ISymbol symbol, CancellationToken cancellationToken) { - var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); + var solution = document.Project.Solution; + var solutionEditor = new SolutionEditor(solution); + var editor = await solutionEditor.GetDocumentEditorAsync(document.Id, cancellationToken).ConfigureAwait(false); - if (nodeToFix is MethodDeclarationSyntax method) + // The member can reference itself, so the declaration is updated from its current node, which contains the updated references + // A static member cannot be readonly (CS8657): readonly void M() => static void M() + editor.ReplaceNode(nodeToFix, (currentNode, _) => currentNode switch { - editor.ReplaceNode(method, method.WithModifiers(method.Modifiers.Add(SyntaxKind.StaticKeyword)).WithAdditionalAnnotations(Formatter.Annotation)); - } - else if (nodeToFix is PropertyDeclarationSyntax property) + MethodDeclarationSyntax method when method.Modifiers.Count == 0 => method.WithReturnType(method.ReturnType.WithoutLeadingTrivia()).WithModifiers(CreateStaticModifier(method.ReturnType)), + MethodDeclarationSyntax method => method.WithModifiers(AddStaticModifier(method.Modifiers)).WithAdditionalAnnotations(Formatter.Annotation), + PropertyDeclarationSyntax property when property.Modifiers.Count == 0 => RemoveReadOnlyAccessors(property.WithType(property.Type.WithoutLeadingTrivia()).WithModifiers(CreateStaticModifier(property.Type))), + PropertyDeclarationSyntax property => RemoveReadOnlyAccessors(property.WithModifiers(AddStaticModifier(property.Modifiers))).WithAdditionalAnnotations(Formatter.Annotation), + _ => currentNode, + }); + + // A static member cannot be accessed with an instance reference (CS0176): this.Member => Type.Member + // The analyzer does not report the members that are accessed through the other instances. + var references = await SymbolFinder.FindReferencesAsync(symbol, solution, cancellationToken).ConfigureAwait(false); + foreach (var location in references.SelectMany(reference => reference.Locations)) { - editor.ReplaceNode(property, property.WithModifiers(property.Modifiers.Add(SyntaxKind.StaticKeyword)).WithAdditionalAnnotations(Formatter.Annotation)); + // Source generated documents cannot be edited + if (solution.GetDocument(location.Document.Id) is null) + continue; + + var referenceEditor = await solutionEditor.GetDocumentEditorAsync(location.Document.Id, cancellationToken).ConfigureAwait(false); + var reference = referenceEditor.OriginalRoot.FindNode(location.Location.SourceSpan, getInnermostNodeForTie: true); + if (reference.Parent is not MemberAccessExpressionSyntax { Expression: ThisExpressionSyntax or BaseExpressionSyntax } memberAccess || memberAccess.Name != reference) + continue; + + // The containing type of the referenced member has the type arguments of the call site: base.Member => Base.Member + if (referenceEditor.SemanticModel.GetSymbolInfo(memberAccess, cancellationToken).Symbol?.ContainingType is not { } containingType) + continue; + + var typeExpression = ((TypeSyntax)referenceEditor.Generator.TypeExpression(containingType)).AsExpressionSyntax().WithTriviaFrom(memberAccess.Expression); + referenceEditor.ReplaceNode(memberAccess, memberAccess.WithExpression(typeExpression).WithAdditionalAnnotations(Simplifier.Annotation)); } - return editor.GetChangedDocument(); + return solutionEditor.GetChangedSolution(); + } + + // Without modifier, the indentation is the leading trivia of the type + private static SyntaxTokenList CreateStaticModifier(TypeSyntax type) => SyntaxFactory.TokenList(SyntaxFactory.Token(type.GetLeadingTrivia(), SyntaxKind.StaticKeyword, SyntaxFactory.TriviaList(SyntaxFactory.Space))); + + private static SyntaxTokenList AddStaticModifier(SyntaxTokenList modifiers) + { + var readOnlyIndex = modifiers.IndexOf(SyntaxKind.ReadOnlyKeyword); + if (readOnlyIndex < 0) + return modifiers.Add(SyntaxKind.StaticKeyword); + + var readOnlyModifier = modifiers[readOnlyIndex]; + return modifiers.Replace(readOnlyModifier, SyntaxFactory.Token(readOnlyModifier.LeadingTrivia, SyntaxKind.StaticKeyword, readOnlyModifier.TrailingTrivia)); + } + + private static PropertyDeclarationSyntax RemoveReadOnlyAccessors(PropertyDeclarationSyntax property) + { + if (property.AccessorList is null) + return property; + + var accessors = property.AccessorList.Accessors.Select(accessor => + { + var readOnlyIndex = accessor.Modifiers.IndexOf(SyntaxKind.ReadOnlyKeyword); + if (readOnlyIndex < 0) + return accessor; + + // The leading trivia of the first modifier is the indentation of the accessor + var leadingTrivia = accessor.GetLeadingTrivia(); + return accessor.WithModifiers(accessor.Modifiers.RemoveAt(readOnlyIndex)).WithoutLeadingTrivia().WithLeadingTrivia(leadingTrivia); + }); + + return property.WithAccessorList(property.AccessorList.WithAccessors(SyntaxFactory.List(accessors))); } } diff --git a/src/Meziantou.Analyzer/Rules/MakeMethodStaticAnalyzer.cs b/src/Meziantou.Analyzer/Rules/MakeMethodStaticAnalyzer.cs index 5fb17340e..3fbb8241e 100644 --- a/src/Meziantou.Analyzer/Rules/MakeMethodStaticAnalyzer.cs +++ b/src/Meziantou.Analyzer/Rules/MakeMethodStaticAnalyzer.cs @@ -1,4 +1,3 @@ -using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; namespace Meziantou.Analyzer.Rules; @@ -37,9 +36,9 @@ public override void Initialize(AnalysisContext context) { var analyzerContext = new AnalyzerContext(ctx.Compilation); - ctx.RegisterSyntaxNodeAction(analyzerContext.AnalyzeMethod, SyntaxKind.MethodDeclaration); - ctx.RegisterSyntaxNodeAction(analyzerContext.AnalyzeProperty, SyntaxKind.PropertyDeclaration); + ctx.RegisterOperationBlockAction(analyzerContext.AnalyzeOperationBlock); ctx.RegisterOperationAction(analyzerContext.AnalyzeDelegateCreation, OperationKind.DelegateCreation); + ctx.RegisterOperationAction(analyzerContext.AnalyzeMemberReference, OperationKind.Invocation, OperationKind.MethodReference, OperationKind.PropertyReference); ctx.RegisterCompilationEndAction(analyzerContext.CompilationEnd); }); } @@ -76,64 +75,71 @@ public void CompilationEnd(CompilationAnalysisContext context) } } - public void AnalyzeMethod(SyntaxNodeAnalysisContext context) + public void AnalyzeOperationBlock(OperationBlockAnalysisContext context) { - var node = (MethodDeclarationSyntax)context.Node; - var methodSymbol = context.SemanticModel.GetDeclaredSymbol(node, context.CancellationToken); - if (methodSymbol is null) + if (context.OwningSymbol is not IMethodSymbol methodSymbol) return; - if (context.Compilation is null) - return; - - if (!IsPotentialStatic(methodSymbol) || methodSymbol.IsUnitTestMethod() || IsAspNetCoreMiddleware(methodSymbol) || IsAspNetCoreStartup(methodSymbol)) + switch (methodSymbol) { - return; + case { MethodKind: MethodKind.Ordinary }: + AnalyzeMethod(context, methodSymbol); + break; + + case { MethodKind: MethodKind.PropertyGet or MethodKind.PropertySet, AssociatedSymbol: IPropertySymbol propertySymbol }: + AnalyzeProperty(context, propertySymbol); + break; } + } - var body = (SyntaxNode?)node.Body ?? node.ExpressionBody; - if (body is null) + private void AnalyzeMethod(OperationBlockAnalysisContext context, IMethodSymbol methodSymbol) + { + if (!IsDeclaredBy(methodSymbol, context.CancellationToken)) return; - var operation = context.SemanticModel.GetOperation(body, context.CancellationToken); - if (operation is null || HasInstanceUsages(operation)) + if (!IsPotentialStatic(methodSymbol) || methodSymbol.IsUnitTestMethod() || IsAspNetCoreMiddleware(methodSymbol) || IsAspNetCoreStartup(methodSymbol)) + return; + + if (HasInstanceUsages(context.OperationBlocks)) return; _potentialSymbols.Add(methodSymbol); } - public void AnalyzeProperty(SyntaxNodeAnalysisContext context) + /// + /// The accessors of a property are analyzed separately, so the property can be made static when none of + /// its accessors uses the instance. The auto-implemented accessors have no operation block, and use the instance. + /// + private void AnalyzeProperty(OperationBlockAnalysisContext context, IPropertySymbol propertySymbol) { - var node = (PropertyDeclarationSyntax)context.Node; - var propertySymbol = context.SemanticModel.GetDeclaredSymbol(node, context.CancellationToken); - if (propertySymbol is null) + if (!IsPotentialStatic(propertySymbol)) return; - if (!IsPotentialStatic(propertySymbol)) + if (propertySymbol.DeclaringSyntaxReferences is not [var syntaxReference] || syntaxReference.GetSyntax(context.CancellationToken) is not PropertyDeclarationSyntax node) return; - if (node.ExpressionBody is not null) + if (HasInstanceUsages(context.OperationBlocks)) { - var operation = context.SemanticModel.GetOperation(node.ExpressionBody, context.CancellationToken); - if (operation is null || HasInstanceUsages(operation)) - return; + _cannotBeStaticSymbols.Add(propertySymbol); + return; } - if (node.AccessorList is not null) - { - foreach (var accessor in node.AccessorList.Accessors) - { - var body = (SyntaxNode?)accessor.Body ?? accessor.ExpressionBody; - if (body is null) - return; + if (node.AccessorList is not null && node.AccessorList.Accessors.Any(accessor => accessor.Body is null && accessor.ExpressionBody is null)) + return; - var operation = context.SemanticModel.GetOperation(body, context.CancellationToken); - if (operation is null || HasInstanceUsages(operation)) - return; - } + _potentialSymbols.Add(propertySymbol); + } + + private static bool IsDeclaredBy(ISymbol symbol, CancellationToken cancellationToken) + where TSyntax : SyntaxNode + { + foreach (var syntaxReference in symbol.DeclaringSyntaxReferences) + { + if (syntaxReference.GetSyntax(cancellationToken) is TSyntax) + return true; } - _potentialSymbols.Add(propertySymbol); + return false; } public void AnalyzeDelegateCreation(OperationAnalysisContext context) @@ -149,6 +155,29 @@ public void AnalyzeDelegateCreation(OperationAnalysisContext context) } } + /// + /// A static member cannot be accessed through an instance (CS0176), nor in an object initializer or a property pattern. + /// The code fixer replaces this.Member with Type.Member, but the other instances cannot be removed, as + /// they can have side effects. + /// + public void AnalyzeMemberReference(OperationAnalysisContext context) + { + var (member, instance) = context.Operation switch + { + IInvocationOperation operation => ((ISymbol)operation.TargetMethod, operation.Instance), + IMemberReferenceOperation operation => (operation.Member, operation.Instance), + _ => default, + }; + + if (member is null || instance is null or IInstanceReferenceOperation { ReferenceKind: InstanceReferenceKind.ContainingTypeInstance }) + return; + + if (member.IsStatic || !member.ContainingAssembly.IsEqualTo(compilation.Assembly)) + return; + + _cannotBeStaticSymbols.Add(member.OriginalDefinition); + } + private static bool IsPotentialStatic(IMethodSymbol symbol) { return @@ -157,6 +186,7 @@ private static bool IsPotentialStatic(IMethodSymbol symbol) !symbol.IsOverride && !symbol.IsStatic && !symbol.IsInterfaceImplementation() && + !IsPatternBasedMember(symbol) && symbol.PartialDefinitionPart is null; } @@ -167,16 +197,28 @@ private static bool IsPotentialStatic(IPropertySymbol symbol) !symbol.IsVirtual && !symbol.IsOverride && !symbol.IsStatic && - !symbol.IsInterfaceImplementation(); + !symbol.IsInterfaceImplementation() && + !IsPatternBasedMember(symbol); } - private static bool HasInstanceUsages(IOperation operation) + /// + /// The compiler binds some members by name on an instance, without operations referencing them: foreach, await, + /// using, deconstruction, collection initializers and collection expressions, fixed statements, indexes and ranges. + /// They cannot be static. + /// + private static bool IsPatternBasedMember(ISymbol symbol) { - if (operation is null) - return false; + return symbol switch + { + IMethodSymbol => symbol.Name is "GetEnumerator" or "GetAsyncEnumerator" or "MoveNext" or "MoveNextAsync" or "Dispose" or "DisposeAsync" or "GetAwaiter" or "GetResult" or "OnCompleted" or "UnsafeOnCompleted" or "Deconstruct" or "Add" or "GetPinnableReference" or "Slice", + IPropertySymbol => symbol.Name is "Current" or "IsCompleted" or "Length" or "Count", + _ => false, + }; + } - var operations = new Queue(); - operations.Enqueue(operation); + private static bool HasInstanceUsages(ImmutableArray operationBlocks) + { + var operations = new Queue(operationBlocks); while (operations.Count > 0) { @@ -190,6 +232,10 @@ private static bool HasInstanceUsages(IOperation operation) { case IInstanceReferenceOperation instanceReferenceOperation when instanceReferenceOperation.ReferenceKind == InstanceReferenceKind.ContainingTypeInstance: return true; + + // The parameters of a constructor can only be referenced from a method or an accessor when they are the parameters of the primary constructor, which are captured by the instance + case IParameterReferenceOperation { Parameter.ContainingSymbol: IMethodSymbol { MethodKind: MethodKind.Constructor } }: + return true; } } diff --git a/tests/Meziantou.Analyzer.Test/Rules/MakeMethodStaticAnalyzerTests.cs b/tests/Meziantou.Analyzer.Test/Rules/MakeMethodStaticAnalyzerTests.cs index d87f7ecb9..969d71e95 100644 --- a/tests/Meziantou.Analyzer.Test/Rules/MakeMethodStaticAnalyzerTests.cs +++ b/tests/Meziantou.Analyzer.Test/Rules/MakeMethodStaticAnalyzerTests.cs @@ -800,4 +800,271 @@ interface ITest return test.RunAsync(); } + + [Fact] + [Trait("Issue", "https://github.com/meziantou/Meziantou.Analyzer/issues/1644")] + public Task InvalidCode_UnconvertedInterpolatedString_NoException() + { + var test = CreateTest(); + // The code does not compile, so the compiler diagnostics cannot be verified + test.CompilerDiagnostics = CompilerDiagnostics.None; + test.TestCode = """ + class Test + { + void Sample() + { + A(B$@ = 0); + } + } + """; + + return test.RunAsync(); + } + + [Fact] + [Trait("Issue", "https://github.com/meziantou/Meziantou.Analyzer/issues/1650")] + public Task CalledThroughConditionalAccess_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + using System.Threading.Tasks; + class Test + { + Task A() { return ((Test)null)?.A(); } + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task CalledThroughInstance_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + class Test + { + void A() => throw null; + static void B(Test instance) => instance.A(); + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task PropertyInObjectInitializer_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + class Test + { + int A { get => 0; set { } } + static Test B() => new Test { A = 1 }; + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task PropertyInPropertyPattern_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + class Test + { + int A => 0; + static bool B(object value) => value is Test { A: 1 }; + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task CalledThroughThis_Simplified() + { + var test = CreateTest(); + test.TestCode = """ + class Test + { + private int _value; + + void {|MA0038:A|}() => throw null; + int {|MA0041:P|} => 0; + + void B() + { + this.A(); + _ = this.P + _value; + } + } + """; + test.FixedCode = """ + class Test + { + private int _value; + + static void A() => throw null; + static int P => 0; + + void B() + { + A(); + _ = P + _value; + } + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task CalledThroughBase_Simplified() + { + var test = CreateTest(); + test.TestCode = """ + class Base + { + internal void {|MA0038:A|}() => throw null; + } + + class Derived : Base + { + private int _value; + + int B() + { + base.A(); + return _value; + } + } + """; + test.FixedCode = """ + class Base + { + internal static void A() => throw null; + } + + class Derived : Base + { + private int _value; + + int B() + { + A(); + return _value; + } + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task PrimaryConstructorParameter_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + class Test(int p) + { + int A() => p++; + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task PrimaryConstructorParameter_RefLocal_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + class Test(string p) + { + void A() + { + ref var a = ref p; + } + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task ReadOnlyStructMembers_CodeFix_RemovesReadOnly() + { + var test = CreateTest(); + test.TestCode = """ + struct Test + { + readonly void {|MA0038:A|}() { } + public readonly int {|MA0041:B|} => 0; + int {|MA0041:C|} { readonly get => 0; set { } } + } + """; + test.FixedCode = """ + struct Test + { + static void A() { } + public static int B => 0; + static int C { get => 0; set { } } + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task AwaitUsingDisposeAsync_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + using System; + using System.Threading.Tasks; + + class Test + { + public async Task {|MA0038:A|}() + { + await using var a = new Sample(); + } + + private class Sample : IDisposable + { + public void Dispose() => throw null; + public ValueTask DisposeAsync() => throw null; + } + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task CollectionExpressionAdd_NoDiagnostic() + { + var test = CreateTest(); + test.ReferenceAssemblies = ReferenceAssemblies.Net.Net80; + test.TestCode = """ + using System.Collections; + using System.Collections.Generic; + + public class Data : IEnumerable + { + public void Add(T value) { } + public IEnumerator GetEnumerator() => throw null; + IEnumerator IEnumerable.GetEnumerator() => throw null; + } + + public class TestClass + { + public static Data Value => ["foo", "bar"]; + } + """; + + return test.RunAsync(); + } }