Reland "[pkg/vm] Handle switch statements in unreachable code elimina…

…tor."

This is a reland of commit 2623117

When running with sound null safety, reducing a logical expression to
the right hand side if the left hand side is constant and does not short
circuit is valid, as the right hand side is guaranteed not to evaluate
to null.

In other modes, however, the right hand side may evaluate to null.
Thus, in thos modes, we return the original node if the RHS is not a
constant boolean value, so that the operator can perform whatever null
checking is required.

Fixes: #54029

Original change's description:
> Reland "[pkg/vm] Handle switch statements in unreachable code eliminator."
>
> This is a reland of commit 92bf76d
>
> In the original CL, the changes to the UCE assumed all
> SwitchStatements were exhaustive. Now if a SwitchStatement isn't
> explicitly or implicitly (via a default case) exhaustive and no case
> matches the tested constant, the SwitchStatement is properly removed.
>
> In addition, if a guaranteed to match case is found, more than one
> cases remain (thus the SwitchStatement is not removed or replaced with
> the single case's body), and the default case is removed, then the
> resulting SwitchStatement is marked as explicitly exhaustive, as this
> serves as a signal to backends that they do not need to handle the
> possibility of no case matching in the absence of a default case.
>
> Original change's description:
> > [pkg/vm] Handle switch statements in unreachable code eliminator.
> >
> > Namely, if the tested expression for a switch statement is constant,
> > then we can remove any constant cases where the constants differ,
> > and if all but a single case is removed, we can replace the switch
> > with the case body.
> >
> > If constant functions are not enabled, then getters annotated with
> > @pragma("vm:platform-const") are still evaluated with the constant
> > function evaluation machinery, but only those and no others (including
> > any functions called within an annotated getter). This way, functions
> > can be annotated with @pragma("vm:platform-const") without having to
> > rewrite them to be a single returned expression.
> >
> > TEST=pkg/vm/test/transformations/unreachable_code_elimination
> >      pkg/vm/test/transformations/vm_constant_evaluator
> >
> > Issue: #50473
> > Issue: #31969
> > Change-Id: Ie290d2f1f469326238d66c3d9631f8e696685ff0
> > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332760
> > Commit-Queue: Tess Strickland <sstrickl@google.com>
> > Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
> > Reviewed-by: Alexander Markov <alexmarkov@google.com>
>
> TEST=pkg/vm/test/transformations/unreachable_code_elimination
>      pkg/vm/test/transformations/vm_constant_evaluator
>
> Issue: #50473
> Issue: #31969
> Cq-Include-Trybots: luci.dart.try:vm-aot-linux-release-x64-try,vm-aot-mac-release-arm64-try
> Change-Id: I557ca933808012e670e306f2d880221a0d7dd670
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/334224
> Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
> Reviewed-by: Alexander Markov <alexmarkov@google.com>
> Commit-Queue: Tess Strickland <sstrickl@google.com>

TEST=pkg/vm/test/transformations/unreachable_code_elimination
     pkg/vm/test/transformations/vm_constant_evaluator

Change-Id: Ia51b7c5f3b51f57a6a306551fe74b47e0cba3c23
Cq-Include-Trybots: luci.dart.try:vm-aot-linux-release-x64-try,vm-aot-mac-release-arm64-try,vm-kernel-precomp-linux-release-x64-try,vm-kernel-linux-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/335828
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Tess Strickland <sstrickl@google.com>

pkg/front_end/lib/src/fasta/kernel/constant_evaluator.dart

@@ -2638,6 +2638,11 @@ class ConstantEvaluator implements ExpressionVisitor<Constant> {
 
   /// Execute a function body using the [StatementConstantEvaluator].
   Constant executeBody(Statement statement) {
+    if (!enableConstFunctions && !inExtensionTypeConstConstructor) {
+      throw new UnsupportedError("Statement evaluation is only supported when "
+          "in extension type const constructors or when the const functions "
+          "feature is enabled.");
+    }
     StatementConstantEvaluator statementEvaluator =
         new StatementConstantEvaluator(this);
     ExecutionStatus status = statement.accept(statementEvaluator);
@@ -2664,6 +2669,11 @@ class ConstantEvaluator implements ExpressionVisitor<Constant> {
   /// Returns [null] on success and an error-"constant" on failure, as such the
   /// return value should be checked.
   AbortConstant? executeConstructorBody(Constructor constructor) {
+    if (!enableConstFunctions && !inExtensionTypeConstConstructor) {
+      throw new UnsupportedError("Statement evaluation is only supported when "
+          "in extension type const constructors or when the const functions "
+          "feature is enabled.");
+    }
     final Statement body = constructor.function.body!;
     StatementConstantEvaluator statementEvaluator =
         new StatementConstantEvaluator(this);
@@ -5485,14 +5495,7 @@ class ConstantEvaluator implements ExpressionVisitor<Constant> {
 class StatementConstantEvaluator implements StatementVisitor<ExecutionStatus> {
   ConstantEvaluator exprEvaluator;
 
-  StatementConstantEvaluator(this.exprEvaluator) {
-    if (!exprEvaluator.enableConstFunctions &&
-        !exprEvaluator.inExtensionTypeConstConstructor) {
-      throw new UnsupportedError("Statement evaluation is only supported when "
-          "in inline class const constructors or when the const functions "
-          "feature is enabled.");
-    }
-  }
+  StatementConstantEvaluator(this.exprEvaluator);
 
   /// Evaluate the expression using the [ConstantEvaluator].
   Constant evaluate(Expression expr) => expr.accept(exprEvaluator);

pkg/vm/lib/kernel_front_end.dart

@@ -579,7 +579,7 @@ Future runGlobalTransformations(
       target, component, os, nnbdMode,
       environmentDefines: environmentDefines, coreTypes: coreTypes);
   unreachable_code_elimination.transformComponent(
-      component, enableAsserts, evaluator);
+      target, component, evaluator, enableAsserts);
 
   if (useGlobalTypeFlowAnalysis) {
     globalTypeFlow.transformComponent(target, coreTypes, component,

pkg/vm/lib/transformations/unreachable_code_elimination.dart

@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'package:kernel/ast.dart';
+import 'package:kernel/target/targets.dart' show Target;
 import 'package:kernel/type_environment.dart' show StaticTypeContext;
 
 import 'vm_constant_evaluator.dart' show VMConstantEvaluator;
@@ -13,19 +14,23 @@ import 'vm_constant_evaluator.dart' show VMConstantEvaluator;
 ///
 /// Also performs some additional constant evaluation via [evaluator], which is
 /// applied to certain types of expressions (currently only StaticGet).
-Component transformComponent(
-    Component component, bool enableAsserts, VMConstantEvaluator evaluator) {
-  SimpleUnreachableCodeElimination(enableAsserts, evaluator)
+Component transformComponent(Target target, Component component,
+    VMConstantEvaluator evaluator, bool enableAsserts) {
+  SimpleUnreachableCodeElimination(evaluator,
+          enableAsserts: enableAsserts,
+          soundNullSafety: target.flags.soundNullSafety)
       .visitComponent(component, null);
   return component;
 }
 
 class SimpleUnreachableCodeElimination extends RemovingTransformer {
+  final bool soundNullSafety;
   final bool enableAsserts;
   final VMConstantEvaluator constantEvaluator;
   StaticTypeContext? _staticTypeContext;
 
-  SimpleUnreachableCodeElimination(this.enableAsserts, this.constantEvaluator);
+  SimpleUnreachableCodeElimination(this.constantEvaluator,
+      {required this.enableAsserts, required this.soundNullSafety});
 
   @override
   TreeNode defaultMember(Member node, TreeNode? removalSentinel) {
@@ -36,25 +41,32 @@ class SimpleUnreachableCodeElimination extends RemovingTransformer {
     return result;
   }
 
-  bool _isBoolConstant(Expression node) =>
-      node is BoolLiteral ||
-      (node is ConstantExpression && node.constant is BoolConstant);
+  bool? _getBoolConstantValue(Expression node) {
+    if (node is BoolLiteral) return node.value;
+    if (node is! ConstantExpression) return null;
+    final constant = node.constant;
+    return constant is BoolConstant ? constant.value : null;
+  }
 
-  bool _getBoolConstantValue(Expression node) {
-    if (node is BoolLiteral) {
-      return node.value;
+  Expression _makeConstantExpression(Constant constant, Expression node) {
+    if (constant is UnevaluatedConstant &&
+        constant.expression is InvalidExpression) {
+      return constant.expression;
     }
-    if (node is ConstantExpression) {
-      final constant = node.constant;
-      if (constant is BoolConstant) {
-        return constant.value;
-      }
+    ConstantExpression constantExpression = new ConstantExpression(
+        constant, node.getStaticType(_staticTypeContext!))
+      ..fileOffset = node.fileOffset;
+    if (node is FileUriExpression) {
+      return new FileUriConstantExpression(constantExpression.constant,
+          type: constantExpression.type, fileUri: node.fileUri)
+        ..fileOffset = node.fileOffset;
     }
-    throw 'Expected bool constant: $node';
+    return constantExpression;
   }
 
-  Expression _createBoolLiteral(bool value, int fileOffset) =>
-      new BoolLiteral(value)..fileOffset = fileOffset;
+  Expression _createBoolConstantExpression(bool value, Expression node) =>
+      _makeConstantExpression(
+          constantEvaluator.canonicalize(BoolConstant(value)), node);
 
   Statement _makeEmptyBlockIfEmptyStatement(Statement node, TreeNode parent) =>
       node is EmptyStatement ? (Block(<Statement>[])..parent = parent) : node;
@@ -63,8 +75,8 @@ class SimpleUnreachableCodeElimination extends RemovingTransformer {
   TreeNode visitIfStatement(IfStatement node, TreeNode? removalSentinel) {
     node.transformOrRemoveChildren(this);
     final condition = node.condition;
-    if (_isBoolConstant(condition)) {
-      final value = _getBoolConstantValue(condition);
+    final value = _getBoolConstantValue(condition);
+    if (value != null) {
       return value
           ? node.then
           : (node.otherwise ?? removalSentinel ?? new EmptyStatement());
@@ -78,8 +90,8 @@ class SimpleUnreachableCodeElimination extends RemovingTransformer {
       ConditionalExpression node, TreeNode? removalSentinel) {
     node.transformOrRemoveChildren(this);
     final condition = node.condition;
-    if (_isBoolConstant(condition)) {
-      final value = _getBoolConstantValue(condition);
+    final value = _getBoolConstantValue(condition);
+    if (value != null) {
       return value ? node.then : node.otherwise;
     }
     return node;
@@ -89,9 +101,9 @@ class SimpleUnreachableCodeElimination extends RemovingTransformer {
   TreeNode visitNot(Not node, TreeNode? removalSentinel) {
     node.transformOrRemoveChildren(this);
     final operand = node.operand;
-    if (_isBoolConstant(operand)) {
-      return _createBoolLiteral(
-          !_getBoolConstantValue(operand), node.fileOffset);
+    final value = _getBoolConstantValue(operand);
+    if (value != null) {
+      return _createBoolConstantExpression(!value, node);
     }
     return node;
   }
@@ -100,47 +112,118 @@ class SimpleUnreachableCodeElimination extends RemovingTransformer {
   TreeNode visitLogicalExpression(
       LogicalExpression node, TreeNode? removalSentinel) {
     node.transformOrRemoveChildren(this);
-    final left = node.left;
-    final right = node.right;
-    final operatorEnum = node.operatorEnum;
-    if (_isBoolConstant(left)) {
-      final leftValue = _getBoolConstantValue(left);
-      if (_isBoolConstant(right)) {
-        final rightValue = _getBoolConstantValue(right);
-        if (operatorEnum == LogicalExpressionOperator.OR) {
-          return _createBoolLiteral(leftValue || rightValue, node.fileOffset);
-        } else if (operatorEnum == LogicalExpressionOperator.AND) {
-          return _createBoolLiteral(leftValue && rightValue, node.fileOffset);
-        } else {
-          throw 'Unexpected LogicalExpression operator ${operatorEnum}: $node';
-        }
-      } else {
-        if (leftValue && operatorEnum == LogicalExpressionOperator.OR) {
-          return _createBoolLiteral(true, node.fileOffset);
-        } else if (!leftValue &&
-            operatorEnum == LogicalExpressionOperator.AND) {
-          return _createBoolLiteral(false, node.fileOffset);
+    bool? value = _getBoolConstantValue(node.left);
+    // Because of short-circuiting, these operators cannot be treated as
+    // symmetric, so a non-constant left and a constant right is left as-is.
+    if (value == null) return node;
+    // If the RHS is not a known constant and may evaluate to null, then
+    // we must keep the whole node if the LHS does not short circuit, as the
+    // operator performs a null check on the RHS value.
+    final evaluateRight =
+        soundNullSafety || _getBoolConstantValue(node.right) != null
+            ? node.right
+            : node;
+    switch (node.operatorEnum) {
+      case LogicalExpressionOperator.OR:
+        return value ? node.left : evaluateRight;
+      case LogicalExpressionOperator.AND:
+        return value ? evaluateRight : node.left;
+    }
+  }
+
+  @override
+  TreeNode visitSwitchStatement(
+      SwitchStatement node, TreeNode? removalSentinel) {
+    node.transformOrRemoveChildren(this);
+    final tested = node.expression;
+    if (tested is! ConstantExpression) return node;
+
+    // First, keep any reachable case. As a side effect, any expressions that
+    // cannot match in the SwitchCases are removed. An expression cannot match
+    // if it is a non-matching constant expression or it follows a constant
+    // expression that is guaranteed to match.
+    final toKeep = <SwitchCase>{};
+    bool foundMatchingCase = false;
+    for (final c in node.cases) {
+      if (foundMatchingCase) {
+        c.expressions.clear();
+        continue;
+      }
+      c.expressions.retainWhere((e) {
+        if (foundMatchingCase) return false;
+        if (e is! ConstantExpression) return true;
+        foundMatchingCase = e.constant == tested.constant;
+        return foundMatchingCase;
+      });
+      if (c.isDefault || c.expressions.isNotEmpty) {
+        toKeep.add(c);
+      }
+    }
+
+    if (toKeep.isEmpty) {
+      if (node.isExhaustive) {
+        throw 'Expected at least one kept case from exhaustive switch: $node';
+      }
+      return removalSentinel ?? new EmptyStatement();
+    }
+
+    // Now iteratively find additional cases to keep by following targets of
+    // continue statements in kept cases.
+    final worklist = [...toKeep];
+    final collector = ContinueSwitchStatementTargetCollector(node);
+    while (worklist.isNotEmpty) {
+      final next = worklist.removeLast();
+      final targets = collector.collectTargets(next);
+      for (final target in targets) {
+        if (toKeep.add(target)) {
+          worklist.add(target);
         }
       }
     }
+
+    // Finally, remove any cases not marked for keeping. If only one case
+    // is kept, then the switch statement can be replaced with its body.
+    if (toKeep.length == 1) {
+      return toKeep.first.body;
+    }
+    node.cases.retainWhere(toKeep.contains);
+    if (foundMatchingCase && !node.hasDefault) {
+      // While the expression may not be explicitly exhaustive for the type
+      // of the tested expression, it is guaranteed to execute at least one
+      // of the remaining cases, so the backends don't need to handle the case
+      // where no listed case is hit for this switch.
+      //
+      // If the original program has the matching case directly falls through
+      // to the default case for some reason:
+      //
+      // switch (4) {
+      //    ...
+      //    case 4:
+      //    default:
+      //      ...
+      // }
+      //
+      // this means the default case is kept despite finding a guaranteed to
+      // match expression, as it contains that matching expression. If that
+      // happens, then we don't do this, to keep the invariant that
+      // isExplicitlyExhaustive is false if there is a default case.
+      node.isExplicitlyExhaustive = true;
+    }
     return node;
   }
 
   @override
-  visitStaticGet(StaticGet node, TreeNode? removalSentinel) {
+  TreeNode visitStaticGet(StaticGet node, TreeNode? removalSentinel) {
     node.transformOrRemoveChildren(this);
     final target = node.target;
     if (target is Field && target.isConst) {
       throw 'StaticGet from const field $target should be evaluated by front-end: $node';
     }
-    if (constantEvaluator.transformerShouldEvaluateExpression(node)) {
-      final context = _staticTypeContext!;
-      final result = constantEvaluator.evaluate(context, node);
-      assert(result is! UnevaluatedConstant);
-      return new ConstantExpression(result, node.getStaticType(context))
-        ..fileOffset = node.fileOffset;
+    if (!constantEvaluator.transformerShouldEvaluateExpression(node)) {
+      return node;
     }
-    return node;
+    final result = constantEvaluator.evaluate(_staticTypeContext!, node);
+    return _makeConstantExpression(result, node);
   }
 
   @override
@@ -235,3 +318,25 @@ class SimpleUnreachableCodeElimination extends RemovingTransformer {
     return node;
   }
 }
+
+class ContinueSwitchStatementTargetCollector extends RecursiveVisitor {
+  final SwitchStatement parent;
+  late Set<SwitchCase> collected;
+
+  ContinueSwitchStatementTargetCollector(this.parent);
+
+  Set<SwitchCase> collectTargets(SwitchCase node) {
+    collected = {};
+    node.accept(this);
+    return collected;
+  }
+
+  @override
+  void visitContinueSwitchStatement(ContinueSwitchStatement node) {
+    node.visitChildren(this);
+    // Only keep targets that are within the original node being checked.
+    if (node.target.parent == parent) {
+      collected.add(node.target);
+    }
+  }
+}