Remove QualifResolver abstraction

This is a relic from earlier attempts at dataflow-based const validation
that attempted to do promotion at the same time. #63812 takes a
different approach: `IsNotPromotable` is no longer a `Qualif` and is
computed lazily instead of eagerly. As a result, there's no need for an
eager `TempPromotionResolver`, and we can use the only implementer of
`QualifResolver` directly instead of through a trait.

Author: ecstatic-morse

src/librustc_mir/transform/check_consts/resolver.rs

@@ -1,21 +1,18 @@
 //! Propagate `Qualif`s between locals and query the results.
 //!
-//! This also contains the dataflow analysis used to track `Qualif`s on complex control-flow
-//! graphs.
+//! This contains the dataflow analysis used to track `Qualif`s on complex control-flow graphs.
 
 use rustc::mir::visit::Visitor;
 use rustc::mir::{self, BasicBlock, Local, Location};
 use rustc_index::bit_set::BitSet;
 
-use std::cell::RefCell;
 use std::marker::PhantomData;
 
 use crate::dataflow::{self as old_dataflow, generic as dataflow};
 use super::{Item, Qualif};
-use self::old_dataflow::IndirectlyMutableLocals;
 
 /// A `Visitor` that propagates qualifs between locals. This defines the transfer function of
-/// `FlowSensitiveAnalysis` as well as the logic underlying `TempPromotionResolver`.
+/// `FlowSensitiveAnalysis`.
 ///
 /// This transfer does nothing when encountering an indirect assignment. Consumers should rely on
 /// the `IndirectlyMutableLocals` dataflow pass to see if a `Local` may have become qualified via
@@ -147,145 +144,6 @@ where
     }
 }
 
-/// Types that can compute the qualifs of each local at each location in a `mir::Body`.
-///
-/// Code that wishes to use a `QualifResolver` must call `visit_{statement,terminator}` for each
-/// statement or terminator, processing blocks in reverse post-order beginning from the
-/// `START_BLOCK`. Calling code may optionally call `get` after visiting each statement or
-/// terminator to query the qualification state immediately before that statement or terminator.
-///
-/// These conditions are much more restrictive than woud be required by `FlowSensitiveResolver`
-/// alone. This is to allow a linear, on-demand `TempPromotionResolver` that can operate
-/// efficiently on simple CFGs.
-pub trait QualifResolver<Q> {
-    /// Get the qualifs of each local at the last location visited.
-    ///
-    /// This takes `&mut self` so qualifs can be computed lazily.
-    fn get(&mut self) -> &BitSet<Local>;
-
-    /// A convenience method for `self.get().contains(local)`.
-    fn contains(&mut self, local: Local) -> bool {
-        self.get().contains(local)
-    }
-
-    /// Resets the resolver to the `START_BLOCK`. This allows a resolver to be reused
-    /// for multiple passes over a `mir::Body`.
-    fn reset(&mut self);
-}
-
-pub type IndirectlyMutableResults<'mir, 'tcx> =
-    old_dataflow::DataflowResultsCursor<'mir, 'tcx, IndirectlyMutableLocals<'mir, 'tcx>>;
-
-/// A resolver for qualifs that works on arbitrarily complex CFGs.
-///
-/// As soon as a `Local` becomes writable through a reference (as determined by the
-/// `IndirectlyMutableLocals` dataflow pass), we must assume that it takes on all other qualifs
-/// possible for its type. This is because no effort is made to track qualifs across indirect
-/// assignments (e.g. `*p = x` or calls to opaque functions).
-///
-/// It is possible to be more precise here by waiting until an indirect assignment actually occurs
-/// before marking a borrowed `Local` as qualified.
-pub struct FlowSensitiveResolver<'a, 'mir, 'tcx, Q>
-where
-    Q: Qualif,
-{
-    location: Location,
-    indirectly_mutable_locals: &'a RefCell<IndirectlyMutableResults<'mir, 'tcx>>,
-    cursor: dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q>>,
-    qualifs_per_local: BitSet<Local>,
-
-    /// The value of `Q::in_any_value_of_ty` for each local.
-    qualifs_in_any_value_of_ty: BitSet<Local>,
-}
-
-impl<Q> FlowSensitiveResolver<'a, 'mir, 'tcx, Q>
-where
-    Q: Qualif,
-{
-    pub fn new(
-        _: Q,
-        item: &'a Item<'mir, 'tcx>,
-        indirectly_mutable_locals: &'a RefCell<IndirectlyMutableResults<'mir, 'tcx>>,
-        dead_unwinds: &BitSet<BasicBlock>,
-    ) -> Self {
-        let analysis = FlowSensitiveAnalysis {
-            item,
-            _qualif: PhantomData,
-        };
-        let results =
-            dataflow::Engine::new(item.tcx, item.body, item.def_id, dead_unwinds, analysis)
-                .iterate_to_fixpoint();
-        let cursor = dataflow::ResultsCursor::new(item.body, results);
-
-        let mut qualifs_in_any_value_of_ty = BitSet::new_empty(item.body.local_decls.len());
-        for (local, decl) in item.body.local_decls.iter_enumerated() {
-            if Q::in_any_value_of_ty(item, decl.ty) {
-                qualifs_in_any_value_of_ty.insert(local);
-            }
-        }
-
-        FlowSensitiveResolver {
-            cursor,
-            indirectly_mutable_locals,
-            qualifs_per_local: BitSet::new_empty(item.body.local_decls.len()),
-            qualifs_in_any_value_of_ty,
-            location: Location { block: mir::START_BLOCK, statement_index: 0 },
-        }
-    }
-}
-
-impl<Q> Visitor<'tcx> for FlowSensitiveResolver<'_, '_, 'tcx, Q>
-where
-    Q: Qualif
-{
-    fn visit_statement(&mut self, _: &mir::Statement<'tcx>, location: Location) {
-        self.location = location;
-    }
-
-    fn visit_terminator(&mut self, _: &mir::Terminator<'tcx>, location: Location) {
-        self.location = location;
-    }
-}
-
-impl<Q> QualifResolver<Q> for FlowSensitiveResolver<'_, '_, '_, Q>
-where
-    Q: Qualif
-{
-    fn get(&mut self) -> &BitSet<Local> {
-        let mut indirectly_mutable_locals = self.indirectly_mutable_locals.borrow_mut();
-
-        indirectly_mutable_locals.seek(self.location);
-        self.cursor.seek_before(self.location);
-
-        self.qualifs_per_local.overwrite(indirectly_mutable_locals.get());
-        self.qualifs_per_local.union(self.cursor.get());
-        self.qualifs_per_local.intersect(&self.qualifs_in_any_value_of_ty);
-        &self.qualifs_per_local
-    }
-
-    fn contains(&mut self, local: Local) -> bool {
-        // No need to update the cursor if we know that `Local` cannot possibly be qualified.
-        if !self.qualifs_in_any_value_of_ty.contains(local) {
-            return false;
-        }
-
-        // Otherwise, return `true` if this local is qualified or was indirectly mutable at any
-        // point before this statement.
-        self.cursor.seek_before(self.location);
-        if self.cursor.get().contains(local) {
-            return true;
-        }
-
-        let mut indirectly_mutable_locals = self.indirectly_mutable_locals.borrow_mut();
-        indirectly_mutable_locals.seek(self.location);
-        indirectly_mutable_locals.get().contains(local)
-    }
-
-    fn reset(&mut self)  {
-        self.location = Location { block: mir::START_BLOCK, statement_index: 0 };
-    }
-}
-
 /// The dataflow analysis used to propagate qualifs on arbitrary CFGs.
 pub(super) struct FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q> {
     item: &'a Item<'mir, 'tcx>,
@@ -296,6 +154,13 @@ impl<'a, 'mir, 'tcx, Q> FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q>
 where
     Q: Qualif,
 {
+    pub(super) fn new(_: Q, item: &'a Item<'mir, 'tcx>) -> Self {
+        FlowSensitiveAnalysis {
+            item,
+            _qualif: PhantomData,
+        }
+    }
+
     fn transfer_function(
         &self,
         state: &'a mut BitSet<Local>,

src/librustc_mir/transform/check_consts/validation.rs

@@ -9,15 +9,15 @@ use rustc_target::spec::abi::Abi;
 use syntax::symbol::sym;
 use syntax_pos::Span;
 
-use std::cell::RefCell;
 use std::fmt;
 use std::ops::Deref;
 
-use crate::dataflow as old_dataflow;
-use super::{ConstKind, Item, Qualif, is_lang_panic_fn};
-use super::resolver::{FlowSensitiveResolver, IndirectlyMutableResults, QualifResolver};
-use super::qualifs::{HasMutInterior, NeedsDrop};
+use crate::dataflow::{self as old_dataflow, generic as dataflow};
+use self::old_dataflow::IndirectlyMutableLocals;
 use super::ops::{self, NonConstOp};
+use super::qualifs::{HasMutInterior, NeedsDrop};
+use super::resolver::FlowSensitiveAnalysis;
+use super::{ConstKind, Item, Qualif, is_lang_panic_fn};
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 pub enum CheckOpResult {
@@ -26,9 +26,75 @@ pub enum CheckOpResult {
     Allowed,
 }
 
+pub type IndirectlyMutableResults<'mir, 'tcx> =
+    old_dataflow::DataflowResultsCursor<'mir, 'tcx, IndirectlyMutableLocals<'mir, 'tcx>>;
+
+struct QualifCursor<'a, 'mir, 'tcx, Q: Qualif> {
+    cursor: dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q>>,
+    in_any_value_of_ty: BitSet<Local>,
+}
+
+impl<Q: Qualif> QualifCursor<'a, 'mir, 'tcx, Q> {
+    pub fn new(
+        q: Q,
+        item: &'a Item<'mir, 'tcx>,
+        dead_unwinds: &BitSet<BasicBlock>,
+    ) -> Self {
+        let analysis = FlowSensitiveAnalysis::new(q, item);
+        let results =
+            dataflow::Engine::new(item.tcx, item.body, item.def_id, dead_unwinds, analysis)
+                .iterate_to_fixpoint();
+        let cursor = dataflow::ResultsCursor::new(item.body, results);
+
+        let mut in_any_value_of_ty = BitSet::new_empty(item.body.local_decls.len());
+        for (local, decl) in item.body.local_decls.iter_enumerated() {
+            if Q::in_any_value_of_ty(item, decl.ty) {
+                in_any_value_of_ty.insert(local);
+            }
+        }
+
+        QualifCursor {
+            cursor,
+            in_any_value_of_ty,
+        }
+    }
+}
+
 pub struct Qualifs<'a, 'mir, 'tcx> {
-    has_mut_interior: FlowSensitiveResolver<'a, 'mir, 'tcx, HasMutInterior>,
-    needs_drop: FlowSensitiveResolver<'a, 'mir, 'tcx, NeedsDrop>,
+    has_mut_interior: QualifCursor<'a, 'mir, 'tcx, HasMutInterior>,
+    needs_drop: QualifCursor<'a, 'mir, 'tcx, NeedsDrop>,
+    indirectly_mutable: IndirectlyMutableResults<'mir, 'tcx>,
+}
+
+impl Qualifs<'a, 'mir, 'tcx> {
+    fn indirectly_mutable(&mut self, local: Local, location: Location) -> bool {
+        self.indirectly_mutable.seek(location);
+        self.indirectly_mutable.get().contains(local)
+    }
+
+    /// Returns `true` if `local` is `NeedsDrop` at the given `Location`.
+    ///
+    /// Only updates the cursor if absolutely necessary
+    fn needs_drop_lazy_seek(&mut self, local: Local, location: Location) -> bool {
+        if !self.needs_drop.in_any_value_of_ty.contains(local) {
+            return false;
+        }
+
+        self.needs_drop.cursor.seek_before(location);
+        self.needs_drop.cursor.get().contains(local)
+            || self.indirectly_mutable(local, location)
+    }
+
+    /// Returns `true` if `local` is `HasMutInterior`, but requires the `has_mut_interior` and
+    /// `indirectly_mutable` cursors to be updated beforehand.
+    fn has_mut_interior_eager_seek(&self, local: Local) -> bool {
+        if !self.has_mut_interior.in_any_value_of_ty.contains(local) {
+            return false;
+        }
+
+        self.has_mut_interior.cursor.get().contains(local)
+            || self.indirectly_mutable.get().contains(local)
+    }
 }
 
 pub struct Validator<'a, 'mir, 'tcx> {
@@ -63,53 +129,43 @@ impl Deref for Validator<'_, 'mir, 'tcx> {
     }
 }
 
-pub fn compute_indirectly_mutable_locals<'mir, 'tcx>(
-    item: &Item<'mir, 'tcx>,
-) -> RefCell<IndirectlyMutableResults<'mir, 'tcx>> {
-    let dead_unwinds = BitSet::new_empty(item.body.basic_blocks().len());
-
-    let indirectly_mutable_locals = old_dataflow::do_dataflow(
-        item.tcx,
-        item.body,
-        item.def_id,
-        &item.tcx.get_attrs(item.def_id),
-        &dead_unwinds,
-        old_dataflow::IndirectlyMutableLocals::new(item.tcx, item.body, item.param_env),
-        |_, local| old_dataflow::DebugFormatted::new(&local),
-    );
-
-    let indirectly_mutable_locals = old_dataflow::DataflowResultsCursor::new(
-        indirectly_mutable_locals,
-        item.body,
-    );
-
-    RefCell::new(indirectly_mutable_locals)
-}
-
 impl Validator<'a, 'mir, 'tcx> {
     pub fn new(
         item: &'a Item<'mir, 'tcx>,
-        indirectly_mutable_locals: &'a RefCell<IndirectlyMutableResults<'mir, 'tcx>>,
     ) -> Self {
         let dead_unwinds = BitSet::new_empty(item.body.basic_blocks().len());
 
-        let needs_drop = FlowSensitiveResolver::new(
+        let needs_drop = QualifCursor::new(
             NeedsDrop,
             item,
-            indirectly_mutable_locals,
             &dead_unwinds,
         );
 
-        let has_mut_interior = FlowSensitiveResolver::new(
+        let has_mut_interior = QualifCursor::new(
             HasMutInterior,
             item,
-            indirectly_mutable_locals,
             &dead_unwinds,
         );
 
+        let indirectly_mutable = old_dataflow::do_dataflow(
+            item.tcx,
+            item.body,
+            item.def_id,
+            &item.tcx.get_attrs(item.def_id),
+            &dead_unwinds,
+            old_dataflow::IndirectlyMutableLocals::new(item.tcx, item.body, item.param_env),
+            |_, local| old_dataflow::DebugFormatted::new(&local),
+        );
+
+        let indirectly_mutable = old_dataflow::DataflowResultsCursor::new(
+            indirectly_mutable,
+            item.body,
+        );
+
         let qualifs = Qualifs {
             needs_drop,
             has_mut_interior,
+            indirectly_mutable,
         };
 
         Validator {
@@ -122,14 +178,6 @@ impl Validator<'a, 'mir, 'tcx> {
         }
     }
 
-    /// Resets the `QualifResolver`s used by this `Validator` and returns them so they can be
-    /// reused.
-    pub fn into_qualifs(mut self) -> Qualifs<'a, 'mir, 'tcx> {
-        self.qualifs.needs_drop.reset();
-        self.qualifs.has_mut_interior.reset();
-        self.qualifs
-    }
-
     pub fn take_errors(&mut self) -> Vec<(Span, String)> {
         std::mem::replace(&mut self.errors, vec![])
     }
@@ -304,10 +352,16 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
         // it depends on `HasMutInterior` being set for mutable borrows as well as values with
         // interior mutability.
         if let Rvalue::Ref(_, kind, ref borrowed_place) = *rvalue {
-            let rvalue_has_mut_interior = {
-                let has_mut_interior = self.qualifs.has_mut_interior.get();
-                HasMutInterior::in_rvalue(&self.item, &|l| has_mut_interior.contains(l), rvalue)
-            };
+            // FIXME: Change the `in_*` methods to take a `FnMut` so we don't have to manually seek
+            // the cursors beforehand.
+            self.qualifs.has_mut_interior.cursor.seek_before(location);
+            self.qualifs.indirectly_mutable.seek(location);
+
+            let rvalue_has_mut_interior = HasMutInterior::in_rvalue(
+                &self.item,
+                &|local| self.qualifs.has_mut_interior_eager_seek(local),
+                rvalue,
+            );
 
             if rvalue_has_mut_interior {
                 let is_derived_from_illegal_borrow = match borrowed_place.as_local() {
@@ -402,9 +456,6 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
     fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
         trace!("visit_statement: statement={:?} location={:?}", statement, location);
 
-        self.qualifs.needs_drop.visit_statement(statement, location);
-        self.qualifs.has_mut_interior.visit_statement(statement, location);
-
         match statement.kind {
             StatementKind::Assign(..) => {
                 self.super_statement(statement, location);
@@ -424,15 +475,6 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
         }
     }
 
-    fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
-        trace!("visit_terminator: terminator={:?} location={:?}", terminator, location);
-
-        self.qualifs.needs_drop.visit_terminator(terminator, location);
-        self.qualifs.has_mut_interior.visit_terminator(terminator, location);
-
-        self.super_terminator(terminator, location);
-    }
-
     fn visit_terminator_kind(&mut self, kind: &TerminatorKind<'tcx>, location: Location) {
         trace!("visit_terminator_kind: kind={:?} location={:?}", kind, location);
         self.super_terminator_kind(kind, location);
@@ -511,7 +553,7 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
                 let needs_drop = if let Some(local) = dropped_place.as_local() {
                     // Use the span where the local was declared as the span of the drop error.
                     err_span = self.body.local_decls[local].source_info.span;
-                    self.qualifs.needs_drop.contains(local)
+                    self.qualifs.needs_drop_lazy_seek(local, location)
                 } else {
                     true
                 };

src/librustc_mir/transform/qualify_consts.rs

@@ -968,8 +968,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
         }
 
         let item = new_checker::Item::new(self.tcx, self.def_id, self.body);
-        let mut_borrowed_locals = new_checker::validation::compute_indirectly_mutable_locals(&item);
-        let mut validator = new_checker::validation::Validator::new(&item, &mut_borrowed_locals);
+        let mut validator = new_checker::validation::Validator::new(&item);
 
         validator.suppress_errors = !use_new_validator;
         self.suppress_errors = use_new_validator;