Merge pull request #18914 from CyrusNajmabadi/convertIfToSwitchOnlyForComplexCases

Don't offer 'convert if to switch' for basic if-tests.

src/EditorFeatures/CSharpTest/CodeActions/ConvertIfToSwitch/ConvertIfToSwitchTests.cs

@@ -223,28 +223,16 @@ void M(int i, int j)
         }
 
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertIfToSwitch)]
-        public async Task TestSingleCase()
+        public async Task TestMissingOnSingleCase()
         {
-            await TestInRegularAndScriptAsync(
+            await TestMissingAsync(
 @"class C
 {
     void M(int i)
     {
         [||]if (i == 5) {}
     }
-}",
-@"class C
-{
-    void M(int i)
-    {
-        switch (i)
-        {
-            case 5:
-                break;
-        }
-    }
-}
-");
+}");
         }
 
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertIfToSwitch)]
@@ -283,7 +271,9 @@ public async Task TestIsPatternExpression_01()
     void M(object o)
     {
         [||]if (o is int i)
-            return;
+                return;
+            else if (o is string s)
+                return;
     }
 }",
 @"class C
@@ -294,6 +284,8 @@ void M(object o)
         {
             case int i:
                 return;
+            case string s:
+                return;
         }
     }
 }");
@@ -308,7 +300,9 @@ public async Task TestIsPatternExpression_02()
     void M(object o)
     {
         [||]if (o is string s && s.Length == 5)
-            return;
+                return;
+            else if (o is int i)
+                return;
     }
 }",
 @"class C
@@ -319,6 +313,8 @@ void M(object o)
         {
             case string s when s.Length == 5:
                 return;
+            case int i:
+                return;
         }
     }
 }");
@@ -333,7 +329,9 @@ public async Task TestIsPatternExpression_03()
     void M(object o)
     {
         [||]if (o is string s && (s.Length > 5 && s.Length < 10))
-            return;
+                return;
+            else if (o is int i)
+                return;
     }
 }",
 @"class C
@@ -344,6 +342,8 @@ void M(object o)
         {
             case string s when s.Length > 5 && s.Length < 10:
                 return;
+            case int i:
+                return;
         }
     }
 }");
@@ -358,7 +358,9 @@ public async Task TestIsPatternExpression_04()
     void M(object o)
     {
         [||]if (o is string s && s.Length > 5 && s.Length < 10)
-            return;
+                return;
+            else if (o is int i)
+                return;
     }
 }",
 @"class C
@@ -369,6 +371,8 @@ void M(object o)
         {
             case string s when s.Length > 5 && s.Length < 10:
                 return;
+            case int i:
+                return;
         }
     }
 }");
@@ -388,6 +392,10 @@ void M(object o)
                 M(o:   0);
 
             }
+            else if (o is int i)
+            {
+                M(o:   0);
+            }
     }
 }",
 @"class C
@@ -397,13 +405,15 @@ void M(object o)
         switch (o)
         {
             case string s when s.Length > 5 &&
-                               s.Length < 10:
+                                 s.Length < 10:
+                M(o:   0);
+                break;
+            case int i:
                 M(o:   0);
-
                 break;
         }
     }
-}");
+}", ignoreTrivia: false);
         }
 
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertIfToSwitch)]
@@ -431,6 +441,9 @@ void M(int i)
         {
             var x = i;
         }
+        else if (i == 4)
+        {
+        }
     }
 }",
 @"class C
@@ -444,6 +457,8 @@ void M(int i)
                     var x = i;
                     break;
                 }
+            case 4:
+                break;
         }
     }
 }");
@@ -497,6 +512,9 @@ void M(int i)
                 break;
             }
         }
+        else if (i == 2)
+        {
+        }
     }
 }",
 @"class C
@@ -511,6 +529,8 @@ void M(int i)
                     break;
                 }
                 break;
+            case 2:
+                break;
         }
     }
 }");
@@ -586,7 +606,7 @@ int M(int? i)
     {
         while (true)
         {
-            [||]if (i == null) return 5;
+            [||]if (i == null) return 5; else if (i == 1) return 1;
             if (i == 0) break;
             if (i == 1) return 6;
             return 7;
@@ -603,6 +623,8 @@ int M(int? i)
             {
                 case null:
                     return 5;
+                case 1:
+                    return 1;
             }
             if (i == 0) break;
             if (i == 1) return 6;
@@ -729,6 +751,11 @@ int M(int i)
         {
             return 4;
         }
+        else if (i == 1)
+        {
+            return 1;
+        }
+
         if (i == 10)
         {
             return 5;
@@ -752,6 +779,8 @@ int M(int i)
         {
             case 5:
                 return 4;
+            case 1:
+                return 1;
         }
         if (i == 10)
         {

src/EditorFeatures/VisualBasicTest/CodeActions/ConvertIfToSwitch/ConvertIfToSwitchTests.vb

@@ -88,20 +88,13 @@ End Class")
         End Function
 
         <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertIfToSwitch)>
-        Public Async Function TestSingleCase() As Task
-            Await TestInRegularAndScriptAsync(
+        Public Async Function TestMissingForSingleCase() As Task
+            Await TestMissingAsync(
 "Class C
     Sub M(i As Integer)
         [||]If i = 5 Then
         End If
     End Sub
-End Class",
-"Class C
-    Sub M(i As Integer)
-        Select i
-            Case 5
-        End Select
-    End Sub
 End Class")
         End Function
 
@@ -111,6 +104,7 @@ End Class")
 "Class C
     Sub M(i As Integer)
         [||]If 5 >= i AndAlso 1 <= i Then
+        Else If 7 >= i AndAlso 6 <= i
         End If
     End Sub
 End Class",
@@ -118,6 +112,7 @@ End Class",
     Sub M(i As Integer)
         Select i
             Case 1 To 5
+            Case 6 To 7
         End Select
     End Sub
 End Class")
@@ -311,7 +306,7 @@ End Class")
             Await TestInRegularAndScriptAsync(
 "Class C
     Function M(i As Integer) As Integer
-        [||]If i = 10 Then Return 5
+        [||]If i = 10 Then Return 5 Else Return 4
         If i = 20 Then
             Return 6
         ElseIf i = i Then
@@ -326,6 +321,8 @@ End Class",
         Select i
             Case 10
                 Return 5
+            Case Else
+                Return 4
         End Select
         If i = 20 Then
             Return 6
@@ -346,6 +343,7 @@ End Class")
         While i = i
             [||]If i = 10 Then
                 Exit While
+            Else If i = 1 Then
             End If
         End While
     End Sub
@@ -356,6 +354,7 @@ End Class",
             Select i
                 Case 10
                     Exit While
+                Case 1
             End Select
         End While
     End Sub

src/Features/Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.cs

@@ -77,6 +77,7 @@ public async Task ComputeRefactoringsAsync(CodeRefactoringContext context)
                     return;
                 }
 
+                // To prevent noisiness, only show this feature on the 'if' keyword of the if-statement.
                 var token = ifStatement.GetFirstToken();
                 if (!token.Span.Contains(context.Span))
                 {
@@ -84,7 +85,22 @@ public async Task ComputeRefactoringsAsync(CodeRefactoringContext context)
                 }
 
                 var switchSections = GetSections(ifStatement).ToList();
-                if (switchSections.Count == 0)
+
+                // To prevent noisiness we don't offer this unless we're going to generate at least
+                // two switch labels.  It can be quite annoying to basically have this offered
+                // on pretty much any simple 'if' like "if (a == 0)" or "if (x == null)".  In these
+                // cases, the converted code just looks and feels worse, and it ends up causing the
+                // lightbulb to appear too much.
+                //
+                // This does mean that if someone has a simple if, and is about to add a lot more 
+                // cases, and says to themselves "let me convert this to a switch first!", then they'll
+                // be out of luck.  However, I believe the core value here is in taking existing large
+                // if-chains/checks and easily converting them over to a switch.  So not offering the
+                // feature on simple if-statements seems like an acceptable compromise to take to ensure
+                // the overall user experience isn't degraded.
+                var labelCount = switchSections.SelectMany(t => t.patterns).Count() +
+                    (_switchDefaultBodyOpt.HasValue ? 1 : 0);
+                if (labelCount < 2)
                 {
                     return;
                 }
@@ -93,7 +109,7 @@ public async Task ComputeRefactoringsAsync(CodeRefactoringContext context)
                     UpdateDocumentAsync(root, document, ifStatement, switchSections)));
             }
 
-            private IEnumerable<(IEnumerable<IPattern<TSwitchLabelSyntax>>, TStatementSyntax)> GetSections(
+            private IEnumerable<(IEnumerable<IPattern<TSwitchLabelSyntax>> patterns, TStatementSyntax statement)> GetSections(
                 TIfStatementSyntax rootIfStatement)
             {
                 // Iterate over subsequent if-statements whose endpoint is unreachable.