MA0038: fix the analyzer crash on invalid code and the code fixes that break the code

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`.
+

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<Document> AddStaticModifier(Document document, SyntaxNode nodeToFix, CancellationToken cancellationToken)
+    private static async Task<Solution> 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<int>.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)));
     }
 }

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<MethodDeclarationSyntax>(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)
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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<TSyntax>(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)
             }
         }
 
+        /// <summary>
+        /// A static member cannot be accessed through an instance (CS0176), nor in an object initializer or a property pattern.
+        /// The code fixer replaces <c>this.Member</c> with <c>Type.Member</c>, but the other instances cannot be removed, as
+        /// they can have side effects.
+        /// </summary>
+        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)
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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<IOperation>();
-            operations.Enqueue(operation);
+        private static bool HasInstanceUsages(ImmutableArray<IOperation> operationBlocks)
+        {
+            var operations = new Queue<IOperation>(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;
                 }
             }