Merge pull request #32744 from dibarbet/issue_32480

Issue 32480 - Don't suggest adding braces with interceding compiler directive

src/EditorFeatures/CSharpTest/AddBraces/AddBracesTests.cs

@@ -8,6 +8,7 @@
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics;
 using Microsoft.CodeAnalysis.Test.Utilities;
+using Roslyn.Test.Utilities;
 using Xunit;
 
 namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.AddBraces
@@ -1322,6 +1323,503 @@ static void Main()
                 expectDiagnostic);
         }
 
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline)]
+        [InlineData((int)PreferBracesPreference.Always)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task DoNotFireForIfWhenIntercedingDirectiveBefore(int bracesPreference)
+        {
+            await TestMissingInRegularAndScriptAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+#if test
+        [|if (true)|]
+#endif
+            return;
+    }
+}",
+                new TestParameters(options: Option(CSharpCodeStyleOptions.PreferBraces, (PreferBracesPreference)bracesPreference, NotificationOption.Silent)));
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline)]
+        [InlineData((int)PreferBracesPreference.Always)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task DoNotFireForIfWithIntercedingDirectiveAfter(int bracesPreference)
+        {
+            await TestMissingInRegularAndScriptAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        [|if (true)|]
+#if test
+            return;
+#endif
+    }
+}",
+                new TestParameters(options: Option(CSharpCodeStyleOptions.PreferBraces, (PreferBracesPreference)bracesPreference, NotificationOption.Silent)));
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline)]
+        [InlineData((int)PreferBracesPreference.Always)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task DoNotFireForIfElseWithIntercedingDirectiveInBoth(int bracesPreference)
+        {
+            await TestMissingInRegularAndScriptAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        [|if (true)
+#if test
+            return;
+        else|]
+#endif
+            return;
+    }
+}",
+                new TestParameters(options: Option(CSharpCodeStyleOptions.PreferBraces, (PreferBracesPreference)bracesPreference, NotificationOption.Silent)));
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None, false)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline, false)]
+        [InlineData((int)PreferBracesPreference.Always, true)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task OnlyFireForIfWithIntercedingDirectiveInElseAroundIf(int bracesPreference, bool expectDiagnostic)
+        {
+            await TestAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+#if test
+        [|if (true)
+            return;
+        else|]
+#endif
+            return;
+    }
+}",
+
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+#if test
+        if (true)
+        {
+            return;
+        }
+        else
+#endif
+            return;
+    }
+}",
+                (PreferBracesPreference)bracesPreference,
+                expectDiagnostic);
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None, false)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline, false)]
+        [InlineData((int)PreferBracesPreference.Always, true)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task OnlyFireForElseWithIntercedingDirectiveInIfAroundElse(int bracesPreference, bool expectDiagnostic)
+        {
+            await TestAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        [|if (true)
+#if test
+            return;
+        else|]
+            return;
+#endif
+    }
+}",
+
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        if (true)
+#if test
+            return;
+        else
+        {
+            return;
+        }
+#endif
+    }
+}",
+                (PreferBracesPreference)bracesPreference,
+                expectDiagnostic);
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None, false)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline, false)]
+        [InlineData((int)PreferBracesPreference.Always, true)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task OnlyFireForElseWithIntercedingDirectiveInIf(int bracesPreference, bool expectDiagnostic)
+        {
+            await TestAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+#if test
+        [|if (true)
+#endif
+            return;
+        else|]
+            return;
+    }
+}",
+
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+#if test
+        if (true)
+#endif
+            return;
+        else
+        {
+            return;
+        }
+    }
+}",
+                (PreferBracesPreference)bracesPreference,
+                expectDiagnostic);
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None, false)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline, false)]
+        [InlineData((int)PreferBracesPreference.Always, true)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task OnlyFireForIfWithIntercedingDirectiveInElse(int bracesPreference, bool expectDiagnostic)
+        {
+            await TestAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        [|if (true)
+            return;
+        else|]
+#if test
+            return;
+#endif
+    }
+}",
+
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        if (true)
+        {
+            return;
+        }
+        else
+#if test
+            return;
+#endif
+    }
+}",
+                (PreferBracesPreference)bracesPreference,
+                expectDiagnostic);
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None, false)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline, true)]
+        [InlineData((int)PreferBracesPreference.Always, true)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task FireForIfElseWithDirectiveAroundIf(int bracesPreference, bool expectDiagnostic)
+        {
+            await TestAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+#if test
+        [|if (true)
+            return;
+#endif
+        else|]
+        {
+            return;
+        }
+    }
+}",
+
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+#if test
+        if (true)
+        {
+            return;
+        }
+#endif
+        else
+        {
+            return;
+        }
+    }
+}",
+                (PreferBracesPreference)bracesPreference,
+                expectDiagnostic);
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None, false)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline, true)]
+        [InlineData((int)PreferBracesPreference.Always, true)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task FireForIfElseWithDirectiveAroundElse(int bracesPreference, bool expectDiagnostic)
+        {
+            await TestAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        [|if (true)
+        {
+            return;
+        }
+#if test
+        else|]
+            return;
+#endif
+    }
+}",
+
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        if (true)
+        {
+            return;
+        }
+#if test
+        else
+        {
+            return;
+        }
+#endif
+    }
+}",
+                (PreferBracesPreference)bracesPreference,
+                expectDiagnostic);
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None, false)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline, false)]
+        [InlineData((int)PreferBracesPreference.Always, true)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task FireForIfWithoutIntercedingDirective(int bracesPreference, bool expectDiagnostic)
+        {
+            await TestAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+#if test
+#endif
+        [|if (true)|]
+            return;
+    }
+}",
+
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+#if test
+#endif
+        if (true)
+        {
+            return;
+        }
+    }
+}",
+                (PreferBracesPreference)bracesPreference,
+                expectDiagnostic);
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None, false)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline, false)]
+        [InlineData((int)PreferBracesPreference.Always, true)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task FireForIfWithDirectiveAfterEmbeddedStatement(int bracesPreference, bool expectDiagnostic)
+        {
+            await TestAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        [|if (true)|]
+            return;
+#if test
+#endif
+    }
+}",
+
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        if (true)
+        {
+            return;
+        }
+#if test
+#endif
+    }
+}",
+                (PreferBracesPreference)bracesPreference,
+                expectDiagnostic);
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None, false)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline, false)]
+        [InlineData((int)PreferBracesPreference.Always, true)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task FireForInnerNestedStatementWhenDirectiveEntirelyInside(int bracesPreference, bool expectDiagnostic)
+        {
+            await TestAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        [|while (true)
+#if test
+            if (true)|]
+                return;
+#endif
+    }
+}",
+
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        while (true)
+#if test
+            if (true)
+            {
+                return;
+            }
+#endif
+    }
+}",
+                (PreferBracesPreference)bracesPreference,
+                expectDiagnostic);
+        }
+
+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddBraces)]
+        [InlineData((int)PreferBracesPreference.None, false)]
+        [InlineData((int)PreferBracesPreference.WhenMultiline, true)]
+        [InlineData((int)PreferBracesPreference.Always, true)]
+        [WorkItem(32480, "https://github.com/dotnet/roslyn/issues/32480")]
+        public async Task FireForOuterNestedStatementWhenDirectiveEntirelyInside(int bracesPreference, bool expectDiagnostic)
+        {
+            await TestAsync(
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        [|while (true)
+#if test
+            if (true)|]
+#endif
+                return;
+    }
+}",
+
+   @"
+#define test
+class Program
+{
+    static void Main()
+    {
+        while (true)
+        {
+#if test
+            if (true)
+#endif
+                return;
+        }
+    }
+}",
+                (PreferBracesPreference)bracesPreference,
+                expectDiagnostic);
+        }
+
         private async Task TestAsync(string initialMarkup, string expectedMarkup, PreferBracesPreference bracesPreference, bool expectDiagnostic)
         {
             if (expectDiagnostic)

src/Features/CSharp/Portable/AddBraces/CSharpAddBracesDiagnosticAnalyzer.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.Diagnostics;
+using System.Threading;
 using Microsoft.CodeAnalysis.CodeStyle;
 using Microsoft.CodeAnalysis.CSharp.CodeStyle;
 using Microsoft.CodeAnalysis.CSharp.Extensions;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Text;
 using FormattingRangeHelper = Microsoft.CodeAnalysis.CSharp.Utilities.FormattingRangeHelper;
 
 namespace Microsoft.CodeAnalysis.CSharp.Diagnostics.AddBraces
@@ -99,6 +101,11 @@ public void AnalyzeNode(SyntaxNodeAnalysisContext context)
                 return;
             }
 
+            if (ContainsInterleavedDirective(statement, embeddedStatement, cancellationToken))
+            {
+                return;
+            }
+
             var firstToken = statement.GetFirstToken();
             context.ReportDiagnostic(DiagnosticHelper.Create(
                 Descriptor,
@@ -109,6 +116,28 @@ public void AnalyzeNode(SyntaxNodeAnalysisContext context)
                 SyntaxFacts.GetText(firstToken.Kind())));
         }
 
+        /// <summary>
+        /// Check if there are interleaved directives on the statement.
+        /// Handles special case with if/else.
+        /// </summary>
+        private static bool ContainsInterleavedDirective(SyntaxNode statement, StatementSyntax embeddedStatement, CancellationToken cancellationToken)
+        {
+            if (statement.Kind() == SyntaxKind.IfStatement)
+            {
+                var ifStatementNode = (IfStatementSyntax)statement;
+                var elseNode = ifStatementNode.Else;
+                if (elseNode != null && !embeddedStatement.IsMissing)
+                {
+                    // For IF/ELSE statements, only the IF part should be checked for interleaved directives when the diagnostic is triggered on the IF.
+                    // A separate diagnostic will be triggered to handle the ELSE part.
+                    var ifStatementSpanWithoutElse = TextSpan.FromBounds(statement.Span.Start, embeddedStatement.Span.End);
+                    return statement.ContainsInterleavedDirective(ifStatementSpanWithoutElse, cancellationToken);
+                }
+            }
+
+            return statement.ContainsInterleavedDirective(cancellationToken);
+        }
+
         /// <summary>
         /// <para>In general, statements are considered multiline if any of the following span more than one line:</para>
         /// <list type="bullet">

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

@@ -254,6 +254,13 @@ public static bool IsAnyLambdaOrAnonymousMethod(this SyntaxNode node)
         public static bool ContainsInterleavedDirective(this SyntaxNode syntaxNode, CancellationToken cancellationToken)
             => CSharpSyntaxFactsService.Instance.ContainsInterleavedDirective(syntaxNode, cancellationToken);
 
+        /// <summary>
+        /// Similar to <see cref="ContainsInterleavedDirective(SyntaxNode, CancellationToken)"/> except that the span to check
+        /// for interleaved directives can be specified separately to the node passed in.
+        /// </summary>
+        public static bool ContainsInterleavedDirective(this SyntaxNode syntaxNode, TextSpan span, CancellationToken cancellationToken)
+            => CSharpSyntaxFactsService.Instance.ContainsInterleavedDirective(span, syntaxNode, cancellationToken);
+
         public static bool ContainsInterleavedDirective(
             this SyntaxToken token,
             TextSpan textSpan,

src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/AbstractSyntaxFactsService.cs

@@ -411,7 +411,7 @@ public ImmutableArray<SyntaxTrivia> GetFileBanner(SyntaxToken firstToken)
         public bool ContainsInterleavedDirective(SyntaxNode node, CancellationToken cancellationToken)
             => ContainsInterleavedDirective(node.Span, node, cancellationToken);
 
-        private bool ContainsInterleavedDirective(
+        public bool ContainsInterleavedDirective(
             TextSpan span, SyntaxNode node, CancellationToken cancellationToken)
         {
             foreach (var token in node.DescendantTokens())