Fix crash in parameter refactoring

dotnet · Aug 12, 2022 · 934aab9 · 934aab9
1 parent afef833
commit 934aab9
Show file tree

Hide file tree

Showing 11 changed files with 55 additions and 104 deletions.
diff --git a/src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs b/src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs
@@ -1701,6 +1701,31 @@ class C
 
             await VerifyCS.VerifyRefactoringAsync(code, code);
         }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
+        [WorkItem(63307, "https://github.com/dotnet/roslyn/issues/63307")]
+        public async Task TestNotOnIndexerParameterInRecordWithParameter()
+        {
+            var code = @"
+record R(string S)
+{
+    int this[[||]string s]
+    {
+        get
+        {
+            return 0;
+        }
+    }
+}";
+            await new VerifyCS.Test
+            {
+                TestCode = code,
+                FixedCode = code,
+                LanguageVersion = LanguageVersion.CSharp11,
+                ReferenceAssemblies = ReferenceAssemblies.Net.Net50,
+            }.RunAsync();
+        }
+
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
         public async Task TestNotOnIndexerParameters()
         {
@@ -2738,15 +2763,17 @@ class C
             await VerifyCS.VerifyRefactoringAsync(source, source);
         }
 
-        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
         [WorkItem(58779, "https://github.com/dotnet/roslyn/issues/58779")]
-        public async Task TestNotInRecord()
+        [InlineData(LanguageVersion.CSharp10)]
+        [InlineData(LanguageVersion.CSharp11)]
+        public async Task TestNotInRecord(LanguageVersion version)
         {
             var code = @"
 record C([||]string s) { public string s; }";
             await new VerifyCS.Test
             {
-                LanguageVersion = LanguageVersion.CSharp10,
+                LanguageVersion = version,
                 TestCode = code,
                 FixedCode = code,
             }.RunAsync();

diff --git a/...res/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs b/...res/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs
@@ -4,23 +4,14 @@
 
 using System;
 using System.Composition;
-using System.Threading;
-using System.Threading.Tasks;
-using Microsoft.CodeAnalysis.CodeGeneration;
 using Microsoft.CodeAnalysis.CodeRefactorings;
-using Microsoft.CodeAnalysis.CSharp.CodeGeneration;
-using Microsoft.CodeAnalysis.CSharp.CodeStyle;
-using Microsoft.CodeAnalysis.CSharp.Extensions;
 using Microsoft.CodeAnalysis.CSharp.LanguageService;
-using Microsoft.CodeAnalysis.CSharp.Shared.Extensions;
 using Microsoft.CodeAnalysis.CSharp.Simplification;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Editing;
 using Microsoft.CodeAnalysis.Host.Mef;
 using Microsoft.CodeAnalysis.InitializeParameter;
 using Microsoft.CodeAnalysis.LanguageService;
-using Microsoft.CodeAnalysis.Options;
-using Microsoft.CodeAnalysis.Simplification;
 using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory;
 
 namespace Microsoft.CodeAnalysis.CSharp.InitializeParameter
@@ -42,13 +33,6 @@ public CSharpAddParameterCheckCodeRefactoringProvider()
         {
         }
 
-        protected override ISyntaxFacts SyntaxFacts
-            => CSharpSyntaxFacts.Instance;
-
-        // We need to be at least on c# 11 to support using !! with records.
-        protected override bool SupportsRecords(ParseOptions options)
-            => options.LanguageVersion().IsCSharp11OrAbove();
-
         protected override bool IsFunctionDeclaration(SyntaxNode node)
             => InitializeParameterHelpers.IsFunctionDeclaration(node);
 

diff --git a/...ortable/InitializeParameter/CSharpInitializeMemberFromParameterCodeRefactoringProvider.cs b/...ortable/InitializeParameter/CSharpInitializeMemberFromParameterCodeRefactoringProvider.cs
@@ -30,12 +30,6 @@ public CSharpInitializeMemberFromParameterCodeRefactoringProvider()
         {
         }
 
-        protected override ISyntaxFacts SyntaxFacts
-            => CSharpSyntaxFacts.Instance;
-
-        protected override bool SupportsRecords(ParseOptions options)
-            => false;
-
         protected override bool IsFunctionDeclaration(SyntaxNode node)
             => InitializeParameterHelpers.IsFunctionDeclaration(node);
 

diff --git a/...res/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs b/...res/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs
@@ -14,7 +14,6 @@
 using Microsoft.CodeAnalysis.Editing;
 using Microsoft.CodeAnalysis.LanguageService;
 using Microsoft.CodeAnalysis.Operations;
-using Microsoft.CodeAnalysis.Options;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Microsoft.CodeAnalysis.Shared.Collections;
 using Microsoft.CodeAnalysis.Shared.Extensions;
@@ -49,7 +48,7 @@ internal abstract class AbstractAddParameterCheckCodeRefactoringProvider<
 
         protected override async Task<ImmutableArray<CodeAction>> GetRefactoringsForAllParametersAsync(
             Document document,
-            SyntaxNode funcOrRecord,
+            SyntaxNode functionDeclaration,
             IMethodSymbol methodSymbol,
             IBlockOperation? blockStatementOpt,
             ImmutableArray<SyntaxNode> listOfParameterNodes,
@@ -83,7 +82,7 @@ internal abstract class AbstractAddParameterCheckCodeRefactoringProvider<
             Document document,
             TParameterSyntax parameterSyntax,
             IParameterSymbol parameter,
-            SyntaxNode funcOrRecord,
+            SyntaxNode functionDeclaration,
             IMethodSymbol methodSymbol,
             IBlockOperation? blockStatementOpt,
             CleanCodeGenerationOptionsProvider fallbackOptions,
@@ -101,22 +100,21 @@ internal abstract class AbstractAddParameterCheckCodeRefactoringProvider<
             using var result = TemporaryArray<CodeAction>.Empty;
             result.Add(CodeAction.Create(
                 FeaturesResources.Add_null_check,
-                c => AddNullCheckAsync(document, parameter, funcOrRecord, methodSymbol, blockStatementOpt, simplifierOptions, c),
+                c => AddNullCheckAsync(document, parameter, functionDeclaration, methodSymbol, blockStatementOpt, simplifierOptions, c),
                 nameof(FeaturesResources.Add_null_check)));
 
             // Also, if this was a string, offer to add the special checks to string.IsNullOrEmpty and
-            // string.IsNullOrWhitespace.  We cannot do this for records though as they have no location
-            // to place the checks.
-            if (parameter.Type.SpecialType == SpecialType.System_String && !IsRecordDeclaration(funcOrRecord))
+            // string.IsNullOrWhitespace.
+            if (parameter.Type.SpecialType == SpecialType.System_String)
             {
                 result.Add(CodeAction.Create(
                     FeaturesResources.Add_string_IsNullOrEmpty_check,
-                    c => AddStringCheckAsync(document, parameter, funcOrRecord, methodSymbol, blockStatementOpt, nameof(string.IsNullOrEmpty), simplifierOptions, c),
+                    c => AddStringCheckAsync(document, parameter, functionDeclaration, methodSymbol, blockStatementOpt, nameof(string.IsNullOrEmpty), simplifierOptions, c),
                     nameof(FeaturesResources.Add_string_IsNullOrEmpty_check)));
 
                 result.Add(CodeAction.Create(
                     FeaturesResources.Add_string_IsNullOrWhiteSpace_check,
-                    c => AddStringCheckAsync(document, parameter, funcOrRecord, methodSymbol, blockStatementOpt, nameof(string.IsNullOrWhiteSpace), simplifierOptions, c),
+                    c => AddStringCheckAsync(document, parameter, functionDeclaration, methodSymbol, blockStatementOpt, nameof(string.IsNullOrWhiteSpace), simplifierOptions, c),
                     nameof(FeaturesResources.Add_string_IsNullOrWhiteSpace_check)));
             }
 
@@ -139,13 +137,12 @@ internal abstract class AbstractAddParameterCheckCodeRefactoringProvider<
                 var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
 
                 var firstParameterNode = (TParameterSyntax)root.FindNode(parameterSpan);
-                var funcOrRecord = firstParameterNode.FirstAncestorOrSelf(_isFunctionDeclarationFunc) ??
-                                   firstParameterNode.FirstAncestorOrSelf(_isRecordDeclarationFunc);
-                if (funcOrRecord == null)
+                var functionDeclaration = firstParameterNode.FirstAncestorOrSelf<SyntaxNode>(IsFunctionDeclaration);
+                if (functionDeclaration == null)
                     continue;
 
                 var generator = SyntaxGenerator.GetGenerator(document);
-                var parameterNodes = (IReadOnlyList<TParameterSyntax>)generator.GetParameters(funcOrRecord);
+                var parameterNodes = (IReadOnlyList<TParameterSyntax>)generator.GetParameters(functionDeclaration);
                 var semanticModel = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
                 var (parameterSyntax, parameter) = GetParameterAtOrdinal(index, parameterNodes, semanticModel, cancellationToken);
                 if (parameter == null)
@@ -154,21 +151,21 @@ internal abstract class AbstractAddParameterCheckCodeRefactoringProvider<
 
                 var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();
 
-                if (!CanOfferRefactoring(funcOrRecord, semanticModel, syntaxFacts, cancellationToken, out blockStatementOpt))
+                if (!CanOfferRefactoring(functionDeclaration, semanticModel, syntaxFacts, cancellationToken, out blockStatementOpt))
                     continue;
 
                 lazySimplifierOptions ??= (TSimplifierOptions)await document.GetSimplifierOptionsAsync(fallbackOptions, cancellationToken).ConfigureAwait(false);
 
                 // If parameter is a string, default check would be IsNullOrEmpty. This is because IsNullOrEmpty is more
                 // commonly used in this regard according to telemetry and UX testing.
-                if (parameter.Type.SpecialType == SpecialType.System_String && !IsRecordDeclaration(funcOrRecord))
+                if (parameter.Type.SpecialType == SpecialType.System_String)
                 {
-                    document = await AddStringCheckAsync(document, parameter, funcOrRecord, (IMethodSymbol)parameter.ContainingSymbol, blockStatementOpt, nameof(string.IsNullOrEmpty), lazySimplifierOptions, cancellationToken).ConfigureAwait(false);
+                    document = await AddStringCheckAsync(document, parameter, functionDeclaration, (IMethodSymbol)parameter.ContainingSymbol, blockStatementOpt, nameof(string.IsNullOrEmpty), lazySimplifierOptions, cancellationToken).ConfigureAwait(false);
                     continue;
                 }
 
                 // For all other parameters, add null check - updates document
-                document = await AddNullCheckAsync(document, parameter, funcOrRecord,
+                document = await AddNullCheckAsync(document, parameter, functionDeclaration,
                     (IMethodSymbol)parameter.ContainingSymbol, blockStatementOpt, lazySimplifierOptions, cancellationToken).ConfigureAwait(false);
             }
 

diff --git a/...zeParameter/AbstractInitializeMemberFromParameterCodeRefactoringProviderMemberCreation.cs b/...zeParameter/AbstractInitializeMemberFromParameterCodeRefactoringProviderMemberCreation.cs
@@ -16,12 +16,10 @@
 using Microsoft.CodeAnalysis.Editing;
 using Microsoft.CodeAnalysis.Formatting;
 using Microsoft.CodeAnalysis.Operations;
-using Microsoft.CodeAnalysis.Options;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Microsoft.CodeAnalysis.Shared.Extensions;
 using Microsoft.CodeAnalysis.Shared.Naming;
 using Microsoft.CodeAnalysis.Shared.Utilities;
-using Microsoft.CodeAnalysis.Simplification;
 using Microsoft.CodeAnalysis.Text;
 using Roslyn.Utilities;
 
@@ -45,9 +43,6 @@ internal abstract partial class AbstractInitializeMemberFromParameterCodeRefacto
         protected abstract Accessibility DetermineDefaultFieldAccessibility(INamedTypeSymbol containingType);
         protected abstract Accessibility DetermineDefaultPropertyAccessibility();
 
-        protected override bool SupportsRecords(ParseOptions options)
-            => false;
-
         protected override Task<ImmutableArray<CodeAction>> GetRefactoringsForAllParametersAsync(
             Document document, SyntaxNode functionDeclaration, IMethodSymbol method, IBlockOperation? blockStatementOpt,
             ImmutableArray<SyntaxNode> listOfParameterNodes, TextSpan parameterSpan,

diff --git a/...s/Core/Portable/InitializeParameter/AbstractInitializeParameterCodeRefactoringProvider.cs b/...s/Core/Portable/InitializeParameter/AbstractInitializeParameterCodeRefactoringProvider.cs
@@ -16,8 +16,8 @@
 using Microsoft.CodeAnalysis.Operations;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Microsoft.CodeAnalysis.Shared.Extensions;
-using Microsoft.CodeAnalysis.Simplification;
 using Microsoft.CodeAnalysis.Text;
+using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis.InitializeParameter
 {
@@ -31,27 +31,11 @@ internal abstract partial class AbstractInitializeParameterCodeRefactoringProvid
         where TStatementSyntax : SyntaxNode
         where TExpressionSyntax : SyntaxNode
     {
-        protected readonly Func<SyntaxNode, bool> _isFunctionDeclarationFunc;
-        protected readonly Func<SyntaxNode, bool> _isRecordDeclarationFunc;
-
-        protected AbstractInitializeParameterCodeRefactoringProvider()
-        {
-            _isFunctionDeclarationFunc = IsFunctionDeclaration;
-            _isRecordDeclarationFunc = IsRecordDeclaration;
-        }
-
-        protected abstract ISyntaxFacts SyntaxFacts { get; }
-
-        protected abstract bool SupportsRecords(ParseOptions options);
         protected abstract bool IsFunctionDeclaration(SyntaxNode node);
         protected abstract bool IsImplicitConversion(Compilation compilation, ITypeSymbol source, ITypeSymbol destination);
 
         protected abstract SyntaxNode GetBody(SyntaxNode functionDeclaration);
 
-        protected bool IsRecordDeclaration(SyntaxNode node)
-            => this.SyntaxFacts.SyntaxKinds.RecordDeclaration == node.RawKind ||
-               this.SyntaxFacts.SyntaxKinds.RecordStructDeclaration == node.RawKind;
-
         protected abstract Task<ImmutableArray<CodeAction>> GetRefactoringsForAllParametersAsync(
             Document document,
             SyntaxNode functionDeclaration,
@@ -78,7 +62,7 @@ protected bool IsRecordDeclaration(SyntaxNode node)
 
         public override async Task ComputeRefactoringsAsync(CodeRefactoringContext context)
         {
-            var (document, textSpan, cancellationToken) = context;
+            var (document, _, cancellationToken) = context;
 
             // TODO: One could try to retrieve TParameterList and then filter out parameters that intersect with
             // textSpan and use that as `parameterNodes`, where `selectedParameter` would be the first one.
@@ -87,17 +71,12 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte
             if (selectedParameter == null)
                 return;
 
-            var funcOrRecord =
-                selectedParameter.FirstAncestorOrSelf(_isFunctionDeclarationFunc) ??
-                selectedParameter.FirstAncestorOrSelf(_isRecordDeclarationFunc);
-            if (funcOrRecord is null)
-                return;
-
-            if (IsRecordDeclaration(funcOrRecord) && !this.SupportsRecords(funcOrRecord.SyntaxTree.Options))
+            var functionDeclaration = selectedParameter.FirstAncestorOrSelf<SyntaxNode>(IsFunctionDeclaration);
+            if (functionDeclaration is null)
                 return;
 
             var generator = SyntaxGenerator.GetGenerator(document);
-            var parameterNodes = generator.GetParameters(funcOrRecord);
+            var parameterNodes = generator.GetParameters(functionDeclaration);
             var semanticModel = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
             var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();
 
@@ -121,12 +100,12 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte
             if (argumentNullExceptionType is null || semanticModel.Compilation.GetTypeByMetadataName(argumentNullExceptionType) is null)
                 return;
 
-            if (CanOfferRefactoring(funcOrRecord, semanticModel, syntaxFacts, cancellationToken, out var blockStatementOpt))
+            if (CanOfferRefactoring(functionDeclaration, semanticModel, syntaxFacts, cancellationToken, out var blockStatementOpt))
             {
                 // Ok.  Looks like the selected parameter could be refactored. Defer to subclass to 
                 // actually determine if there are any viable refactorings here.
                 var refactorings = await GetRefactoringsForSingleParameterAsync(
-                    document, selectedParameter, parameter, funcOrRecord, methodSymbol, blockStatementOpt, context.Options, cancellationToken).ConfigureAwait(false);
+                    document, selectedParameter, parameter, functionDeclaration, methodSymbol, blockStatementOpt, context.Options, cancellationToken).ConfigureAwait(false);
                 context.RegisterRefactorings(refactorings, context.Span);
             }
 
@@ -146,7 +125,7 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte
                 // Looks like we can offer a refactoring for more than one parameter. Defer to subclass to 
                 // actually determine if there are any viable refactorings here.
                 var refactorings = await GetRefactoringsForAllParametersAsync(
-                    document, funcOrRecord, methodSymbol, blockStatementOpt,
+                    document, functionDeclaration, methodSymbol, blockStatementOpt,
                     listOfPotentiallyValidParametersNodes.ToImmutable(), selectedParameter.Span, context.Options, cancellationToken).ConfigureAwait(false);
                 context.RegisterRefactorings(refactorings, context.Span);
             }
@@ -166,15 +145,12 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte
         }
 
         protected bool CanOfferRefactoring(
-            SyntaxNode funcOrRecord, SemanticModel semanticModel, ISyntaxFactsService syntaxFacts,
+            SyntaxNode functionDeclaration, SemanticModel semanticModel, ISyntaxFactsService syntaxFacts,
             CancellationToken cancellationToken, out IBlockOperation? blockStatementOpt)
         {
             blockStatementOpt = null;
 
-            if (IsRecordDeclaration(funcOrRecord))
-                return true;
-
-            var functionBody = GetBody(funcOrRecord);
+            var functionBody = GetBody(functionDeclaration);
             if (functionBody == null)
             {
                 // We support initializing parameters, even when the containing member doesn't have a
@@ -187,7 +163,7 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte
             // get it via `IAnonymousFunctionOperation.Body` instead of getting it directly from the body syntax.
 
             var operation = semanticModel.GetOperation(
-                syntaxFacts.IsAnonymousFunctionExpression(funcOrRecord) ? funcOrRecord : functionBody,
+                syntaxFacts.IsAnonymousFunctionExpression(functionDeclaration) ? functionDeclaration : functionBody,
                 cancellationToken);
 
             if (operation == null)

diff --git a/...Basic/Portable/InitializeParameter/VisualBasicAddParameterCheckCodeRefactoringProvider.vb b/...Basic/Portable/InitializeParameter/VisualBasicAddParameterCheckCodeRefactoringProvider.vb
@@ -4,14 +4,10 @@
 
 Imports System.Composition
 Imports System.Diagnostics.CodeAnalysis
-Imports System.Threading
-Imports Microsoft.CodeAnalysis.CodeGeneration
 Imports Microsoft.CodeAnalysis.CodeRefactorings
 Imports Microsoft.CodeAnalysis.Editing
 Imports Microsoft.CodeAnalysis.InitializeParameter
 Imports Microsoft.CodeAnalysis.LanguageService
-Imports Microsoft.CodeAnalysis.Options
-Imports Microsoft.CodeAnalysis.Simplification
 Imports Microsoft.CodeAnalysis.VisualBasic.LanguageService
 Imports Microsoft.CodeAnalysis.VisualBasic.Simplification
 Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
@@ -33,12 +29,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.InitializeParameter
         Public Sub New()
         End Sub
 
-        Protected Overrides ReadOnly Property SyntaxFacts As ISyntaxFacts = VisualBasicSyntaxFacts.Instance
-
-        Protected Overrides Function SupportsRecords(options As ParseOptions) As Boolean
-            Return False
-        End Function
-
         Protected Overrides Function IsFunctionDeclaration(node As SyntaxNode) As Boolean
             Return InitializeParameterHelpers.IsFunctionDeclaration(node)
         End Function