Bug Fixes found in Experimentation (#71)

## What Changed?

1. Improved `FnResolution::sig` to create as precise a signature as
possible and emit warnings if it cannot be done.
2. Main runner now skips analysis when compiling build scripts
3. Fixes a condition in `DiagnosticsRecorder::emit` which now aborts
correctly

## Why Does It Need To?

1. `probably_performs_side_effects` can now deal properly with
generators. Users are also now warned of unsoundness issues when a
precise function signature is not available.
2. Fixes spuriously aborting analysis because external annotations could
not be resolved in the context of build script build.
3. previously it was only aborting if the last messages were errors, now
it's if any of them are.

## Checklist

- [x] Above description has been filled out so that upon quash merge we
have a
  good record of what changed.
- [x] New functions, methods, types are documented. Old documentation is
updated
  if necessary
- [x] Documentation in Notion has been updated
- [x] Tests for new behaviors are provided
  - [x] New test suites (if any) ave been added to the CI tests (in
`.github/workflows/rust.yml`) either as compiler test or integration
test.
*Or* justification for their omission from CI has been provided in this
PR
    description.

crates/paralegal-flow/src/ana/inline/judge.rs

@@ -17,11 +17,14 @@ impl<'tcx> InlineJudge<'tcx> {
     /// performs side effects?
     fn probably_performs_side_effects(
         &self,
-        func: DefId,
+        func: FnResolution<'tcx>,
         args: &[Option<Place<'tcx>>],
         place_has_dependencies: impl Fn(Place<'tcx>) -> bool,
     ) -> bool {
-        let sig = self.tcx.fn_sig(func).skip_binder().skip_binder();
+        let Ok(sig) = func.sig(self.tcx) else {
+            return true;
+        };
+
         let has_no_outputs =
             sig.output().is_unit() && !sig.inputs().iter().any(|i| i.is_mutable_ptr());
         has_no_outputs || !args.iter().cloned().flatten().any(place_has_dependencies)
@@ -38,7 +41,7 @@ impl<'tcx> InlineJudge<'tcx> {
         self.analysis_control.avoid_inlining()
             && !self.function_has_markers(function)
             && !self.marker_is_reachable(function.def_id())
-            && !self.probably_performs_side_effects(function.def_id(), args, place_has_dependencies)
+            && !self.probably_performs_side_effects(function, args, place_has_dependencies)
     }
 
     /// Are there any markers on this function (direct or output type)?

crates/paralegal-flow/src/ana/inline/mod.rs

@@ -663,7 +663,7 @@ impl<'tcx> Inliner<'tcx> {
                 let local_as_global = GlobalLocal::at_root;
                 let call = self.get_call(*location);
                 debug!("Abstracting {function:?}");
-                let fn_sig = function.sig(self.tcx).skip_binder();
+                let fn_sig = function.sig(self.tcx).unwrap();
                 let writeables = Self::writeable_arguments(&fn_sig)
                     .filter_map(|idx| call.arguments[idx].as_ref().map(|i| i.0))
                     .chain(call.return_to.into_iter())

crates/paralegal-flow/src/lib.rs

@@ -183,6 +183,7 @@ impl rustc_driver::Callbacks for Callbacks {
                     )
                     .unwrap();
                 }
+                tcx.sess.abort_if_errors();
                 info!("All elems walked");
                 let result_path = compiler
                     .build_output_filenames(compiler.session(), &[])
@@ -294,7 +295,11 @@ impl rustc_plugin::RustcPlugin for DfppPlugin {
             .and_then(|s| plugin_args.target().map(|t| s == t))
             .unwrap_or(false);
 
-        if !is_target && std::env::var("CARGO_PRIMARY_PACKAGE").is_err() {
+        let is_build_script = crate_name
+            .as_ref()
+            .map_or(false, |n| n == "build_script_build");
+
+        if !is_target && (std::env::var("CARGO_PRIMARY_PACKAGE").is_err() || is_build_script) {
             return rustc_driver::RunCompiler::new(&compiler_args, &mut NoopCallbacks {}).run();
         }
 

crates/paralegal-flow/src/marker_db.rs

@@ -234,22 +234,42 @@ impl<'tcx> MarkerCtx<'tcx> {
         })
     }
 
-    /// All markers placed on this function, directly or through the type.
+    /// All markers placed on this function, directly or through the type plus
+    /// the type that was marked (if any).
     pub fn all_function_markers<'a>(
         &'a self,
         function: FnResolution<'tcx>,
     ) -> impl Iterator<Item = (&'a MarkerAnnotation, Option<(ty::Ty<'tcx>, DefId)>)> {
-        self.combined_markers(function.def_id())
-            .zip(std::iter::repeat(None))
-            .chain(
-                (self.0.config.local_function_type_marking() || !function.def_id().is_local())
-                    .then(|| {
-                        self.all_type_markers(function.sig(self.tcx()).skip_binder().output())
-                            .map(|(marker, typeinfo)| (marker, Some(typeinfo)))
-                    })
-                    .into_iter()
-                    .flatten(),
+        // Markers not coming from types, hence the "None"
+        let direct_markers = self
+            .combined_markers(function.def_id())
+            .zip(std::iter::repeat(None));
+        let get_type_markers = || {
+            let sig = function.sig(self.tcx()).ok()?;
+            let output = sig.output();
+            // XXX I'm not entirely sure this is how we should do
+            // this. For now I'm calling this "okay" because it's
+            // basically the old behavior
+            if output.is_closure() || output.is_generator() {
+                return None;
+            }
+            Some(
+                self.all_type_markers(output)
+                    .map(|(marker, typeinfo)| (marker, Some(typeinfo))),
             )
+        };
+
+        let include_type_markers =
+            self.0.config.local_function_type_marking() || !function.def_id().is_local();
+        direct_markers.chain(
+            if include_type_markers {
+                get_type_markers()
+            } else {
+                None
+            }
+            .into_iter()
+            .flatten(),
+        )
     }
 }
 

crates/paralegal-flow/src/utils/mod.rs

@@ -25,6 +25,7 @@ use crate::{
         rustc_target::spec::abi::Abi,
         ty,
     },
+    rustc_span::ErrorGuaranteed,
     Either, HashMap, HashSet, Symbol, TyCtxt,
 };
 
@@ -221,26 +222,102 @@ impl<'tcx> FnResolution<'tcx> {
         }
     }
 
-    pub fn sig(self, tcx: TyCtxt<'tcx>) -> ty::PolyFnSig {
-        match self {
-            FnResolution::Final(sub) => {
-                let ty = sub.ty(tcx, ty::ParamEnv::reveal_all());
-                if ty.is_closure() {
-                    sub.substs.as_closure().sig()
-                } else if ty.is_generator() {
-                    let gen_sig = sub.substs.as_generator().sig();
-                    ty::Binder::dummy(ty::FnSig {
-                        inputs_and_output: tcx
-                            .mk_type_list(&[gen_sig.resume_ty, gen_sig.return_ty]),
-                        c_variadic: false,
-                        unsafety: hir::Unsafety::Normal,
-                        abi: Abi::Rust,
-                    })
+    /// Get the most precise type signature we can for this function, erase any
+    /// regions and discharge binders.
+    ///
+    /// Returns an error if it was impossible to get any signature.
+    ///
+    /// Emits warnings if a precise signature could not be obtained or there
+    /// were type variables not instantiated.
+    pub fn sig(self, tcx: TyCtxt<'tcx>) -> Result<ty::FnSig<'tcx>, ErrorGuaranteed> {
+        let sess = tcx.sess;
+        let def_id = self.def_id();
+        let def_span = tcx.def_span(def_id);
+        let fn_kind = FunctionKind::for_def_id(tcx, def_id)?;
+        let late_bound_sig = match (self, fn_kind) {
+            (FnResolution::Final(sub), FunctionKind::Generator) => {
+                let gen = sub.substs.as_generator();
+                ty::Binder::dummy(ty::FnSig {
+                    inputs_and_output: tcx.mk_type_list(&[gen.resume_ty(), gen.return_ty()]),
+                    c_variadic: false,
+                    unsafety: hir::Unsafety::Normal,
+                    abi: Abi::Rust,
+                })
+            }
+            (FnResolution::Final(sub), FunctionKind::Closure) => sub.substs.as_closure().sig(),
+            (FnResolution::Final(sub), FunctionKind::Plain) => {
+                sub.ty(tcx, ty::ParamEnv::reveal_all()).fn_sig(tcx)
+            }
+            (FnResolution::Partial(_), FunctionKind::Closure) => {
+                if let Some(local) = def_id.as_local() {
+                    sess.span_warn(
+                        def_span,
+                        "Precise variable instantiation for \
+                            closure not known, using user type annotation.",
+                    );
+                    let sig = tcx.closure_user_provided_sig(local);
+                    Ok(sig.value)
                 } else {
-                    ty.fn_sig(tcx)
-                }
+                    Err(sess.span_err(
+                        def_span,
+                        format!(
+                            "Could not determine type signature for external closure {def_id:?}"
+                        ),
+                    ))
+                }?
             }
-            FnResolution::Partial(p) => tcx.fn_sig(p).skip_binder(),
+            (FnResolution::Partial(_), FunctionKind::Generator) => Err(sess.span_err(
+                def_span,
+                format!(
+                    "Cannot determine signature of generator {def_id:?} without monomorphization"
+                ),
+            ))?,
+            (FnResolution::Partial(_), FunctionKind::Plain) => {
+                let sig = tcx.fn_sig(def_id);
+                sig.no_bound_vars().unwrap_or_else(|| {
+                        sess.span_warn(def_span, format!("Cannot discharge bound variables for {sig:?}, they will not be considered by the analysis"));
+                        sig.skip_binder()
+                    })
+            }
+        };
+        Ok(tcx
+            .try_normalize_erasing_late_bound_regions(ty::ParamEnv::reveal_all(), late_bound_sig)
+            .unwrap_or_else(|e| {
+                sess.span_warn(
+                    def_span,
+                    format!("Could not erase regions in {late_bound_sig:?}: {e:?}"),
+                );
+                late_bound_sig.skip_binder()
+            }))
+    }
+}
+
+/// This exists to distinguish different types of functions, which is necessary
+/// because depending on the type of function, the method of requesting its
+/// signature from `TyCtxt` differs.
+///
+/// In addition generators also return true for `TyCtxt::is_closure` but must
+/// request their signature differently. Thus we factor that determination out
+/// into this enum.
+#[derive(Clone, Copy, Eq, PartialEq)]
+enum FunctionKind {
+    Closure,
+    Generator,
+    Plain,
+}
+
+impl FunctionKind {
+    fn for_def_id(tcx: TyCtxt, def_id: DefId) -> Result<Self, ErrorGuaranteed> {
+        if tcx.generator_kind(def_id).is_some() {
+            Ok(Self::Generator)
+        } else if tcx.is_closure(def_id) {
+            Ok(Self::Closure)
+        } else if tcx.def_kind(def_id).is_fn_like() {
+            Ok(Self::Plain)
+        } else {
+            Err(tcx
+                .sess
+                .span_err(tcx.def_span(def_id), "Expected this item to be a function."))
         }
     }
 }

crates/paralegal-policy/src/diagnostics.rs

@@ -419,10 +419,10 @@ impl DiagnosticsRecorder {
         let mut can_continue = true;
         for diag in self.0.lock().unwrap().drain(..) {
             for ctx in diag.context.iter().rev() {
-                write!(w, "[{ctx}] ")?;
+                write!(w, "{ctx} ")?;
             }
             writeln!(w, "{}", diag.message)?;
-            can_continue |= diag.severity.must_abort();
+            can_continue &= !diag.severity.must_abort();
         }
         Ok(can_continue)
     }