Merge pull request #4430 from Owen-CH-Leung/add_try_generate_signature

ctest: Revise generate function to return Result

Author: tgross35

ctest-test/Cargo.toml

@@ -9,6 +9,9 @@ edition = "2021"
 ctest = { path = "../ctest" }
 cc = "1.0"
 
+[dev-dependencies]
+ctest = { path = "../ctest" }
+
 [dependencies]
 cfg-if = "1.0.0"
 libc = { path = ".." }

ctest-test/tests/all.rs

@@ -128,3 +128,75 @@ fn t2_cxx() {
         panic!();
     }
 }
+
+#[test]
+fn test_missing_out_dir() {
+    // Save original OUT_DIR
+    let orig_out_dir = env::var_os("OUT_DIR");
+    env::remove_var("OUT_DIR");
+
+    // Test error handling for OUT_DIR missing
+    let result = ctest::TestGenerator::new()
+        .header("t1.h")
+        .try_generate("src/t1.rs", "out_dir_gen.rs");
+
+    // Restore OUT_DIR
+    if let Some(dir) = orig_out_dir {
+        env::set_var("OUT_DIR", dir);
+    }
+
+    assert!(result.is_err(), "Expected error when OUT_DIR is missing");
+}
+
+#[test]
+fn test_invalid_output_path() {
+    // Test error handling for invalid output path
+    let err = ctest::TestGenerator::new()
+        .header("t1.h")
+        .include("src")
+        .out_dir("/nonexistent_dir") // Should fail with permission error
+        .try_generate("src/t1.rs", "out_path_gen.rs");
+
+    assert!(err.is_err(), "Expected error with invalid output path");
+}
+
+#[test]
+fn test_parsing_error() {
+    // Test parsing error
+    // Create a temporary file with invalid Rust syntax
+    let temp_dir = env::temp_dir();
+    let invalid_file = temp_dir.join("invalid.rs");
+    std::fs::write(&invalid_file, "fn invalid_syntax {").unwrap();
+
+    let err = ctest::TestGenerator::new()
+        .header("t1.h")
+        .include("src")
+        .target("x86_64-unknown-linux-gnu")
+        .try_generate(&invalid_file, "parse_gen.rs");
+
+    assert!(err.is_err(), "Expected error when parsing invalid syntax");
+    let _ = std::fs::remove_file(invalid_file);
+}
+
+#[test]
+fn test_non_existent_header() {
+    // Test non-existent header
+    let err = ctest::TestGenerator::new()
+        .header("nonexistent_header.h")
+        .include("src")
+        .try_generate("src/t1.rs", "missing_header_gen.rs");
+
+    assert!(err.is_err(), "Expected error with non-existent header");
+}
+
+#[test]
+fn test_invalid_include_path() {
+    // Test invalid include path
+    let err = ctest::TestGenerator::new()
+        .header("t1.h")
+        .include("nonexistent_directory")
+        .try_generate("src/t1.rs", "invalid_include_gen.rs");
+
+    assert!(err.is_err(), "Expected error with invalid include path");
+}
+

ctest/Cargo.toml

@@ -9,6 +9,7 @@ repository = "https://github.com/rust-lang/libc"
 rust-version = "1.63.0"
 
 [dependencies]
+anyhow = "1.0"
 garando_syntax = "0.1"
 cc = "1.0.1"
 rustc_version = "0.4"

ctest/src/lib.rs

@@ -13,16 +13,16 @@
 #![recursion_limit = "256"]
 #![deny(missing_docs)]
 
+use anyhow::{anyhow, Context, Result};
+use garando_syntax as syntax;
+use indoc::writedoc;
 use std::collections::{HashMap, HashSet};
 use std::env;
 use std::fs::File;
 use std::io::prelude::*;
 use std::io::BufWriter;
 use std::path::{Path, PathBuf};
 use std::rc::Rc;
-
-use garando_syntax as syntax;
-use indoc::writedoc;
 use syntax::abi::Abi;
 use syntax::ast;
 use syntax::ast::{Attribute, Name};
@@ -41,9 +41,6 @@ use syntax::ptr::P;
 use syntax::util::small_vector::SmallVector;
 use syntax::visit::{self, Visitor};
 
-type Error = Box<dyn std::error::Error>;
-type Result<T, E = Error> = std::result::Result<T, E>;
-
 /// Programming language
 #[derive(Debug)]
 pub enum Lang {
@@ -773,6 +770,17 @@ impl TestGenerator {
         self
     }
 
+    /// Generate all tests and panic on any errors.
+    ///
+    /// This function is a convenience wrapper around `try_generate` that panics instead of returning
+    /// errors.
+    ///
+    /// See `try_generate` for the error-handling version of this function.
+    pub fn generate<P: AsRef<Path>>(&mut self, krate: P, out_file: &str) {
+        self.try_generate(krate, out_file)
+            .unwrap_or_else(|e| panic!("Failed to generate tests: {e}"));
+    }
+
     /// Generate all tests.
     ///
     /// This function is first given the path to the `*-sys` crate which is
@@ -791,16 +799,16 @@ impl TestGenerator {
     /// use ctest::TestGenerator;
     ///
     /// let mut cfg = TestGenerator::new();
-    /// cfg.generate("../path/to/libfoo-sys/lib.rs", "all.rs");
+    /// cfg.try_generate("../path/to/libfoo-sys/lib.rs", "all.rs");
     /// ```
-    pub fn generate<P: AsRef<Path>>(&mut self, krate: P, out_file: &str) {
+    pub fn try_generate<P: AsRef<Path>>(&mut self, krate: P, out_file: &str) -> Result<PathBuf> {
         let krate = krate.as_ref();
-        let out = self.generate_files(krate, out_file);
+        let out = self.generate_files(krate, out_file)?;
 
-        let target = self
-            .target
-            .clone()
-            .unwrap_or_else(|| env::var("TARGET").unwrap());
+        let target = match self.target.clone() {
+            Some(t) => t,
+            None => env::var("TARGET").context("TARGET environment variable not found")?,
+        };
 
         // Compile our C shim to be linked into tests
         let mut cfg = cc::Build::new();
@@ -815,19 +823,19 @@ impl TestGenerator {
         if target.contains("msvc") {
             cfg.flag("/W3").flag("/Wall").flag("/WX")
                 // ignored warnings
-               .flag("/wd4820")  // warning about adding padding?
-               .flag("/wd4100")  // unused parameters
-               .flag("/wd4996")  // deprecated functions
-               .flag("/wd4296")  // '<' being always false
-               .flag("/wd4255")  // converting () to (void)
-               .flag("/wd4668")  // using an undefined thing in preprocessor?
-               .flag("/wd4366")  // taking ref to packed struct field might be unaligned
-               .flag("/wd4189")  // local variable initialized but not referenced
-               .flag("/wd4710")  // function not inlined
-               .flag("/wd5045")  // compiler will insert Spectre mitigation
-               .flag("/wd4514")  // unreferenced inline function removed
-               .flag("/wd4711")  // function selected for automatic inline
-                ;
+                .flag("/wd4820")  // warning about adding padding?
+                .flag("/wd4100")  // unused parameters
+                .flag("/wd4996")  // deprecated functions
+                .flag("/wd4296")  // '<' being always false
+                .flag("/wd4255")  // converting () to (void)
+                .flag("/wd4668")  // using an undefined thing in preprocessor?
+                .flag("/wd4366")  // taking ref to packed struct field might be unaligned
+                .flag("/wd4189")  // local variable initialized but not referenced
+                .flag("/wd4710")  // function not inlined
+                .flag("/wd5045")  // compiler will insert Spectre mitigation
+                .flag("/wd4514")  // unreferenced inline function removed
+                .flag("/wd4711")  // function selected for automatic inline
+            ;
         } else {
             cfg.flag("-Wall")
                 .flag("-Wextra")
@@ -851,25 +859,40 @@ impl TestGenerator {
             cfg.include(p);
         }
 
-        let stem = out.file_stem().unwrap().to_str().unwrap();
-        cfg.target(&target)
-            .out_dir(out.parent().unwrap())
-            .compile(&format!("lib{stem}.a"));
+        let stem = out
+            .file_stem()
+            .context("Failed to get file stem")?
+            .to_str()
+            .context("Failed to convert to str")?;
+
+        let parent = out
+            .parent()
+            .context("Output file has no parent directory")?;
+
+        cfg.target(&target).out_dir(parent);
+
+        let name = format!("lib{stem}.a");
+
+        cfg.try_compile(&name)
+            .context(format!("failed to compile `{}`", name))
+            .map(|_| out)
     }
 
     #[doc(hidden)] // TODO: needs docs
-    pub fn generate_files<P: AsRef<Path>>(&mut self, krate: P, out_file: &str) -> PathBuf {
+    pub fn generate_files<P: AsRef<Path>>(&mut self, krate: P, out_file: &str) -> Result<PathBuf> {
         self.generate_files_impl(krate, out_file)
-            .expect("generation failed")
     }
 
     fn generate_files_impl<P: AsRef<Path>>(&mut self, krate: P, out_file: &str) -> Result<PathBuf> {
         let krate = krate.as_ref();
+
         // Prep the test generator
         let out_dir = self
             .out_dir
             .clone()
-            .unwrap_or_else(|| PathBuf::from(env::var_os("OUT_DIR").unwrap()));
+            .or_else(|| env::var_os("OUT_DIR").map(PathBuf::from))
+            .context("Neither out_dir nor OUT_DIR environment variable is set")?;
+
         let out_file = out_dir.join(out_file);
         let ext = match self.lang {
             Lang::C => "c",
@@ -878,18 +901,26 @@ impl TestGenerator {
         let c_file = out_file.with_extension(ext);
         let rust_out = BufWriter::new(File::create(&out_file)?);
         let c_out = BufWriter::new(File::create(&c_file)?);
-        let mut sess = ParseSess::new(FilePathMapping::empty());
+
         let target = self
             .target
             .clone()
-            .unwrap_or_else(|| env::var("TARGET").unwrap());
+            .or_else(|| env::var("TARGET").ok())
+            .filter(|t| !t.is_empty())
+            .context("TARGET environment variable not set or empty")?;
+
+        let mut sess = ParseSess::new(FilePathMapping::empty());
         for (k, v) in default_cfg(&target).into_iter().chain(self.cfg.clone()) {
             let s = |s: &str| Name::intern(s);
             sess.config.insert((s(&k), v.as_ref().map(|n| s(n))));
         }
 
-        // Parse the libc crate
-        let krate = parse::parse_crate_from_file(krate, &sess).ok().unwrap();
+        // Convert DiagnosticBuilder -> Error so the `?` works
+        let krate = parse::parse_crate_from_file(krate, &sess).map_err(|mut d| {
+            // Emit the diagnostic to properly handle it and show error to the user
+            d.emit();
+            anyhow!("failed to parse crate: {:?}", d)
+        })?;
 
         // Remove things like functions, impls, traits, etc, that we're not
         // looking at