Merge pull request #36124 from CyrusNajmabadi/directiveCheck

Don't offer 'convert if to conditional' if it crosses a preprocessor directive.

src/EditorFeatures/CSharpTest/UseConditionalExpression/UseConditionalExpressionForReturnTests.cs

@@ -821,6 +821,26 @@ IEnumerable<bool> M(int a)
     {
         yield return a != 0;
     }
+}");
+        }
+
+        [WorkItem(36117, "https://github.com/dotnet/roslyn/issues/36117")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseConditionalExpression)]
+        public async Task TestMissingWhenCrossingPreprocessorDirective()
+        {
+            await TestMissingInRegularAndScriptAsync(
+@"
+class C
+{
+    int M()
+    {
+        bool check = true;
+#if true
+        [||]if (check)
+            return 3;
+#endif
+        return 2;
+    }
 }");
         }
     }

src/EditorFeatures/VisualBasicTest/UseConditionalExpression/UseConditionalExpressionForReturnTests.vb

@@ -18,7 +18,7 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.UseConditionalExpr
         Public Async Function TestOnSimpleReturn() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         [||]if true
             return 0
@@ -28,7 +28,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as integer
         Return If(true, 0, 1)
     end function
@@ -39,7 +39,7 @@ end class")
         Public Async Function TestOnSimpleReturnNoBlocks() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         [||]if true
             return 0
@@ -49,7 +49,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as integer
         Return If(true, 0, 1)
     end function
@@ -60,7 +60,7 @@ end class")
         Public Async Function TestOnSimpleReturnNoBlocks_NotInBlock() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         if true
             [||]if true
@@ -72,7 +72,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as integer
         if true
             Return If(true, 0, 1)
@@ -85,7 +85,7 @@ end class")
         Public Async Function TestMissingReturnValue1() As Task
             Await TestMissingInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         [||]if true
             return 0
@@ -100,7 +100,7 @@ end class")
         Public Async Function TestMissingReturnValue2() As Task
             Await TestMissingInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         [||]if true
             return
@@ -115,7 +115,7 @@ end class")
         Public Async Function TestMissingReturnValue3() As Task
             Await TestMissingInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         [||]if true
             return
@@ -130,7 +130,7 @@ end class")
         Public Async Function TestWithNoElseBlockButFollowingReturn() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         [||]if true
             return 0
@@ -140,7 +140,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as integer
         Return If(true, 0, 1)
     end function
@@ -151,7 +151,7 @@ end class")
         Public Async Function TestMissingWithoutElse() As Task
             Await TestMissingInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         [||]if true
             return 0
@@ -164,7 +164,7 @@ end class")
         Public Async Function TestConversion1() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as object
         [||]if true
             return ""a""
@@ -174,7 +174,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as object
         Return If(true, ""a"", ""b"")
     end function
@@ -185,7 +185,7 @@ end class")
         Public Async Function TestConversion2() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as string
         [||]if true
             return ""a""
@@ -195,7 +195,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as string
         Return If(true, ""a"", nothing)
     end function
@@ -206,7 +206,7 @@ end class")
         Public Async Function TestConversion3() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as string
         [||]if true
             return nothing
@@ -216,7 +216,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as string
         Return If(true, nothing, DirectCast(nothing, String))
     end function
@@ -227,7 +227,7 @@ end class")
         Public Async Function TestKeepTriviaAroundIf() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         ' leading
         [||]if true
@@ -238,7 +238,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as integer
         ' leading
         Return If(true, 0, 1) ' trailing
@@ -250,7 +250,7 @@ end class")
         Public Async Function TestFixAll1() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         {|FixAllInDocument:if|} true
             return 0
@@ -266,7 +266,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as integer
         Return If(true, 0, 1)
 
@@ -279,7 +279,7 @@ end class")
         Public Async Function TestMultiLine1() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         [||]if true
             return Foo(
@@ -290,7 +290,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as integer
         Return If(true,
             Foo(
@@ -304,7 +304,7 @@ end class")
         Public Async Function TestMultiLine2() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         [||]if true
             return 0
@@ -315,7 +315,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as integer
         Return If(true,
             0,
@@ -329,7 +329,7 @@ end class")
         Public Async Function TestMultiLine3() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     function M() as integer
         [||]if true
             return Foo(
@@ -341,7 +341,7 @@ class
     end function
 end class",
 "
-class 
+class C
     function M() as integer
         Return If(true,
             Foo(
@@ -357,7 +357,7 @@ end class")
         Public Async Function TestOnYield() As Task
             Await TestInRegularAndScriptAsync(
 "
-class 
+class C
     iterator function M() as integer
         [||]if true
             yield 0
@@ -367,7 +367,7 @@ class
     end function
 end class",
 "
-class 
+class C
     iterator function M() as integer
         Yield If(true, 0, 1)
     end function
@@ -381,7 +381,7 @@ end class")
 "
 imports system.collections.generic
 
-class 
+class C
     iterator function M() as IEnumerable(of integer)
         [||]if true
             yield 0
@@ -393,10 +393,45 @@ end class",
 "
 imports system.collections.generic
 
-class 
+class C
     iterator function M() as IEnumerable(of integer)
         Yield If(true, 0, 1)
     end function
+end class")
+        End Function
+
+        <WorkItem(36117, "https://github.com/dotnet/roslyn/issues/36117")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseConditionalExpression)>
+        Public Async Function TestMissingWhenCrossingPreprocessorDirective1() As Task
+            Await TestMissingInRegularAndScriptAsync(
+"
+class C
+    function M() as integer
+        dim check as boolean = true
+#if true
+        [||]if check
+            return 3
+#end if
+        return 2
+    end function
+end class")
+        End Function
+
+        <WorkItem(36117, "https://github.com/dotnet/roslyn/issues/36117")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseConditionalExpression)>
+        Public Async Function TestMissingWhenCrossingPreprocessorDirective2() As Task
+            Await TestMissingInRegularAndScriptAsync(
+"
+class C
+    function M() as integer
+        dim check as boolean = true
+#if true
+        [||]if check
+            return 3
+        end if
+#end if
+        return 2
+    end function
 end class")
         End Function
     End Class

src/Features/Core/Portable/UseConditionalExpression/AbstractUseConditionalExpressionDiagnosticAnalyzer.cs

@@ -39,8 +39,7 @@ protected sealed override void InitializeWorker(AnalysisContext context)
         private void AnalyzeOperation(OperationAnalysisContext context)
         {
             var ifOperation = (IConditionalOperation)context.Operation;
-            var ifStatement = ifOperation.Syntax as TIfStatementSyntax;
-            if (ifStatement == null)
+            if (!(ifOperation.Syntax is TIfStatementSyntax ifStatement))
             {
                 return;
             }

src/Features/Core/Portable/UseConditionalExpression/ForReturn/UseConditionalExpressionForReturnHelpers.cs

@@ -35,8 +35,7 @@ internal static class UseConditionalExpressionForReturnHelpers
 
             if (falseStatement == null)
             {
-                var parentBlock = ifOperation.Parent as IBlockOperation;
-                if (parentBlock == null)
+                if (!(ifOperation.Parent is IBlockOperation parentBlock))
                 {
                     return false;
                 }

src/Features/Core/Portable/UseConditionalExpression/UseConditionalExpressionHelpers.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;
-using System.Collections.Generic;
-using System.Collections.Immutable;
-using System.Linq;
-using System.Threading;
-using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Editing;
-using Microsoft.CodeAnalysis.Formatting;
-using Microsoft.CodeAnalysis.Formatting.Rules;
 using Microsoft.CodeAnalysis.LanguageServices;
 using Microsoft.CodeAnalysis.Operations;
-using Microsoft.CodeAnalysis.Shared.Extensions;
 
 namespace Microsoft.CodeAnalysis.UseConditionalExpression
 {
@@ -23,9 +14,21 @@ internal static class UseConditionalExpressionHelpers
             ISyntaxFactsService syntaxFacts, IConditionalOperation ifOperation,
             IOperation whenTrue, IOperation whenFalse)
         {
-            // Will likely screw things up if the if directive spans any preprocessor directives.
-            // So do not offer for now.
-            if (syntaxFacts.SpansPreprocessorDirective(ifOperation.Syntax))
+            // Will likely screw things up if the if directive spans any preprocessor directives. So
+            // do not offer for now.  Note: we pass in both the node for the ifOperation and the
+            // whenFalse portion.  The whenFalse portion isn't necessary under the ifOperation.  For
+            // example in:
+            //
+            //  ```c#
+            //  #if DEBUG
+            //  if (check)
+            //      return 3;
+            //  #endif  
+            //  return 2;
+            //  ```
+            //
+            // In this case, we want to see that this cross the `#endif`
+            if (syntaxFacts.SpansPreprocessorDirective(ifOperation.Syntax, whenFalse.Syntax))
             {
                 return false;
             }

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

@@ -170,20 +170,8 @@ public static bool IsReturnableConstruct(this SyntaxNode node)
             return false;
         }
 
-        public static bool SpansPreprocessorDirective<TSyntaxNode>(
-            this IEnumerable<TSyntaxNode> list)
-            where TSyntaxNode : SyntaxNode
-        {
-            if (list == null || list.IsEmpty())
-            {
-                return false;
-            }
-
-            var tokens = list.SelectMany(n => n.DescendantTokens());
-
-            // todo: we need to dive into trivia here.
-            return tokens.SpansPreprocessorDirective();
-        }
+        public static bool SpansPreprocessorDirective<TSyntaxNode>(this IEnumerable<TSyntaxNode> list) where TSyntaxNode : SyntaxNode
+            => CSharpSyntaxFactsService.Instance.SpansPreprocessorDirective(list);
 
         public static TNode ConvertToSingleLine<TNode>(this TNode node, bool useElasticTrivia = false)
             where TNode : SyntaxNode

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

@@ -71,41 +71,7 @@ public static bool IsFirstTokenOnLine(this SyntaxToken token, SourceText text)
         }
 
         public static bool SpansPreprocessorDirective(this IEnumerable<SyntaxToken> tokens)
-        {
-            // we want to check all leading trivia of all tokens (except the 
-            // first one), and all trailing trivia of all tokens (except the
-            // last one).
-
-            var first = true;
-            var previousToken = default(SyntaxToken);
-
-            foreach (var token in tokens)
-            {
-                if (first)
-                {
-                    first = false;
-                }
-                else
-                {
-                    // check the leading trivia of this token, and the trailing trivia
-                    // of the previous token.
-                    if (SpansPreprocessorDirective(token.LeadingTrivia) ||
-                        SpansPreprocessorDirective(previousToken.TrailingTrivia))
-                    {
-                        return true;
-                    }
-                }
-
-                previousToken = token;
-            }
-
-            return false;
-        }
-
-        private static bool SpansPreprocessorDirective(SyntaxTriviaList list)
-        {
-            return list.Any(t => t.GetStructure() is DirectiveTriviaSyntax);
-        }
+            => CSharpSyntaxFactsService.Instance.SpansPreprocessorDirective(tokens);
 
         /// <summary>
         /// Retrieves all trivia after this token, including it's trailing trivia and

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs

@@ -126,9 +126,7 @@ public bool IsEntirelyWithinStringOrCharOrNumericLiteral(SyntaxTree syntaxTree,
         }
 
         public bool IsDirective(SyntaxNode node)
-        {
-            return node is DirectiveTriviaSyntax;
-        }
+            => node is DirectiveTriviaSyntax;
 
         public bool TryGetExternalSourceInfo(SyntaxNode node, out ExternalSourceInfo info)
         {
@@ -1949,8 +1947,5 @@ public void GetPartsOfCastExpression(SyntaxNode node, out SyntaxNode type, out S
 
             return null;
         }
-
-        public bool SpansPreprocessorDirective(IEnumerable<SyntaxNode> nodes)
-            => nodes.SpansPreprocessorDirective();
     }
 }

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

@@ -431,5 +431,61 @@ public string GetBannerText(SyntaxNode documentationCommentTriviaSyntax, int ban
             => DocumentationCommentService.GetBannerText(documentationCommentTriviaSyntax, bannerLength, cancellationToken);
 
         protected abstract IDocumentationCommentService DocumentationCommentService { get; }
+
+        public bool SpansPreprocessorDirective(IEnumerable<SyntaxNode> nodes)
+        {
+            if (nodes == null || nodes.IsEmpty())
+            {
+                return false;
+            }
+
+            return SpansPreprocessorDirective(nodes.SelectMany(n => n.DescendantTokens()));
+        }
+
+        /// <summary>
+        /// Determines if there is preprocessor trivia *between* any of the <paramref name="tokens"/>
+        /// provided.  The <paramref name="tokens"/> will be deduped and then ordered by position.
+        /// Specifically, the first token will not have it's leading trivia checked, and the last
+        /// token will not have it's trailing trivia checked.  All other trivia will be checked to
+        /// see if it contains a preprocessor directive.
+        /// </summary>
+        public bool SpansPreprocessorDirective(IEnumerable<SyntaxToken> tokens)
+        {
+            // we want to check all leading trivia of all tokens (except the 
+            // first one), and all trailing trivia of all tokens (except the
+            // last one).
+
+            var first = true;
+            var previousToken = default(SyntaxToken);
+
+            // Allow duplicate nodes/tokens to be passed in.  Also, allow the nodes/tokens
+            // to not be in any particular order when passed in.
+            var orderedTokens = tokens.Distinct().OrderBy(t => t.SpanStart);
+
+            foreach (var token in orderedTokens)
+            {
+                if (first)
+                {
+                    first = false;
+                }
+                else
+                {
+                    // check the leading trivia of this token, and the trailing trivia
+                    // of the previous token.
+                    if (SpansPreprocessorDirective(token.LeadingTrivia) ||
+                        SpansPreprocessorDirective(previousToken.TrailingTrivia))
+                    {
+                        return true;
+                    }
+                }
+
+                previousToken = token;
+            }
+
+            return false;
+        }
+
+        private bool SpansPreprocessorDirective(SyntaxTriviaList list)
+            => list.Any(t => IsPreprocessorDirective(t));
     }
 }

src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsServiceExtensions.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.Collections.Generic;
 using System.Collections.Immutable;
 using System.Linq;
 using Microsoft.CodeAnalysis.Shared.Extensions;
@@ -119,6 +120,9 @@ private static bool IsWordOrNumber(this ISyntaxFactsService syntaxFacts, SyntaxT
         public static bool SpansPreprocessorDirective(this ISyntaxFactsService service, SyntaxNode node)
             => service.SpansPreprocessorDirective(SpecializedCollections.SingletonEnumerable(node));
 
+        public static bool SpansPreprocessorDirective(this ISyntaxFactsService service, params SyntaxNode[] nodes)
+            => service.SpansPreprocessorDirective(nodes);
+
         public static bool IsWhitespaceOrEndOfLineTrivia(this ISyntaxFactsService syntaxFacts, SyntaxTrivia trivia)
             => syntaxFacts.IsWhitespaceTrivia(trivia) || syntaxFacts.IsEndOfLineTrivia(trivia);
 

src/Workspaces/VisualBasic/Portable/Extensions/SyntaxNodeExtensions.vb

@@ -235,13 +235,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Extensions
 
         <Extension()>
         Public Function SpansPreprocessorDirective(Of TSyntaxNode As SyntaxNode)(list As IEnumerable(Of TSyntaxNode)) As Boolean
-            If list Is Nothing OrElse Not list.Any() Then
-                Return False
-            End If
-
-            Dim tokens = list.SelectMany(Function(n) n.DescendantTokens())
-
-            Return tokens.SpansPreprocessorDirective()
+            Return VisualBasicSyntaxFactsService.Instance.SpansPreprocessorDirective(list)
         End Function
 
         <Extension()>

src/Workspaces/VisualBasic/Portable/Extensions/SyntaxTokenExtensions.vb

@@ -195,29 +195,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Extensions
 
         <Extension()>
         Public Function SpansPreprocessorDirective(tokens As IEnumerable(Of SyntaxToken)) As Boolean
-            ' we want to check all leading trivia of all tokens (except the 
-            ' first one), and all trailing trivia of all tokens (except the
-            ' last one).
-
-            Dim first As Boolean = True
-            Dim previousToken As SyntaxToken = Nothing
-
-            For Each token In tokens
-                If first Then
-                    first = False
-                Else
-                    ' check the leading trivia of this token, and the trailing trivia
-                    ' of the previous token.
-                    If token.LeadingTrivia.ContainsPreprocessorDirective() OrElse
-                       previousToken.TrailingTrivia.ContainsPreprocessorDirective() Then
-                        Return True
-                    End If
-                End If
-
-                previousToken = token
-            Next token
-
-            Return False
+            Return VisualBasicSyntaxFactsService.Instance.SpansPreprocessorDirective(tokens)
         End Function
 
         <Extension()>

src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSyntaxFactsService.vb

@@ -1913,8 +1913,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
             Return Nothing
         End Function
 
-        Public Function SpansPreprocessorDirective(nodes As IEnumerable(Of SyntaxNode)) As Boolean Implements ISyntaxFactsService.SpansPreprocessorDirective
-            Return nodes.SpansPreprocessorDirective()
+        Public Shadows Function SpansPreprocessorDirective(nodes As IEnumerable(Of SyntaxNode)) As Boolean Implements ISyntaxFactsService.SpansPreprocessorDirective
+            Return MyBase.SpansPreprocessorDirective(nodes)
+        End Function
+
+        Public Shadows Function SpansPreprocessorDirective(tokens As IEnumerable(Of SyntaxToken)) As Boolean
+            Return MyBase.SpansPreprocessorDirective(tokens)
         End Function
     End Class
 End Namespace