[analysis_server] Resolve TODO in convert_to_switch_expression quick assist

I'm looking to make some additions to this assist, but to make review easier and assist my understanding, I combined the logic for determining if a switch statement is supported, and if so, what kind it is.

Change-Id: Ic5c48df82c305c31898dd884178ce78ce7da28f7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/336782
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Auto-Submit: Parker Lougheed <parlough@gmail.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>

pkg/analysis_server/lib/src/services/correction/dart/convert_to_switch_expression.dart

@@ -40,13 +40,16 @@ class ConvertToSwitchExpression extends ResolvedCorrectionProducer {
       }
     }
 
-    if (isReturnSwitch(switchStatement)) {
-      await convertReturnSwitchExpression(
-          builder, switchStatement, followingThrow);
-    } else if (isAssignmentSwitch(switchStatement)) {
-      await convertAssignmentSwitchExpression(builder, switchStatement);
-    } else if (isArgumentSwitch(switchStatement)) {
-      await convertArgumentSwitchExpression(builder, switchStatement);
+    switch (_getSupportedSwitchType(switchStatement)) {
+      case _SupportedSwitchType.returnValue:
+        await convertReturnSwitchExpression(
+            builder, switchStatement, followingThrow);
+      case _SupportedSwitchType.assignment:
+        await convertAssignmentSwitchExpression(builder, switchStatement);
+      case _SupportedSwitchType.argument:
+        await convertArgumentSwitchExpression(builder, switchStatement);
+      case null:
+        return;
     }
   }
 
@@ -262,73 +265,6 @@ class ConvertToSwitchExpression extends ResolvedCorrectionProducer {
     return deletion;
   }
 
-  // TODO(pq): refactor the `is` checks to a single `getSwitchKind`
-  // that only looks at members once
-  // see: https://dart-review.googlesource.com/c/sdk/+/287904
-  bool isArgumentSwitch(SwitchStatement node) {
-    for (var member in node.members) {
-      if (member is! SwitchPatternCase && member is! SwitchDefault) {
-        return false;
-      }
-      if (member.labels.isNotEmpty) return false;
-      var statements = member.statements;
-      if (statements.length == 1) {
-        if (statements.first.isThrowExpressionStatement) continue;
-      } else if (statements.length == 2) {
-        if (statements[1] is! BreakStatement) return false;
-      } else {
-        return false;
-      }
-
-      var s = statements.first;
-      if (s is! ExpressionStatement) return false;
-      var expression = s.expression;
-      if (expression is! MethodInvocation) return false;
-      var element = expression.methodName.staticElement;
-      if (element is! FunctionElement) return false;
-      if (functionElement == null) {
-        functionElement = element;
-      } else {
-        if (functionElement != element) return false;
-      }
-    }
-
-    return functionElement != null;
-  }
-
-  bool isAssignmentSwitch(SwitchStatement node) {
-    for (var member in node.members) {
-      if (member is! SwitchPatternCase && member is! SwitchDefault) {
-        return false;
-      }
-      if (member.labels.isNotEmpty) return false;
-      var statements = member.statements;
-      if (statements.length == 1) {
-        if (statements.first.isThrowExpressionStatement) continue;
-      } else if (statements.length == 2) {
-        if (statements[1] is! BreakStatement) return false;
-      } else {
-        return false;
-      }
-
-      var s = statements.first;
-      if (s is! ExpressionStatement) return false;
-      var expression = s.expression;
-      if (expression is! AssignmentExpression) return false;
-      var leftHandSide = expression.leftHandSide;
-      if (leftHandSide is! SimpleIdentifier) return false;
-      if (writeElement == null) {
-        var element = leftHandSide.staticElement;
-        if (element is! LocalVariableElement) return false;
-        writeElement = element;
-      } else {
-        if (writeElement != leftHandSide.staticElement) return false;
-      }
-    }
-
-    return writeElement != null;
-  }
-
   bool isEffectivelyExhaustive(SwitchStatement node, DartType? expressionType) {
     if (expressionType == null) return false;
     if ((typeSystem as TypeSystemImpl).isAlwaysExhaustive(expressionType)) {
@@ -342,23 +278,6 @@ class ConvertToSwitchExpression extends ResolvedCorrectionProducer {
     return last is SwitchDefault;
   }
 
-  bool isReturnSwitch(SwitchStatement node) {
-    for (var member in node.members) {
-      if (member is! SwitchPatternCase && member is! SwitchDefault) {
-        return false;
-      }
-      if (member.labels.isNotEmpty) return false;
-      var statements = member.statements;
-      if (statements.length != 1) return false;
-      var s = statements.first;
-      if (s is ReturnStatement && s.expression != null) continue;
-      if (s is! ExpressionStatement || s.expression is! ThrowExpression) {
-        return false;
-      }
-    }
-    return true;
-  }
-
   void _deleteStatements(
     DartFileEditBuilder builder,
     List<Statement> statements,
@@ -367,6 +286,118 @@ class ConvertToSwitchExpression extends ResolvedCorrectionProducer {
     builder.addDeletion(range);
   }
 
+  _SupportedSwitchType? _getSupportedSwitchType(SwitchStatement node) {
+    var members = node.members;
+    if (members.isEmpty) {
+      return null;
+    }
+
+    var canBeReturn = true;
+    var canBeAssignment = true;
+    var canBeArgument = true;
+
+    for (var member in members) {
+      // Each member must be a pattern-based case or a default.
+      if (member is! SwitchPatternCase && member is! SwitchDefault) {
+        return null;
+      }
+
+      if (member.labels.isNotEmpty) return null;
+
+      var statements = member.statements;
+      // We currently only support converting switch members
+      // with one non-break statement.
+      if (statements.isEmpty || statements.length > 2) {
+        return null;
+      }
+
+      if (statements case [_, var secondStatement]) {
+        // If there is a second statement, it must be a break statement.
+        if (secondStatement is! BreakStatement) return null;
+
+        // A return switch case can't have a second statement.
+        canBeReturn = false;
+      }
+
+      var statement = statements.first;
+      if (statement is ExpressionStatement) {
+        var expression = statement.expression;
+        // Any type of switch can have a throw expression as a statement.
+        if (expression is ThrowExpression) {
+          if (members.length == 1) {
+            // If there is only one case and it's a throw expression,
+            // then assume it's a return switch.
+            canBeAssignment = false;
+            canBeArgument = false;
+          }
+          continue;
+        }
+
+        // A return switch case's statement can't be a non-throw expression.
+        canBeReturn = false;
+
+        if (canBeArgument && expression is MethodInvocation) {
+          // An assignment switch case's statement can't be a method invocation.
+          canBeAssignment = false;
+
+          var element = expression.methodName.staticElement;
+          if (element is! FunctionElement) return null;
+          if (functionElement == null) {
+            functionElement = element;
+          } else if (functionElement != element) {
+            // The function invoked in each case must be the same.
+            return null;
+          }
+        } else if (canBeAssignment && expression is AssignmentExpression) {
+          // An argument switch case's statement can't be an assignment.
+          canBeArgument = false;
+
+          var leftHandSide = expression.leftHandSide;
+          if (leftHandSide is! SimpleIdentifier) return null;
+          if (writeElement == null) {
+            var element = leftHandSide.staticElement;
+            if (element is! LocalVariableElement) return null;
+            writeElement = element;
+          } else if (writeElement != leftHandSide.staticElement) {
+            // The variable written to in each case must be the same.
+            return null;
+          }
+        } else {
+          // The expression has an unsupported type.
+          return null;
+        }
+      } else {
+        // If the statement is not an expression,
+        // it must be a return statement with a
+        // non-null expression as part of a return switch.
+        if (!canBeReturn ||
+            statement is! ReturnStatement ||
+            statement.expression == null) {
+          return null;
+        }
+
+        canBeAssignment = false;
+        canBeArgument = false;
+      }
+
+      if (!canBeReturn && !canBeAssignment && !canBeArgument) {
+        return null;
+      }
+    }
+
+    if (canBeReturn) {
+      assert(!canBeAssignment && !canBeArgument);
+      return _SupportedSwitchType.returnValue;
+    } else if (canBeAssignment) {
+      assert(!canBeArgument);
+      return _SupportedSwitchType.assignment;
+    } else if (canBeArgument) {
+      return _SupportedSwitchType.argument;
+    }
+
+    return null;
+  }
+
   /// Given [nextLineOffset] that is an offset on the next line (the line
   /// before which we want to insert), inserts potentially multi-line [text]
   /// as separate full lines. Always adds EOL after [text].
@@ -405,6 +436,18 @@ final class _IndentationFullFirstRightAll extends _Indentation {
   });
 }
 
+/// The different switch types supported by this conversion.
+enum _SupportedSwitchType {
+  /// Each case statement returns a value.
+  returnValue,
+
+  /// Each case statement assigns to a local variable.
+  assignment,
+
+  /// Each case statement passes a value to the same function.
+  argument,
+}
+
 extension on Statement {
   bool get isThrowExpressionStatement {
     var self = this;

pkg/analysis_server/pubspec.yaml

@@ -93,6 +93,8 @@ dependency_overrides:
     path: ../../third_party/pkg/logging
   matcher:
     path: ../../third_party/pkg/matcher
+  memory_usage:
+    path: ../../third_party/pkg/leak_tracker/pkgs/memory_usage
   meta:
     path: ../meta
   mime:

pkg/analysis_server/test/src/services/correction/assist/convert_to_switch_expression_test.dart

@@ -19,12 +19,28 @@ class ConvertToSwitchExpressionTest extends AssistProcessorTest {
   @override
   AssistKind get kind => DartAssistKind.CONVERT_TO_SWITCH_EXPRESSION;
 
+  Future<void> test_argument_differentFunctions() async {
+    await resolveTestCode('''
+void f(String s) {
+  switch (s) {
+    case 'foo':
+      print('foo');
+    case _:
+      g('bar');
+  }
+}
+
+void g(String s) {}
+''');
+    await assertNoAssistAt('switch');
+  }
+
   Future<void> test_argument_switchExpression() async {
     await resolveTestCode('''
 enum Color {
   red, blue, green, yellow
 }
-    
+
 void f(Color color) {
   switch (color) {
     case Color.red:
@@ -47,7 +63,7 @@ void f(Color color) {
 enum Color {
   red, blue, green, yellow
 }
-    
+
 void f(Color color) {
   print(switch (color) {
     Color.red => 'red', // Red.
@@ -91,7 +107,7 @@ void f(String s) {
 enum Color {
   red, blue, green, yellow
 }
-    
+
 void f(Color color) {
   switch (color) {
     case Color.red:
@@ -111,7 +127,7 @@ void f(Color color) {
 enum Color {
   red, blue, green, yellow
 }
-    
+
 void f(Color color) {
   print(switch (color) {
     Color.red => 'red', // Red.
@@ -150,12 +166,37 @@ void f(String s) {
 ''');
   }
 
+  Future<void> test_assignment_differentVariables() async {
+    await resolveTestCode('''
+enum Color {
+  red, blue, green, yellow
+}
+
+String f(Color color) {
+  var name = '';
+  var favorite = '';
+  switch (color) {
+    case Color.red:
+      name = 'red';
+    case Color.blue:
+      favorite = 'blue';
+    case Color.green:
+      name = 'green';
+    case Color.yellow:
+      name = 'yellow';
+  }
+  return name;
+}
+''');
+    await assertNoAssistAt('switch');
+  }
+
   Future<void> test_assignment_switchExpression() async {
     await resolveTestCode('''
 enum Color {
   red, blue, green, yellow
 }
-    
+
 String f(Color color) {
   var name = '';
   switch (color) {
@@ -180,7 +221,7 @@ String f(Color color) {
 enum Color {
   red, blue, green, yellow
 }
-    
+
 String f(Color color) {
   var name = '';
   name = switch (color) {
@@ -230,7 +271,7 @@ String f(String s) {
 enum Color {
   red, blue, green, yellow
 }
-    
+
 String f(Color color) {
   var name = '';
   switch (color) {
@@ -252,7 +293,7 @@ String f(Color color) {
 enum Color {
   red, blue, green, yellow
 }
-    
+
 String f(Color color) {
   var name = '';
   name = switch (color) {
@@ -306,7 +347,67 @@ void f(int x) {
     await assertNoAssistAt('(x)');
   }
 
-  Future<void> test_return_notExhaustive_noAssist() async {
+  Future<void> test_return_justDefault_throw() async {
+    await resolveTestCode('''
+String f(int x) {
+  switch (x) {
+    default:
+      throw 'foo';
+  }
+}
+''');
+    await assertHasAssistAt('switch', '''
+String f(int x) {
+  return switch (x) {
+    _ => throw 'foo'
+  };
+}
+''');
+  }
+
+  Future<void> test_return_justDefault_value() async {
+    await resolveTestCode('''
+String f(int x) {
+  switch (x) {
+    default:
+      return 'foo';
+  }
+}
+''');
+    await assertHasAssistAt('switch', '''
+String f(int x) {
+  return switch (x) {
+    _ => 'foo'
+  };
+}
+''');
+  }
+
+  Future<void> test_return_multipleStatements() async {
+    await resolveTestCode('''
+enum Color {
+  red, orange, yellow, green
+}
+
+String name(Color color) {
+  switch (color) {
+    case Color.red:
+      print('red');
+      return 'red';
+    case Color.orange:
+      return 'orange';
+    case Color.green:
+      return 'green';
+    case Color.yellow:
+      return 'yellow';
+  }
+}
+''');
+
+    await assertNoAssistAt('switch');
+  }
+
+  Future<void> test_return_notExhaustive() async {
     await resolveTestCode('''
 String f(int i) {
   switch(i) {
@@ -327,7 +428,7 @@ String f(int i) {
 enum Color {
   red, orange, yellow, green
 }
-    
+
 String name(Color color) {
   switch (color) {
     case Color.red:
@@ -345,7 +446,7 @@ String name(Color color) {
 enum Color {
   red, orange, yellow, green
 }
-    
+
 String name(Color color) {
   return switch (color) {
     Color.red => throw 'red!',
@@ -362,7 +463,7 @@ String name(Color color) {
 enum Color {
   red, orange, yellow, green
 }
-    
+
 String name(Color color) {
   switch (color) {
     case Color.red:
@@ -380,7 +481,7 @@ String name(Color color) {
 enum Color {
   red, orange, yellow, green
 }
-    
+
 String name(Color color) {
   return switch (color) {
     Color.red => throw 'red!',
@@ -463,7 +564,7 @@ Color fromName(String name) {
 enum Color {
   red, orange, yellow, green
 }
-    
+
 String name(Color color) {
   switch (color) {
     case Color.red:
@@ -481,7 +582,7 @@ String name(Color color) {
 enum Color {
   red, orange, yellow, green
 }
-    
+
 String name(Color color) {
   return switch (color) {
     Color.red => throw 'red!',
@@ -498,7 +599,7 @@ String name(Color color) {
 enum Color {
   red, orange, yellow, green
 }
-    
+
 String name(Color color) {
   switch (color) {
     // Uh-oh.
@@ -519,7 +620,7 @@ String name(Color color) {
 enum Color {
   red, orange, yellow, green
 }
-    
+
 String name(Color color) {
   return switch (color) {
     // Uh-oh.