Add missing documentation, update indexical and rustc_plugin

Cargo.toml

@@ -3,5 +3,10 @@ members = ["crates/*"]
 exclude = ["ide/src/tests/mock_project"]
 resolver = "2"
 
+[workspace.dependencies]
+rustc_plugin = "=0.7.2-nightly-2023-08-25"
+rustc_utils = {version = "=0.7.2-nightly-2023-08-25", features = ["indexical"]}
+indexical = {version = "0.3.1", default-features = false, features = ["rustc"]}
+
 [profile.bench]
 debug = true

crates/flowistry/Cargo.toml

@@ -20,8 +20,8 @@ log = "0.4"
 fluid-let = "1.0"
 cfg-if = "1.0"
 serde = {version = "1", features = ["derive"]}
-rustc_utils = {version = "0.7.1-nightly-2023-08-25", features = ["indexical"]}
-indexical = { version = "0.2.2", default-features = false, features = ["rustc"] }
+rustc_utils = {workspace = true, features = ["indexical"]}
+indexical = {workspace = true}
 
 # For local debugging
 html-escape = {version = "0.2", optional = true}

crates/flowistry/src/extensions.rs

@@ -1,12 +1,17 @@
 //! Extra features for evaluating / ablating the precision of Flowistry's algorithm.
+#![allow(missing_docs)]
+
 use std::{cell::RefCell, str::FromStr};
 
 use fluid_let::fluid_let;
 use serde::{Deserialize, Serialize};
 
+/// Whether Flowistry should ignore the distinction between mutable and immtuable references
 #[derive(Debug, PartialEq, Eq, Clone, Copy, Deserialize, Serialize, Hash)]
 pub enum MutabilityMode {
+  /// Precise behavior, distinguish them
   DistinguishMut,
+  /// Imprecise behavior, do not distinguish them (assume everything is mutable)
   IgnoreMut,
 }
 
@@ -21,9 +26,12 @@ impl FromStr for MutabilityMode {
   }
 }
 
+/// Whether Flowistry should attempt to recurse into call-sites to analyze them
 #[derive(Debug, PartialEq, Eq, Clone, Copy, Deserialize, Serialize, Hash)]
 pub enum ContextMode {
+  /// Imprecise behavior, only use the modular approximation
   SigOnly,
+  /// Precise behavior, recurse into call sites when possible
   Recurse,
 }
 
@@ -38,9 +46,12 @@ impl FromStr for ContextMode {
   }
 }
 
+/// Whether Flowistry should use lifetimes to distinguish pointers
 #[derive(Debug, PartialEq, Eq, Clone, Copy, Deserialize, Serialize, Hash)]
 pub enum PointerMode {
+  /// Precise behavior, use lifetimes
   Precise,
+  /// Imprecise behavior, assume all pointers alias
   Conservative,
 }
 
@@ -55,6 +66,7 @@ impl FromStr for PointerMode {
   }
 }
 
+/// A combination of all the precision levers.
 #[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Hash)]
 pub struct EvalMode {
   pub mutability_mode: MutabilityMode,

crates/flowistry/src/infoflow/analysis.rs

@@ -1,6 +1,6 @@
 use std::{cell::RefCell, rc::Rc};
 
-use indexical::impls::RustcIndexMatrix;
+use indexical::impls::RustcIndexMatrix as IndexMatrix;
 use log::{debug, trace};
 use rustc_data_structures::fx::FxHashMap as HashMap;
 use rustc_hir::{def_id::DefId, BodyId};
@@ -34,37 +34,49 @@ use crate::{
 ///
 /// `FlowDomain` represents $\Theta$ that maps from places $p$ to dependencies $\kappa$. To efficiently represent $\kappa$, a set of locations,
 /// we use the bit-set data structures in [`rustc_index::bit_set`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_index/bit_set/index.html).
-/// However instead of using those datatypes directly, we provide several abstractions in the [`indexed`](crate::indexed)
-/// module that have a more ergonomic interface and more efficient implementation than their `bit_set` counterparts.
+/// However instead of using a bit-set directly, we use the [`indexical`] crate to map between raw indices and the objects they represent.
 ///
 /// The [`IndexMatrix`] maps from a [`Place`] to a [`LocationOrArgSet`] via the [`IndexMatrix::row_set`] method. The [`LocationOrArgSet`] is an
-/// [`IndexSet`](crate::indexed::IndexSet) of locations (or arguments, see note below), which wraps a
+/// [`IndexSet`](indexical::IndexSet) of locations (or arguments, see note below), which wraps a
 /// [`rustc_index::bit_set::HybridBitSet`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_index/bit_set/enum.HybridBitSet.html) and
-/// has roughly the same API. The main difference is that `HybridBitSet` operates only on values that implement the
-/// [`rustc_index::vec::Idx`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_index/vec/trait.Idx.html) trait (usually created via
-/// the [`rustc_index::newtype_index`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_index/macro.newtype_index.html) macro). The `indexed`
-/// module has a concept of [`IndexedDomain`] to represent the mapping from a set of values to the indexes those values --- [`LocationOrArgDomain`]
-/// is the implementation for locations.
+/// has roughly the same API. The [`indexical`] crate has a concept of an [`IndexedDomain`](indexical::IndexedDomain) to represent the mapping from
+/// a set of values to the indexes those values --- [`LocationOrArgDomain`] is the implementation for locations.
 ///
+/// # **Note:** reading dependencies from `FlowDomain`
+/// In general, you should *not* use [`FlowDomain::row_set`] directly. This is because the `FlowDomain` does not have exactly the same structure as
+/// the $\Theta$ described in the paper. Based on performance profiling, we have determined that the size of the `FlowDomain` is the primary factor that
+/// increases Flowistry's memory usage and runtime. So we generally trade-off making `FlowDomain` smaller in exchange for making dependency lookups more
+/// computationally expensive.
 ///
-/// # Note: arguments as dependencies
+/// Instead, you should use [`FlowAnalysis::deps_for`](crate::infoflow::FlowAnalysis::deps_for) to read a place's dependencies out of a given `FlowDomain`.
+///
+/// # **Note:** arguments as dependencies
 /// Because function arguments are never initialized, there is no "root" location for argument places. This fact poses a problem for
 /// information flow analysis: an instruction `bb[0]: _2 = _1` (where `_1` is an argument) would set $\Theta(\verb|_2|) = \Theta(\verb|_1|) \cup \\{\verb|bb0\[0\]|\\}\$.
 /// However, $\Theta(\verb|_1|)$ would be empty, so it would be imposible to determine that `_2` depends on `_1`. To solve this issue, we
 /// enrich the domain of locations with arguments, using the [`LocationOrArg`] type. Any dependency can be on *either* a location or an argument.
-pub type FlowDomain<'tcx> = RustcIndexMatrix<Place<'tcx>, LocationOrArg>;
+pub type FlowDomain<'tcx> = IndexMatrix<Place<'tcx>, LocationOrArg>;
 
 /// Data structure that holds context for performing the information flow analysis.
 pub struct FlowAnalysis<'a, 'tcx> {
+  /// The type context used for the analysis.
   pub tcx: TyCtxt<'tcx>,
+
+  /// The ID of the body being analyzed.
   pub def_id: DefId,
+
+  /// The body being analyzed.
   pub body: &'a Body<'tcx>,
-  pub control_dependencies: ControlDependencies<BasicBlock>,
+
+  /// The metadata about places used in the analysis.
   pub place_info: PlaceInfo<'a, 'tcx>,
+
+  pub(crate) control_dependencies: ControlDependencies<BasicBlock>,
   pub(crate) recurse_cache: RefCell<HashMap<BodyId, FlowResults<'a, 'tcx>>>,
 }
 
 impl<'a, 'tcx> FlowAnalysis<'a, 'tcx> {
+  /// Constructs (but does not execute) a new FlowAnalysis.
   pub fn new(
     tcx: TyCtxt<'tcx>,
     def_id: DefId,
@@ -84,6 +96,7 @@ impl<'a, 'tcx> FlowAnalysis<'a, 'tcx> {
     }
   }
 
+  /// Returns the [`LocationOrArgDomain`] used by the analysis.
   pub fn location_domain(&self) -> &Rc<LocationOrArgDomain> {
     self.place_info.location_domain()
   }
@@ -103,6 +116,10 @@ impl<'a, 'tcx> FlowAnalysis<'a, 'tcx> {
     conflicts.chain(provenance).copied().collect()
   }
 
+  /// Returns all the dependencies of `place` within `state`.
+  ///
+  /// Prefer using this method instead of accessing `FlowDomain` directly,
+  /// unless you *really* know what you're doing.
   pub fn deps_for(
     &self,
     state: &FlowDomain<'tcx>,
@@ -115,7 +132,7 @@ impl<'a, 'tcx> FlowAnalysis<'a, 'tcx> {
       .iter()
       .flat_map(|place| self.influences(*place))
     {
-      deps.union(&state.row_set(&self.place_info.normalize(subplace)));
+      deps.union(state.row_set(&self.place_info.normalize(subplace)));
     }
     deps
   }
@@ -144,7 +161,7 @@ impl<'a, 'tcx> FlowAnalysis<'a, 'tcx> {
       for relevant in self.influences(input) {
         let relevant_deps = state.row_set(&self.place_info.normalize(relevant));
         trace!("    For relevant {relevant:?} for input {input:?} adding deps {relevant_deps:?}");
-        target_deps.union(&relevant_deps);
+        target_deps.union(relevant_deps);
       }
     };
 

crates/flowistry/src/infoflow/dependencies.rs

@@ -63,7 +63,7 @@ impl TargetDeps {
           // conflict should already be normalized because the input to aliases.children is normalized
           let deps = state.row_set(&conflict);
           trace!("place={place:?}, conflict={conflict:?}, deps={deps:?}");
-          forward.intersect(&deps);
+          forward.intersect(deps);
         }
 
         forward.insert(location);

crates/flowistry/src/infoflow/mod.rs

@@ -25,9 +25,9 @@ mod recursive;
 /// The output of the information flow analysis.
 ///
 /// Using the metavariables in [the paper](https://arxiv.org/abs/2111.13662): for each
-/// [`LocationOrArg`](crate::indexed::impls::LocationOrArg) $\ell$ in a [`Body`](rustc_middle::mir::Body) $f$,
+/// [`LocationOrArg`](rustc_utils::mir::location_or_arg::LocationOrArg) $\ell$ in a [`Body`](rustc_middle::mir::Body) $f$,
 /// this type contains a [`FlowDomain`] $\Theta_\ell$ that maps from a [`Place`](rustc_middle::mir::Place) $p$
-/// to a [`LocationOrArgSet`](crate::indexed::impls::LocationOrArgSet) $\kappa$. The domain of $\Theta_\ell$
+/// to a [`LocationOrArgSet`](rustc_utils::mir::location_or_arg::index::LocationOrArgSet) $\kappa$. The domain of $\Theta_\ell$
 /// is all places that have been defined up to $\ell$. For each place, $\Theta_\ell(p)$ contains the set of locations
 /// (or arguments) that could influence the value of that place, i.e. the place's dependencies.
 ///
@@ -39,10 +39,10 @@ mod recursive;
 /// # use flowistry::{infoflow::{FlowDomain, FlowResults}};
 /// # use rustc_utils::{mir::location_or_arg::index::LocationOrArgSet, PlaceExt};
 /// fn example<'tcx>(tcx: TyCtxt<'tcx>, results: &FlowResults<'_, 'tcx>) {
-///   let ℓ: Location          = Location::START;
-///   let Θ: &FlowDomain       = results.state_at(ℓ);
-///   let p: Place             = Place::make(Local::from_usize(1), &[], tcx);
-///   let κ: &LocationOrArgSet = Θ.row_set(&p);
+///   let ℓ: Location         = Location::START;
+///   let Θ: &FlowDomain      = results.state_at(ℓ);
+///   let p: Place            = Place::make(Local::from_usize(1), &[], tcx);
+///   let κ: LocationOrArgSet = results.analysis.deps_for(Θ, p);
 ///   for ℓ2 in κ.iter() {
 ///     println!("at location {ℓ:?}, place {p:?} depends on location {ℓ2:?}");
 ///   }
@@ -72,7 +72,7 @@ thread_local! {
 /// for a complete example of how to call this function.
 ///
 /// To get a `BodyWithBorrowckFacts`, you can use the
-/// [`get_body_with_borrowck_facts`](crate::mir::borrowck_facts::get_body_with_borrowck_facts)
+/// [`get_body_with_borrowck_facts`](rustc_utils::mir::borrowck_facts::get_body_with_borrowck_facts)
 /// function.
 ///
 /// See [`FlowResults`] for an explanation of how to use the return value.

crates/flowistry/src/infoflow/mutation.rs

@@ -43,9 +43,6 @@ pub struct Mutation<'tcx> {
 /// MIR [`Visitor`] methods.
 pub struct ModularMutationVisitor<'a, 'tcx, F>
 where
-  // API design note: wcrichto tried making FnMut(...) a trait alias, but this
-  // interacted poorly with type inference and required ModularMutationVisitor
-  // clients to explicitly write out the type parameter of every closure argument.
   F: FnMut(Location, Vec<Mutation<'tcx>>),
 {
   f: F,
@@ -56,6 +53,7 @@ impl<'a, 'tcx, F> ModularMutationVisitor<'a, 'tcx, F>
 where
   F: FnMut(Location, Vec<Mutation<'tcx>>),
 {
+  /// Constructs a new visitor.
   pub fn new(place_info: &'a PlaceInfo<'a, 'tcx>, f: F) -> Self {
     ModularMutationVisitor { place_info, f }
   }

crates/flowistry/src/lib.rs

@@ -48,6 +48,7 @@
   clippy::len_zero,
   clippy::len_without_is_empty
 )]
+#![warn(missing_docs)]
 
 extern crate either;
 extern crate polonius_engine;

crates/flowistry/src/mir/aliases.rs

@@ -68,6 +68,7 @@ rustc_index::newtype_index! {
 }
 
 impl<'a, 'tcx> Aliases<'a, 'tcx> {
+  /// Runs the alias analysis on a given `body_with_facts`.
   pub fn build(
     tcx: TyCtxt<'tcx>,
     def_id: DefId,
@@ -81,6 +82,9 @@ impl<'a, 'tcx> Aliases<'a, 'tcx> {
     }
   }
 
+  /// Alternative constructor if you need to filter out certain borrowck facts.
+  /// 
+  /// Just use [`Aliases::build`] unless you know what you're doing.
   pub fn build_with_fact_selection(
     tcx: TyCtxt<'tcx>,
     def_id: DefId,
@@ -104,7 +108,7 @@ impl<'a, 'tcx> Aliases<'a, 'tcx> {
     let start = Instant::now();
     let body = &body_with_facts.body;
     let static_region = RegionVid::from_usize(0);
-    let ref subset_base = body_with_facts
+    let subset_base = &body_with_facts
       .input_facts
       .as_ref()
       .unwrap()
@@ -342,6 +346,22 @@ impl<'a, 'tcx> Aliases<'a, 'tcx> {
     contains
   }
 
+  /// Given a `place`, returns the set of direct places it could refer to.
+  ///
+  /// For example, in the program:
+  /// ```
+  /// let x = 1;
+  /// let y = &x;
+  /// ```
+  ///
+  /// The place `*y` (but NOT `y`) is an alias for `x`. Similarly, in the program:
+  ///
+  /// ```
+  /// let v = vec![1, 2, 3];
+  /// let n = &v[0];
+  /// ```
+  ///
+  /// The place `*n` is an alias for `v` (even though they have different types!).
   pub fn aliases(&self, place: Place<'tcx>) -> PlaceSet<'tcx> {
     let mut aliases = HashSet::default();
     aliases.insert(place);

crates/flowistry/src/mir/engine.rs

@@ -33,12 +33,14 @@ use rustc_utils::{
 /// An alternative implementation of
 /// [`rustc_mir_dataflow::Results`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_mir_dataflow/struct.Results.html).
 pub struct AnalysisResults<'tcx, A: Analysis<'tcx>> {
+  /// The underlying analysis that was used to generate the results.
   pub analysis: A,
   location_domain: Rc<LocationOrArgDomain>,
   state: IndexVec<LocationOrArgIndex, A::Domain>,
 }
 
 impl<'tcx, A: Analysis<'tcx>> AnalysisResults<'tcx, A> {
+  /// Same as [`rustc_mir_dataflow::Results::visit_reachable_with`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_mir_dataflow/struct.Results.html#method.visit_reachable_with).
   pub fn visit_reachable_with<'mir, V>(&self, body: &'mir Body<'tcx>, visitor: &mut V)
   where
     V: ResultsVisitor<'mir, 'tcx, Self, FlowState = A::Domain>,

crates/flowistry/src/mir/placeinfo.rs

@@ -29,9 +29,9 @@ use crate::extensions::{is_extension_active, MutabilityMode};
 
 /// Utilities for analyzing places: children, aliases, etc.
 pub struct PlaceInfo<'a, 'tcx> {
-  pub tcx: TyCtxt<'tcx>,
-  pub body: &'a Body<'tcx>,
-  pub def_id: DefId,
+  pub(crate) tcx: TyCtxt<'tcx>,
+  pub(crate) body: &'a Body<'tcx>,
+  pub(crate) def_id: DefId,
   location_domain: Rc<LocationOrArgDomain>,
 
   // Core computed data structure
@@ -44,22 +44,23 @@ pub struct PlaceInfo<'a, 'tcx> {
   reachable_cache: Cache<(Place<'tcx>, Mutability), PlaceSet<'tcx>>,
 }
 
-pub fn build_location_arg_domain(body: &Body) -> Rc<LocationOrArgDomain> {
-  let all_locations = body.all_locations().map(LocationOrArg::Location);
-  let all_locals = body.args_iter().map(LocationOrArg::Arg);
-  let domain = all_locations.chain(all_locals).collect();
-  Rc::new(LocationOrArgDomain::new(domain))
-}
-
 impl<'a, 'tcx> PlaceInfo<'a, 'tcx> {
+  fn build_location_arg_domain(body: &Body) -> Rc<LocationOrArgDomain> {
+    let all_locations = body.all_locations().map(LocationOrArg::Location);
+    let all_locals = body.args_iter().map(LocationOrArg::Arg);
+    let domain = all_locations.chain(all_locals).collect();
+    Rc::new(LocationOrArgDomain::new(domain))
+  }
+
+  /// Computes all the metadata about places used within the infoflow analysis.
   pub fn build(
     tcx: TyCtxt<'tcx>,
     def_id: DefId,
     body_with_facts: &'a BodyWithBorrowckFacts<'tcx>,
   ) -> Self {
     block_timer!("aliases");
     let body = &body_with_facts.body;
-    let location_domain = build_location_arg_domain(body);
+    let location_domain = Self::build_location_arg_domain(body);
     let aliases = Aliases::build(tcx, def_id, body_with_facts);
 
     PlaceInfo {
@@ -183,6 +184,7 @@ impl<'a, 'tcx> PlaceInfo<'a, 'tcx> {
     })
   }
 
+  /// Returns the [`LocationOrArgDomain`] for the current body.
   pub fn location_domain(&self) -> &Rc<LocationOrArgDomain> {
     &self.location_domain
   }