Skip to content

Improve usability of "enable nullable reference types" refactoring #58316

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

Merged
merged 3 commits into from
Dec 15, 2021
Merged

Improve usability of "enable nullable reference types" refactoring #58316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ac435c
Support "Enable nullable reference types" on the entire directive
sharwell Dec 14, 2021
81d83ce
Support "Enable nullable reference types" from restore keyword
sharwell Dec 14, 2021
b885da9
Support "Enable nullable reference types" from disable keyword
sharwell Dec 14, 2021
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
256 changes: 242 additions & 14 deletions src/EditorFeatures/CSharpTest/CodeActions/EnableNullable/EnableNullableTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Globalization;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CSharp;
Expand All @@ -25,8 +26,8 @@ public class EnableNullableTests
var project = solution.GetRequiredProject(projectId);
var document = project.Documents.First();

// Only the input solution contains '#nullable enable'
if (!document.GetTextSynchronously(CancellationToken.None).ToString().Contains("#nullable enable"))
// Only the input solution contains '#nullable enable' or '#nullable enable' in the first document
if (!Regex.IsMatch(document.GetTextSynchronously(CancellationToken.None).ToString(), "#nullable ?enable"))
Copy link
Contributor

@ryzngard ryzngard Dec 14, 2021

Choose a reason for hiding this comment

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

❓My regex fu is limited. Is this matching 0 or 1 spaces before enable? Or is the ? applied to enable somehow?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Dec 14, 2021

Choose a reason for hiding this comment

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

yeah, i don't quite get the need for this. afaict, this is allowing for either one or two spaces after #nullable. Is that really needed?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Dec 14, 2021

Choose a reason for hiding this comment

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

maybe + or \s+ if you want to match any amount of whitespace?

Copy link
Member

@Youssef1313 Youssef1313 Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Spaces between # and nullable are allowed by the compiler (it's a test code anyway, so probably doesn't matter).

Copy link
Contributor Author

@sharwell sharwell Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

afaict, this is allowing for either one or two spaces after #nullable. Is that really needed?

Yes. All of the tests have one space except for one test that has two spaces. I'll update the comment.

Copy link
Contributor Author

@sharwell sharwell Dec 14, 2021

Choose a reason for hiding this comment

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

➡️ Comment is now updated

Copy link
Contributor Author

@sharwell sharwell Dec 14, 2021

Choose a reason for hiding this comment

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

This whole method is a workaround for the fact that the test library can't cleanly represent a code action that changes compilation options. Considering a few options and this will be updated after.

{
var compilationOptions = (CSharpCompilationOptions)solution.GetRequiredProject(projectId).CompilationOptions!;
solution = solution.WithProjectCompilationOptions(projectId, compilationOptions.WithNullableContextOptions(NullableContextOptions.Enable));
Expand All @@ -35,16 +36,43 @@ public class EnableNullableTests
return solution;
};

[Fact]
public async Task EnabledOnNullableEnable()
private static readonly Func<Solution, ProjectId, Solution> s_enableNullableInFixedSolutionFromRestoreKeyword =
(solution, projectId) =>
{
var project = solution.GetRequiredProject(projectId);
var document = project.Documents.First();

// Only the input solution contains '#nullable restore' or '#nullable restore' in the first document
if (!Regex.IsMatch(document.GetTextSynchronously(CancellationToken.None).ToString(), "#nullable ?restore"))
{
var compilationOptions = (CSharpCompilationOptions)solution.GetRequiredProject(projectId).CompilationOptions!;
solution = solution.WithProjectCompilationOptions(projectId, compilationOptions.WithNullableContextOptions(NullableContextOptions.Enable));
}

return solution;
};

private static readonly Func<Solution, ProjectId, Solution> s_enableNullableInFixedSolutionFromDisableKeyword =
s_enableNullableInFixedSolutionFromRestoreKeyword;

[Theory]
[InlineData("$$#nullable enable")]
[InlineData("#$$nullable enable")]
[InlineData("#null$$able enable")]
[InlineData("#nullable$$ enable")]
[InlineData("#nullable $$ enable")]
[InlineData("#nullable $$enable")]
[InlineData("#nullable ena$$ble")]
[InlineData("#nullable enable$$")]
public async Task EnabledOnNullableEnable(string directive)
{
var code1 = @"
#nullable enable$$
var code1 = $@"
{directive}

class Example
{
{{
string? value;
}
}}
";
var code2 = @"
class Example2
Expand Down Expand Up @@ -573,17 +601,217 @@ public async Task DisabledForUnsupportedLanguageVersion(LanguageVersion language
}.RunAsync();
}

[Fact]
public async Task DisabledOnNullableDisable()
[Theory]
[InlineData("$$#nullable restore")]
[InlineData("#$$nullable restore")]
[InlineData("#null$$able restore")]
[InlineData("#nullable$$ restore")]
[InlineData("#nullable $$ restore")]
[InlineData("#nullable $$restore")]
[InlineData("#nullable res$$tore")]
[InlineData("#nullable restore$$")]
public async Task EnabledOnNullableRestore(string directive)
{
var code = @"
#nullable disable$$
var code1 = $@"
{directive}

class Example
{{
string value;
}}
";
var code2 = @"
class Example2
{
string value;
}
";
var code3 = @"
class Example3
{
#nullable enable
string? value;
#nullable restore
}
";
var code4 = @"
#nullable disable

class Example4
{
string value;
}
";

var fixedDirective = directive.Replace("$$", "").Replace("restore", "disable");

var fixedCode1 = $@"
{fixedDirective}

class Example
{{
string value;
}}
";
var fixedCode2 = @"
#nullable disable

class Example2
{
string value;
}
";
var fixedCode3 = @"
#nullable disable

class Example3
{
#nullable restore
string? value;
#nullable disable
}
";
var fixedCode4 = @"
#nullable disable

class Example4
{
string value;
}
";

await new VerifyCS.Test
{
TestCode = code,
FixedCode = code,
TestState =
{
Sources =
{
code1,
code2,
code3,
code4,
},
},
FixedState =
{
Sources =
{
fixedCode1,
fixedCode2,
fixedCode3,
fixedCode4,
},
},
SolutionTransforms = { s_enableNullableInFixedSolutionFromRestoreKeyword },
}.RunAsync();
}

[Theory]
[InlineData("$$#nullable disable")]
[InlineData("#$$nullable disable")]
[InlineData("#null$$able disable")]
[InlineData("#nullable$$ disable")]
[InlineData("#nullable $$ disable")]
[InlineData("#nullable $$disable")]
[InlineData("#nullable dis$$able")]
[InlineData("#nullable disable$$")]
public async Task EnabledOnNullableDisable(string directive)
{
var code1 = $@"
{directive}

class Example
{{
string value;
}}

#nullable restore
";
var code2 = @"
class Example2
{
string value;
}
";
var code3 = @"
class Example3
{
#nullable enable
string? value;
#nullable restore
}
";
var code4 = @"
#nullable disable

class Example4
{
string value;
}
";

var fixedDirective = directive.Replace("$$", "");

var fixedCode1 = $@"
{fixedDirective}

class Example
{{
string value;
}}

#nullable disable
";
var fixedCode2 = @"
#nullable disable

class Example2
{
string value;
}
";
var fixedCode3 = @"
#nullable disable

class Example3
{
#nullable restore
string? value;
#nullable disable
}
";
var fixedCode4 = @"
#nullable disable

class Example4
{
string value;
}
";

await new VerifyCS.Test
{
TestState =
{
Sources =
{
code1,
code2,
code3,
code4,
},
},
FixedState =
{
Sources =
{
fixedCode1,
fixedCode2,
fixedCode3,
fixedCode4,
},
},
SolutionTransforms = { s_enableNullableInFixedSolutionFromDisableKeyword },
}.RunAsync();
}
}
Expand Down
5 changes: 4 additions & 1 deletion .../CSharp/Portable/CodeRefactorings/EnableNullable/EnableNullableCodeRefactoringProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,11 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte
if (token.IsKind(SyntaxKind.EndOfDirectiveToken))
token = root.FindToken(textSpan.Start - 1, findInsideTrivia: true);

if (!token.IsKind(SyntaxKind.EnableKeyword) || !token.Parent.IsKind(SyntaxKind.NullableDirectiveTrivia))
if (!token.IsKind(SyntaxKind.EnableKeyword, SyntaxKind.RestoreKeyword, SyntaxKind.DisableKeyword, SyntaxKind.NullableKeyword, SyntaxKind.HashToken)
|| !token.Parent.IsKind(SyntaxKind.NullableDirectiveTrivia, out NullableDirectiveTriviaSyntax? nullableDirectiveTrivia))
{
return;
}

context.RegisterRefactoring(
new MyCodeAction((purpose, cancellationToken) => EnableNullableReferenceTypesAsync(document.Project, purpose, cancellationToken)));
Expand Down