Merge pull request #19285 from CyrusNajmabadi/directiveHandling

Better handling of directive-trivia in UseExpressionBody/ReplacePropWithMethod.

src/EditorFeatures/CSharpTest/CodeActions/ReplacePropertyWithMethods/ReplacePropertyWithMethodsTests.cs

@@ -1416,6 +1416,120 @@ public async Task TestDocumentationComment3()
 }", ignoreTrivia: false);
         }
 
+        [WorkItem(19235, "https://github.com/dotnet/roslyn/issues/19235")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsReplacePropertyWithMethods)]
+        public async Task TestWithDirectives1()
+        {
+            await TestInRegularAndScriptAsync(
+@"class C
+{
+    int [||]Prop
+    {
+        get
+        {
+#if true
+            return 0;
+#else
+            return 1;
+#endif
+        }
+    }
+}",
+    @"class C
+{
+    private int GetProp()
+    {
+#if true
+        return 0;
+#else
+            return 1;
+#endif
+    }
+}", ignoreTrivia: false);
+        }
+
+        [WorkItem(19235, "https://github.com/dotnet/roslyn/issues/19235")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsReplacePropertyWithMethods)]
+        public async Task TestWithDirectives2()
+        {
+            await TestInRegularAndScriptAsync(
+@"class C
+{
+    int [||]Prop
+    {
+        get
+        {
+#if true
+            return 0;
+#else
+            return 1;
+#endif
+        }
+    }
+}",
+    @"class C
+{
+    private int GetProp() =>
+#if true
+            0;
+#else
+            return 1;
+#endif
+}", ignoreTrivia: false,
+    options: PreferExpressionBodiedMethods);
+        }
+
+        [WorkItem(19235, "https://github.com/dotnet/roslyn/issues/19235")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsReplacePropertyWithMethods)]
+        public async Task TestWithDirectives3()
+        {
+            await TestInRegularAndScriptAsync(
+@"class C
+{
+    int [||]Prop =>
+#if true
+        0;
+#else
+        1;
+#endif
+}",
+@"class C
+{
+    private int GetProp() =>
+#if true
+        0;
+#else
+        1;
+#endif
+}", ignoreTrivia: false);
+        }
+
+        [WorkItem(19235, "https://github.com/dotnet/roslyn/issues/19235")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsReplacePropertyWithMethods)]
+        public async Task TestWithDirectives4()
+        {
+            await TestInRegularAndScriptAsync(
+@"class C
+{
+    int [||]Prop =>
+#if true
+        0;
+#else
+        1;
+#endif
+}",
+@"class C
+{
+    private int GetProp() =>
+#if true
+        0;
+#else
+        1;
+#endif
+}", ignoreTrivia: false,
+    options: PreferExpressionBodiedMethods);
+        }
+
         private IDictionary<OptionKey, object> PreferExpressionBodiedMethods =>
             OptionsSet(SingleOption(CSharpCodeStyleOptions.PreferExpressionBodiedMethods, CSharpCodeStyleOptions.WhenPossibleWithSuggestionEnforcement));
     }

src/EditorFeatures/CSharpTest/UseExpressionBody/Analyzer/UseExpressionBodyForPropertiesAnalyzerTests.cs

@@ -244,5 +244,125 @@ public async Task TestUseExpressionBodyKeepTrailingTrivia()
     public string OtherThing => ""Pickles"";
 }", ignoreTrivia: false, options: UseExpressionBody);
         }
+
+        [WorkItem(19235, "https://github.com/dotnet/roslyn/issues/19235")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)]
+        public async Task TestDirectivesInBlockBody1()
+        {
+            await TestInRegularAndScript1Async(
+@"class C
+{
+    int Foo
+    {
+        get
+        {
+#if true
+            [|return|] Bar();
+#else
+            return Baz();
+#endif
+        }
+    }
+}",
+
+@"class C
+{
+    int Foo =>
+#if true
+            Bar();
+#else
+            return Baz();
+#endif
+
+}", ignoreTrivia: false,
+    parameters: new TestParameters(options: UseExpressionBody));
+        }
+
+        [WorkItem(19235, "https://github.com/dotnet/roslyn/issues/19235")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)]
+        public async Task TestDirectivesInBlockBody2()
+        {
+            await TestInRegularAndScript1Async(
+@"class C
+{
+    int Foo
+    {
+        get
+        {
+#if false
+            return Bar();
+#else
+            [|return|] Baz();
+#endif
+        }
+    }
+}",
+
+@"class C
+{
+    int Foo =>
+#if false
+            return Bar();
+#else
+            Baz();
+#endif
+
+}", ignoreTrivia: false,
+    parameters: new TestParameters(options: UseExpressionBody));
+        }
+
+        [WorkItem(19235, "https://github.com/dotnet/roslyn/issues/19235")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)]
+        public async Task TestMissingWithDirectivesInExpressionBody1()
+        {
+            await TestMissingInRegularAndScriptAsync(
+@"class C
+{
+    int Foo [|=>|]
+#if true
+            Bar();
+#else
+            Baz();
+#endif
+}", parameters: new TestParameters(options: UseBlockBody));
+        }
+
+        [WorkItem(19235, "https://github.com/dotnet/roslyn/issues/19235")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)]
+        public async Task TestMissingWithDirectivesInExpressionBody2()
+        {
+            await TestMissingInRegularAndScriptAsync(
+@"class C
+{
+    int Foo [|=>|]
+#if false
+            Bar();
+#else
+            Baz();
+#endif
+}", parameters: new TestParameters(options: UseBlockBody));
+        }
+
+        [WorkItem(19193, "https://github.com/dotnet/roslyn/issues/19193")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)]
+        public async Task TestMoveTriviaFromExpressionToReturnStatement()
+        {
+            await TestInRegularAndScriptAsync(
+@"class C
+{
+    int Foo(int i) [|=>|]
+        //comment
+        i * i;
+}",
+@"class C
+{
+    int Foo(int i)
+    {
+        //comment
+        return i * i;
+    }
+}", ignoreTrivia: false,
+    options: UseBlockBody);
+        }
     }
 }
\ No newline at end of file

src/Features/CSharp/Portable/ReplaceMethodWithProperty/CSharpReplaceMethodWithPropertyService.cs

@@ -95,7 +95,7 @@ public void RemoveSetMethod(SyntaxEditor editor, SyntaxNode setMethodDeclaration
                     }
                     else if (getAccessor.Body != null &&
                              getAccessor.Body.TryConvertToExpressionBody(
-                                 parseOptions, expressionBodyPreference, 
+                                 parseOptions, expressionBodyPreference,
                                  out var arrowExpression, out var semicolonToken))
                     {
                         return propertyDeclaration.WithExpressionBody(arrowExpression)
@@ -106,10 +106,12 @@ public void RemoveSetMethod(SyntaxEditor editor, SyntaxNode setMethodDeclaration
             }
             else
             {
-                if (propertyDeclaration.ExpressionBody != null)
+                if (propertyDeclaration.ExpressionBody != null &&
+                    propertyDeclaration.ExpressionBody.TryConvertToBlock(
+                        propertyDeclaration.SemicolonToken,
+                        createReturnStatementForExpression: true,
+                        block: out var block))
                 {
-                    var block = propertyDeclaration.ExpressionBody.ConvertToBlock(
-                        propertyDeclaration.SemicolonToken, createReturnStatementForExpression: true);
                     var accessor =
                         SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration)
                                      .WithBody(block);
@@ -181,7 +183,7 @@ private SyntaxToken GetPropertyName(SyntaxToken identifier, string propertyName,
             if (accessorDeclaration?.Body != null && expressionBodyPreference != ExpressionBodyPreference.Never)
             {
                 if (accessorDeclaration.Body.TryConvertToExpressionBody(
-                        parseOptions, expressionBodyPreference, 
+                        parseOptions, expressionBodyPreference,
                         out var arrowExpression, out var semicolonToken))
                 {
                     return accessorDeclaration.WithBody(null)
@@ -192,13 +194,16 @@ private SyntaxToken GetPropertyName(SyntaxToken identifier, string propertyName,
             }
             else if (accessorDeclaration?.ExpressionBody != null && expressionBodyPreference == ExpressionBodyPreference.Never)
             {
-                var block = accessorDeclaration.ExpressionBody.ConvertToBlock(
-                    accessorDeclaration.SemicolonToken,
-                    createReturnStatementForExpression: accessorDeclaration.Kind() == SyntaxKind.GetAccessorDeclaration);
-                return accessorDeclaration.WithExpressionBody(null)
-                                          .WithSemicolonToken(default(SyntaxToken))
-                                          .WithBody(block)
-                                          .WithAdditionalAnnotations(Formatter.Annotation);
+                if (accessorDeclaration.ExpressionBody.TryConvertToBlock(
+                        accessorDeclaration.SemicolonToken,
+                        createReturnStatementForExpression: accessorDeclaration.Kind() == SyntaxKind.GetAccessorDeclaration,
+                        block: out var block))
+                {
+                    return accessorDeclaration.WithExpressionBody(null)
+                                              .WithSemicolonToken(default(SyntaxToken))
+                                              .WithBody(block)
+                                              .WithAdditionalAnnotations(Formatter.Annotation);
+                }
             }
 
             return accessorDeclaration;

src/Features/CSharp/Portable/ReplacePropertyWithMethods/CSharpReplacePropertyWithMethodsService.cs

@@ -246,12 +246,14 @@ private static SyntaxTrivia ConvertDocumentationComment(SyntaxTrivia trivia)
             }
             else if (methodDeclaration?.ExpressionBody != null && expressionBodyPreference == ExpressionBodyPreference.Never)
             {
-                var block = methodDeclaration?.ExpressionBody.ConvertToBlock(
-                    methodDeclaration.SemicolonToken, createReturnStatementForExpression);
-                return methodDeclaration.WithExpressionBody(null)
-                                        .WithSemicolonToken(default(SyntaxToken))
-                                        .WithBody(block)
-                                        .WithAdditionalAnnotations(Formatter.Annotation);
+                if (methodDeclaration.ExpressionBody.TryConvertToBlock(
+                        methodDeclaration.SemicolonToken, createReturnStatementForExpression, out var block))
+                {
+                    return methodDeclaration.WithExpressionBody(null)
+                                            .WithSemicolonToken(default(SyntaxToken))
+                                            .WithBody(block)
+                                            .WithAdditionalAnnotations(Formatter.Annotation);
+                }
             }
 
             return methodDeclaration;

src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyHelper`1.cs

@@ -128,8 +128,8 @@ protected virtual Location GetDiagnosticLocation(TDeclaration declaration)
             // If the user does not like block bodies then we offer block bodies from the refactoring provider.
             if (userPrefersBlockBodies == forAnalyzer)
             {
-                // If we have an expression body, we can always convert it to a block body.
-                return this.GetExpressionBody(declaration) != null;
+                return this.GetExpressionBody(declaration)?.TryConvertToBlock(
+                    SyntaxFactory.Token(SyntaxKind.SemicolonToken), false, out var block) == true;
             }
 
             return false;
@@ -180,11 +180,16 @@ public TDeclaration Update(TDeclaration declaration, OptionSet options, bool use
         {
             var expressionBody = GetExpressionBody(declaration);
             var semicolonToken = GetSemicolonToken(declaration);
-            var block = expressionBody.ConvertToBlock(
-                GetSemicolonToken(declaration),
-                CreateReturnStatementForExpression(declaration));
 
-            return WithBody(declaration, block);
+            if (expressionBody.TryConvertToBlock(
+                    GetSemicolonToken(declaration),
+                    CreateReturnStatementForExpression(declaration),
+                    out var block))
+            {
+                return WithBody(declaration, block);
+            }
+
+            return declaration;
         }
 
         protected TDeclaration WithAccessorList(
@@ -196,7 +201,8 @@ public TDeclaration Update(TDeclaration declaration, OptionSet options, bool use
             var expressionBodyPreference = options.GetOption(CSharpCodeStyleOptions.PreferExpressionBodiedAccessors).Value;
 
             AccessorDeclarationSyntax accessor;
-            if (expressionBodyPreference != ExpressionBodyPreference.Never)
+            if (expressionBodyPreference != ExpressionBodyPreference.Never ||
+                !expressionBody.TryConvertToBlock(GetSemicolonToken(declaration), CreateReturnStatementForExpression(declaration), out var block))
             {
                 accessor = SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration)
                                         .WithExpressionBody(expressionBody)
@@ -204,9 +210,6 @@ public TDeclaration Update(TDeclaration declaration, OptionSet options, bool use
             }
             else
             {
-                var block = expressionBody.ConvertToBlock(
-                    GetSemicolonToken(declaration),
-                    CreateReturnStatementForExpression(declaration));
                 accessor = SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration, block);
             }
 

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

 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
+using System.Linq;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Shared.Extensions;
 
@@ -7,14 +8,29 @@ namespace Microsoft.CodeAnalysis.CSharp.Extensions
 {
     internal static class ArrowExpressionClauseSyntaxExtensions
     {
-        public static BlockSyntax ConvertToBlock(
+        public static bool TryConvertToBlock(
             this ArrowExpressionClauseSyntax arrowExpression,
             SyntaxToken semicolonToken,
-            bool createReturnStatementForExpression)
+            bool createReturnStatementForExpression,
+            out BlockSyntax block)
         {
+            // It's tricky to convert an arrow expression with directives over to a block.
+            // We'd need to find and remove the directives *after* the arrow expression and
+            // move them accordingly.  So, for now, we just disallow this.
+            if (arrowExpression.Expression.GetLeadingTrivia().Any(t => t.IsDirective))
+            {
+                block = null;
+                return false;
+            }
+
             var statement = ConvertToStatement(arrowExpression.Expression, semicolonToken, createReturnStatementForExpression);
-            statement = statement.WithPrependedLeadingTrivia(arrowExpression.ArrowToken.TrailingTrivia);
-            return SyntaxFactory.Block(statement);
+            if (arrowExpression.ArrowToken.TrailingTrivia.Any(t => t.IsSingleOrMultiLineComment()))
+            {
+                statement = statement.WithPrependedLeadingTrivia(arrowExpression.ArrowToken.TrailingTrivia);
+            }
+
+            block = SyntaxFactory.Block(statement);
+            return true;
         }
 
         private static StatementSyntax ConvertToStatement(
@@ -29,8 +45,18 @@ internal static class ArrowExpressionClauseSyntaxExtensions
             }
             else if (createReturnStatementForExpression)
             {
-                return SyntaxFactory.ReturnStatement(expression)
-                                    .WithSemicolonToken(semicolonToken);
+                if (expression.GetLeadingTrivia().Any(t => t.IsSingleOrMultiLineComment()))
+                {
+                    return SyntaxFactory.ReturnStatement(expression.WithLeadingTrivia(SyntaxFactory.ElasticSpace))
+                                        .WithSemicolonToken(semicolonToken)
+                                        .WithLeadingTrivia(expression.GetLeadingTrivia())
+                                        .WithPrependedLeadingTrivia(SyntaxFactory.ElasticMarker);
+                }
+                else
+                {
+                    return SyntaxFactory.ReturnStatement(expression)
+                                        .WithSemicolonToken(semicolonToken);
+                }
             }
             else
             {

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

@@ -70,9 +70,9 @@ internal static class BlockSyntaxExtensions
             {
                 if (returnStatement.Expression != null)
                 {
-                    // If there are any comments on the return keyword, move them to
+                    // If there are any comments or directives on the return keyword, move them to
                     // the expression.
-                    expression = firstStatement.GetLeadingTrivia().Any(t => t.IsSingleOrMultiLineComment())
+                    expression = firstStatement.GetLeadingTrivia().Any(t => t.IsDirective || t.IsSingleOrMultiLineComment())
                         ? returnStatement.Expression.WithLeadingTrivia(returnStatement.GetLeadingTrivia())
                         : returnStatement.Expression;
                     semicolonToken = returnStatement.SemicolonToken;