Improve our tests to check instances and visitor (rust-lang#51)

Add new checks to verify instance body and visitor. Also fix an issue with the rustc script when running with custom toolchain and add --bless option to `compiletest`.

Author: celinval

.github/scripts/run_rustc_tests.sh

@@ -37,8 +37,12 @@ function setup_rustc_repo() {
     git clone -b master https://github.com/rust-lang/rust.git "${RUST_REPO}"
     pushd "${RUST_REPO}"
     commit="$(rustc +${TOOLCHAIN} -vV | awk '/^commit-hash/ { print $2 }')"
-    git checkout ${commit}
-    git submodule init -- "${RUST_REPO}/library/stdarch"
+    if [[ "${commit}" != "unknown" ]]; then
+       # For custom toolchain, this may return "unknown". Skip this step if that's the case.
+       # In that case, we will use the HEAD of the main branch.
+      git checkout "${commit}"
+    fi
+    git submodule init -- "library/stdarch"
     git submodule update
   else
     pushd "${RUST_REPO}"
@@ -65,6 +69,14 @@ function run_tests() {
   HOST=$(rustc +${TOOLCHAIN} -vV | awk '/^host/ { print $2 }')
   FILE_CHECK="$(which FileCheck-12 || which FileCheck-13 || which FileCheck-14)"
 
+  echo "#---------- Variables -------------"
+  echo "RUST_REPO: ${RUST_REPO}"
+  echo "TOOLS_BIN: ${TOOLS_BIN}"
+  echo "TOOLCHAIN: ${TOOLCHAIN}"
+  echo "SYSROOT: ${SYSROOT}"
+  echo "FILE_CHECK: ${FILE_CHECK}"
+  echo "-----------------------------------"
+
   for suite_cfg in "${SUITES[@]}"; do
     # Hack to work on older bash like the ones on MacOS.
     suite_pair=($suite_cfg)

tests/fixme/associated-items/methods.stderr

@@ -0,0 +1,2 @@
+Test sanity_checks::test_traits: Failed:
+    - Failed to find trait definition: `<redacted>`

tools/compiletest/src/args.rs

@@ -48,6 +48,10 @@ pub struct Args {
     /// Run test-driver on verbose mode to print test outputs.
     #[arg(long)]
     pub no_capture: bool,
+
+    /// Override the output files when there's a difference instead of failing the test.
+    #[arg(long)]
+    pub bless: bool,
 }
 
 impl From<Mode> for ui_test::Mode {
@@ -76,8 +80,15 @@ impl From<Args> for Config {
             driver_config(&args)
         };
         config.filter(r"\[T-DRIVE\].*\n", "");
+        // Remove stable mir details, since they can include internal variables which are fairly
+        // unstable.
+        config.filter(r"(?<a>[^`]*)`[^`]+`(?<b>[^`]*)", "$a`<redacted>`$b");
         config.mode = ui_test::Mode::from(args.mode);
-        config.output_conflict_handling = OutputConflictHandling::Error("Should Fail".to_string());
+        config.output_conflict_handling = if args.bless {
+            OutputConflictHandling::Bless
+        } else {
+            OutputConflictHandling::Error("Should Fail".to_string())
+        };
         config.out_dir = args.output_dir;
         //config.run_lib_path = PathBuf::from(env!("RUSTC_LIB_PATH"));
         config

tools/test-drive/src/main.rs

@@ -62,7 +62,7 @@ fn main() -> ExitCode {
 }
 
 macro_rules! run_tests {
-    ($( $test:path ),+) => {
+    ($( $test:path ),+ $(,)?) => {
         [$({
             run_test(stringify!($test), || { $test() })
         },)+]
@@ -82,7 +82,8 @@ fn test_stable_mir(_tcx: TyCtxt<'_>) -> ControlFlow<()> {
     let mut results = Vec::from(run_tests![
         sanity_checks::test_entry_fn,
         sanity_checks::test_all_fns,
-        sanity_checks::test_crates
+        sanity_checks::test_crates,
+        sanity_checks::test_instances,
     ]);
     if FIXME_CHECKS.load(Ordering::Relaxed) {
         results.extend_from_slice(&run_tests!(sanity_checks::test_traits))
@@ -103,13 +104,13 @@ fn test_stable_mir(_tcx: TyCtxt<'_>) -> ControlFlow<()> {
 
 fn run_test<F: FnOnce() -> TestResult>(name: &str, f: F) -> TestResult {
     let result = match catch_unwind(AssertUnwindSafe(f)) {
-        Err(_) => Err("Panic: {}".to_string()),
+        Err(_) => Err("Panic!".to_string()),
         Ok(result) => result,
     };
-    info(format!(
-        "Test {}: {}",
-        name,
-        result.as_ref().err().unwrap_or(&"Ok".to_string())
-    ));
+    if let Err(ref msg) = result {
+        eprintln!("Test {}: Failed:\n    - {}", name, msg);
+    } else {
+        info(format!("Test {}: Ok", name,));
+    }
     result
 }

tools/test-drive/src/sanity_checks.rs

@@ -4,17 +4,18 @@
 //! These checks should only depend on StableMIR APIs. See other modules for tests that compare
 //! the result between StableMIR and internal APIs.
 use crate::TestResult;
-use stable_mir;
+use stable_mir::{self, mir, mir::MirVisitor, ty};
+use std::collections::HashSet;
 use std::fmt::Debug;
-use std::hint::black_box;
+use std::iter::zip;
 
 fn check_equal<T>(val: T, expected: T, msg: &str) -> TestResult
 where
     T: Debug + PartialEq,
 {
     if val != expected {
         Err(format!(
-            "{}: \n Expected: {:?}\n Found: {:?}",
+            "{}: \n Expected: `{:?}`\n Found: `{:?}`",
             msg, expected, val
         ))
     } else {
@@ -34,11 +35,16 @@ pub fn check(val: bool, msg: String) -> TestResult {
 pub fn test_entry_fn() -> TestResult {
     let entry_fn = stable_mir::entry_fn();
     entry_fn.map_or(Ok(()), |entry_fn| {
-        check_body(entry_fn.body());
+        check_body(&entry_fn.name(), &entry_fn.body())?;
         let all_items = stable_mir::all_local_items();
         check(
             all_items.contains(&entry_fn),
-            format!("Failed to find entry_fn `{:?}`", entry_fn),
+            format!("Failed to find entry_fn: `{:?}`", entry_fn),
+        )?;
+        check_equal(
+            entry_fn.kind(),
+            stable_mir::ItemKind::Fn,
+            "Entry must be a function",
         )
     })
 }
@@ -49,7 +55,7 @@ pub fn test_all_fns() -> TestResult {
     for item in all_items {
         // Get body and iterate over items
         let body = item.body();
-        check_body(body);
+        check_body(&item.name(), &body)?;
     }
     Ok(())
 }
@@ -75,7 +81,7 @@ pub fn test_traits() -> TestResult {
     {
         check(
             all_traits.contains(&trait_impl.value.def_id),
-            format!("Failed to find trait definition {trait_impl:?}"),
+            format!("Failed to find trait definition: `{trait_impl:?}`"),
         )?;
     }
     Ok(())
@@ -85,30 +91,127 @@ pub fn test_crates() -> TestResult {
     for krate in stable_mir::external_crates() {
         check(
             stable_mir::find_crates(&krate.name.as_str()).contains(&krate),
-            format!("Cannot find {krate:?}"),
+            format!("Cannot find `{krate:?}`"),
         )?;
     }
 
     let local = stable_mir::local_crate();
     check(
         stable_mir::find_crates(&local.name.as_str()).contains(&local),
-        format!("Cannot find {local:?}"),
+        format!("Cannot find local: `{local:?}`"),
     )
 }
 
+pub fn test_instances() -> TestResult {
+    let all_items = stable_mir::all_local_items();
+    let mut queue = all_items
+        .iter()
+        .filter_map(|item| {
+            (item.kind() == stable_mir::ItemKind::Fn)
+                .then(|| mir::mono::Instance::try_from(*item).ok())
+                .flatten()
+        })
+        .collect::<Vec<_>>();
+
+    let mut visited = HashSet::<mir::mono::Instance>::default();
+    while let Some(next_item) = queue.pop() {
+        if visited.insert(next_item.clone()) {
+            let Some(body) = next_item.body() else {
+                continue;
+            };
+            let visitor = check_body(&next_item.mangled_name(), &body)?;
+            for term in visitor.terminators {
+                match &term.kind {
+                    // We currently don't support Copy / Move `ty()` due to missing Place::ty().
+                    // https://github.com/rust-lang/project-stable-mir/issues/49
+                    mir::TerminatorKind::Call {
+                        func: mir::Operand::Constant(constant),
+                        ..
+                    } => {
+                        match constant.ty().kind().rigid() {
+                            Some(ty::RigidTy::FnDef(def, args)) => {
+                                queue.push(mir::mono::Instance::resolve(*def, &args).unwrap());
+                            }
+                            Some(ty::RigidTy::FnPtr(..)) => { /* ignore FnPtr for now */ }
+                            ty => check(false, format!("Unexpected call: `{ty:?}`"))?,
+                        }
+                    }
+                    _ => { /* Do nothing */ }
+                }
+            }
+        }
+    }
+    Ok(())
+}
+
 /// Visit all local types, statements and terminator to ensure nothing crashes.
-fn check_body(body: stable_mir::mir::Body) {
-    for bb in &body.blocks {
-        for stable_mir::mir::Statement { kind, .. } in &bb.statements {
-            black_box(matches!(kind, stable_mir::mir::StatementKind::Assign(..)));
+fn check_body(name: &str, body: &mir::Body) -> Result<BodyVisitor, String> {
+    let mut visitor = BodyVisitor::default();
+    visitor.visit_body(body);
+
+    check_equal(
+        body.blocks.len(),
+        visitor.statements.len(),
+        &format!("Function `{name}`: Unexpected visited BB statements"),
+    )?;
+    check_equal(
+        body.blocks.len(),
+        visitor.terminators.len(),
+        &format!("Function `{name}`: Visited terminals"),
+    )?;
+    for (idx, bb) in body.blocks.iter().enumerate() {
+        for (stmt, visited_stmt) in zip(&bb.statements, &visitor.statements[idx]) {
+            check_equal(
+                stmt,
+                visited_stmt,
+                &format!("Function `{name}`: Visited statement"),
+            )?;
         }
-        black_box(matches!(
-            bb.terminator.kind,
-            stable_mir::mir::TerminatorKind::Goto { .. }
-        ));
+        check_equal(
+            &bb.terminator,
+            &visitor.terminators[idx],
+            &format!("Function `{name}`: Terminator"),
+        )?;
     }
 
     for local in body.locals() {
-        black_box(matches!(local.ty.kind(), stable_mir::ty::TyKind::Alias(..)));
+        if !visitor.types.contains(&local.ty) {
+            // Format fails due to unsupported CoroutineWitness.
+            // See https://github.com/rust-lang/project-stable-mir/issues/50.
+            check(
+                false,
+                format!("Function `{name}`: Missing type `{:?}`", local.ty),
+            )?;
+        };
+    }
+    Ok(visitor)
+}
+
+#[derive(Debug, Default)]
+struct BodyVisitor {
+    statements: Vec<Vec<mir::Statement>>,
+    terminators: Vec<mir::Terminator>,
+    types: HashSet<ty::Ty>,
+}
+
+impl mir::MirVisitor for BodyVisitor {
+    fn visit_basic_block(&mut self, bb: &mir::BasicBlock) {
+        assert_eq!(self.statements.len(), self.terminators.len());
+        self.statements.push(vec![]);
+        self.super_basic_block(bb)
+    }
+    fn visit_statement(&mut self, stmt: &mir::Statement, loc: mir::visit::Location) {
+        self.statements.last_mut().unwrap().push(stmt.clone());
+        self.super_statement(stmt, loc)
+    }
+
+    fn visit_terminator(&mut self, term: &mir::Terminator, location: mir::visit::Location) {
+        self.terminators.push(term.clone());
+        self.super_terminator(term, location);
+    }
+
+    fn visit_ty(&mut self, ty: &ty::Ty, _location: mir::visit::Location) {
+        self.types.insert(*ty);
+        self.super_ty(ty)
     }
 }