Add nullability information to extract method #37851

ryzngard · 2019-08-08T23:35:32Z

The general premise is to favor non nullable reference types when possible, but we need to use flow analysis to be sure.

Use analysis on all parameters to determine if they are ever assigned to null (if a ref parameter)
Analyze if a parameter is never null in reference locations
Analyze all returns of a generated method to determine if null is possible
Add, hopefully, enough comprehensive tests to cover a wide array of possibilities

jasonmalinowski

Bunch of nitpick questions or things that looked like you went one way and then decided to go another way.

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs

src/Features/Core/Portable/ExtractMethod/Extensions.cs

src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs

jasonmalinowski · 2019-08-13T23:12:41Z

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs

+                {
+                    foreach (var reference in references)
+                    {
+                        var typeInfo = semanticModel.GetTypeInfo(reference.Syntax);


In the assignment case, is this the flow state of the left hand or right hand side? Does it matter?

I'm guessing it's left hand, but I'm not really sure. @333fred ?

Flow state is always RHS, Annotations are always LHS. If I understand your question.

So in the case of assignment, we're looking to see if it ever can be assigned to null. That makes sense then, we want the RHS.

I guess the question was "if you ask for the flow state of foo" in "foo = bar", is that the flow state after the assignment to bar (so it would have the same flow state of bar) or is that the flow state before the assignment of bar?

CyrusNajmabadi · 2019-08-13T23:30:52Z

Add, hopefully, enough comprehensive tests to cover a wide array of possibilities

👍

src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs

CyrusNajmabadi · 2019-08-14T00:22:44Z

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs

+                var newRoot = oldRoot.ReplaceNode(methodDeclaration, newMethodDeclaration);
+
+                var newDocument = originalDocument.Document.WithSyntaxRoot(newRoot);
+                return await SemanticDocument.CreateAsync(newDocument, cancellationToken).ConfigureAwait(false);


this fixing up "after the fact" seems super hinky and error prone. is there a reason we can't generate it with the right nullability right from the start?

So I tried to make this work in generating the right nullability annotation and I found it too difficult since we can modify the code by adding return statements. I ended up basically writing a predictive flow analyzer for all the cases we could have to deal with where we would inject a return statement. I'm time boxing to the current design of fixing up after the fact, and I'll do my best to put a reasonable explanation as a comment here as to why it's this way. The only option I see is to accept the annotated nullability as desired and move on, but we asserted that we would like to be smarter than that.

I'm very much open to ideas and discussion on this, but I think this is the most robust solution at this time based on my investigation.

The nullability project has a "IDE cleanup" column to track debt we created; this might be worth an entry there.

…hod after generating it

…hod_nullability

jasonmalinowski

A few remaining questions; happy to sign off assuming they're either answered or we've appropriately tracked any deferred work.

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs

jasonmalinowski · 2019-08-19T23:44:48Z

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs

+
+                foreach (var returnOperation in returnOperations)
+                {
+                    var syntax = returnOperation.ReturnedValue?.Syntax ?? returnOperation.Syntax;


Is this going to blow up if we have a return; inside a method that is non-void-returning?

Not exactly. We end up not modifying the return type because it reports as None for flow state

jasonmalinowski · 2019-08-19T23:45:47Z

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs

+                var newRoot = oldRoot.ReplaceNode(methodDeclaration, newMethodDeclaration);
+
+                var newDocument = originalDocument.Document.WithSyntaxRoot(newRoot);
+                return await SemanticDocument.CreateAsync(newDocument, cancellationToken).ConfigureAwait(false);


The nullability project has a "IDE cleanup" column to track debt we created; this might be worth an entry there.

src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs

jasonmalinowski · 2019-08-19T23:50:15Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs

+                // follow our recommendation of only using nullable when necessary.
+                // This is done after method generation instead of at analyzer time because it's purely
+                // based on the resulting code, which the generator can modify as needed. If return statements
+                // are added, the flow analysis could change to indicate something different. It's cleaner to rely


I'm still not sure I understand the case where that would happen. That would seem like the transformation we're doing in extract method has some deeper issue.

src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs

…tracted method

CyrusNajmabadi · 2019-08-21T04:17:32Z

src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs

+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractMethod)]
+        public Task TestFlowNullable_LambdaWithReturn()
+            => TestInRegularAndScriptAsync(


in a future PR, could you update everything to be consistent here :)

Naming style, or the expression body style?

expression-body style.

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs

CyrusNajmabadi · 2019-08-21T04:23:44Z

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs

+                    {
+                        BaseMethodDeclarationSyntax _ => true,
+                        AnonymousFunctionExpressionSyntax _ => true,
+                        LocalFunctionStatementSyntax _ => true,


i feel like there's are ISyntaxFacts for this...

I didn't see one. I saw for method body, and a few that take a position for method head.

jasonmalinowski

Remaining comments may be addressed after merge.

jasonmalinowski · 2019-08-21T23:46:55Z

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs

+                }
+
+                bool AreAllReferencesNotNull(IEnumerable<IOperation> references)
+                => references.All(r => semanticModel.GetTypeInfo(r.Syntax).Nullability.FlowState == NullableFlowState.NotNull);


Suggested change

=> references.All(r => semanticModel.GetTypeInfo(r.Syntax).Nullability.FlowState == NullableFlowState.NotNull);

=> references.All(r => semanticModel.GetTypeInfo(r.Syntax, cancellationToken).Nullability.FlowState == NullableFlowState.NotNull);

(which will also need it to be passed in)

jasonmalinowski · 2019-08-21T23:49:25Z

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs

+
+                foreach (var returnOperation in returnOperations)
+                {
+                    // If thereturn statement is located in a nested local function or lambda it


Suggested change

// If thereturn statement is located in a nested local function or lambda it

// If the return statement is located in a nested local function or lambda it

ryzngard and others added 6 commits August 12, 2019 14:55

first pass with one nullability test for extract method

3c3d918

Use flow state

b436141

Add tests for state change

3d15242

Add return type fix based on flow state

cfb883a

Include assignment as part of conclusion of nullability

afce4a6

Unwrap in a few places that need it

56f48e2

ryzngard force-pushed the issue/extract_method_nullability branch from 4031564 to 56f48e2 Compare August 13, 2019 00:01

ryzngard marked this pull request as ready for review August 13, 2019 00:01

ryzngard requested a review from a team as a code owner August 13, 2019 00:01

ryzngard requested a review from jasonmalinowski August 13, 2019 00:02

jinujoseph added the Area-IDE label Aug 13, 2019

Add another ConstructWithNullability

25598cf

jasonmalinowski reviewed Aug 13, 2019

View reviewed changes

CyrusNajmabadi reviewed Aug 13, 2019

View reviewed changes

src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 14, 2019

View reviewed changes

ryzngard added 10 commits August 14, 2019 16:05

Small PR fixups

1dd3aa5

Use correct nullability

11543dc

Add more tests

48b9fb4

Fix null ref if ReturnValue is null on a return operation

db74834

Flow doesn't include lambda

822938e

Simplify some code. Add a comment to help explain why we update a met…

0d27151

…hod after generating it

Add reference to issue to cleanup code

cc13d6b

Remove unneeded WithNullability call

e274e92

Merge remote-tracking branch 'upstream/master' into issue/extract_met…

6013d1e

…hod_nullability

Fix nullability annotations

1e5bc1f

jasonmalinowski reviewed Aug 19, 2019

View reviewed changes

ryzngard added 2 commits August 20, 2019 00:16

Fix nullability annotation

ca9b5ba

Logic is hard

5a4e59c

ryzngard added 2 commits August 20, 2019 17:17

Don't include return statements unless they are returning from the ex…

c88ddae

…tracted method

Make local static and rename

824a348

CyrusNajmabadi reviewed Aug 21, 2019

View reviewed changes

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 21, 2019

View reviewed changes

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 21, 2019

View reviewed changes

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 21, 2019

View reviewed changes

PR feedback and test fixes

4881f5f

jasonmalinowski approved these changes Aug 21, 2019

View reviewed changes

ryzngard merged commit bee0632 into dotnet:master Aug 22, 2019

jasonmalinowski mentioned this pull request Aug 22, 2019

ExtractMethod isn't aware of nullability #30314

Closed

ryzngard mentioned this pull request Aug 22, 2019

Cleanup after adding nullable information to extract method #38221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nullability information to extract method #37851

Add nullability information to extract method #37851

ryzngard commented Aug 8, 2019

jasonmalinowski left a comment

jasonmalinowski Aug 13, 2019

ryzngard Aug 14, 2019

333fred Aug 14, 2019

ryzngard Aug 14, 2019

jasonmalinowski Aug 15, 2019

CyrusNajmabadi commented Aug 13, 2019

CyrusNajmabadi Aug 14, 2019

ryzngard Aug 16, 2019

jasonmalinowski Aug 19, 2019

jasonmalinowski left a comment

jasonmalinowski Aug 19, 2019

ryzngard Aug 21, 2019

jasonmalinowski Aug 19, 2019

jasonmalinowski Aug 19, 2019

CyrusNajmabadi Aug 21, 2019

ryzngard Aug 21, 2019

CyrusNajmabadi Aug 22, 2019

CyrusNajmabadi Aug 21, 2019

ryzngard Aug 21, 2019

jasonmalinowski left a comment

jasonmalinowski Aug 21, 2019

jasonmalinowski Aug 21, 2019

jasonmalinowski Aug 21, 2019

	=> references.All(r => semanticModel.GetTypeInfo(r.Syntax).Nullability.FlowState == NullableFlowState.NotNull);
	=> references.All(r => semanticModel.GetTypeInfo(r.Syntax, cancellationToken).Nullability.FlowState == NullableFlowState.NotNull);

	// If thereturn statement is located in a nested local function or lambda it
	// If the return statement is located in a nested local function or lambda it

Add nullability information to extract method #37851

Add nullability information to extract method #37851

Conversation

ryzngard commented Aug 8, 2019

jasonmalinowski left a 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

CyrusNajmabadi commented Aug 13, 2019

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

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

jasonmalinowski left a 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