Use !! in AddParameterCheck refactoring by RikkiGibson · Pull Request #57934 · dotnet/roslyn

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


+        [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

                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

ghost 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

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

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

Conversation

RikkiGibson commented Nov 23, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Dec 2, 2021

Uh oh!

cston left a comment

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RikkiGibson Nov 30, 2021 •

edited

Loading

Youssef1313 Dec 3, 2021 •

edited

Loading