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.

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

Use !! in AddParameterCheck refactoring #57934

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,33 @@ public async Task TestEmptyFile()

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestSimpleReferenceType()
{
await new VerifyCS.Test
{
LanguageVersion = LanguageVersion.Preview,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this Preview and not the new CSharpNext? I understand both are equivalent, but CSharpNext should help updating this to CSharp11 when released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an oversight. Thanks for catching it.

TestCode = @"
using System;

class C
{
public C([||]string s)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
}
}",
FixedCode = @"
using System;

class C
{
public C(string s!!)
{
}
}"
}.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestSimpleReferenceType_CSharp8()
{
await VerifyCS.VerifyRefactoringAsync(
@"
Expand Down Expand Up @@ -210,6 +237,28 @@ public async Task TestNotOnPartialMethodDefinition1()
var code = @"
using System;

partial class C
{
partial void M([||]string s);

partial void M(string s)
{
}
}";
await new VerifyCS.Test
{
LanguageVersion = LanguageVersion.Preview,
TestCode = code,
FixedCode = code
}.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestNotOnPartialMethodDefinition1_CSharp8()
{
var code = @"
using System;

partial class C
{
partial void M([||]string s);
Expand Down Expand Up @@ -249,6 +298,28 @@ public async Task TestNotOnPartialMethodDefinition2()
var code = @"
using System;

partial class C
{
partial void M(string s)
{
}

partial void M([||]string s);
}";
await new VerifyCS.Test
{
LanguageVersion = LanguageVersion.Preview,
Copy link
Member

Choose a reason for hiding this comment

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

For the cases where the test is the same other than changing language version, consider using a xUnit theory instead of a fact to avoid all the extra duplication.

TestCode = code,
FixedCode = code
}.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestNotOnPartialMethodDefinition2_CSharp8()
{
var code = @"
using System;

partial class C
{
partial void M(string s)
Expand Down Expand Up @@ -284,6 +355,37 @@ public partial void M(string s)

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestOnPartialMethodImplementation1()
{
await new VerifyCS.Test
{
LanguageVersion = LanguageVersion.Preview,
TestCode = @"
using System;

partial class C
{
partial void M(string s);

partial void M([||]string s)
{
}
}",
FixedCode = @"
using System;

partial class C
{
partial void M(string s);

partial void M(string s!!)
{
}
}"
}.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestOnPartialMethodImplementation1_CSharp8()
{
await VerifyCS.VerifyRefactoringAsync(
@"
Expand Down Expand Up @@ -351,6 +453,37 @@ public partial void M(string s)

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestOnPartialMethodImplementation2()
{
await new VerifyCS.Test
{
LanguageVersion = LanguageVersion.Preview,
TestCode = @"
using System;

partial class C
{
partial void M([||]string s)
{
}

partial void M(string s);
}",
FixedCode = @"
using System;

partial class C
{
partial void M(string s!!)
{
}

partial void M(string s);
}"
}.RunAsync();
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestOnPartialMethodImplementation2_CSharp9()
{
await VerifyCS.VerifyRefactoringAsync(
@"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@
// 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;
using System.Composition;
using System.Diagnostics.CodeAnalysis;
using System.Threading.Tasks;
using System.Threading;
using Microsoft.CodeAnalysis.CodeRefactorings;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
Expand Down Expand Up @@ -92,5 +91,27 @@ protected override StatementSyntax CreateParameterCheckIfStatement(DocumentOptio
statement: ifTrueStatement,
@else: null);
}

protected override async Task<Document?> TryAddNullCheckToParameterDeclarationAsync(Document document, IParameterSymbol parameter, CancellationToken cancellationToken)
{
var tree = await document.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
if (tree is null)
{
return null;
}

var options = (CSharpParseOptions)tree.Options;
if (options.LanguageVersion < LanguageVersion.Preview)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
return null;
}

var syntaxRoot = await tree.GetRootAsync(cancellationToken).ConfigureAwait(false);
var syntax = (ParameterSyntax)await parameter.DeclaringSyntaxReferences[0].GetSyntaxAsync(cancellationToken).ConfigureAwait(false);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
syntaxRoot = syntaxRoot.ReplaceNode(
syntax,
syntax.WithExclamationExclamationToken(Token(SyntaxKind.ExclamationExclamationToken)));
return document.WithSyntaxRoot(syntaxRoot);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ protected AbstractAddParameterCheckCodeRefactoringProvider()
protected abstract bool PrefersThrowExpression(DocumentOptionSet options);
protected abstract string EscapeResourceString(string input);
protected abstract TStatementSyntax CreateParameterCheckIfStatement(DocumentOptionSet options, TExpressionSyntax condition, TStatementSyntax ifTrueStatement);
protected abstract Task<Document?> TryAddNullCheckToParameterDeclarationAsync(
Document document,
IParameterSymbol parameter,
CancellationToken cancellationToken);

protected override async Task<ImmutableArray<CodeAction>> GetRefactoringsForAllParametersAsync(
Document document, SyntaxNode functionDeclaration, IMethodSymbol methodSymbol,
Expand Down Expand Up @@ -318,8 +322,15 @@ private async Task<Document> AddNullCheckAsync(
IBlockOperation? blockStatementOpt,
CancellationToken cancellationToken)
{
// First see if we can convert a statement of the form "this.s = s" into "this.s = s ?? throw ...".
var documentOpt = await TryAddNullCheckToAssignmentAsync(
// First see if we can adopt the '!!' parameter null checking syntax.
var documentOpt = await TryAddNullCheckToParameterDeclarationAsync(document, parameter, cancellationToken).ConfigureAwait(false);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
if (documentOpt != null)
{
return documentOpt;
}

// Then see if we can convert a statement of the form "this.s = s" into "this.s = s ?? throw ...".
documentOpt = await TryAddNullCheckToAssignmentAsync(
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
document, parameter, blockStatementOpt, cancellationToken).ConfigureAwait(false);

if (documentOpt != null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

Imports System.Composition
Imports System.Diagnostics.CodeAnalysis
Imports System.Threading
Imports Microsoft.CodeAnalysis.CodeRefactorings
Imports Microsoft.CodeAnalysis.Editing
Imports Microsoft.CodeAnalysis.InitializeParameter
Expand Down Expand Up @@ -62,5 +63,9 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.InitializeParameter
elseIfBlocks:=Nothing,
elseBlock:=Nothing)
End Function

Protected Overrides Function TryAddNullCheckToParameterDeclarationAsync(document As Document, parameter As IParameterSymbol, cancellationToken As CancellationToken) As Task(Of Document)
Return Task.FromResult(Of Document)(Nothing)
End Function
End Class
End Namespace
22 changes: 22 additions & 0 deletions src/Workspaces/CSharpTest/Formatting/FormattingTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5775,6 +5775,28 @@ static object F()
await AssertFormatAsync(expectedCode, code);
}

[Fact]
[Trait(Traits.Feature, Traits.Features.Formatting)]
public async Task SpacingInNullCheckedParameter()
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to also test/handle the syntax normalize (SyntaxNormalizerTests.cs) which generally needs similar fixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see a similar test in SyntaxNormalizerTests for the ! operator. Didn't we have to do something similar with that for formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't clear to me what needs to be done here.

{
var code =
@"class C
{
static object F(string s !!)
{
}
}";
var expectedCode =
@"class C
{
static object F(string s!!)
{
}
}";

await AssertFormatAsync(expectedCode, code);
}

[Fact]
[WorkItem(545335, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/545335")]
[Trait(Traits.Feature, Traits.Features.Formatting)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,13 @@ SyntaxKind.ImplicitObjectCreationExpression or
return CreateAdjustSpacesOperation(0, AdjustSpacesOption.ForceSpacesIfOnSingleLine);
}

// paramName!!
if (currentToken.Kind() == SyntaxKind.ExclamationExclamationToken &&
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
currentToken.Parent.IsKind(SyntaxKind.Parameter))
{
return CreateAdjustSpacesOperation(0, AdjustSpacesOption.ForceSpacesIfOnSingleLine);
}
Copy link
Member

Choose a reason for hiding this comment

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

Consider also adding a test to SyntacticClassifierTests.cs and SemanticQuickInfoSourceTests.cs. This way, the PR covers all IDE basics (formatting, classifier, quick info, since completion is N/A).


// pointer case for regular pointers
if (currentToken.Kind() == SyntaxKind.AsteriskToken && currentToken.Parent is PointerTypeSyntax)
{
Expand Down