Move field-sensitivity logic out of transfer_function and into ModularMutationVisitor

willcrichton · willcrichton · commit c975eee6459e · 2023-03-10T10:28:49.000-08:00
diff --git a/Makefile.toml b/Makefile.toml
@@ -1,11 +1,13 @@
+[config]
+skip_core_tasks = true
+default_to_workspace = false
+
 [tasks.watch-cargo]
 script = "cargo watch -x 'install --path crates/flowistry_ide --debug --offline'"
 install_crate = { crate_name = "cargo-watch" }
-workspace = false
 
 [tasks.watch-js]
 script = "cd ide && npm run watch"
-workspace = false
 
 [tasks.watch.run_task]
 name = ["watch-cargo", "watch-js"]
diff --git a/crates/flowistry/src/infoflow/analysis.rs b/crates/flowistry/src/infoflow/analysis.rs
@@ -135,22 +135,8 @@ impl<'a, 'tcx> FlowAnalysis<'a, 'tcx> {
     add_deps(mutated, &mut input_location_deps);
 
     // Add deps of all inputs
-    let mut children = Vec::new();
-    for (place, elem) in inputs.iter() {
+    for place in inputs.iter() {
       add_deps(*place, &mut input_location_deps);
-
-      // If the input is associated to a specific projection of the mutated
-      // place, then save that input's dependencies with the projection
-      if let Some(elem) = elem {
-        let mut projection = mutated.projection.to_vec();
-        projection.push(*elem);
-        let mut child_deps = LocationOrArgSet::new(location_domain);
-        add_deps(*place, &mut child_deps);
-        children.push((
-          Place::make(mutated.local, &projection, self.tcx),
-          child_deps,
-        ));
-      }
     }
 
     // Add control dependencies
@@ -168,52 +154,33 @@ impl<'a, 'tcx> FlowAnalysis<'a, 'tcx> {
       }
     }
 
-    if children.len() > 0 {
-      // In the special case of mutated = aggregate { x: .., y: .. }
-      // then we ensure that deps(mutated.x) != deps(mutated)
-
-      // First, ensure that all children have the current in their deps.
-      // See test struct_read_constant for where this is needed.
-      for child in all_aliases.children(mutated) {
-        state.insert(all_aliases.normalize(child), location);
-      }
-
-      // Then for constructor arguments that were places, add dependencies of those places.
-      for (child, deps) in children {
-        state.union_into_row(all_aliases.normalize(child), &deps);
-      }
-
-      // Finally add input_location_deps *JUST* to mutated, not conflicts of mutated.
-      state.union_into_row(all_aliases.normalize(mutated), &input_location_deps);
-    } else {
-      // Union dependencies into all conflicting places of the mutated place
-      let mut mutable_conflicts = all_aliases.conflicts(mutated).to_owned();
-
-      // Remove any conflicts that aren't actually mutable, e.g. if x : &T ends up
-      // as an alias of y: &mut T. See test function_lifetime_alias_mut for an example.
-      let ignore_mut =
-        is_extension_active(|mode| mode.mutability_mode == MutabilityMode::IgnoreMut);
-      if !ignore_mut {
-        let body = self.body;
-        let tcx = self.tcx;
-        mutable_conflicts = mutable_conflicts
-          .iter()
-          .filter(|place| {
-            place.iter_projections().all(|(sub_place, _)| {
-              let ty = sub_place.ty(body.local_decls(), tcx).ty;
-              !matches!(ty.ref_mutability(), Some(Mutability::Not))
-            })
+    // Union dependencies into all conflicting places of the mutated place
+    let mut mutable_conflicts = all_aliases.conflicts(mutated).to_owned();
+
+    // Remove any conflicts that aren't actually mutable, e.g. if x : &T ends up
+    // as an alias of y: &mut T. See test function_lifetime_alias_mut for an example.
+    let ignore_mut =
+      is_extension_active(|mode| mode.mutability_mode == MutabilityMode::IgnoreMut);
+    if !ignore_mut {
+      let body = self.body;
+      let tcx = self.tcx;
+      mutable_conflicts = mutable_conflicts
+        .iter()
+        .filter(|place| {
+          place.iter_projections().all(|(sub_place, _)| {
+            let ty = sub_place.ty(body.local_decls(), tcx).ty;
+            !matches!(ty.ref_mutability(), Some(Mutability::Not))
           })
-          .copied()
-          .collect::<HashSet<_>>();
-      };
+        })
+        .copied()
+        .collect::<HashSet<_>>();
+    };
 
-      debug!("  Mutated conflicting places: {mutable_conflicts:?}");
-      debug!("    with deps {input_location_deps:?}");
+    debug!("  Mutated conflicting places: {mutable_conflicts:?}");
+    debug!("    with deps {input_location_deps:?}");
 
-      for place in mutable_conflicts.into_iter() {
-        state.union_into_row(all_aliases.normalize(place), &input_location_deps);
-      }
+    for place in mutable_conflicts.into_iter() {
+      state.union_into_row(all_aliases.normalize(place), &input_location_deps);
     }
   }
 }
diff --git a/crates/flowistry/src/infoflow/mutation.rs b/crates/flowistry/src/infoflow/mutation.rs
@@ -1,7 +1,10 @@
 //! Identifies the mutated places in a MIR instruction via modular approximation based on types.
 
 use log::debug;
-use rustc_middle::mir::{visit::Visitor, *};
+use rustc_middle::{
+  mir::{visit::Visitor, *},
+  ty::TyKind,
+};
 
 use crate::mir::{
   aliases::Aliases,
@@ -22,9 +25,8 @@ pub struct Mutation<'a, 'tcx> {
   /// The place that is being mutated.
   pub mutated: Place<'tcx>,
 
-  /// The set of inputs to the mutating operation, each paired with
-  /// an optional [`ProjectionElem::Field`] in the case of aggregate constructors.
-  pub inputs: &'a [(Place<'tcx>, Option<PlaceElem<'tcx>>)],
+  /// The set of inputs to the mutating operation.
+  pub inputs: &'a [Place<'tcx>],
 
   /// Where the mutation is occuring.
   pub location: Location,
@@ -63,19 +65,74 @@ where
 {
   fn visit_assign(
     &mut self,
-    place: &Place<'tcx>,
+    mutated: &Place<'tcx>,
     rvalue: &Rvalue<'tcx>,
     location: Location,
   ) {
-    debug!("Checking {location:?}: {place:?} = {rvalue:?}");
-    let mut collector = PlaceCollector {
-      places: Vec::new(),
-      tcx: self.aliases.tcx,
-    };
+    debug!("Checking {location:?}: {mutated:?} = {rvalue:?}");
+    let body = self.aliases.body;
+    let tcx = self.aliases.tcx;
+
+    match rvalue {
+      // In the case of _1 = aggregate { field1: op1, field2: op2, ... },
+      // then destructure this into a series of mutations like
+      // _1.field1 = op1, _1.field2 = op2, and so on.
+      Rvalue::Aggregate(box AggregateKind::Adt(def_id, idx, substs, _, _), ops) => {
+        let adt_def = tcx.adt_def(*def_id);
+        let variant = adt_def.variant(*idx);
+        if variant.fields.len() > 0 {
+          let fields = variant.fields.iter().enumerate().zip(ops.iter()).map(
+            |((i, field), input_op)| {
+              let input_place = input_op.to_place();
+              let field = PlaceElem::Field(Field::from_usize(i), field.ty(tcx, substs));
+              (mutated.project_deeper(&[field], tcx), input_place)
+            },
+          );
+          for (mutated, input) in fields {
+            (self.f)(Mutation {
+              mutated,
+              inputs: input.as_ref().map(std::slice::from_ref).unwrap_or_default(),
+              location,
+              status: MutationStatus::Definitely,
+            });
+          }
+          return;
+        }
+      }
+
+      // In the case of _1 = _2 where _2 : struct Foo { x: T, y: S, .. },
+      // then destructure this into a series of mutations like
+      // _1.x = _2.x, _1.y = _2.y, and so on.
+      Rvalue::Use(Operand::Move(place) | Operand::Copy(place)) => {
+        let place_ty = place.ty(&body.local_decls, tcx).ty;
+        if let TyKind::Adt(adt_def, substs) = place_ty.kind() {
+          if adt_def.is_struct() {
+            let fields = adt_def.all_fields().enumerate().map(|(i, field_def)| {
+              PlaceElem::Field(Field::from_usize(i), field_def.ty(tcx, substs))
+            });
+            for field in fields {
+              let mutated_field = mutated.project_deeper(&[field], tcx);
+              let input_field = place.project_deeper(&[field], tcx);
+              (self.f)(Mutation {
+                mutated: mutated_field,
+                inputs: &[input_field],
+                location,
+                status: MutationStatus::Definitely,
+              });
+            }
+            return;
+          }
+        }
+      }
+
+      _ => {}
+    }
+
+    let mut collector = PlaceCollector::default();
     collector.visit_rvalue(rvalue, location);
     (self.f)(Mutation {
-      mutated: *place,
-      inputs: &collector.places,
+      mutated: *mutated,
+      inputs: &collector.0,
       location,
       status: MutationStatus::Definitely,
     });
@@ -99,10 +156,7 @@ where
           .map(|(_, place)| place)
           .filter(|place| !async_hack.ignore_place(*place))
           .collect::<Vec<_>>();
-        let arg_inputs = arg_places
-          .iter()
-          .map(|place| (*place, None))
-          .collect::<Vec<_>>();
+        let arg_inputs = arg_places.clone();
 
         let ret_is_unit = destination
           .ty(self.aliases.body.local_decls(), tcx)
@@ -140,7 +194,7 @@ where
         if let Some(src) = value.to_place() {
           (self.f)(Mutation {
             mutated: *place,
-            inputs: &[(src, None)],
+            inputs: &[src],
             location,
             status: MutationStatus::Definitely,
           });
diff --git a/crates/flowistry/src/infoflow/recursive.rs b/crates/flowistry/src/infoflow/recursive.rs
@@ -201,7 +201,7 @@ impl<'tcx> FlowAnalysis<'_, 'tcx> {
         let parent_deps = return_state
           .rows()
           .filter(|(_, deps)| child_deps.is_superset(deps))
-          .filter_map(|(row, _)| Some((translate_child_to_parent(row, false)?, None)))
+          .filter_map(|(row, _)| translate_child_to_parent(row, false))
           .collect::<Vec<_>>();
 
         debug!(
diff --git a/crates/flowistry/src/mir/utils.rs b/crates/flowistry/src/mir/utils.rs
@@ -163,10 +163,8 @@ impl PlaceRelation {
   }
 }
 
-pub struct PlaceCollector<'tcx> {
-  pub tcx: TyCtxt<'tcx>,
-  pub places: Vec<(Place<'tcx>, Option<PlaceElem<'tcx>>)>,
-}
+#[derive(Default)]
+pub struct PlaceCollector<'tcx>(pub Vec<Place<'tcx>>);
 
 impl<'tcx> Visitor<'tcx> for PlaceCollector<'tcx> {
   fn visit_place(
@@ -175,32 +173,7 @@ impl<'tcx> Visitor<'tcx> for PlaceCollector<'tcx> {
     _context: PlaceContext,
     _location: Location,
   ) {
-    self.places.push((*place, None));
-  }
-
-  fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) {
-    match rvalue {
-      Rvalue::Aggregate(box AggregateKind::Adt(def_id, idx, substs, _, _), ops) => {
-        // In the case of _1 = aggregate { field1: op1, field2: op2, ... }
-        // we want to remember which places correspond to which fields so the infoflow
-        // analysis can be field-sensitive for constructors.
-        let adt_def = self.tcx.adt_def(*def_id);
-        let variant = adt_def.variant(*idx);
-        let places = variant
-          .fields
-          .iter()
-          .enumerate()
-          .zip(ops.iter())
-          .filter_map(|((i, field), op)| {
-            let place = op.to_place()?;
-            let field =
-              ProjectionElem::Field(Field::from_usize(i), field.ty(self.tcx, substs));
-            Some((place, Some(field)))
-          });
-        self.places.extend(places);
-      }
-      _ => self.super_rvalue(rvalue, location),
-    }
+    self.0.push(*place);
   }
 }
 
diff --git a/crates/flowistry_ide/src/focus/direct_influence.rs b/crates/flowistry_ide/src/focus/direct_influence.rs
@@ -28,7 +28,7 @@ impl<'a, 'tcx> DirectInfluence<'a, 'tcx> {
           }
         };
 
-        for (input, _) in inputs {
+        for input in inputs {
           add(*input, Mutability::Not);
         }
 
diff --git a/crates/rustc_plugin/src/lib.rs b/crates/rustc_plugin/src/lib.rs
@@ -12,7 +12,7 @@ use std::{
   env, fs,
   ops::Deref,
   path::{Path, PathBuf},
-  process::{exit, Command},
+  process::{exit, Command, Stdio},
 };
 
 pub use cargo_metadata::camino::Utf8Path;
@@ -101,6 +101,7 @@ pub fn cli_main<T: RustcPlugin>(plugin: T) {
   let args = plugin.args(&target_dir);
 
   let mut cmd = Command::new("cargo");
+  cmd.stdout(Stdio::inherit()).stderr(Stdio::inherit());
 
   let mut path = env::current_exe()
     .expect("current executable path invalid")
@@ -243,11 +244,7 @@ pub fn cli_main<T: RustcPlugin>(plugin: T) {
     cmd.args(flags);
   }
 
-  let exit_status = cmd
-    .spawn()
-    .expect("could not run cargo")
-    .wait()
-    .expect("failed to wait for cargo?");
+  let exit_status = cmd.status().expect("failed to wait for cargo?");
 
   exit(exit_status.code().unwrap_or(-1));
 }

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ impl<'a, 'tcx> DirectInfluence<'a, 'tcx> {`
`28`	`28`	`}`
`29`	`29`	`};`
`30`	`30`
`31`		`- for (input, _) in inputs {`
	`31`	`+ for input in inputs {`
`32`	`32`	`add(*input, Mutability::Not);`
`33`	`33`	`}`
`34`	`34`