Move code in extract method to shared location (#76421)

Author: CyrusNajmabadi

src/Analyzers/CSharp/CodeFixes/UseImplicitOrExplicitType/UseExplicitTypeCodeFixProvider.cs

@@ -14,6 +14,7 @@
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Editing;
+using Microsoft.CodeAnalysis.LanguageService;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Microsoft.CodeAnalysis.Shared.Extensions;
 using Roslyn.Utilities;
@@ -125,33 +126,41 @@ private static Task HandleVariableDeclarationAsync(Document document, SyntaxEdit
             varDecl.Variables.Single().Identifier.Parent!,
             cancellationToken);
 
-    private static async Task UpdateTypeSyntaxAsync(Document document, SyntaxEditor editor, TypeSyntax typeSyntax, SyntaxNode declarationSyntax, CancellationToken cancellationToken)
+    private static async Task UpdateTypeSyntaxAsync(
+        Document document,
+        SyntaxEditor editor,
+        TypeSyntax typeSyntax,
+        SyntaxNode declarationSyntax,
+        CancellationToken cancellationToken)
     {
         typeSyntax = typeSyntax.StripRefIfNeeded();
 
+        var semanticFacts = document.GetRequiredLanguageService<ISemanticFactsService>();
         var semanticModel = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
         var typeSymbol = GetConvertedType(semanticModel, typeSyntax, cancellationToken);
 
         typeSymbol = AdjustNullabilityOfTypeSymbol(
-            typeSymbol,
+            semanticFacts,
             semanticModel,
+            typeSymbol,
             declarationSyntax,
             cancellationToken);
 
         editor.ReplaceNode(typeSyntax, GenerateTypeDeclaration(typeSyntax, typeSymbol));
     }
 
     private static ITypeSymbol AdjustNullabilityOfTypeSymbol(
-        ITypeSymbol typeSymbol,
+        ISemanticFacts semanticFacts,
         SemanticModel semanticModel,
+        ITypeSymbol typeSymbol,
         SyntaxNode declarationSyntax,
         CancellationToken cancellationToken)
     {
         if (typeSymbol.NullableAnnotation == NullableAnnotation.Annotated)
         {
             // It's possible that the var shouldn't be annotated nullable, check assignments to the variable and 
             // determine if it needs to be null
-            var isPossiblyAssignedNull = NullableHelpers.IsDeclaredSymbolAssignedPossiblyNullValue(semanticModel, declarationSyntax, cancellationToken);
+            var isPossiblyAssignedNull = NullableHelpers.IsDeclaredSymbolAssignedPossiblyNullValue(semanticFacts, semanticModel, declarationSyntax, cancellationToken);
             if (!isPossiblyAssignedNull)
             {
                 // If the symbol is never assigned null we can update the type symbol to also be non-null

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

@@ -54,21 +54,6 @@ protected override bool ReadOnlyFieldAllowed()
             return scope == null;
         }
 
-        protected override ITypeSymbol? GetSymbolType(SemanticModel semanticModel, ISymbol symbol)
-        {
-            var selectionOperation = semanticModel.GetOperation(SelectionResult.GetContainingScope());
-
-            // Check if null is possibly assigned to the symbol. If it is, leave nullable annotation as is, otherwise
-            // we can modify the annotation to be NotAnnotated to code that more likely matches the user's intent.
-            if (selectionOperation is not null &&
-                NullableHelpers.IsSymbolAssignedPossiblyNullValue(semanticModel, selectionOperation, symbol) == false)
-            {
-                return base.GetSymbolType(semanticModel, symbol)?.WithNullableAnnotation(NullableAnnotation.NotAnnotated);
-            }
-
-            return base.GetSymbolType(semanticModel, symbol);
-        }
-
         protected override bool IsReadOutside(ISymbol symbol, HashSet<ISymbol> readOutsideMap)
         {
             if (!base.IsReadOutside(symbol, readOutsideMap))

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

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System;
 using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.Linq;
@@ -11,6 +12,7 @@
 using Microsoft.CodeAnalysis.PooledObjects;
 using Microsoft.CodeAnalysis.Shared.Collections;
 using Microsoft.CodeAnalysis.Shared.Extensions;
+using Microsoft.CodeAnalysis.Text;
 using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis.ExtractMethod;
@@ -25,6 +27,7 @@ protected abstract partial class Analyzer
         protected readonly TSelectionResult SelectionResult;
         protected readonly bool LocalFunction;
 
+        protected ISemanticFactsService SemanticFacts => _semanticDocument.Document.GetRequiredLanguageService<ISemanticFactsService>();
         protected ISyntaxFactsService SyntaxFacts => _semanticDocument.Document.GetRequiredLanguageService<ISyntaxFactsService>();
 
         protected Analyzer(TSelectionResult selectionResult, bool localFunction, CancellationToken cancellationToken)
@@ -532,6 +535,10 @@ private void GenerateVariableInfoMap(
             candidates.UnionWith(writtenInsideMap);
             candidates.UnionWith(variableDeclaredMap);
 
+            // Need to analyze from the start of what we're extracting to the end of the scope that this variable could
+            // have been referenced in.
+            var analysisRange = TextSpan.FromBounds(SelectionResult.FinalSpan.Start, SelectionResult.GetContainingScope().Span.End);
+
             foreach (var symbol in candidates)
             {
                 // We don't care about the 'this' parameter.  It will be available to an extracted method already.
@@ -585,7 +592,7 @@ private void GenerateVariableInfoMap(
                     continue;
                 }
 
-                var type = GetSymbolType(model, symbol);
+                var type = GetSymbolType(model, symbol, analysisRange);
                 if (type == null)
                 {
                     continue;
@@ -608,60 +615,11 @@ private void GenerateVariableInfoMap(
                     continue;
                 }
 
-                type = FixNullability(symbol, type, variableStyle);
-
                 AddVariableToMap(
                     variableInfoMap,
                     symbol,
                     CreateFromSymbol(symbol, type, variableStyle, variableDeclared));
             }
-
-            ITypeSymbol FixNullability(ISymbol symbol, ITypeSymbol type, VariableStyle style)
-            {
-                if (style.ParameterStyle.ParameterBehavior != ParameterBehavior.None &&
-                    type.NullableAnnotation is NullableAnnotation.Annotated &&
-                    !IsMaybeNullWithinSelection(symbol))
-                {
-                    // We're going to pass this nullable variable in as a parameter to the new method we're creating. If the
-                    // variable is actually never null within the section we're extracting, then change its return type to
-                    // be non-nullable so that any usage of it within the new method will not warn.
-                    return type.WithNullableAnnotation(NullableAnnotation.NotAnnotated);
-                }
-
-                return type;
-            }
-
-            bool IsMaybeNullWithinSelection(ISymbol symbol)
-            {
-                if (!symbolMap.TryGetValue(symbol, out var tokens))
-                    return true;
-
-                var syntaxFacts = this.SyntaxFacts;
-                foreach (var token in tokens)
-                {
-                    if (token.Parent is not TExpressionSyntax expression)
-                        continue;
-
-                    // a = b;
-                    //
-                    // Need to ask what the nullability of 'b' is since 'a' may be currently non-null but may be
-                    // assigned a null value.
-                    if (syntaxFacts.IsLeftSideOfAssignment(expression))
-                    {
-                        syntaxFacts.GetPartsOfAssignmentExpressionOrStatement(expression.GetRequiredParent(), out _, out _, out var right);
-                        expression = (TExpressionSyntax)right;
-                    }
-
-                    var typeInfo = model.GetTypeInfo(expression, this.CancellationToken);
-                    if (typeInfo.Nullability.FlowState == NullableFlowState.MaybeNull ||
-                        typeInfo.ConvertedNullability.FlowState == NullableFlowState.MaybeNull)
-                    {
-                        return true;
-                    }
-                }
-
-                return false;
-            }
         }
 
         private static void AddVariableToMap(IDictionary<ISymbol, VariableInfo> variableInfoMap, ISymbol localOrParameter, VariableInfo variableInfo)
@@ -731,7 +689,7 @@ private bool IsWrittenInsideForFrameworkValueType(
             if (!symbolMap.TryGetValue(symbol, out var tokens))
                 return writtenInside;
 
-            var semanticFacts = _semanticDocument.Document.Project.Services.GetRequiredService<ISemanticFactsService>();
+            var semanticFacts = this.SemanticFacts;
             return tokens.Any(t => semanticFacts.IsWrittenTo(model, t.Parent, this.CancellationToken));
         }
 
@@ -753,15 +711,44 @@ private bool SelectionContainsOnlyIdentifierWithSameType(ITypeSymbol type)
             return type.Equals(SelectionResult.GetContainingScopeType());
         }
 
-        protected virtual ITypeSymbol? GetSymbolType(SemanticModel model, ISymbol symbol)
-            => symbol switch
+        private ITypeSymbol? GetSymbolType(
+            SemanticModel semanticModel,
+            ISymbol symbol,
+            TextSpan analysisRange)
+        {
+            var type = symbol switch
             {
                 ILocalSymbol local => local.Type,
                 IParameterSymbol parameter => parameter.Type,
-                IRangeVariableSymbol rangeVariable => GetRangeVariableType(model, rangeVariable),
+                IRangeVariableSymbol rangeVariable => GetRangeVariableType(semanticModel, rangeVariable),
                 _ => throw ExceptionUtilities.UnexpectedValue(symbol)
             };
 
+            if (type is null)
+                return type;
+
+            // Check if null is possibly assigned to the symbol. If it is, leave nullable annotation as is, otherwise we
+            // can modify the annotation to be NotAnnotated to code that more likely matches the user's intent.
+
+            if (type.NullableAnnotation is not NullableAnnotation.Annotated)
+                return type;
+
+            var selectionOperation = semanticModel.GetOperation(SelectionResult.GetContainingScope());
+
+            // For Extract-Method we don't care about analyzing the declaration of this variable. For example, even if
+            // it was initially assigned 'null' for the purposes of determining the type of it for a return value, all
+            // we care is if it is null at the end of the selection.  If it is only assigned non-null values, for
+            // example, we want to treat it as non-null.
+            if (selectionOperation is not null &&
+                NullableHelpers.IsSymbolAssignedPossiblyNullValue(
+                    this.SemanticFacts, semanticModel, selectionOperation, symbol, analysisRange, includeDeclaration: false, this.CancellationToken) == false)
+            {
+                return type.WithNullableAnnotation(NullableAnnotation.NotAnnotated);
+            }
+
+            return type;
+        }
+
         protected static VariableStyle AlwaysReturn(VariableStyle style)
         {
             if (style == VariableStyle.InputOnly)
@@ -1021,7 +1008,7 @@ private OperationStatus CheckReadOnlyFields(SemanticModel semanticModel, Diction
                 return OperationStatus.SucceededStatus;
 
             using var _ = ArrayBuilder<string>.GetInstance(out var names);
-            var semanticFacts = _semanticDocument.Document.Project.Services.GetRequiredService<ISemanticFactsService>();
+            var semanticFacts = this.SemanticFacts;
             foreach (var pair in symbolMap.Where(p => p.Key.Kind == SymbolKind.Field))
             {
                 var field = (IFieldSymbol)pair.Key;

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs

@@ -364,7 +364,7 @@ public static bool IsWrittenTo(
             return true;
 
         // An extension method invocation with a ref-this parameter can write to an expression.
-        if (expression.Parent is MemberAccessExpressionSyntax memberAccess &&
+        if (expression.Parent is MemberAccessExpressionSyntax { Parent: InvocationExpressionSyntax } memberAccess &&
             expression == memberAccess.Expression)
         {
             var symbol = semanticModel.GetSymbolInfo(memberAccess, cancellationToken).Symbol;

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxKinds.cs

@@ -114,6 +114,7 @@ public int Convert<TSyntaxKind>(TSyntaxKind kind) where TSyntaxKind : struct
     public int? RefExpression => (int)SyntaxKind.RefExpression;
     public int ReferenceEqualsExpression => (int)SyntaxKind.EqualsExpression;
     public int ReferenceNotEqualsExpression => (int)SyntaxKind.NotEqualsExpression;
+    public int SimpleAssignmentExpression => (int)SyntaxKind.SimpleAssignmentExpression;
     public int SimpleMemberAccessExpression => (int)SyntaxKind.SimpleMemberAccessExpression;
     public int? SuppressNullableWarningExpression => (int)SyntaxKind.SuppressNullableWarningExpression;
     public int TernaryConditionalExpression => (int)SyntaxKind.ConditionalExpression;

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Services/SyntaxFacts/ISyntaxKinds.cs

@@ -157,6 +157,7 @@ internal interface ISyntaxKinds
     int? RefExpression { get; }
     int ReferenceEqualsExpression { get; }
     int ReferenceNotEqualsExpression { get; }
+    int SimpleAssignmentExpression { get; }
     int SimpleMemberAccessExpression { get; }
     int? SuppressNullableWarningExpression { get; }
     int TernaryConditionalExpression { get; }

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxKinds.vb

@@ -116,6 +116,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.LanguageService
         Public ReadOnly Property RefExpression As Integer? Implements ISyntaxKinds.RefExpression
         Public ReadOnly Property ReferenceEqualsExpression As Integer = SyntaxKind.IsExpression Implements ISyntaxKinds.ReferenceEqualsExpression
         Public ReadOnly Property ReferenceNotEqualsExpression As Integer = SyntaxKind.IsNotExpression Implements ISyntaxKinds.ReferenceNotEqualsExpression
+        Public ReadOnly Property SimpleAssignmentExpression As Integer = SyntaxKind.SimpleAssignmentStatement Implements ISyntaxKinds.SimpleAssignmentExpression
         Public ReadOnly Property SimpleMemberAccessExpression As Integer = SyntaxKind.SimpleMemberAccessExpression Implements ISyntaxKinds.SimpleMemberAccessExpression
         Public ReadOnly Property SuppressNullableWarningExpression As Integer? Implements ISyntaxKinds.SuppressNullableWarningExpression
         Public ReadOnly Property TernaryConditionalExpression As Integer = SyntaxKind.TernaryConditionalExpression Implements ISyntaxKinds.TernaryConditionalExpression

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CSharpWorkspaceExtensions.projitems

@@ -90,7 +90,6 @@
     <Compile Include="$(MSBuildThisFileDirectory)Utilities\CSharpSimplificationHelpers.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Utilities\NullableHelpers\NullableExtensions.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Utilities\SyntaxKindSet.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)Utilities\NullableHelpers\NullableHelpers.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Formatting\CSharpSyntaxFormattingOptionsProviders.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Simplification\CSharpSimplifierOptionsProviders.cs" />
   </ItemGroup>