InitializeParameter refactorings support for local & anonymous functions #24624

Neme12 · 2018-02-04T01:54:17Z

fixes #20983

…ndling of return statement insertion

Neme12 · 2018-02-08T18:31:33Z

src/Workspaces/CSharp/Portable/Extensions/ArrowExpressionClauseSyntaxExtensions.cs

@@ -48,7 +48,7 @@ internal static class ArrowExpressionClauseSyntaxExtensions
            return true;
        }

-        private static StatementSyntax ConvertToStatement(
+        public static StatementSyntax ConvertToStatement(


This is obviously not ideal... The thing is that this method is also useful for converting expression lambdas to statements, not just expression-bodied members. Maybe it could be moved to ExpressionSyntaxExtensions and then there would be a separate class for lambda extensions? I didn't want to restructure the code significantly in my first contribution, especially in layers below that I wasn't supposed to touch, just to make a simple change in a refactoring.

i would leave it here for now.

Neme12 · 2018-02-08T18:34:14Z

@CyrusNajmabadi could you please give me some feedback? there's a few rough spots I'm not sure what to do about

Neme12 · 2018-02-08T18:45:48Z

src/EditorFeatures/VisualBasicTest/InitializeParameter/AddParameterCheckTests.vb

+class C
+    public sub new()
+        dim f = sub (s as string)
+


All these blank lines in VB... the VB syntax generator does that for some reason, I have no idea what causes it.

yeah... that's not good. we'd def want to fix this. or at least open a bug here. we don't do this for normal sub/function methods right?

i checked again and it does create blank lines between the statements in methods, but not before the first one... in lambdas it also creates a blank line before the first statement.

In both cases (methods & lambdas) it does not create a blank line after the last statement - so if the check inserted is the last statement, there's no blank line after it, but if there's a return statement at the end, it creates a blank line before the return statement.

I was suspicious this might have been caused by the call to Isolate (which calls PreserveTrivia) in VisualBasicSyntaxGenerator.WithStatements - there's no such thing in CSharpSyntaxGenerator, but that's not the case, I get same behavior if I directly call WithStatementsInternal. It must be some sort of formatting issue.

@CyrusNajmabadi I identified the issue. It's in ElasticTriviaFormattingRule.TopLevelStatement. It's a one-line fix to add TypeOf statement Is LambdaHeaderSyntax. Should I include it in this PR?

I'd have no problem with that.

CyrusNajmabadi · 2018-02-08T20:23:39Z

src/Features/CSharp/Portable/InitializeParameter/InitializeParameterHelpers.cs

+                case AnonymousFunctionExpressionSyntax anonymousFunction:
+                    return (SyntaxNode)anonymousFunction.Body;
+                default:
+                    Debug.Fail($"Unexpected {nameof(functionDeclaration)} type");


CyrusNajmabadi · 2018-02-08T20:23:47Z

src/Features/CSharp/Portable/InitializeParameter/InitializeParameterHelpers.cs

+                case AnonymousFunctionExpressionSyntax _:
+                    return null;
+                default:
+                    Debug.Fail($"Unexpected {nameof(functionDeclaration)} type");


CyrusNajmabadi · 2018-02-08T20:43:41Z

src/Features/CSharp/Portable/InitializeParameter/InitializeParameterHelpers.cs

+                editor.SetStatements(functionDeclaration, statementToAddAfterOpt == null
+                    ? ImmutableArray.Create(statement, convertedStatement)
+                    : ImmutableArray.Create(convertedStatement, statement)
+                    );


put on previous line.

just the );?

CyrusNajmabadi · 2018-02-08T20:44:06Z

src/Features/CSharp/Portable/InitializeParameter/InitializeParameterHelpers.cs

        {
-            switch(declarationSyntax)
+            // either expression lambda or expression bodied member
+            return body is ExpressionSyntax || body is ArrowExpressionClauseSyntax;


consider using => here.

CyrusNajmabadi · 2018-02-08T20:45:46Z

...ures/Core/Portable/InitializeParameter/AbstractInitializeParameterCodeRefactoringProvider.cs

            {
                return;
            }

-            if (parameter.Name == "")
+            var method = (IMethodSymbol)parameter.ContainingSymbol;
+            if (method == null ||


null check not needed.

CyrusNajmabadi · 2018-02-08T20:45:58Z

...ures/Core/Portable/InitializeParameter/AbstractInitializeParameterCodeRefactoringProvider.cs

+                method.IsAbstract ||
+                method.IsExtern ||
+                method.PartialImplementationPart != null ||
+                method.ContainingType?.TypeKind == TypeKind.Interface)


?. not needed.

CyrusNajmabadi · 2018-02-08T20:46:24Z

...ures/Core/Portable/InitializeParameter/AbstractInitializeParameterCodeRefactoringProvider.cs

@@ -107,16 +104,12 @@ internal abstract partial class AbstractInitializeParameterCodeRefactoringProvid
            if (bodyOpt != null)
            {
                blockStatementOpt = semanticModel.GetOperation(bodyOpt, cancellationToken) as IBlockOperation;
-                if (blockStatementOpt == null)


this is just paranoia until IOperation is complete.

CyrusNajmabadi · 2018-02-08T20:46:54Z

src/Features/VisualBasic/Portable/InitializeParameter/InitializeParameterHelpers.vb

+                    SyntaxFactory.ReturnStatement(DirectCast(singleLineLambda.Body, ExpressionSyntax)))
+                return SyntaxFactory.List(ImmutableArray.Create(convertedStatement))
+            Else
+                Debug.Fail($"Unexpected {NameOf(functionDeclaration)} type")


CyrusNajmabadi · 2018-02-08T20:48:29Z

Looking awesome!

Neme12 · 2018-02-09T09:08:39Z

I found a bug: in VB lambdas the null checks don't get ordered correctly and are offered even if there is an existing one. The IOperation wireup must be wrong there.

…in lambdas

Neme12 · 2018-02-09T20:22:37Z

All fixed... I think this is ready

CyrusNajmabadi · 2018-02-09T20:24:25Z

src/EditorFeatures/VisualBasicTest/CodeActions/IntroduceVariable/IntroduceVariableTests.vb

@@ -1282,7 +1282,6 @@ End Module",
 Module Program
    Sub Main
        Dim a = Sub(x As Integer)
-
                    If True Then


these deletions are good. this PR also fixes a VB formatting bug around elastic trivia and lambdas.

CyrusNajmabadi · 2018-02-09T20:25:24Z

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

+                return InitializeParameterHelpers.TryConvertExpressionBodyToStatement(body,
+                    semicolonToken: SyntaxFactory.Token(SyntaxKind.SemicolonToken),
+                    createReturnStatementForExpression: false,
+                    statement: out var _);


do we have a test that exercises this codepath when TryConvertExpressionBodyToStatement returns false?

yes:

roslyn/src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

Line 1170 in 957882b

public async Task TestMissingInArrowExpression1()

great. thanks!

CyrusNajmabadi · 2018-02-09T20:26:08Z

src/Features/CSharp/Portable/InitializeParameter/InitializeParameterHelpers.cs

+        {
+            var bodyOpt = GetBody(functionDeclaration);
+            if (bodyOpt == null)
+                return null;


curlies always required.

CyrusNajmabadi · 2018-02-09T20:26:48Z

src/Features/CSharp/Portable/InitializeParameter/InitializeParameterHelpers.cs

+                case LocalFunctionStatementSyntax localFunction:
+                    return (SyntaxNode)localFunction.Body ?? localFunction.ExpressionBody;
+                case AnonymousFunctionExpressionSyntax anonymousFunction:
+                    return (SyntaxNode)anonymousFunction.Body;


this cast should not be necessary.

it's not, i wanted it to look the same as on previous lines... i'll remove it

…ze-lambdas

Neme12 · 2018-03-29T23:43:51Z

retest windows_coreclr_debug_prtest please

Neme12 · 2018-04-11T22:13:20Z

retest windows_debug_spanish_unit32_prtest please

sharwell · 2018-05-25T15:23:11Z

src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs

@@ -462,6 +462,150 @@ public C(string s)
    Option(CSharpCodeStyleOptions.PreferExpressionBodiedConstructors, CSharpCodeStyleOptions.WhenPossibleWithSuggestionEnforcement)));
        }

+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]


💡 Would be good to see the new tests in this PR have a WorkItem attribute pointing to #20983.

added in d5dbed4

sharwell · 2018-05-25T15:28:17Z

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

+                else if (condition is IIsPatternOperation isPatternOperation &&
+                         isPatternOperation.Pattern is IConstantPatternOperation constantPattern)
+                {
+                    if (IsNullCheck(constantPattern.Value, isPatternOperation.Value, parameter))


💡

// Look for code of the form "if (p is null)"

added in 9876619

sharwell · 2018-05-25T15:34:06Z

src/Workspaces/VisualBasic/Portable/Formatting/Rules/ElasticTriviaFormattingRule.vb

@@ -370,6 +370,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Formatting
        Private Function TopLevelStatement(statement As StatementSyntax) As Boolean
            Return TypeOf statement Is MethodStatementSyntax OrElse
                   TypeOf statement Is SubNewStatementSyntax OrElse
+                   TypeOf statement Is LambdaHeaderSyntax OrElse


sharwell · 2018-05-25T15:43:50Z

src/Features/VisualBasic/Portable/InitializeParameter/InitializeParameterHelpers.vb

-            Return containingMember
+        Public Shared Function IsFunctionDeclaration(node As SyntaxNode) As Boolean
+            Return TypeOf node Is MethodBlockBaseSyntax OrElse
+                   TypeOf node Is LambdaExpressionSyntax


💭 The code could be simplified to ISyntaxFactsService if we could resolve the difference between TypeOf node Is MethodBlockBaseSyntax (which is VisualBasicSyntaxFactsService.IsBlockBody) and node is BaseMethodDeclarationSyntax (which is a bit different). The rest is just IsAnonymousFunction||IsLocalFunction (where the latter always returns false for VB).

I would love to simplify this more but there's nothing I could come up with that would make this simpler (unless I created a new API, which of course would result in more code overall, not less)

sharwell · 2018-05-25T16:18:36Z

src/Features/CSharp/Portable/InitializeParameter/InitializeParameterHelpers.cs

+            || node is LocalFunctionStatementSyntax
+            || node is AnonymousFunctionExpressionSyntax;
+
+        public static IBlockOperation GetBlockOperation(SyntaxNode functionDeclaration, SemanticModel semanticModel, CancellationToken cancellationToken)


💭 It feels like some of the helpers could be unified, but it wasn't obvious how it would work so this seems fine for now.

I tried a few approaches but nothing actually made things simpler. I don't think there's a unified way to get a block operation from a method-level declaration, local function or anonymous function, is there? Unfortunately I think this has to implemented separately.

jinujoseph · 2018-06-03T01:37:09Z

@sharwell any thing pending here ? LGTM approved to merge for 15.8.preview3

Neme12 · 2018-06-04T13:18:20Z

I think this is my first PR 🎉 Thanks!

Neme12 added 5 commits February 3, 2018 14:55

Adding local functions support to InitializeParameter refactorings

8f2aa8c

I think this is useless

6c59b6a

InitializeParameter: Initial support for C# anonymous functions

9f48a24

InitializeParameter: Adding tests for anonymous functions + proper ha…

39e6ee0

…ndling of return statement insertion

InitializeParameter: VB gets some love too

ad25f24

Neme12 requested a review from a team as a code owner February 4, 2018 01:54

Neme12 commented Feb 8, 2018

View reviewed changes

Neme12 changed the title ~~Support for local & anonymous functions in InitializeParameter refactorings~~ InitializeParameter refactorings support for local & anonymous functions Feb 8, 2018

Neme12 commented Feb 8, 2018

View reviewed changes

CyrusNajmabadi reviewed Feb 8, 2018

View reviewed changes

Cleanup after review

31438b7

jinujoseph assigned sharwell Feb 9, 2018

jinujoseph added this to the 15.8 milestone Feb 9, 2018

Neme12 added 2 commits February 9, 2018 14:28

InitializeParameter: Fixing IOperation inside VB lambdas

151f8f6

VB formatter fix: no more blank line inserted before first statement …

957882b

…in lambdas

CyrusNajmabadi reviewed Feb 9, 2018

View reviewed changes

Neme12 and others added 5 commits February 16, 2018 20:46

Merge remote-tracking branch 'upstream/master' into initialize-lambdas

cb164b1

Improve newlines.

f0291ed

Add comments.

ffbc813

Merge remote-tracking branch 'cyrus/initialize-lambdas' into initiali…

679cc3f

…ze-lambdas

Fixing formatting in the tests

9bd4844

jcouv added the Area-IDE label Mar 30, 2018

Neme12 added 4 commits April 4, 2018 18:17

Merge branch 'master' into initialize-lambdas

b8f8e88

Adding more tests

0a1521d

Fixing a NRE

d282ae6

Recognizing 'is null' check

0cc740a

jinujoseph closed this May 24, 2018

jinujoseph reopened this May 24, 2018

sharwell mentioned this pull request May 25, 2018

'Use block body' is broken with async #25202

Closed

sharwell approved these changes May 25, 2018

View reviewed changes

Neme12 added 3 commits May 26, 2018 20:33

Merge remote-tracking branch 'upstream/master' into initialize-lambdas

beef387

Adding WorkItemAttribute

d5dbed4

Adding a comment

9876619

jinujoseph added the Approved to merge label Jun 3, 2018

sharwell merged commit 419ba0a into dotnet:master Jun 4, 2018

Neme12 deleted the initialize-lambdas branch June 4, 2018 13:17

Neme12 mentioned this pull request Dec 17, 2018

Code fix for CS0122 to expand accessibility #31837

Closed

InitializeParameter refactorings support for local & anonymous functions #24624

InitializeParameter refactorings support for local & anonymous functions #24624

Conversation

Neme12 commented Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 commented Feb 8, 2018

Neme12 Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Feb 8, 2018 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 8, 2018

Neme12 commented Feb 9, 2018

Neme12 commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Feb 9, 2018 • edited by sharwell 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

Neme12 commented Mar 29, 2018

Neme12 commented Apr 11, 2018

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

Neme12 May 28, 2018 • edited Loading

Choose a reason for hiding this comment

jinujoseph commented Jun 3, 2018

Neme12 commented Jun 4, 2018 • edited Loading

Neme12 commented Feb 4, 2018 •

edited

Loading

Neme12 Feb 8, 2018 •

edited

Loading

Neme12 Feb 8, 2018 •

edited

Loading

Neme12 Feb 9, 2018 •

edited by sharwell

Loading

Neme12 May 28, 2018 •

edited

Loading

Neme12 commented Jun 4, 2018 •

edited

Loading