Use !! in AddParameterCheck refactoring #57934

RikkiGibson · 2021-11-23T01:06:06Z

Related to #36024

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs

jcouv · 2021-11-23T04:40:51Z

src/Workspaces/CSharpTest/Formatting/FormattingTests.cs

@@ -5775,6 +5775,28 @@ static object F()
            await AssertFormatAsync(expectedCode, code);
        }

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


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

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

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

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs

jcouv · 2021-11-23T04:45:47Z

...es/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/TokenBasedFormattingRule.cs

+                currentToken.Parent.IsKind(SyntaxKind.Parameter))
+            {
+                return CreateAdjustSpacesOperation(0, AdjustSpacesOption.ForceSpacesIfOnSingleLine);
+            }


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).

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs

...es/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/TokenBasedFormattingRule.cs

...atures/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs

jasonmalinowski · 2021-11-23T19:47:47Z

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

+}";
+            await new VerifyCS.Test
+            {
+                LanguageVersion = LanguageVersion.Preview,


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.

jasonmalinowski

Some comments from me, but wait until @CyrusNajmabadi signs off.

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs

...atures/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs

jcouv

Compiler-side (comments-only change) LGTM Thanks (iteration 4)

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs

CyrusNajmabadi · 2021-11-30T00:53:13Z

...atures/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs

        {
            foreach (var parameterNode in parameterNodes)
            {
                var parameter = (IParameterSymbol)semanticModel.GetRequiredDeclaredSymbol(parameterNode, cancellationToken);
                if (index == parameter.Ordinal)
                {
-                    return parameter;
+                    return ((TParameterSyntax)parameterNode, parameter);


this worries me. were we blindly casting before?

I think we were not casting the items in parameterNodes before this PR.

It appears parameterNodes contains a SeparatedSyntaxList<ParameterNode> which was converted to an IReadOnlyList<SyntaxNode> within SyntaxGenerator.GetParameters.

I would be open to any refactoring/additional helpers which would make this code safer/more maintainable.

I changed to simply cast the list of parameter nodes which I think will make it more clear what exactly our assumption is.

RikkiGibson · 2021-12-02T18:23:14Z

Could I please get a second review on the relatively small (I think only comments) changes on the @dotnet/roslyn-compiler side?

cston

Compiler changes LGTM.

Youssef1313 · 2021-12-03T05:39:52Z

src/Compilers/Core/Portable/Symbols/IParameterSymbol.cs

        /// <summary>
-        /// True if the compiler will synthesize a null check for this parameter (the parameter is declared in source with a '!' following the parameter name).
+        /// True if the compiler will synthesize a null check for this parameter (the parameter is declared in source with a '!!' following the parameter name).


nit: I don't know whether this should be <c>!!</c> or not. This will render differently on docs.microsoft.com.

Perhaps it would make sense to have a tracking issue to verify the rendering/appearance of documentation comments, and perform adjustments like the one you've suggested.

Youssef1313 · 2021-12-03T05:41:36Z

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

+        {
+            await new VerifyCS.Test
+            {
+                LanguageVersion = LanguageVersion.Preview,


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

It's an oversight. Thanks for catching it.

jaredpar · 2021-12-03T21:26:38Z

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs

+            if (options.LanguageVersion < LanguageVersionExtensions.CSharpNext)
+            {
+                return null;
+            }


I was a bit surprised to see this. My expectation is that we would let the operation proceed without syntax checks and let later diagnostics catch that. Is there any other place where we have syntax based ops that depend on /langVersion?

this is very normal on the IDE side. remember that the user is just saying they want to add a "null check", not that they want to add !!. The IDE normally takes a stance with these types of operations on choosing the most idiomatic form that is legal for the version of the language the user is on.

That way users on C#10 will get the best option they can use, wihle those on 11 will get !!.

If users explicitly write out the C#11 version, then our 'upgrade project' analyzer kicks in and will help them upgrade if they want.

CyrusNajmabadi · 2021-12-07T22:52:17Z

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

+        [InlineData(LanguageVersionExtensions.CSharpNext)]
+        [InlineData(LanguageVersion.CSharp8)]


Suggested change

[InlineData(LanguageVersionExtensions.CSharpNext)]

[InlineData(LanguageVersion.CSharp8)]

[CombinatorialData]

This ends up giving us unexpected diagnostics about use of partial methods in C# 2.

CyrusNajmabadi · 2021-12-07T22:52:37Z

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

+        [InlineData(LanguageVersionExtensions.CSharpNext)]
+        [InlineData(LanguageVersion.CSharp8)]


Suggested change

[InlineData(LanguageVersionExtensions.CSharpNext)]

[InlineData(LanguageVersion.CSharp8)]

[CombinatorialData]

Use !! in AddParameterCheck refactoring

0b0ea12

RikkiGibson requested a review from a team as a code owner November 23, 2021 01:06

dotnet-issue-labeler bot added the Area-IDE label Nov 23, 2021

CyrusNajmabadi reviewed Nov 23, 2021

View reviewed changes

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 23, 2021

View reviewed changes

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs Outdated Show resolved Hide resolved

RikkiGibson added 2 commits November 22, 2021 18:36

Fix nullability

2cc5e6c

Use GetRequiredSyntaxTree. Fix additional warning.

0c54df5

jcouv reviewed Nov 23, 2021

View reviewed changes

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs Show resolved Hide resolved

jcouv reviewed Nov 23, 2021

View reviewed changes

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs Outdated Show resolved Hide resolved

jcouv reviewed Nov 23, 2021

View reviewed changes

jcouv self-assigned this Nov 23, 2021

Youssef1313 reviewed Nov 23, 2021

View reviewed changes

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Nov 23, 2021

View reviewed changes

...es/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/TokenBasedFormattingRule.cs Outdated Show resolved Hide resolved

runfoapp bot mentioned this pull request Nov 23, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

CyrusNajmabadi reviewed Nov 23, 2021

View reviewed changes

...atures/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs Outdated Show resolved Hide resolved

jasonmalinowski reviewed Nov 23, 2021

View reviewed changes

jasonmalinowski approved these changes Nov 23, 2021

View reviewed changes

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs Outdated Show resolved Hide resolved

...atures/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs Outdated Show resolved Hide resolved

Address some feedback

08ce98f

RikkiGibson requested a review from a team as a code owner November 24, 2021 01:23

jcouv mentioned this pull request Nov 24, 2021

Merge "list-patterns" feature into main branch #57962

Merged

jcouv approved these changes Nov 25, 2021

View reviewed changes

Youssef1313 reviewed Nov 29, 2021

View reviewed changes

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs Outdated Show resolved Hide resolved

Simplify

e050285

CyrusNajmabadi reviewed Nov 30, 2021

View reviewed changes

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 30, 2021

View reviewed changes

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 30, 2021

View reviewed changes

...atures/CSharp/Portable/InitializeParameter/CSharpAddParameterCheckCodeRefactoringProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 30, 2021

View reviewed changes

CyrusNajmabadi approved these changes Nov 30, 2021

View reviewed changes

Address feedback

828a71d

runfoapp bot mentioned this pull request Dec 1, 2021

[Flaky Test] VisualBasic.BasicAddMissingReference.InvokeSomeFixesInVisualBasicThenVerifyReferences #57980

Closed

cston approved these changes Dec 2, 2021

View reviewed changes

runfoapp bot mentioned this pull request Dec 2, 2021

Microsoft.CodeAnalysis.CSharp.CommandLine.UnitTests.CommandLineTests.ArgumentParsing is flaky #58077

Open

Youssef1313 reviewed Dec 3, 2021

View reviewed changes

jaredpar reviewed Dec 3, 2021

View reviewed changes

Address feedback

9053d0d

RikkiGibson enabled auto-merge (squash) December 7, 2021 22:23

CyrusNajmabadi reviewed Dec 7, 2021

View reviewed changes

RikkiGibson disabled auto-merge December 7, 2021 23:24

RikkiGibson merged commit ed3de52 into dotnet:features/param-nullchecking Dec 8, 2021

RikkiGibson deleted the pnc-refactoring branch December 8, 2021 01:18

This was referenced Dec 8, 2021

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

[Flaky Test] CSharp.CSharpCodeActions.AddUsingExactMatchBeforeRenameTracking #57982

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use !! in AddParameterCheck refactoring #57934

Use !! in AddParameterCheck refactoring #57934

RikkiGibson commented Nov 23, 2021

jcouv Nov 23, 2021

RikkiGibson Nov 24, 2021

RikkiGibson Nov 29, 2021

jcouv Nov 23, 2021

jasonmalinowski Nov 23, 2021

jasonmalinowski left a comment

jcouv left a comment

CyrusNajmabadi Nov 30, 2021

RikkiGibson Nov 30, 2021 •

edited

Loading

RikkiGibson Dec 1, 2021

RikkiGibson commented Dec 2, 2021

cston left a comment

Youssef1313 Dec 3, 2021 •

edited

Loading

RikkiGibson Dec 6, 2021

Youssef1313 Dec 3, 2021

RikkiGibson Dec 7, 2021

jaredpar Dec 3, 2021

CyrusNajmabadi Dec 4, 2021

CyrusNajmabadi Dec 7, 2021

RikkiGibson Dec 7, 2021

CyrusNajmabadi Dec 7, 2021

		[InlineData(LanguageVersionExtensions.CSharpNext)]
		[InlineData(LanguageVersion.CSharp8)]

	[InlineData(LanguageVersionExtensions.CSharpNext)]
	[InlineData(LanguageVersion.CSharp8)]
	[CombinatorialData]

Use !! in AddParameterCheck refactoring #57934

Use !! in AddParameterCheck refactoring #57934

Conversation

RikkiGibson commented Nov 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Dec 2, 2021

cston left a comment

Choose a reason for hiding this comment

Youssef1313 Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Nov 30, 2021 •

edited

Loading

Youssef1313 Dec 3, 2021 •

edited

Loading