Merge pull request #25431 from Neme12/useAutoProperty

 Not offering UseAutoProperty for volatile fields & when field used with ref

src/EditorFeatures/CSharp/UseAutoProperty/CSharpUseAutoPropertyAnalyzer.cs

@@ -14,7 +14,8 @@
 namespace Microsoft.CodeAnalysis.Editor.CSharp.UseAutoProperty
 {
     [Export, DiagnosticAnalyzer(LanguageNames.CSharp)]
-    internal class CSharpUseAutoPropertyAnalyzer : AbstractUseAutoPropertyAnalyzer<PropertyDeclarationSyntax, FieldDeclarationSyntax, VariableDeclaratorSyntax, ExpressionSyntax>
+    internal class CSharpUseAutoPropertyAnalyzer : AbstractUseAutoPropertyAnalyzer<
+        PropertyDeclarationSyntax, FieldDeclarationSyntax, VariableDeclaratorSyntax, ExpressionSyntax>
     {
         protected override bool SupportsReadOnlyProperties(Compilation compilation)
             => ((CSharpCompilation)compilation).LanguageVersion >= LanguageVersion.CSharp6;
@@ -69,50 +70,53 @@ protected override bool CanExplicitInterfaceImplementationsBeFixed()
                                         .Distinct()
                                         .GroupBy(n => n.SyntaxTree);
 
-            foreach (var group in groups)
+            foreach (var (tree, typeDeclarations) in groups)
             {
-                var tree = group.Key;
                 var semanticModel = compilation.GetSemanticModel(tree);
 
-                foreach (var typeDeclaration in group)
+                foreach (var typeDeclaration in typeDeclarations)
                 {
                     foreach (var argument in typeDeclaration.DescendantNodesAndSelf().OfType<ArgumentSyntax>())
                     {
-                        AnalyzeArgument(semanticModel, argument, ineligibleFields, cancellationToken);
+                        // An argument will disqualify a field if that field is used in a ref/out position.  
+                        // We can't change such field references to be property references in C#.
+                        if (argument.RefKindKeyword.Kind() != SyntaxKind.None)
+                        {
+                            AddIneligibleFields(semanticModel, argument.Expression, ineligibleFields, cancellationToken);
+                        }
+                    }
+
+                    foreach (var refExpression in typeDeclaration.DescendantNodesAndSelf().OfType<RefExpressionSyntax>())
+                    {
+                        AddIneligibleFields(semanticModel, refExpression.Expression, ineligibleFields, cancellationToken);
                     }
                 }
             }
         }
 
-        protected override ExpressionSyntax GetFieldInitializer(VariableDeclaratorSyntax variable, CancellationToken cancellationToken)
+        protected override ExpressionSyntax GetFieldInitializer(
+            VariableDeclaratorSyntax variable, CancellationToken cancellationToken)
         {
             return variable.Initializer?.Value;
         }
 
-        private void AnalyzeArgument(
-            SemanticModel semanticModel, ArgumentSyntax argument,
+        private static void AddIneligibleFields(
+            SemanticModel semanticModel, ExpressionSyntax expression,
             HashSet<IFieldSymbol> ineligibleFields, CancellationToken cancellationToken)
         {
-            // An argument will disqualify a field if that field is used in a ref/out position.  
-            // We can't change such field references to be property references in C#.
-            if (argument.RefOrOutKeyword.Kind() == SyntaxKind.None)
-            {
-                return;
-            }
-
-            var symbolInfo = semanticModel.GetSymbolInfo(argument.Expression, cancellationToken);
-            AddIneligibleField(symbolInfo.Symbol, ineligibleFields);
+            var symbolInfo = semanticModel.GetSymbolInfo(expression, cancellationToken);
+            AddIneligibleField(symbolInfo.Symbol);
             foreach (var symbol in symbolInfo.CandidateSymbols)
             {
-                AddIneligibleField(symbol, ineligibleFields);
+                AddIneligibleField(symbol);
             }
-        }
 
-        private static void AddIneligibleField(ISymbol symbol, HashSet<IFieldSymbol> ineligibleFields)
-        {
-            if (symbol is IFieldSymbol field)
+            void AddIneligibleField(ISymbol symbol)
             {
-                ineligibleFields.Add(field);
+                if (symbol is IFieldSymbol field)
+                {
+                    ineligibleFields.Add(field);
+                }
             }
         }
 
@@ -178,7 +182,8 @@ private ExpressionSyntax GetGetterExpressionFromSymbol(IMethodSymbol getMethod,
             return null;
         }
 
-        protected override ExpressionSyntax GetSetterExpression(IMethodSymbol setMethod, SemanticModel semanticModel, CancellationToken cancellationToken)
+        protected override ExpressionSyntax GetSetterExpression(
+            IMethodSymbol setMethod, SemanticModel semanticModel, CancellationToken cancellationToken)
         {
             // Setter has to be of the form:
             //
@@ -205,7 +210,8 @@ private ExpressionSyntax GetExpressionFromSetter(AccessorDeclarationSyntax setAc
             => setAccessor?.ExpressionBody?.Expression ??
                GetSingleStatementFromAccessor<ExpressionStatementSyntax>(setAccessor)?.Expression;
 
-        protected override SyntaxNode GetNodeToFade(FieldDeclarationSyntax fieldDeclaration, VariableDeclaratorSyntax variableDeclarator)
+        protected override SyntaxNode GetNodeToFade(
+            FieldDeclarationSyntax fieldDeclaration, VariableDeclaratorSyntax variableDeclarator)
         {
             return fieldDeclaration.Declaration.Variables.Count == 1
                 ? fieldDeclaration

src/EditorFeatures/CSharpTest/Diagnostics/UseAutoProperty/UseAutoPropertyTests.cs

@@ -365,7 +365,7 @@ int P
         }
 
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseAutoProperty)]
-        public async Task TestFieldUseInRefArgument1()
+        public async Task TestNotIfFieldUsedInRefArgument1()
         {
             await TestMissingInRegularAndScriptAsync(
 @"class Class
@@ -388,7 +388,7 @@ void M(ref int x)
         }
 
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseAutoProperty)]
-        public async Task TestFieldUseInRefArgument2()
+        public async Task TestNotIfFieldUsedInRefArgument2()
         {
             await TestMissingInRegularAndScriptAsync(
 @"class Class
@@ -411,7 +411,7 @@ void M(ref int x)
         }
 
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseAutoProperty)]
-        public async Task TestFieldUseInOutArgument()
+        public async Task TestNotIfFieldUsedInOutArgument()
         {
             await TestMissingInRegularAndScriptAsync(
 @"class Class
@@ -426,13 +426,124 @@ int P
         }
     }
 
-    void M(out x)
+    void M(out int x)
     {
         M(out i);
     }
 }");
         }
 
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseAutoProperty)]
+        public async Task TestNotIfFieldUsedInInArgument()
+        {
+            await TestMissingInRegularAndScriptAsync(
+@"class Class
+{
+    [|int i|];
+
+    int P
+    {
+        get
+        {
+            return i;
+        }
+    }
+
+    void M(in int x)
+    {
+        M(in i);
+    }
+}");
+        }
+
+        [WorkItem(25429, "https://github.com/dotnet/roslyn/issues/25429")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseAutoProperty)]
+        public async Task TestNotIfFieldUsedInRefExpression()
+        {
+            await TestMissingInRegularAndScriptAsync(
+@"class Class
+{
+    [|int i|];
+
+    int P
+    {
+        get
+        {
+            return i;
+        }
+    }
+
+    void M()
+    {
+        ref int x = ref i;
+    }
+}");
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseAutoProperty)]
+        public async Task TestNotIfFieldUsedInRefExpression_AsCandidateSymbol()
+        {
+            await TestMissingInRegularAndScriptAsync(
+@"class Class
+{
+    [|int i|];
+
+    int P
+    {
+        get
+        {
+            return i;
+        }
+    }
+
+    void M()
+    {
+        // because we refer to 'i' statically, it only gets resolved as a candidate symbol
+        // let's be conservative here and disable the analyzer if we're not sure
+        ref int x = ref Class.i;
+    }
+}");
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseAutoProperty)]
+        public async Task TestIfUnrelatedSymbolUsedInRefExpression()
+        {
+            await TestInRegularAndScriptAsync(
+@"class Class
+{
+    [|int i|];
+    int j;
+
+    int P
+    {
+        get
+        {
+            return i;
+        }
+    }
+
+    void M()
+    {
+        int i;
+        ref int x = ref i;
+        ref int y = ref j;
+    }
+}",
+@"class Class
+{
+    int j;
+
+    int P { get; }
+
+    void M()
+    {
+        int i;
+        ref int x = ref i;
+        ref int y = ref j;
+    }
+}");
+        }
+
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseAutoProperty)]
         public async Task TestNotWithVirtualProperty()
         {
@@ -459,6 +570,25 @@ public async Task TestNotWithConstField()
 {
     [|const int i|];
 
+    int P
+    {
+        get
+        {
+            return i;
+        }
+    }
+}");
+        }
+
+        [WorkItem(25379, "https://github.com/dotnet/roslyn/issues/25379")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseAutoProperty)]
+        public async Task TestNotWithVolatileField()
+        {
+            await TestMissingInRegularAndScriptAsync(
+@"class Class
+{
+    [|volatile int i|];
+
     int P
     {
         get

src/Features/Core/Portable/UseAutoProperty/AbstractUseAutoPropertyAnalyzer.cs

@@ -8,15 +8,16 @@
 
 namespace Microsoft.CodeAnalysis.UseAutoProperty
 {
-    internal abstract class AbstractUseAutoPropertyAnalyzer<TPropertyDeclaration, TFieldDeclaration, TVariableDeclarator, TExpression> :
-        AbstractCodeStyleDiagnosticAnalyzer
+    internal abstract class AbstractUseAutoPropertyAnalyzer<
+        TPropertyDeclaration, TFieldDeclaration, TVariableDeclarator, TExpression> : AbstractCodeStyleDiagnosticAnalyzer
         where TPropertyDeclaration : SyntaxNode
         where TFieldDeclaration : SyntaxNode
         where TVariableDeclarator : SyntaxNode
         where TExpression : SyntaxNode
     {
         private static readonly LocalizableString s_title =
-            new LocalizableResourceString(nameof(FeaturesResources.Use_auto_property), FeaturesResources.ResourceManager, typeof(FeaturesResources));
+            new LocalizableResourceString(nameof(FeaturesResources.Use_auto_property),
+                FeaturesResources.ResourceManager, typeof(FeaturesResources));
 
         protected AbstractUseAutoPropertyAnalyzer()
             : base(IDEDiagnosticIds.UseAutoPropertyDiagnosticId, s_title, s_title)
@@ -73,7 +74,8 @@ private void AnalyzeSemanticModel(SemanticModelAnalysisContext context)
             Process(analysisResults, ineligibleFields, context);
         }
 
-        protected void AnalyzeProperty(SemanticModelAnalysisContext context, TPropertyDeclaration propertyDeclaration, List<AnalysisResult> analysisResults)
+        protected void AnalyzeProperty(
+            SemanticModelAnalysisContext context, TPropertyDeclaration propertyDeclaration, List<AnalysisResult> analysisResults)
         {
             var cancellationToken = context.CancellationToken;
             var semanticModel = context.SemanticModel;
@@ -161,8 +163,8 @@ protected void AnalyzeProperty(SemanticModelAnalysisContext context, TPropertyDe
                 return;
             }
 
-            // Don't want to remove constants.
-            if (getterField.IsConst)
+            // Don't want to remove constants and volatile fields.
+            if (getterField.IsConst || getterField.IsVolatile)
             {
                 return;
             }
@@ -222,8 +224,8 @@ protected void AnalyzeProperty(SemanticModelAnalysisContext context, TPropertyDe
             }
 
             // Looks like a viable property/field to convert into an auto property.
-            analysisResults.Add(new AnalysisResult(property, getterField, propertyDeclaration, fieldDeclaration, variableDeclarator,
-                property.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat)));
+            analysisResults.Add(new AnalysisResult(property, getterField, propertyDeclaration,
+                fieldDeclaration, variableDeclarator, property.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat)));
         }
 
         protected virtual bool CanConvert(IPropertySymbol property)
@@ -235,7 +237,8 @@ protected virtual bool CanConvert(IPropertySymbol property)
             return CheckFieldAccessExpression(semanticModel, GetSetterExpression(setMethod, semanticModel, cancellationToken));
         }
 
-        private IFieldSymbol GetGetterField(SemanticModel semanticModel, IMethodSymbol getMethod, CancellationToken cancellationToken)
+        private IFieldSymbol GetGetterField(
+            SemanticModel semanticModel, IMethodSymbol getMethod, CancellationToken cancellationToken)
         {
             return CheckFieldAccessExpression(semanticModel, GetGetterExpression(getMethod, cancellationToken));
         }
@@ -342,7 +345,9 @@ private DiagnosticDescriptor GetFieldDescriptor(CodeStyleOption<bool> styleOptio
             return UnnecessaryWithSuggestionDescriptor;
         }
 
-        protected virtual bool IsEligibleHeuristic(IFieldSymbol field, TPropertyDeclaration propertyDeclaration, Compilation compilation, CancellationToken cancellationToken)
+        protected virtual bool IsEligibleHeuristic(
+            IFieldSymbol field, TPropertyDeclaration propertyDeclaration,
+            Compilation compilation, CancellationToken cancellationToken)
         {
             return true;
         }

src/Workspaces/Core/Portable/Utilities/IGroupingExtensions.cs

+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
+
+using System.Collections.Generic;
+using System.Linq;
+
+namespace Roslyn.Utilities
+{
+    internal static class IGroupingExtensions
+    {
+        public static void Deconstruct<TKey, TElement>(this IGrouping<TKey, TElement> grouping,
+            out TKey key, out IEnumerable<TElement> values)
+        {
+            key = grouping.Key;
+            values = grouping;
+        }
+    }
+}