Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for adding null checks to records. #59404

Merged
merged 1 commit into from Feb 9, 2022
Merged

Add support for adding null checks to records. #59404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 48 additions & 26 deletions src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs
View file
Open in desktop
Expand Up @@ -2,23 +2,23 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.InitializeParameter;
using Microsoft.CodeAnalysis.CSharp.Shared.Extensions;
using Microsoft.CodeAnalysis.Editor.UnitTests.CodeActions;
using Microsoft.CodeAnalysis.Test.Utilities;
using Microsoft.CodeAnalysis.Testing;
using Roslyn.Test.Utilities;
using Xunit;

using VerifyCS = Microsoft.CodeAnalysis.Editor.UnitTests.CodeActions.CSharpCodeRefactoringVerifier<
Microsoft.CodeAnalysis.CSharp.InitializeParameter.CSharpAddParameterCheckCodeRefactoringProvider>;

namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.InitializeParameter
{
using VerifyCS = CSharpCodeRefactoringVerifier<
CSharpAddParameterCheckCodeRefactoringProvider>;

public class AddParameterCheckTests
{
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
Expand Down Expand Up @@ -76,27 +76,6 @@ public C([||]string s!!)
}.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestRecordPrimaryConstructor()
{
// https://github.com/dotnet/roslyn/issues/58779
// Note: we declare a field within the record to work around missing IsExternalInit errors
await new VerifyCS.Test
{
LanguageVersion = LanguageVersionExtensions.CSharpNext,
TestCode = @"
using System;

record Rec([||]string s) { public string s = s; }
",
FixedCode = @"
using System;

record Rec(string s) { public string s = s; }
"
}.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestSimpleReferenceType_AlreadyNullChecked2()
{
Expand Down Expand Up @@ -2859,5 +2838,48 @@ class C
}";
await VerifyCS.VerifyRefactoringAsync(source, source);
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
[WorkItem(58779, "https://github.com/dotnet/roslyn/issues/58779")]
public async Task TestNotInRecordBeforeCSharp11()
{
var code = @"
record C([||]string s) { public string s; }";
await new VerifyCS.Test
{
LanguageVersion = LanguageVersion.CSharp10,
TestCode = code,
FixedCode = code,
}.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
[WorkItem(58779, "https://github.com/dotnet/roslyn/issues/58779")]
public async Task TestInRecordAfterCSharp11()
{
await new VerifyCS.Test
{
LanguageVersion = LanguageVersionExtensions.CSharpNext,
TestCode = @"
record C([||]string s) { public string s; }",
FixedCode = @"
record C(string s!!) { public string s; }",
}.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
[WorkItem(58779, "https://github.com/dotnet/roslyn/issues/58779")]
public async Task TestInRecordWithMultipleParametersAfterCSharp11()
{
await new VerifyCS.Test
{
LanguageVersion = LanguageVersionExtensions.CSharpNext,
TestCode = @"
record C([||]string s, string t) { public string s, t; }",
FixedCode = @"
record C(string s!!, string t!!) { public string s, t; }",
CodeActionIndex = 1,
}.RunAsync();
}
}
}
19 changes: 12 additions & 7 deletions ...res/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs
View file
Open in desktop
Expand Up @@ -4,17 +4,18 @@

using System;
using System.Composition;
using System.Threading.Tasks;
using System.Threading;
using Microsoft.CodeAnalysis.CodeRefactorings;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.Extensions;
using Microsoft.CodeAnalysis.CSharp.LanguageServices;
using Microsoft.CodeAnalysis.CSharp.Shared.Extensions;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.InitializeParameter;
using Microsoft.CodeAnalysis.LanguageServices;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Shared.Extensions;
using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory;

namespace Microsoft.CodeAnalysis.CSharp.InitializeParameter
Expand All @@ -35,6 +36,13 @@ 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);

Expand Down Expand Up @@ -97,11 +105,8 @@ protected override StatementSyntax CreateParameterCheckIfStatement(DocumentOptio
protected override Document? TryAddNullCheckToParameterDeclaration(Document document, ParameterSyntax parameterSyntax, CancellationToken cancellationToken)
{
var tree = parameterSyntax.SyntaxTree;
var options = (CSharpParseOptions)tree.Options;
if (options.LanguageVersion < LanguageVersionExtensions.CSharpNext)
{
if (!tree.Options.LanguageVersion().IsCSharp11OrAbove())
return null;
}

// We expect the syntax tree to already be in memory since we already have a node from the tree
var syntaxRoot = tree.GetRoot(cancellationToken);
Expand Down
8 changes: 8 additions & 0 deletions ...ortable/InitializeParameter/CSharpInitializeMemberFromParameterCodeRefactoringProvider.cs
View file
Open in desktop
Expand Up @@ -5,9 +5,11 @@
using System.Composition;
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeRefactorings;
using Microsoft.CodeAnalysis.CSharp.LanguageServices;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.InitializeParameter;
using Microsoft.CodeAnalysis.LanguageServices;
using Microsoft.CodeAnalysis.Operations;

namespace Microsoft.CodeAnalysis.CSharp.InitializeParameter
Expand All @@ -28,6 +30,12 @@ public CSharpInitializeMemberFromParameterCodeRefactoringProvider()
{
}

protected override ISyntaxFacts SyntaxFacts
=> CSharpSyntaxFacts.Instance;

protected override bool SupportsRecords(ParseOptions options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand this bit. It seems like a primary constructor parameter is perfectly valid to use to initialize a member. It just has to be in a field initializer. Maybe the refactoring isn't intended for that scenario?

record Rec(int X)
{
    public int X; // can offer `= X;` refactoring here?
}

=> false;

protected override bool IsFunctionDeclaration(SyntaxNode node)
=> InitializeParameterHelpers.IsFunctionDeclaration(node);

Expand Down
62 changes: 28 additions & 34 deletions ...res/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@
using Microsoft.CodeAnalysis.Operations;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Collections;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;
Expand All @@ -37,22 +38,20 @@ internal abstract partial class AbstractAddParameterCheckCodeRefactoringProvider
where TExpressionSyntax : SyntaxNode
where TBinaryExpressionSyntax : TExpressionSyntax
{
private readonly Func<SyntaxNode, bool> _isFunctionDeclarationFunc;

protected AbstractAddParameterCheckCodeRefactoringProvider()
{
_isFunctionDeclarationFunc = IsFunctionDeclaration;
}

protected abstract bool CanOffer(SyntaxNode body);
protected abstract bool PrefersThrowExpression(DocumentOptionSet options);
protected abstract string EscapeResourceString(string input);
protected abstract TStatementSyntax CreateParameterCheckIfStatement(DocumentOptionSet options, TExpressionSyntax condition, TStatementSyntax ifTrueStatement);
protected abstract Document? TryAddNullCheckToParameterDeclaration(Document document, TParameterSyntax parameterSyntax, CancellationToken cancellationToken);

protected override async Task<ImmutableArray<CodeAction>> GetRefactoringsForAllParametersAsync(
Document document, SyntaxNode functionDeclaration, IMethodSymbol methodSymbol,
IBlockOperation? blockStatementOpt, ImmutableArray<SyntaxNode> listOfParameterNodes, TextSpan parameterSpan, CancellationToken cancellationToken)
Document document,
SyntaxNode funcOrRecord,
IMethodSymbol methodSymbol,
IBlockOperation? blockStatementOpt,
ImmutableArray<SyntaxNode> listOfParameterNodes,
TextSpan parameterSpan,
CancellationToken cancellationToken)
{
// List to keep track of the valid parameters
var listOfParametersOrdinals = new List<int>();
Expand All @@ -62,16 +61,12 @@ protected AbstractAddParameterCheckCodeRefactoringProvider()
{
var parameter = (IParameterSymbol)semanticModel.GetRequiredDeclaredSymbol(parameterNode, cancellationToken);
if (ParameterValidForNullCheck(document, parameter, semanticModel, blockStatementOpt, cancellationToken))
{
listOfParametersOrdinals.Add(parameter.Ordinal);
}
}

// Min 2 parameters to offer the refactoring
if (listOfParametersOrdinals.Count < 2)
{
return ImmutableArray<CodeAction>.Empty;
}

// Great. The list has parameters that need null checks. Offer to add null checks for all.
return ImmutableArray.Create<CodeAction>(new MyCodeAction(
Expand All @@ -84,7 +79,7 @@ protected AbstractAddParameterCheckCodeRefactoringProvider()
Document document,
TParameterSyntax parameterSyntax,
IParameterSymbol parameter,
SyntaxNode functionDeclaration,
SyntaxNode funcOrRecord,
IMethodSymbol methodSymbol,
IBlockOperation? blockStatementOpt,
CancellationToken cancellationToken)
Expand All @@ -93,33 +88,32 @@ protected AbstractAddParameterCheckCodeRefactoringProvider()

// Only should provide null-checks for reference types and nullable types.
if (!ParameterValidForNullCheck(document, parameter, semanticModel, blockStatementOpt, cancellationToken))
{
return ImmutableArray<CodeAction>.Empty;
}

// Great. There was no null check. Offer to add one.
using var _ = ArrayBuilder<CodeAction>.GetInstance(out var result);
using var result = TemporaryArray<CodeAction>.Empty;
result.Add(new MyCodeAction(
FeaturesResources.Add_null_check,
c => AddNullCheckAsync(document, parameterSyntax, parameter, functionDeclaration, methodSymbol, blockStatementOpt, c),
c => AddNullCheckAsync(document, parameterSyntax, parameter, funcOrRecord, methodSymbol, blockStatementOpt, c),
nameof(FeaturesResources.Add_null_check)));

// Also, if this was a string, offer to add the special checks to
// string.IsNullOrEmpty and string.IsNullOrWhitespace.
if (parameter.Type.SpecialType == SpecialType.System_String)
// 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))
{
result.Add(new MyCodeAction(
FeaturesResources.Add_string_IsNullOrEmpty_check,
c => AddStringCheckAsync(document, parameter, functionDeclaration, methodSymbol, blockStatementOpt, nameof(string.IsNullOrEmpty), c),
c => AddStringCheckAsync(document, parameter, funcOrRecord, methodSymbol, blockStatementOpt, nameof(string.IsNullOrEmpty), c),
nameof(FeaturesResources.Add_string_IsNullOrEmpty_check)));

result.Add(new MyCodeAction(
FeaturesResources.Add_string_IsNullOrWhiteSpace_check,
c => AddStringCheckAsync(document, parameter, functionDeclaration, methodSymbol, blockStatementOpt, nameof(string.IsNullOrWhiteSpace), c),
c => AddStringCheckAsync(document, parameter, funcOrRecord, methodSymbol, blockStatementOpt, nameof(string.IsNullOrWhiteSpace), c),
nameof(FeaturesResources.Add_string_IsNullOrWhiteSpace_check)));
}

return result.ToImmutable();
return result.ToImmutableAndClear();
}

private async Task<Document> UpdateDocumentForRefactoringAsync(
Expand All @@ -135,12 +129,13 @@ protected AbstractAddParameterCheckCodeRefactoringProvider()
var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);

var firstParameterNode = (TParameterSyntax)root.FindNode(parameterSpan);
var functionDeclaration = firstParameterNode.FirstAncestorOrSelf(_isFunctionDeclarationFunc);
if (functionDeclaration == null)
var funcOrRecord = firstParameterNode.FirstAncestorOrSelf(_isFunctionDeclarationFunc) ??
firstParameterNode.FirstAncestorOrSelf(_isRecordDeclarationFunc);
if (funcOrRecord == null)
continue;

var generator = SyntaxGenerator.GetGenerator(document);
var parameterNodes = (IReadOnlyList<TParameterSyntax>)generator.GetParameters(functionDeclaration);
var parameterNodes = (IReadOnlyList<TParameterSyntax>)generator.GetParameters(funcOrRecord);
var semanticModel = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
var (parameterSyntax, parameter) = GetParameterAtOrdinal(index, parameterNodes, semanticModel, cancellationToken);
if (parameter == null)
Expand All @@ -149,20 +144,19 @@ protected AbstractAddParameterCheckCodeRefactoringProvider()

var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();

if (!CanOfferRefactoring(functionDeclaration, semanticModel, syntaxFacts, cancellationToken, out blockStatementOpt))
{
if (!CanOfferRefactoring(funcOrRecord, semanticModel, syntaxFacts, cancellationToken, out blockStatementOpt))
continue;
}

// 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)
// 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))
{
document = await AddStringCheckAsync(document, parameter, functionDeclaration, (IMethodSymbol)parameter.ContainingSymbol, blockStatementOpt, nameof(string.IsNullOrEmpty), cancellationToken).ConfigureAwait(false);
document = await AddStringCheckAsync(document, parameter, funcOrRecord, (IMethodSymbol)parameter.ContainingSymbol, blockStatementOpt, nameof(string.IsNullOrEmpty), cancellationToken).ConfigureAwait(false);
continue;
}

// For all other parameters, add null check - updates document
document = await AddNullCheckAsync(document, parameterSyntax, parameter, functionDeclaration,
document = await AddNullCheckAsync(document, parameterSyntax, parameter, funcOrRecord,
(IMethodSymbol)parameter.ContainingSymbol, blockStatementOpt, cancellationToken).ConfigureAwait(false);
}

Expand Down
5 changes: 3 additions & 2 deletions ...zeParameter/AbstractInitializeMemberFromParameterCodeRefactoringProviderMemberCreation.cs
View file
Open in desktop
Expand Up @@ -40,11 +40,12 @@ internal abstract partial class AbstractInitializeMemberFromParameterCodeRefacto
where TExpressionSyntax : SyntaxNode
{
protected abstract SyntaxNode? TryGetLastStatement(IBlockOperation? blockStatementOpt);

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, CancellationToken cancellationToken)
Expand Down