diff --git a/Cargo.toml b/Cargo.toml index fa7ec2ae6..a5ff3c46e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,10 +32,14 @@ cc = "1.0.83" cxxbridge-flags = { version = "=1.0.160", path = "flags", default-features = false } [dev-dependencies] +cc = "1.0.83" cxx-build = { version = "=1.0.160", path = "gen/build" } cxx-gen = { version = "0.7", path = "gen/lib" } cxx-test-suite = { version = "0", path = "tests/ffi" } +proc-macro2 = "1.0.95" +quote = "1.0.40" rustversion = "1.0.13" +tempdir = "0.3.7" trybuild = { version = "1.0.81", features = ["diff"] } # Disallow incompatible cxxbridge-cmd version appearing in the same lockfile. diff --git a/build.rs b/build.rs index 2fbb018ab..34501f901 100644 --- a/build.rs +++ b/build.rs @@ -55,6 +55,8 @@ fn main() { println!("cargo:rustc-cfg=error_in_core"); } } + + persist_target_triple(); } struct RustVersion { @@ -73,3 +75,32 @@ fn rustc_version() -> Option { let minor = pieces.next()?.parse().ok()?; Some(RustVersion { version, minor }) } + +/// `tests/cpp_ui_tests.rs` needs to know the target triple when invoking a +/// C/C++ compiler through the `cc` crate. The function below facilitates this +/// by capturing the value of the `TARGET` environment variable seen during +/// `build.rs` execution, and writing this value to a file that the +/// `cpp_ui_tests` can pick up using `include_str!`. +/// +/// An alternative approach would be to drive `cpp_ui_tests` from `build.rs` +/// during build time. This seems less desirable than the current approach, +/// which benefits from being a set of regular test cases (which can be +/// filtered, have their stderr captured, etc.). FWIW the `tests/ui` tests also +/// invoke build tools (e.g. `rustc`) at test time, rather than build time, so +/// this seems okay. +/// +/// This function ignores errors, because we don't want to avoid disrupting +/// production builds (even if failure to generate `target_triple.txt` may +/// disrupt test builds). +fn persist_target_triple() { + let Some(out_dir) = env::var_os("OUT_DIR") else { + return; + }; + let Ok(target) = env::var("TARGET") else { + return; + }; + println!("cargo:rerun-if-env-changed=TARGET"); + + let out_dir = Path::new(&out_dir); + let _ = std::fs::write(out_dir.join("target_triple.txt"), target); +} diff --git a/gen/src/write.rs b/gen/src/write.rs index 96dfa1bfe..0b37c67e9 100644 --- a/gen/src/write.rs +++ b/gen/src/write.rs @@ -1707,25 +1707,12 @@ fn write_unique_ptr_common(out: &mut OutFile, ty: UniquePtr) { UniquePtr::CxxVector(_) => false, }; - let conditional_delete = match ty { - UniquePtr::Ident(ident) => { - !out.types.structs.contains_key(ident) && !out.types.enums.contains_key(ident) - } - UniquePtr::CxxVector(_) => false, - }; - - if conditional_delete { - out.builtin.is_complete = true; - let definition = match ty { - UniquePtr::Ident(ty) => &out.types.resolve(ty).name.cxx, - UniquePtr::CxxVector(_) => unreachable!(), - }; - writeln!( - out, - "static_assert(::rust::detail::is_complete<{}>::value, \"definition of {} is required\");", - inner, definition, - ); - } + out.builtin.is_complete = true; + writeln!( + out, + "static_assert(::rust::detail::is_complete<{}>::value, \"definition of `{}` is required\");", + inner, inner, + ); writeln!( out, "static_assert(sizeof(::std::unique_ptr<{}>) == sizeof(void *), \"\");", @@ -1797,16 +1784,12 @@ fn write_unique_ptr_common(out: &mut OutFile, ty: UniquePtr) { "void cxxbridge1$unique_ptr${}$drop(::std::unique_ptr<{}> *ptr) noexcept {{", instance, inner, ); - if conditional_delete { - out.builtin.deleter_if = true; - writeln!( - out, - " ::rust::deleter_if<::rust::detail::is_complete<{}>::value>{{}}(ptr);", - inner, - ); - } else { - writeln!(out, " ptr->~unique_ptr();"); - } + out.builtin.deleter_if = true; + writeln!( + out, + " ::rust::deleter_if<::rust::detail::is_complete<{}>::value>{{}}(ptr);", + inner, + ); writeln!(out, "}}"); } diff --git a/testing.md b/testing.md new file mode 100644 index 000000000..97cb84658 --- /dev/null +++ b/testing.md @@ -0,0 +1,48 @@ +# Testing + +This document tries to provide an outline of different kinds of tests +used by the `cxx` project. + +## Errors from proc macro + +In some situations, we want to verify that the `#[cxx::bridge]` macro reports +expected error messages when invoked by `rustc`. + +Such verification is handled +by test cases underneath `tests/ui` directory and driven by +`tests/compiletest.rs`. The test cases consist of a pair of files: + +* `foo.rs` is the input +* `foo.stderr` is the expected output + +## Errors from C++ compiler + +In some situations, we want to verify that +the C++ code +generated by a successful invocation of the `cxxbridge-cmd` command +results in expected error messages +when compiled by a C++ compiler. +(Errors from unsuccessful invocations of the `cxxbridge-cmd` command +should have test coverage provided by the `tests/ui` test suite.) + +Such verification is covered by `tests/cpp_ui_tests.rs`. + +## End-to-end functionality + +End-to-end functional tests are structured as follows: + +* The code under test is contained underneath `tests/ffi` directory which + contains: + - Rust code under test - the `cxx-test-suite` crate + (`lib.rs` and `module.rs`) with: + - A few `#[cxx::bridge]` declarations + - Rust types under test (e.g. `struct R`) + - Rust functions and methods under test (e.g. `r_return_primitive`) + - C/C++ code under test (`tests.h` and `tests.cc`) + - C++ types under test (e.g. `class C`) + - C++ functions and methods under test (e.g. `c_return_primitive`) +* The testcases can be found in: + - Rust calling into C++: `tests/test.rs` + - C++ calling into Rust: `tests/ffi/test.cc`. + The tests are transitively, manually invoked from the + `cxx_run_test` function in `tests/ffi/test.cc` diff --git a/tests/cpp_ui_tests.rs b/tests/cpp_ui_tests.rs new file mode 100644 index 000000000..893530a66 --- /dev/null +++ b/tests/cpp_ui_tests.rs @@ -0,0 +1,23 @@ +mod cpp_ui_tests_harness; +use cpp_ui_tests_harness::Test; + +use quote::quote; + +/// This is a regression test for `static_assert(::rust::is_complete...)` +/// which we started to emit in +#[test] +fn test_unique_ptr_of_incomplete_foward_declared_pointee() { + let test = Test::new(quote! { + #[cxx::bridge] + mod ffi { + unsafe extern "C++" { + include!("include.h"); + type ForwardDeclaredType; + } + impl UniquePtr {} + } + }); + test.write_file("include.h", "class ForwardDeclaredType;"); + let err_msg = test.compile().expect_single_error(); + assert!(err_msg.contains("definition of `::ForwardDeclaredType` is required")); +} diff --git a/tests/cpp_ui_tests_harness.rs b/tests/cpp_ui_tests_harness.rs new file mode 100644 index 000000000..5296d328e --- /dev/null +++ b/tests/cpp_ui_tests_harness.rs @@ -0,0 +1,254 @@ +//! This test harness helps to verify that +//! the C++ code +//! generated by a successful invocations of `cxx_gen` APIs +//! results in expected error messages +//! when compiled by a C++ compiler. + +use proc_macro2::TokenStream; +use std::borrow::Cow; +use std::path::{Path, PathBuf}; + +/// Helper for setting up a test that: +/// +/// 1. Takes a `#[cxx::bridge]` and generates `.cc` and `.h` files, +/// 2. Optionally sets up other files (e.g. supplementary header files), +/// 3. Tests compiling the generated `.cc` file. +pub struct Test { + temp_dir: tempdir::TempDir, + + /// Path to the `.cc` file (in `temp_dir`) that is generated by the + /// `cxx_gen` crate out of the `cxx_bridge` argument passed to `Test::new`. + generated_cc: PathBuf, +} + +impl Test { + /// Creates a new test for the given `cxx_bridge`. + /// + /// Example: + /// + /// ```rs + /// let test = Test::new(quote!{ + /// #[cxx::bridge] + /// mod ffi { + /// unsafe extern "C++" { + /// include!("include.h"); + /// pub fn do_cpp_thing(); + /// } + /// } + /// }); + /// ``` + /// + /// # Panics + /// + /// Panics if there is a failure when generating `.cc` and `.h` files from the `cxx_bridge`. + #[must_use] + pub fn new(cxx_bridge: TokenStream) -> Self { + let temp_dir = tempdir::TempDir::new("cxx--cpp_ui_tests").unwrap(); + let generated_h = temp_dir.path().join("cxx_bridge.generated.h"); + let generated_cc = temp_dir.path().join("cxx_bridge.generated.cc"); + + { + let opt = cxx_gen::Opt::default(); + let generated = cxx_gen::generate_header_and_cc(cxx_bridge, &opt).unwrap(); + std::fs::write(&generated_h, &generated.header).unwrap(); + std::fs::write(&generated_cc, &generated.implementation).unwrap(); + } + + Self { + temp_dir, + generated_cc, + } + } + + /// Writes a file to the temporary test directory. + /// The new file will be present in the `-I` include path passed to the compiler. + /// + /// # Panics + /// + /// Panics if there is an error when writing the file. + pub fn write_file(&self, filename: impl AsRef, contents: &str) { + std::fs::write(self.temp_dir.path().join(filename), contents).unwrap(); + } + + /// Compiles the `.cc` file generated `Self::new`. + /// + /// # Panics + /// + /// Panics if there is a problem with spawning the C++ compiler. + /// (Compilation errors will *not* result in a panic.) + #[must_use] + pub fn compile(&self) -> CompilationResult { + let mut build = cc::Build::new(); + build + .include(self.temp_dir.path()) + .out_dir(self.temp_dir.path()) + .cpp(true); + + // Arbitrarily using `c++20` for now. If some test cases require a specific C++ version, + // then in the future we can make this configurable with a new field of `Test`. + build.std("c++20"); + + // Set info required by the `cc` crate. + // + // We assume that tests are run on the host. This assumption is a bit icky, but works in + // practice (and FWIW `tests/compiletest.rs` can be seen as a precedent). + let target = include_str!(concat!(env!("OUT_DIR"), "/target_triple.txt")); + build.opt_level(3).host(target).target(target); + + // It seems that the `cc` crate doesn't currently provide an API for getting + // a `Command` for building a single C++ source file. We can work around that + // by adding `-c ` ourselves - it seems to work for all the compilers + // where these tests run... + let mut command = build.get_compiler().to_command(); + command + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .current_dir(self.temp_dir.path()) + .arg("-c") + .arg(&self.generated_cc); + let output = command.spawn().unwrap().wait_with_output().unwrap(); + CompilationResult(output) + } +} + +/// Wrapper around the output from a C++ compiler. +pub struct CompilationResult(std::process::Output); + +impl CompilationResult { + fn stdout(&self) -> Cow<'_, str> { + String::from_utf8_lossy(&self.0.stdout) + } + + fn stderr(&self) -> Cow<'_, str> { + String::from_utf8_lossy(&self.0.stderr) + } + + fn dump_output_and_panic(&self, msg: &str) { + eprintln!("{}", self.stdout()); + eprintln!("{}", self.stderr()); + panic!("{msg}"); + } + + fn error_lines(&self) -> Vec { + assert!(!self.0.status.success()); + + // It seems that MSVC reports errors to stdout rather than stderr, so + // let's just analyze all the lines - this should work for all compilers + // exercised by the CI. + let stdout = self.stdout(); + let stderr = self.stderr(); + let all_lines = stdout.lines().chain(stderr.lines()); + + all_lines + .filter(|line| { + // This should match MSVC error output + // (e.g. `file.cc(): error C2338: static_assert failed: ...`) + // as well as Clang or GCC error output + // (e.g. `file.cc::: error: static assertion failed: ...` + line.contains(": error") + }) + .map(ToString::to_string) + .collect::>() + } + + /// Asserts that the C++ compilation succeeded. + /// + /// # Panics + /// + /// Panics if the C++ compiler reported an error. + pub fn assert_success(&self) { + if !self.0.status.success() { + self.dump_output_and_panic("Compiler reported an error"); + } + } + + /// Verifies that the compilation failed with a single error, and return the + /// stderr line describing this error. + /// + /// Note that different compilers may return slightly different error + /// messages, so tests should be careful to only verify presence of some + /// substrings. + /// + /// # Panics + /// + /// Panics if there was no error, or if there was more than a single error. + #[must_use] + pub fn expect_single_error(&self) -> String { + let error_lines = self.error_lines(); + if error_lines.is_empty() { + self.dump_output_and_panic("No error lines found, despite non-zero exit code?"); + } + if error_lines.len() > 1 { + self.dump_output_and_panic("Unexpectedly more than 1 error line was present"); + } + + // `eprintln` to help with debugging test failues that may happen later. + let single_error_line = error_lines.into_iter().next().unwrap(); + eprintln!("Got single error as expected: {single_error_line}"); + single_error_line + } +} + +#[cfg(test)] +mod test { + use super::Test; + use quote::quote; + + #[test] + fn test_success_smoke_test() { + let test = Test::new(quote! { + #[cxx::bridge] + mod ffi { + unsafe extern "C++" { + include!("include.h"); + pub fn do_cpp_thing(); + } + } + }); + test.write_file("include.h", "void do_cpp_thing();"); + test.compile().assert_success(); + } + + #[test] + fn test_failure_smoke_test() { + let test = Test::new(quote! { + #[cxx::bridge] + mod ffi { + unsafe extern "C++" { + include!("include.h"); + } + } + }); + test.write_file( + "include.h", + r#" + static_assert(false, "This is a failure smoke test"); + "#, + ); + let err_msg = test.compile().expect_single_error(); + assert!(err_msg.contains("This is a failure smoke test")); + } + + #[test] + #[should_panic = "Unexpectedly more than 1 error line was present"] + fn test_failure_with_unexpected_extra_error_line() { + let test = Test::new(quote! { + #[cxx::bridge] + mod ffi { + unsafe extern "C++" { + include!("include.h"); + } + } + }); + test.write_file( + "include.h", + r#" + static_assert(false, "First error line"); + static_assert(false, "Second error line"); + "#, + ); + + // We `should_panic` inside `expect_single_error` below: + let _ = test.compile().expect_single_error(); + } +}