Extract readonly method

src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs

@@ -1136,18 +1136,32 @@ public override BoundNode VisitCall(BoundCall node)
 
         private void VisitReceiverBeforeCall(BoundExpression receiverOpt, MethodSymbol method)
         {
-            if ((object)method == null || method.MethodKind != MethodKind.Constructor)
+            if (method is null || method.MethodKind != MethodKind.Constructor)
             {
                 VisitRvalue(receiverOpt);
             }
         }
 
         private void VisitReceiverAfterCall(BoundExpression receiverOpt, MethodSymbol method)
         {
-            NamedTypeSymbol containingType;
-            if (receiverOpt != null && ((object)method == null || method.MethodKind == MethodKind.Constructor || (object)(containingType = method.ContainingType) != null && method.RequiresInstanceReceiver && !containingType.IsReferenceType && !TypeIsImmutable(containingType)))
+            if (receiverOpt is null)
             {
-                WriteArgument(receiverOpt, method?.MethodKind == MethodKind.Constructor ? RefKind.Out : RefKind.Ref, method);
+                return;
+            }
+
+            if (method is null)
+            {
+                WriteArgument(receiverOpt, RefKind.Ref, method: null);
+            }
+            else if (method.TryGetThisParameter(out var thisParameter)
+                && thisParameter is object
+                && !TypeIsImmutable(thisParameter.Type))
+            {
+                var thisRefKind = thisParameter.RefKind;
+                if (thisRefKind.IsWritableReference())
+                {
+                    WriteArgument(receiverOpt, thisRefKind, method);
+                }
             }
         }
 

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs

@@ -2092,7 +2092,11 @@ .maxstack  1
   IL_000b:  ret
 }");
 
-            comp.VerifyDiagnostics();
+            comp.VerifyDiagnostics(
+                // (9,8): warning CS0649: Field 'S2.s1' is never assigned to, and will always have its default value
+                //     S1 s1;
+                Diagnostic(ErrorCode.WRN_UnassignedInternalField, "s1").WithArguments("S2.s1", "").WithLocation(9, 8)
+);
         }
 
         [Fact]

src/Compilers/CSharp/Test/Semantic/FlowAnalysis/FlowDiagnosticTests.cs

@@ -1339,13 +1339,15 @@ static void Main(string[] args)
     // (16,9): error CS0170: Use of possibly unassigned field 'i'
     //         x.i = 1;
     Diagnostic(ErrorCode.ERR_UseDefViolationField, "x.i").WithArguments("i").WithLocation(16, 9),
-    // (17,34): error CS8079: Use of automatically implemented property 'x2' whose backing field is possibly unassigned 
+    // (17,34): error CS8079: Use of possibly unassigned auto-implemented property 'x2'
     //         System.Console.WriteLine(x2.ii);
     Diagnostic(ErrorCode.ERR_UseDefViolationProperty, "x2").WithArguments("x2").WithLocation(17, 34),
-    // (14,12): error CS0843: Backing field for automatically implemented property 'Program.x' must be fully assigned before control is returned to the caller. Consider calling the default constructor from a constructor initializer.
-    //     public Program()
-    Diagnostic(ErrorCode.ERR_UnassignedThisAutoProperty, "Program").WithArguments("Program.x").WithLocation(14, 12)
-    );
+    // (14,12): error CS0843: Auto-implemented property 'Program.x' must be fully assigned before control is returned to the caller.
+    //     public Program(int dummy)
+    Diagnostic(ErrorCode.ERR_UnassignedThisAutoProperty, "Program").WithArguments("Program.x").WithLocation(14, 12),
+    // (14,12): error CS0843: Auto-implemented property 'Program.x2' must be fully assigned before control is returned to the caller.
+    //     public Program(int dummy)
+    Diagnostic(ErrorCode.ERR_UnassignedThisAutoProperty, "Program").WithArguments("Program.x2").WithLocation(14, 12));
         }
 
         [Fact]
@@ -1388,13 +1390,15 @@ static void Main(string[] args)
     // (16,9): error CS0170: Use of possibly unassigned field 'i'
     //         x.i = 1;
     Diagnostic(ErrorCode.ERR_UseDefViolationField, "x.i").WithArguments("i").WithLocation(16, 9),
-    // (17,34): error CS8079: Use of automatically implemented property 'x2' whose backing field is possibly unassigned 
+    // (17,34): error CS8079: Use of possibly unassigned auto-implemented property 'x2'
     //         System.Console.WriteLine(x2.ii);
     Diagnostic(ErrorCode.ERR_UseDefViolationProperty, "x2").WithArguments("x2").WithLocation(17, 34),
-    // (14,12): error CS0843: Backing field for automatically implemented property 'Program.x' must be fully assigned before control is returned to the caller. Consider calling the default constructor from a constructor initializer.
-    //     public Program()
-    Diagnostic(ErrorCode.ERR_UnassignedThisAutoProperty, "Program").WithArguments("Program.x").WithLocation(14, 12)
-    );
+    // (14,12): error CS0843: Auto-implemented property 'Program.x' must be fully assigned before control is returned to the caller.
+    //     public Program(int dummy)
+    Diagnostic(ErrorCode.ERR_UnassignedThisAutoProperty, "Program").WithArguments("Program.x").WithLocation(14, 12),
+    // (14,12): error CS0843: Auto-implemented property 'Program.x2' must be fully assigned before control is returned to the caller.
+    //     public Program(int dummy)
+    Diagnostic(ErrorCode.ERR_UnassignedThisAutoProperty, "Program").WithArguments("Program.x2").WithLocation(14, 12));
         }
 
         [Fact]

src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs

@@ -127,7 +127,7 @@ static void Main(string[] args)
             Assert.Equal(null, GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsOut));
             Assert.Equal("i, s", GetSymbolNamesJoined(dataFlowAnalysisResults.ReadInside));
             Assert.Equal(null, GetSymbolNamesJoined(dataFlowAnalysisResults.ReadOutside));
-            Assert.Equal("s", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenInside));
+            Assert.Equal(null, GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenInside));
             Assert.Equal("args, i, s", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenOutside));
             Assert.Equal(null, GetSymbolNamesJoined(dataFlowAnalysisResults.Captured));
             Assert.Equal(null, GetSymbolNamesJoined(dataFlowAnalysisResults.CapturedInside));

src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs

@@ -2094,6 +2094,87 @@ private static System.Range NewMethod()
     {
         return 1..2;
     }
+}");
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractMethod)]
+        public async Task TestExtractReadOnlyMethod()
+        {
+            await TestInRegularAndScriptAsync(
+@"struct S1
+{
+    readonly int M1() => 42;
+    void Main()
+    {
+        [|int i = M1() + M1()|];
+    }
+}",
+@"struct S1
+{
+    readonly int M1() => 42;
+    void Main()
+    {
+        {|Rename:NewMethod|}();
+    }
+
+    private readonly void NewMethod()
+    {
+        int i = M1() + M1();
+    }
+}");
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractMethod)]
+        public async Task TestExtractReadOnlyMethodInReadOnlyStruct()
+        {
+            await TestInRegularAndScriptAsync(
+@"readonly struct S1
+{
+    int M1() => 42;
+    void Main()
+    {
+        [|int i = M1() + M1()|];
+    }
+}",
+@"readonly struct S1
+{
+    int M1() => 42;
+    void Main()
+    {
+        {|Rename:NewMethod|}();
+    }
+
+    private void NewMethod()
+    {
+        int i = M1() + M1();
+    }
+}");
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractMethod)]
+        public async Task TestExtractNonReadOnlyMethodInReadOnlyMethod()
+        {
+            await TestInRegularAndScriptAsync(
+@"struct S1
+{
+    int M1() => 42;
+    readonly void Main()
+    {
+        [|int i = M1() + M1()|];
+    }
+}",
+@"struct S1
+{
+    int M1() => 42;
+    readonly void Main()
+    {
+        {|Rename:NewMethod|}();
+    }
+
+    private void NewMethod()
+    {
+        int i = M1() + M1();
+    }
 }");
         }
     }

src/EditorFeatures/CSharpTest/ConvertForEachToFor/ConvertForEachToForTests.cs

@@ -606,7 +606,7 @@ void Method()
         var array = new[] { new Struct() };
         for (int {|Rename:i|} = 0; i < array.Length; i++)
         {
-            {|Warning:Struct a = array[i];|}
+            Struct a = array[i];
             _ = a.Property;
         }
     }
@@ -649,7 +649,7 @@ void Method()
         var array = new[] { new Struct() };
         for (int {|Rename:i|} = 0; i < array.Length; i++)
         {
-            {|Warning:Struct a = array[i];|}
+            Struct a = array[i];
             var b = a.Property;
         }
     }

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs

@@ -202,11 +202,14 @@ private DeclarationModifiers CreateMethodModifiers()
             {
                 var isUnsafe = this.CSharpSelectionResult.ShouldPutUnsafeModifier();
                 var isAsync = this.CSharpSelectionResult.ShouldPutAsyncModifier();
+                var isStatic = !this.AnalyzerResult.UseInstanceMember;
+                var isReadOnly = this.AnalyzerResult.ShouldBeReadOnly;
 
                 return new DeclarationModifiers(
                     isUnsafe: isUnsafe,
                     isAsync: isAsync,
-                    isStatic: !this.AnalyzerResult.UseInstanceMember);
+                    isStatic: isStatic,
+                    isReadOnly: isReadOnly);
             }
 
             private static SyntaxKind GetParameterRefSyntaxKind(ParameterBehavior parameterBehavior)

src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs

@@ -86,8 +86,13 @@ public async Task<AnalyzerResult> AnalyzeAsync()
                     Contract.ThrowIfFalse(unused.Count == 0);
                 }
 
-                // check whether instance member is used inside of the selection
-                var instanceMemberIsUsed = IsInstanceMemberUsedInSelectedCode(dataFlowAnalysisData);
+                var thisParameterBeingRead = (IParameterSymbol)dataFlowAnalysisData.ReadInside.FirstOrDefault(s => IsThisParameter(s));
+                var isThisParameterWritten = dataFlowAnalysisData.WrittenInside.Any(s => IsThisParameter(s));
+
+                var instanceMemberIsUsed = thisParameterBeingRead != null || isThisParameterWritten;
+                var shouldBeReadOnly = !isThisParameterWritten
+                    && thisParameterBeingRead != null
+                    && thisParameterBeingRead.Type is { TypeKind: TypeKind.Struct, IsReadOnly: false };
 
                 // check whether end of selection is reachable
                 var endOfSelectionReachable = IsEndOfSelectionReachable(model);
@@ -118,9 +123,16 @@ public async Task<AnalyzerResult> AnalyzeAsync()
 
                 return new AnalyzerResult(
                     newDocument,
-                    typeParametersInDeclaration, typeParametersInConstraintList,
-                    parameters, variableToUseAsReturnValue, returnType, awaitTaskReturn,
-                    instanceMemberIsUsed, endOfSelectionReachable, operationStatus);
+                    typeParametersInDeclaration,
+                    typeParametersInConstraintList,
+                    parameters,
+                    variableToUseAsReturnValue,
+                    returnType,
+                    awaitTaskReturn,
+                    instanceMemberIsUsed,
+                    shouldBeReadOnly,
+                    endOfSelectionReachable,
+                    operationStatus);
             }
 
             private Tuple<ITypeSymbol, bool, bool> AdjustReturnType(SemanticModel model, ITypeSymbol returnType)
@@ -668,7 +680,7 @@ private bool IsParameterAssigned(ISymbol localOrParameter)
                 return parameter.RefKind != RefKind.Out;
             }
 
-            private bool IsThisParameter(ISymbol localOrParameter)
+            private static bool IsThisParameter(ISymbol localOrParameter)
             {
                 var parameter = localOrParameter as IParameterSymbol;
                 if (parameter == null)
@@ -939,15 +951,6 @@ private OperationStatus CheckReadOnlyFields(SemanticModel semanticModel, Diction
                 return OperationStatus.Succeeded;
             }
 
-            private bool IsInstanceMemberUsedInSelectedCode(DataFlowAnalysis dataFlowAnalysisData)
-            {
-                Contract.ThrowIfNull(dataFlowAnalysisData);
-
-                // "this" can be used as a lvalue in a struct, check WrittenInside as well
-                return dataFlowAnalysisData.ReadInside.Any(s => IsThisParameter(s)) ||
-                       dataFlowAnalysisData.WrittenInside.Any(s => IsThisParameter(s));
-            }
-
             protected VariableInfo CreateFromSymbolCommon<T>(
                 Compilation compilation,
                 ISymbol symbol,

src/Features/Core/Portable/ExtractMethod/MethodExtractor.AnalyzerResult.cs

@@ -26,12 +26,14 @@ public AnalyzerResult(
                 ITypeSymbol returnType,
                 bool awaitTaskReturn,
                 bool instanceMemberIsUsed,
+                bool shouldBeReadOnly,
                 bool endOfSelectionReachable,
                 OperationStatus status)
             {
                 var semanticModel = document.SemanticModel;
 
                 UseInstanceMember = instanceMemberIsUsed;
+                ShouldBeReadOnly = shouldBeReadOnly;
                 EndOfSelectionReachable = endOfSelectionReachable;
                 AwaitTaskReturn = awaitTaskReturn;
                 SemanticDocument = document;
@@ -59,6 +61,7 @@ public AnalyzerResult With(SemanticDocument document)
                     ReturnType,
                     AwaitTaskReturn,
                     UseInstanceMember,
+                    ShouldBeReadOnly,
                     EndOfSelectionReachable,
                     Status);
             }
@@ -68,6 +71,11 @@ public AnalyzerResult With(SemanticDocument document)
             /// </summary>
             public bool UseInstanceMember { get; }
 
+            /// <summary>
+            /// Indicates whether the extracted method should have a 'readonly' modifier.
+            /// </summary>
+            public bool ShouldBeReadOnly { get; }
+
             /// <summary>
             /// used to determine whether "return" statement needs to be inserted
             /// </summary>