Simplify dataflow SwitchInt handling.

Current `SwitchInt` handling has complicated control flow.

- The dataflow engine calls `Analysis::apply_switch_int_edge_effects`,
  passing in an "applier" that impls `SwitchIntEdgeEffects`.
- `apply_switch_int_edge_effects` possibly calls `apply` on the applier,
  passing it a closure.
- The `apply` method calls the closure on each `SwitchInt` edge.
- The closure operates on the edge.

I.e. control flow goes from the engine, to the analysis, to the applier
(which came from the engine), to the closure (which came from the
analysis). It took me a while to work this out.

This commit changes to a simpler structure that maintains the important
characteristics.

- The dataflow engine calls `Analysis::get_switch_int_data`.
- `get_switch_int_data` returns an `Option<Self::SwitchIntData>` value.
- If that returned value was `Some`, the dataflow engine calls
  `Analysis::apply_switch_int_edge_effect` on each edge, passing the
  `Self::SwitchIntData` value.
- `Analysis::apply_switch_int_edge_effect` operates on the edge.

I.e. control flow goes from the engine, to the analysis, to the
engine, to the analysis.

Added:
- The `Analysis::SwitchIntData` assoc type and the
  `Analysis::get_switch_int_data` method. Both only need to be
  defined by analyses that look at `SwitchInt` terminators.
- The `MaybePlacesSwitchIntData` struct, which has three fields.

Changes:
- `Analysis::apply_switch_int_edge_effects` becomes
  `Analysis::apply_switch_int_edge_effect`, which is a little simpler
  because it's dealing with a single edge instead of all edges.

Removed:
- The `SwitchIntEdgeEffects` trait, and its two impls:
  `BackwardSwitchIntEdgeEffectsApplier` (which has six fields) and
  `ForwardSwitchIntEdgeEffectsApplier` structs (which has four fields).
- The closure.

The new structure is more concise and simpler.

Author: nnethercote

compiler/rustc_borrowck/src/dataflow.rs

@@ -9,7 +9,7 @@ use rustc_middle::mir::{
 use rustc_middle::ty::{RegionVid, TyCtxt};
 use rustc_mir_dataflow::fmt::DebugWithContext;
 use rustc_mir_dataflow::impls::{EverInitializedPlaces, MaybeUninitializedPlaces};
-use rustc_mir_dataflow::{Analysis, GenKill, JoinSemiLattice, SwitchIntEdgeEffects};
+use rustc_mir_dataflow::{Analysis, GenKill, JoinSemiLattice};
 use tracing::debug;
 
 use crate::{BorrowSet, PlaceConflictBias, PlaceExt, RegionInferenceContext, places_conflict};
@@ -98,16 +98,6 @@ impl<'a, 'tcx> Analysis<'tcx> for Borrowck<'a, 'tcx> {
         // This is only reachable from `iterate_to_fixpoint`, which this analysis doesn't use.
         unreachable!();
     }
-
-    fn apply_switch_int_edge_effects(
-        &mut self,
-        _block: BasicBlock,
-        _discr: &mir::Operand<'tcx>,
-        _apply_edge_effects: &mut impl SwitchIntEdgeEffects<Self::Domain>,
-    ) {
-        // This is only reachable from `iterate_to_fixpoint`, which this analysis doesn't use.
-        unreachable!();
-    }
 }
 
 impl JoinSemiLattice for BorrowckDomain<'_, '_> {

compiler/rustc_mir_dataflow/src/framework/direction.rs

@@ -1,8 +1,6 @@
 use std::ops::RangeInclusive;
 
-use rustc_middle::mir::{
-    self, BasicBlock, CallReturnPlaces, Location, SwitchTargets, TerminatorEdges,
-};
+use rustc_middle::mir::{self, BasicBlock, CallReturnPlaces, Location, TerminatorEdges};
 
 use super::visitor::ResultsVisitor;
 use super::{Analysis, Effect, EffectIndex, Results, SwitchIntTarget};
@@ -237,18 +235,18 @@ impl Direction for Backward {
                 }
 
                 mir::TerminatorKind::SwitchInt { targets: _, ref discr } => {
-                    let mut applier = BackwardSwitchIntEdgeEffectsApplier {
-                        body,
-                        pred,
-                        exit_state,
-                        bb,
-                        propagate: &mut propagate,
-                        effects_applied: false,
-                    };
-
-                    analysis.apply_switch_int_edge_effects(pred, discr, &mut applier);
-
-                    if !applier.effects_applied {
+                    if let Some(mut data) = analysis.get_switch_int_data(bb, discr) {
+                        let values = &body.basic_blocks.switch_sources()[&(bb, pred)];
+                        let targets =
+                            values.iter().map(|&value| SwitchIntTarget { value, target: bb });
+
+                        let mut tmp = None;
+                        for target in targets {
+                            let tmp = opt_clone_from_or_clone(&mut tmp, exit_state);
+                            analysis.apply_switch_int_edge_effect(&mut data, tmp, target);
+                            propagate(pred, tmp);
+                        }
+                    } else {
                         propagate(pred, exit_state)
                     }
                 }
@@ -259,37 +257,6 @@ impl Direction for Backward {
     }
 }
 
-struct BackwardSwitchIntEdgeEffectsApplier<'mir, 'tcx, D, F> {
-    body: &'mir mir::Body<'tcx>,
-    pred: BasicBlock,
-    exit_state: &'mir mut D,
-    bb: BasicBlock,
-    propagate: &'mir mut F,
-    effects_applied: bool,
-}
-
-impl<D, F> super::SwitchIntEdgeEffects<D> for BackwardSwitchIntEdgeEffectsApplier<'_, '_, D, F>
-where
-    D: Clone,
-    F: FnMut(BasicBlock, &D),
-{
-    fn apply(&mut self, mut apply_edge_effect: impl FnMut(&mut D, SwitchIntTarget)) {
-        assert!(!self.effects_applied);
-
-        let values = &self.body.basic_blocks.switch_sources()[&(self.bb, self.pred)];
-        let targets = values.iter().map(|&value| SwitchIntTarget { value, target: self.bb });
-
-        let mut tmp = None;
-        for target in targets {
-            let tmp = opt_clone_from_or_clone(&mut tmp, self.exit_state);
-            apply_edge_effect(tmp, target);
-            (self.propagate)(self.pred, tmp);
-        }
-
-        self.effects_applied = true;
-    }
-}
-
 /// Dataflow that runs from the entry of a block (the first statement), to its exit (terminator).
 pub struct Forward;
 
@@ -451,23 +418,27 @@ impl Direction for Forward {
                 }
             }
             TerminatorEdges::SwitchInt { targets, discr } => {
-                let mut applier = ForwardSwitchIntEdgeEffectsApplier {
-                    exit_state,
-                    targets,
-                    propagate,
-                    effects_applied: false,
-                };
-
-                analysis.apply_switch_int_edge_effects(bb, discr, &mut applier);
-
-                let ForwardSwitchIntEdgeEffectsApplier {
-                    exit_state,
-                    mut propagate,
-                    effects_applied,
-                    ..
-                } = applier;
-
-                if !effects_applied {
+                if let Some(mut data) = analysis.get_switch_int_data(bb, discr) {
+                    let mut tmp = None;
+                    for (value, target) in targets.iter() {
+                        let tmp = opt_clone_from_or_clone(&mut tmp, exit_state);
+                        analysis.apply_switch_int_edge_effect(&mut data, tmp, SwitchIntTarget {
+                            value: Some(value),
+                            target,
+                        });
+                        propagate(target, tmp);
+                    }
+
+                    // Once we get to the final, "otherwise" branch, there is no need to preserve
+                    // `exit_state`, so pass it directly to `apply_switch_int_edge_effect` to save
+                    // a clone of the dataflow state.
+                    let otherwise = targets.otherwise();
+                    analysis.apply_switch_int_edge_effect(&mut data, exit_state, SwitchIntTarget {
+                        value: None,
+                        target: otherwise,
+                    });
+                    propagate(otherwise, exit_state);
+                } else {
                     for target in targets.all_targets() {
                         propagate(*target, exit_state);
                     }
@@ -477,39 +448,6 @@ impl Direction for Forward {
     }
 }
 
-struct ForwardSwitchIntEdgeEffectsApplier<'mir, D, F> {
-    exit_state: &'mir mut D,
-    targets: &'mir SwitchTargets,
-    propagate: F,
-
-    effects_applied: bool,
-}
-
-impl<D, F> super::SwitchIntEdgeEffects<D> for ForwardSwitchIntEdgeEffectsApplier<'_, D, F>
-where
-    D: Clone,
-    F: FnMut(BasicBlock, &D),
-{
-    fn apply(&mut self, mut apply_edge_effect: impl FnMut(&mut D, SwitchIntTarget)) {
-        assert!(!self.effects_applied);
-
-        let mut tmp = None;
-        for (value, target) in self.targets.iter() {
-            let tmp = opt_clone_from_or_clone(&mut tmp, self.exit_state);
-            apply_edge_effect(tmp, SwitchIntTarget { value: Some(value), target });
-            (self.propagate)(target, tmp);
-        }
-
-        // Once we get to the final, "otherwise" branch, there is no need to preserve `exit_state`,
-        // so pass it directly to `apply_edge_effect` to save a clone of the dataflow state.
-        let otherwise = self.targets.otherwise();
-        apply_edge_effect(self.exit_state, SwitchIntTarget { value: None, target: otherwise });
-        (self.propagate)(otherwise, self.exit_state);
-
-        self.effects_applied = true;
-    }
-}
-
 /// An analogue of `Option::get_or_insert_with` that stores a clone of `val` into `opt`, but uses
 /// the more efficient `clone_from` if `opt` was `Some`.
 ///

compiler/rustc_mir_dataflow/src/framework/mod.rs

@@ -120,6 +120,9 @@ pub trait Analysis<'tcx> {
     /// The direction of this analysis. Either `Forward` or `Backward`.
     type Direction: Direction = Forward;
 
+    /// Auxiliary data used for analyzing `SwitchInt` terminators, if necessary.
+    type SwitchIntData = !;
+
     /// A descriptive name for this analysis. Used only for debugging.
     ///
     /// This name should be brief and contain no spaces, periods or other characters that are not
@@ -205,25 +208,36 @@ pub trait Analysis<'tcx> {
     ) {
     }
 
-    /// Updates the current dataflow state with the effect of taking a particular branch in a
-    /// `SwitchInt` terminator.
+    /// Used to update the current dataflow state with the effect of taking a particular branch in
+    /// a `SwitchInt` terminator.
     ///
     /// Unlike the other edge-specific effects, which are allowed to mutate `Self::Domain`
-    /// directly, overriders of this method must pass a callback to
-    /// `SwitchIntEdgeEffects::apply`. The callback will be run once for each outgoing edge and
-    /// will have access to the dataflow state that will be propagated along that edge.
+    /// directly, overriders of this method must return a `Self::SwitchIntData` value (wrapped in
+    /// `Some`). The `apply_switch_int_edge_effect` method will then be called once for each
+    /// outgoing edge and will have access to the dataflow state that will be propagated along that
+    /// edge, and also the `Self::SwitchIntData` value.
     ///
     /// This interface is somewhat more complex than the other visitor-like "effect" methods.
     /// However, it is both more ergonomic—callers don't need to recompute or cache information
     /// about a given `SwitchInt` terminator for each one of its edges—and more efficient—the
     /// engine doesn't need to clone the exit state for a block unless
-    /// `SwitchIntEdgeEffects::apply` is actually called.
-    fn apply_switch_int_edge_effects(
+    /// `get_switch_int_data` is actually called.
+    fn get_switch_int_data(
         &mut self,
-        _block: BasicBlock,
+        _block: mir::BasicBlock,
         _discr: &mir::Operand<'tcx>,
-        _apply_edge_effects: &mut impl SwitchIntEdgeEffects<Self::Domain>,
+    ) -> Option<Self::SwitchIntData> {
+        None
+    }
+
+    /// See comments on `get_switch_int_data`.
+    fn apply_switch_int_edge_effect(
+        &mut self,
+        _data: &mut Self::SwitchIntData,
+        _state: &mut Self::Domain,
+        _edge: SwitchIntTarget,
     ) {
+        unreachable!();
     }
 
     /* Extension methods */
@@ -285,7 +299,6 @@ pub trait Analysis<'tcx> {
             // but it saves an allocation, thus improving compile times.
             state.clone_from(&entry_sets[bb]);
 
-            // Apply the block transfer function, using the cached one if it exists.
             let edges = Self::Direction::apply_effects_in_block(&mut self, &mut state, bb, bb_data);
 
             Self::Direction::join_state_into_successors_of(
@@ -452,12 +465,5 @@ pub struct SwitchIntTarget {
     pub target: BasicBlock,
 }
 
-/// A type that records the edge-specific effects for a `SwitchInt` terminator.
-pub trait SwitchIntEdgeEffects<D> {
-    /// Calls `apply_edge_effect` for each outgoing edge from a `SwitchInt` terminator and
-    /// records the results.
-    fn apply(&mut self, apply_edge_effect: impl FnMut(&mut D, SwitchIntTarget));
-}
-
 #[cfg(test)]
 mod tests;