From 53a048dcdf2db5e10c66bbe99c92d781cd9dd7e4 Mon Sep 17 00:00:00 2001 From: Target-san Date: Sat, 5 Oct 2024 15:57:37 +0300 Subject: [PATCH 01/27] Add 'handle-panics' feature --- proptest/Cargo.toml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/proptest/Cargo.toml b/proptest/Cargo.toml index 467abde6..3745377d 100644 --- a/proptest/Cargo.toml +++ b/proptest/Cargo.toml @@ -59,6 +59,10 @@ atomic64bit = [] bit-set = [ "dep:bit-set", "dep:bit-vec" ] +# Enables proper handling of panics +# In particular, hides all intermediate panics flowing into stderr during shrink phase +handle-panics = ["std"] + [dependencies] bitflags = "2" unarray = "0.1.4" From 639f68633b64ab1ac3cc2b71a66ecd464e3b9ebd Mon Sep 17 00:00:00 2001 From: Target-san Date: Sat, 5 Oct 2024 15:58:07 +0300 Subject: [PATCH 02/27] Add conditional scoped panic hook implementation --- proptest/src/test_runner/mod.rs | 1 + proptest/src/test_runner/scoped_panic_hook.rs | 123 ++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 proptest/src/test_runner/scoped_panic_hook.rs diff --git a/proptest/src/test_runner/mod.rs b/proptest/src/test_runner/mod.rs index d7516ab9..836d26d5 100644 --- a/proptest/src/test_runner/mod.rs +++ b/proptest/src/test_runner/mod.rs @@ -21,6 +21,7 @@ mod replay; mod result_cache; mod rng; mod runner; +mod scoped_panic_hook; pub use self::config::*; pub use self::errors::*; diff --git a/proptest/src/test_runner/scoped_panic_hook.rs b/proptest/src/test_runner/scoped_panic_hook.rs new file mode 100644 index 00000000..95793470 --- /dev/null +++ b/proptest/src/test_runner/scoped_panic_hook.rs @@ -0,0 +1,123 @@ +#[cfg(feature = "handle-panics")] +mod panicky { + //! Implementation of scoped panic hooks + //! + //! 1. `with_hook` serves as entry point, it executes body closure with panic hook closure + //! installed as scoped panic hook + //! 2. Upon first execution, current panic hook is replaced with `scoped_hook_dispatcher` + //! in a thread-safe manner, and original hook is stored for later use + //! 3. When panic occurs, `scoped_hook_dispatcher` either delegates execution to scoped + //! panic hook, if one is installed, or back to original hook stored earlier. + //! This preserves original behavior when scoped hook isn't used + //! 4. When `with_hook` is used, it replaces stored scoped hook pointer with pointer to + //! hook closure passed as parameter. Old hook pointer is set to be restored unconditionally + //! via drop guard. Then, normal body closure is executed. + use std::boxed::Box; + use std::cell::Cell; + use std::panic::{set_hook, take_hook, PanicInfo}; + use std::sync::Once; + use std::{mem, ptr}; + + thread_local! { + /// Pointer to currently installed scoped panic hook, if any + /// + /// NB: pointers to arbitrary fn's are fat, and Rust doesn't allow crafting null pointers + /// to fat objects. So we just store const pointer to tuple with whatever data we need + static SCOPED_HOOK_PTR: Cell<*const (*mut dyn FnMut(&PanicInfo<'_>),)> = Cell::new(ptr::null()); + } + + static INIT_ONCE: Once = Once::new(); + /// Default panic hook, the one which was present before installing scoped one + /// + /// NB: no need for external sync, value is mutated only once, when init is performed + static mut DEFAULT_HOOK: Option) + Send + Sync>> = + None; + /// Replaces currently installed panic hook with `scoped_hook_dispatcher` once, + /// in a thread-safe manner + fn init() { + INIT_ONCE.call_once(|| { + let old_handler = take_hook(); + set_hook(Box::new(scoped_hook_dispatcher)); + unsafe { + DEFAULT_HOOK = Some(old_handler); + } + }); + } + /// Panic hook which delegates execution to scoped hook, + /// if one installed, or to default hook + fn scoped_hook_dispatcher(info: &PanicInfo<'_>) { + let handler = SCOPED_HOOK_PTR.get(); + if !handler.is_null() { + // It's assumed that if container's ptr is not null, ptr to `FnMut` is non-null too. + // Correctness **must** be ensured by hook switch code in `with_hook` + let hook = unsafe { &mut *(*handler).0 }; + (hook)(info); + return; + } + + if let Some(hook) = unsafe { DEFAULT_HOOK.as_ref() } { + (hook)(info); + } + } + /// Executes stored closure when dropped + struct Finally(Option); + + impl Finally { + fn new(body: F) -> Self { + Self(Some(body)) + } + } + + impl Drop for Finally { + fn drop(&mut self) { + if let Some(body) = self.0.take() { + body(); + } + } + } + /// Executes main closure `body` while installing `guard` as scoped panic hook, + /// for execution duration. + /// + /// Any panics which happen during execution of `body` are passed to `guard` hook + /// to collect any info necessary, although unwind process is **NOT** interrupted. + /// See module documentation for details + /// + /// # Parameters + /// * `panic_hook` - scoped panic hook, functions for the duration of `body` execution + /// * `body` - actual logic covered by `panic_hook` + /// + /// # Returns + /// `body`'s return value + pub fn with_hook( + mut panic_hook: impl FnMut(&PanicInfo<'_>), + body: impl FnOnce() -> R, + ) -> R { + init(); + // Construct scoped hook pointer + let guard_tuple = (unsafe { + // `mem::transmute` is needed due to borrow checker restrictions to erase all lifetimes + mem::transmute(&mut panic_hook as *mut dyn FnMut(&PanicInfo<'_>)) + },); + let old_tuple = SCOPED_HOOK_PTR.replace(&guard_tuple); + // Old scoped hook **must** be restored before leaving function scope to keep it sound + let _undo = Finally::new(|| { + SCOPED_HOOK_PTR.set(old_tuple); + }); + body() + } +} + +#[cfg(not(feature = "handle-panics"))] +mod panicky { + use std::panic::PanicInfo; + /// Simply executes `body` and returns its execution result. + /// Hook parameter is ignored + pub fn with_hook( + _: impl FnMut(&PanicInfo<'_>), + body: impl FnOnce() -> R, + ) -> R { + body() + } +} + +pub use panicky::with_hook; From 343d74f18a21f15fcb38a0e5346534670a88b0d7 Mon Sep 17 00:00:00 2001 From: Target-san Date: Sat, 5 Oct 2024 16:04:10 +0300 Subject: [PATCH 03/27] Silence out panic from test case --- proptest/src/test_runner/runner.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/proptest/src/test_runner/runner.rs b/proptest/src/test_runner/runner.rs index a1af43ea..248fc994 100644 --- a/proptest/src/test_runner/runner.rs +++ b/proptest/src/test_runner/runner.rs @@ -1,5 +1,5 @@ //- -// Copyright 2017, 2018, 2019 The proptest developers +// Copyright 2017, 2018, 2019, 2024 The proptest developers // // Licensed under the Apache License, Version 2.0 or the MIT license @@ -253,7 +253,10 @@ where let time_start = time::Instant::now(); let mut result = unwrap_or!( - panic::catch_unwind(AssertUnwindSafe(|| test(case))), + super::scoped_panic_hook::with_hook( + |_| { /* Silence out panic backtrace */ }, + || panic::catch_unwind(AssertUnwindSafe(|| test(case))) + ), what => Err(TestCaseError::Fail( what.downcast::<&'static str>().map(|s| (*s).into()) .or_else(|what| what.downcast::().map(|b| (*b).into())) From b438970a7ed60cea5018a7033ccca439c22f9e6a Mon Sep 17 00:00:00 2001 From: Target-san Date: Sat, 5 Oct 2024 16:06:50 +0300 Subject: [PATCH 04/27] Updated changelog --- proptest/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/proptest/CHANGELOG.md b/proptest/CHANGELOG.md index adc6b9da..e8e1c393 100644 --- a/proptest/CHANGELOG.md +++ b/proptest/CHANGELOG.md @@ -3,6 +3,7 @@ ### New Features - When running persisted regressions, the most recently added regression is now run first. +- Added `handle-panics` feature which enables catching panics raised in tests and turning them into failures ## 1.5.0 From 6081fe85cad7dd2e33461897065eb8263b5269d4 Mon Sep 17 00:00:00 2001 From: Target-san Date: Sat, 5 Oct 2024 16:42:44 +0300 Subject: [PATCH 05/27] Rename internal module to `internal` --- proptest/src/test_runner/scoped_panic_hook.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/proptest/src/test_runner/scoped_panic_hook.rs b/proptest/src/test_runner/scoped_panic_hook.rs index 95793470..4b84c700 100644 --- a/proptest/src/test_runner/scoped_panic_hook.rs +++ b/proptest/src/test_runner/scoped_panic_hook.rs @@ -1,5 +1,5 @@ #[cfg(feature = "handle-panics")] -mod panicky { +mod internal { //! Implementation of scoped panic hooks //! //! 1. `with_hook` serves as entry point, it executes body closure with panic hook closure @@ -108,7 +108,7 @@ mod panicky { } #[cfg(not(feature = "handle-panics"))] -mod panicky { +mod internal { use std::panic::PanicInfo; /// Simply executes `body` and returns its execution result. /// Hook parameter is ignored @@ -120,4 +120,4 @@ mod panicky { } } -pub use panicky::with_hook; +pub use internal::with_hook; From eb7fdfcec05377f444402af850ae1441f7fa8897 Mon Sep 17 00:00:00 2001 From: Target-san Date: Sat, 5 Oct 2024 16:45:48 +0300 Subject: [PATCH 06/27] Licensing header --- proptest/src/test_runner/scoped_panic_hook.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/proptest/src/test_runner/scoped_panic_hook.rs b/proptest/src/test_runner/scoped_panic_hook.rs index 4b84c700..63183fbd 100644 --- a/proptest/src/test_runner/scoped_panic_hook.rs +++ b/proptest/src/test_runner/scoped_panic_hook.rs @@ -1,3 +1,12 @@ +//- +// Copyright 2024 The proptest developers +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + #[cfg(feature = "handle-panics")] mod internal { //! Implementation of scoped panic hooks From bb9a900ce65b70633580c4422540161082092b7a Mon Sep 17 00:00:00 2001 From: Target-san Date: Sat, 5 Oct 2024 16:53:00 +0300 Subject: [PATCH 07/27] 'backtrace' feature and changelog entry --- proptest/CHANGELOG.md | 2 ++ proptest/Cargo.toml | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/proptest/CHANGELOG.md b/proptest/CHANGELOG.md index e8e1c393..e3e5a1d0 100644 --- a/proptest/CHANGELOG.md +++ b/proptest/CHANGELOG.md @@ -4,6 +4,8 @@ - When running persisted regressions, the most recently added regression is now run first. - Added `handle-panics` feature which enables catching panics raised in tests and turning them into failures +- Added `backtrace` feature which enables capturing backtraces for both test failures and panics, + if `handle-panics` feature is enabled ## 1.5.0 diff --git a/proptest/Cargo.toml b/proptest/Cargo.toml index 3745377d..4ca8329d 100644 --- a/proptest/Cargo.toml +++ b/proptest/Cargo.toml @@ -63,6 +63,11 @@ bit-set = [ "dep:bit-set", "dep:bit-vec" ] # In particular, hides all intermediate panics flowing into stderr during shrink phase handle-panics = ["std"] +# Enables gathering of failure backtraces +# * when test failure is reported via `prop_assert_*` macro +# * when normal assertion fails or panic fires, if `handle-panics` feature is enabled too +backtrace = ["std"] + [dependencies] bitflags = "2" unarray = "0.1.4" From b9f344f3dad67af32bf656822bb39f0d1f3d7b32 Mon Sep 17 00:00:00 2001 From: Target-san Date: Sat, 5 Oct 2024 16:53:18 +0300 Subject: [PATCH 08/27] Conditional implementation of backtrace object --- proptest/src/test_runner/backtrace.rs | 135 ++++++++++++++++++++++++++ proptest/src/test_runner/mod.rs | 1 + 2 files changed, 136 insertions(+) create mode 100644 proptest/src/test_runner/backtrace.rs diff --git a/proptest/src/test_runner/backtrace.rs b/proptest/src/test_runner/backtrace.rs new file mode 100644 index 00000000..d5a77b2e --- /dev/null +++ b/proptest/src/test_runner/backtrace.rs @@ -0,0 +1,135 @@ +//- +// Copyright 2024 +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use core::fmt; +/// Holds test failure backtrace, if captured +/// +/// If feature `backtrace` is disabled, it's a zero-sized struct with no logic +/// +/// If `backtrace` is enabled, attempts to capture backtrace using `std::backtrace::Backtrace` - +/// if requested +#[derive(Clone, Default)] +pub struct Backtrace(internal::Backtrace); + +impl Backtrace { + /// Creates empty backtrace object + /// + /// Used when client code doesn't care + pub fn empty() -> Self { + Self(internal::Backtrace::empty()) + } + /// Tells whether there's backtrace captured + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + /// Attempts to capture backtrace - but only if `backtrace` feature is enabled + #[inline(always)] + pub fn capture() -> Self { + Self(internal::Backtrace::capture()) + } +} + +impl fmt::Debug for Backtrace { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Debug::fmt(&self.0, f) + } +} + +impl fmt::Display for Backtrace { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(&self.0, f) + } +} + +#[cfg(feature = "backtrace")] +mod internal { + use core::fmt; + use std::backtrace as bt; + use std::sync::Arc; + + // `std::backtrace::Backtrace` isn't `Clone`, so we have + // to use `Arc` to also maintain `Send + Sync` + #[derive(Clone, Default)] + pub struct Backtrace(Option>); + + impl Backtrace { + pub fn empty() -> Self { + Self(None) + } + + pub fn is_empty(&self) -> bool { + self.0.is_none() + } + + #[inline(always)] + pub fn capture() -> Self { + let bt = bt::Backtrace::capture(); + // Store only if we have backtrace + if bt.status() == bt::BacktraceStatus::Captured { + Self(Some(Arc::new(bt))) + } else { + Self(None) + } + } + } + + impl fmt::Debug for Backtrace { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if let Some(ref arc) = self.0 { + fmt::Debug::fmt(arc.as_ref(), f) + } else { + Ok(()) + } + } + } + + impl fmt::Display for Backtrace { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if let Some(ref arc) = self.0 { + fmt::Display::fmt(arc.as_ref(), f) + } else { + Ok(()) + } + } + } +} + +#[cfg(not(feature = "backtrace"))] +mod internal { + use core::fmt; + + #[derive(Clone, Default)] + pub struct Backtrace; + + impl Backtrace { + pub fn empty() -> Self { + Self + } + + pub fn is_empty(&self) -> bool { + true + } + + pub fn capture() -> Self { + Self + } + } + + impl fmt::Debug for Backtrace { + fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result { + Ok(()) + } + } + + impl fmt::Display for Backtrace { + fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result { + Ok(()) + } + } +} diff --git a/proptest/src/test_runner/mod.rs b/proptest/src/test_runner/mod.rs index 836d26d5..dec9b28a 100644 --- a/proptest/src/test_runner/mod.rs +++ b/proptest/src/test_runner/mod.rs @@ -22,6 +22,7 @@ mod result_cache; mod rng; mod runner; mod scoped_panic_hook; +mod backtrace; pub use self::config::*; pub use self::errors::*; From 4b0382829383482223ec2c146fb8cdcb3400d576 Mon Sep 17 00:00:00 2001 From: Target-san Date: Sat, 5 Oct 2024 18:09:25 +0300 Subject: [PATCH 09/27] Extend `Reason` with ability to store backtrace --- proptest/src/test_runner/reason.rs | 110 ++++++++++++++++++++++++++--- 1 file changed, 102 insertions(+), 8 deletions(-) diff --git a/proptest/src/test_runner/reason.rs b/proptest/src/test_runner/reason.rs index 38cc7e32..c7029483 100644 --- a/proptest/src/test_runner/reason.rs +++ b/proptest/src/test_runner/reason.rs @@ -8,18 +8,44 @@ // except according to those terms. use crate::std_facade::{fmt, Box, Cow, String}; +use super::backtrace::Backtrace; /// The reason for why something, such as a generated value, was rejected. /// -/// Currently this is merely a wrapper around a message, but more properties -/// may be added in the future. +/// Contains message which describes reason and optionally backtrace +/// (depending on several factors like features `backtrace` and +/// `handle-panics`, and actual spot where reason was created). /// /// This is constructed via `.into()` on a `String`, `&'static str`, or /// `Box`. -#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct Reason(Cow<'static, str>); +#[derive(Clone)] +pub struct Reason(Cow<'static, str>, Backtrace); impl Reason { + /// Creates reason from provided message + /// + /// # Parameters + /// * `message` - anything convertible to message + /// + /// # Returns + /// Reason object + pub fn new(message: impl Into>) -> Self { + Self(message.into(), Backtrace::empty()) + } + /// Creates reason from provided message and captures backtrace at callsite + /// + /// NOTE: Backtrace is actually captured only if `backtrace` feature is enabled, + /// otherwise it'll be empty + /// + /// # Parameters + /// * `message` - anything convertible to message + /// + /// # Returns + /// Reason object with provided message and captured backtrace + #[inline(always)] + pub fn with_backtrace(message: impl Into>) -> Self { + Self(message.into(), Backtrace::capture()) + } /// Return the message for this `Reason`. /// /// The message is intended for human consumption, and is not guaranteed to @@ -27,28 +53,96 @@ impl Reason { pub fn message(&self) -> &str { &*self.0 } + /// Produces displayable value which displays all data stored in Reason, + /// unlike normal `Display` implementation which shows only message + pub fn display_detailed(&self) -> impl fmt::Display + '_ { + DisplayReason(self) + } +} + +impl core::cmp::PartialEq for Reason { + fn eq(&self, other: &Self) -> bool { + self.0 == other.0 + } +} + +impl core::cmp::Eq for Reason {} + +impl core::cmp::PartialOrd for Reason { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl core::cmp::Ord for Reason { + fn cmp(&self, other: &Self) -> core::cmp::Ordering { + self.0.cmp(&other.0) + } +} + +impl core::hash::Hash for Reason { + fn hash(&self, state: &mut H) { + self.0.hash(state); + } +} + +impl From<(&'static str, Backtrace)> for Reason { + fn from((s, b): (&'static str, Backtrace)) -> Self { + Self(s.into(), b) + } +} + +impl From<(String, Backtrace)> for Reason { + fn from((s, b): (String, Backtrace)) -> Self { + Self(s.into(), b) + } +} + +impl From<(Box, Backtrace)> for Reason { + fn from((s, b): (Box, Backtrace)) -> Self { + Self(String::from(s).into(), b) + } } impl From<&'static str> for Reason { fn from(s: &'static str) -> Self { - Reason(s.into()) + (s, Backtrace::empty()).into() } } impl From for Reason { fn from(s: String) -> Self { - Reason(s.into()) + (s, Backtrace::empty()).into() } } impl From> for Reason { fn from(s: Box) -> Self { - Reason(String::from(s).into()) + (s, Backtrace::empty()).into() + } +} + +impl fmt::Debug for Reason { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.debug_tuple("Reason").field(&self.0).field(&"Backtrace(...)").finish() } } impl fmt::Display for Reason { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - fmt::Display::fmt(self.message(), f) + write!(f, "{}", self.message()) + } +} + +struct DisplayReason<'a>(&'a Reason); + +impl<'a> fmt::Display for DisplayReason<'a> { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + let Self(Reason(msg, bt)) = self; + if bt.is_empty() { + write!(f, "{msg}") + } else { + write!(f, "{msg}\n{bt}") + } } } From bfffcf439e7e298bc03dc6dad74f60cce33e99cd Mon Sep 17 00:00:00 2001 From: Target-san Date: Sun, 6 Oct 2024 00:54:15 +0300 Subject: [PATCH 10/27] Collect backtrace upon panic or with prop_assert --- proptest/src/sugar.rs | 5 ++++- proptest/src/test_runner/reason.rs | 23 ++++++++++++++++------- proptest/src/test_runner/runner.rs | 19 ++++++++++++------- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/proptest/src/sugar.rs b/proptest/src/sugar.rs index 7d80d07b..5a50d7db 100644 --- a/proptest/src/sugar.rs +++ b/proptest/src/sugar.rs @@ -754,7 +754,10 @@ macro_rules! prop_assert { let message = format!($($fmt)*); let message = format!("{} at {}:{}", message, file!(), line!()); return ::core::result::Result::Err( - $crate::test_runner::TestCaseError::fail(message)); + $crate::test_runner::TestCaseError::fail( + $crate::test_runner::Reason::with_backtrace(message) + ) + ); } }; } diff --git a/proptest/src/test_runner/reason.rs b/proptest/src/test_runner/reason.rs index c7029483..afff7d2f 100644 --- a/proptest/src/test_runner/reason.rs +++ b/proptest/src/test_runner/reason.rs @@ -7,8 +7,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::std_facade::{fmt, Box, Cow, String}; use super::backtrace::Backtrace; +use crate::std_facade::{fmt, Box, Cow, String}; /// The reason for why something, such as a generated value, was rejected. /// @@ -23,23 +23,23 @@ pub struct Reason(Cow<'static, str>, Backtrace); impl Reason { /// Creates reason from provided message - /// + /// /// # Parameters /// * `message` - anything convertible to message - /// + /// /// # Returns /// Reason object pub fn new(message: impl Into>) -> Self { Self(message.into(), Backtrace::empty()) } /// Creates reason from provided message and captures backtrace at callsite - /// + /// /// NOTE: Backtrace is actually captured only if `backtrace` feature is enabled, /// otherwise it'll be empty - /// + /// /// # Parameters /// * `message` - anything convertible to message - /// + /// /// # Returns /// Reason object with provided message and captured backtrace #[inline(always)] @@ -92,6 +92,12 @@ impl From<(&'static str, Backtrace)> for Reason { } } +impl From<(Cow<'static, str>, Backtrace)> for Reason { + fn from((msg, bt): (Cow<'static, str>, Backtrace)) -> Self { + Self(msg, bt) + } +} + impl From<(String, Backtrace)> for Reason { fn from((s, b): (String, Backtrace)) -> Self { Self(s.into(), b) @@ -124,7 +130,10 @@ impl From> for Reason { impl fmt::Debug for Reason { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - f.debug_tuple("Reason").field(&self.0).field(&"Backtrace(...)").finish() + f.debug_tuple("Reason") + .field(&self.0) + .field(&"Backtrace(...)") + .finish() } } diff --git a/proptest/src/test_runner/runner.rs b/proptest/src/test_runner/runner.rs index 248fc994..ef3a1e33 100644 --- a/proptest/src/test_runner/runner.rs +++ b/proptest/src/test_runner/runner.rs @@ -225,7 +225,14 @@ where F: Fn(V) -> TestCaseResult, R: Iterator, { - use std::time; + use std::{borrow::Cow, time}; + + fn panic_message(p: Box) -> Cow<'static, str> { + p.downcast::<&'static str>().map(|s| (*s).into()) + .or_else(|what| what.downcast::().map(|b| (*b).into())) + .or_else(|what| what.downcast::>().map(|b| String::from(*b).into())) + .unwrap_or_else(|_| "".into()) + } let timeout = runner.config.timeout(); @@ -252,16 +259,14 @@ where let time_start = time::Instant::now(); + let mut bt = super::backtrace::Backtrace::empty(); let mut result = unwrap_or!( super::scoped_panic_hook::with_hook( - |_| { /* Silence out panic backtrace */ }, + |_| { bt = super::backtrace::Backtrace::capture(); }, || panic::catch_unwind(AssertUnwindSafe(|| test(case))) ), - what => Err(TestCaseError::Fail( - what.downcast::<&'static str>().map(|s| (*s).into()) - .or_else(|what| what.downcast::().map(|b| (*b).into())) - .or_else(|what| what.downcast::>().map(|b| (*b).into())) - .unwrap_or_else(|_| "".into())))); + what => Err(TestCaseError::Fail((panic_message(what), bt).into())) + ); // If there is a timeout and we exceeded it, fail the test here so we get // consistent behaviour. (The parent process cannot precisely time the test From 55499bf6cdb7f97e67c73b09befcb2a362a740c4 Mon Sep 17 00:00:00 2001 From: Target-san Date: Sun, 6 Oct 2024 14:47:39 +0300 Subject: [PATCH 11/27] Slight tweak of reason's detailed display --- proptest/src/test_runner/reason.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proptest/src/test_runner/reason.rs b/proptest/src/test_runner/reason.rs index afff7d2f..13ff575e 100644 --- a/proptest/src/test_runner/reason.rs +++ b/proptest/src/test_runner/reason.rs @@ -151,7 +151,7 @@ impl<'a> fmt::Display for DisplayReason<'a> { if bt.is_empty() { write!(f, "{msg}") } else { - write!(f, "{msg}\n{bt}") + write!(f, "{msg}\nstack backtrace:\n{bt}") } } } From 692e56a04644b39a215f9f13490ae9fcdc5b0590 Mon Sep 17 00:00:00 2001 From: Target-san Date: Sun, 6 Oct 2024 14:59:01 +0300 Subject: [PATCH 12/27] Tweak errors display --- proptest/src/test_runner/errors.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/proptest/src/test_runner/errors.rs b/proptest/src/test_runner/errors.rs index bf17f201..407de5dd 100644 --- a/proptest/src/test_runner/errors.rs +++ b/proptest/src/test_runner/errors.rs @@ -86,7 +86,7 @@ impl fmt::Display for TestCaseError { TestCaseError::Reject(ref whence) => { write!(f, "Input rejected at {}", whence) } - TestCaseError::Fail(ref why) => write!(f, "Case failed: {}", why), + TestCaseError::Fail(ref why) => write!(f, "Case failed: {}", why.display_detailed()), } } } @@ -115,7 +115,7 @@ impl fmt::Display for TestError { match *self { TestError::Abort(ref why) => write!(f, "Test aborted: {}", why), TestError::Fail(ref why, ref what) => { - writeln!(f, "Test failed: {}.", why)?; + writeln!(f, "Test failed: {}", why.display_detailed())?; write!(f, "minimal failing input: {:#?}", what) } } From b1356672a029516e56721f3264aaedd1de1dd6bc Mon Sep 17 00:00:00 2001 From: Target-san Date: Sun, 6 Oct 2024 23:33:09 +0300 Subject: [PATCH 13/27] Add location handling and construction from PanicInfo, if needed --- proptest/src/test_runner/reason.rs | 74 +++++++++++++++++++----------- 1 file changed, 47 insertions(+), 27 deletions(-) diff --git a/proptest/src/test_runner/reason.rs b/proptest/src/test_runner/reason.rs index 13ff575e..06126005 100644 --- a/proptest/src/test_runner/reason.rs +++ b/proptest/src/test_runner/reason.rs @@ -7,6 +7,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use std::string::ToString; + use super::backtrace::Backtrace; use crate::std_facade::{fmt, Box, Cow, String}; @@ -25,26 +27,41 @@ impl Reason { /// Creates reason from provided message /// /// # Parameters - /// * `message` - anything convertible to message + /// * `message` - anything convertible to shared or owned string /// /// # Returns /// Reason object pub fn new(message: impl Into>) -> Self { Self(message.into(), Backtrace::empty()) } - /// Creates reason from provided message and captures backtrace at callsite + /// Creates reason from provided message, adding location info as its part + /// + /// # Parameters + /// * `message` - anything convertible to shared or owned string + /// + /// # Returns + /// Reason object + #[track_caller] + pub fn with_location(message: impl Into>) -> Self { + let message: Cow<'static, str> = message.into(); + let loc = core::panic::Location::caller(); + Self(format!("{message} at {loc}").into(), Backtrace::empty()) + } + /// Creates reason from provided message, adding location info as its part, + /// and captures backtrace at callsite /// /// NOTE: Backtrace is actually captured only if `backtrace` feature is enabled, /// otherwise it'll be empty /// /// # Parameters - /// * `message` - anything convertible to message + /// * `message` - anything convertible to shared or owned string /// /// # Returns - /// Reason object with provided message and captured backtrace + /// Reason object with provided message, augmented with location info, and captured backtrace #[inline(always)] - pub fn with_backtrace(message: impl Into>) -> Self { - Self(message.into(), Backtrace::capture()) + #[track_caller] + pub fn with_location_and_backtrace(message: impl Into>) -> Self { + Self(Self::with_location(message).0, Backtrace::capture()) } /// Return the message for this `Reason`. /// @@ -86,45 +103,48 @@ impl core::hash::Hash for Reason { } } -impl From<(&'static str, Backtrace)> for Reason { - fn from((s, b): (&'static str, Backtrace)) -> Self { - Self(s.into(), b) - } -} - impl From<(Cow<'static, str>, Backtrace)> for Reason { fn from((msg, bt): (Cow<'static, str>, Backtrace)) -> Self { Self(msg, bt) } } -impl From<(String, Backtrace)> for Reason { - fn from((s, b): (String, Backtrace)) -> Self { - Self(s.into(), b) - } -} - -impl From<(Box, Backtrace)> for Reason { - fn from((s, b): (Box, Backtrace)) -> Self { - Self(String::from(s).into(), b) - } -} - impl From<&'static str> for Reason { fn from(s: &'static str) -> Self { - (s, Backtrace::empty()).into() + Self(s.into(), Backtrace::empty()) } } impl From for Reason { fn from(s: String) -> Self { - (s, Backtrace::empty()).into() + Self(s.into(), Backtrace::empty()) } } impl From> for Reason { fn from(s: Box) -> Self { - (s, Backtrace::empty()).into() + Self(String::from(s).into(), Backtrace::empty()) + } +} + +#[cfg(feature = "std")] +impl<'a, 'b> From<&'b std::panic::PanicInfo<'a>> for Reason { + #[inline(always)] + fn from(value: &'b std::panic::PanicInfo<'a>) -> Self { + let payload = value.payload(); + let message: String = payload.downcast_ref::<&'static str>() + .map(|s| s.to_string()) + .or_else(|| payload.downcast_ref::().map(|s| s.clone())) + .or_else(|| payload.downcast_ref::>().map(|s| s.to_string())) + .unwrap_or_else(|| "".to_string()); + + let message = if let Some(loc) = value.location() { + format!("{message} at {loc}") + } else { + message + }; + + Self(message.into(), Backtrace::capture()) } } From c74159c21aa702e187f4e5ed37d5fec7cdf6fd0c Mon Sep 17 00:00:00 2001 From: Target-san Date: Sun, 6 Oct 2024 23:34:08 +0300 Subject: [PATCH 14/27] Fix prop_assert macro --- proptest/src/sugar.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/proptest/src/sugar.rs b/proptest/src/sugar.rs index 5a50d7db..b09ed1d3 100644 --- a/proptest/src/sugar.rs +++ b/proptest/src/sugar.rs @@ -751,11 +751,9 @@ macro_rules! prop_assert { ($cond:expr, $($fmt:tt)*) => { if !$cond { - let message = format!($($fmt)*); - let message = format!("{} at {}:{}", message, file!(), line!()); return ::core::result::Result::Err( $crate::test_runner::TestCaseError::fail( - $crate::test_runner::Reason::with_backtrace(message) + $crate::test_runner::Reason::with_location_and_backtrace(format!($($fmt)*)) ) ); } From 1eb50fef1ecbb48c2911afa5c441a592732d03b6 Mon Sep 17 00:00:00 2001 From: Target-san Date: Sun, 6 Oct 2024 23:34:32 +0300 Subject: [PATCH 15/27] Reason from PanicInfo --- proptest/src/test_runner/runner.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/proptest/src/test_runner/runner.rs b/proptest/src/test_runner/runner.rs index ef3a1e33..7ec3167a 100644 --- a/proptest/src/test_runner/runner.rs +++ b/proptest/src/test_runner/runner.rs @@ -7,7 +7,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::std_facade::{Arc, BTreeMap, Box, String, Vec}; +use crate::std_facade::{Arc, BTreeMap, Box, Vec}; use core::sync::atomic::AtomicUsize; use core::sync::atomic::Ordering::SeqCst; use core::{fmt, iter}; @@ -225,14 +225,7 @@ where F: Fn(V) -> TestCaseResult, R: Iterator, { - use std::{borrow::Cow, time}; - - fn panic_message(p: Box) -> Cow<'static, str> { - p.downcast::<&'static str>().map(|s| (*s).into()) - .or_else(|what| what.downcast::().map(|b| (*b).into())) - .or_else(|what| what.downcast::>().map(|b| String::from(*b).into())) - .unwrap_or_else(|_| "".into()) - } + use std::time; let timeout = runner.config.timeout(); @@ -259,13 +252,15 @@ where let time_start = time::Instant::now(); - let mut bt = super::backtrace::Backtrace::empty(); + let mut reason = None; let mut result = unwrap_or!( super::scoped_panic_hook::with_hook( - |_| { bt = super::backtrace::Backtrace::capture(); }, + |panic_info| { reason = Some(panic_info.into()); }, || panic::catch_unwind(AssertUnwindSafe(|| test(case))) ), - what => Err(TestCaseError::Fail((panic_message(what), bt).into())) + _panic => Err(TestCaseError::Fail(reason.expect( + "Reaon should have been obtained from panic hook" + ))) ); // If there is a timeout and we exceeded it, fail the test here so we get From 9031491866c700dc410ca12e4ce91a7b6408fe13 Mon Sep 17 00:00:00 2001 From: Target-san Date: Sun, 6 Oct 2024 23:44:34 +0300 Subject: [PATCH 16/27] Formatting of affected files --- proptest/src/test_runner/errors.rs | 4 +++- proptest/src/test_runner/mod.rs | 2 +- proptest/src/test_runner/reason.rs | 15 ++++++++++----- proptest/src/test_runner/runner.rs | 2 +- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/proptest/src/test_runner/errors.rs b/proptest/src/test_runner/errors.rs index 407de5dd..ef3c455b 100644 --- a/proptest/src/test_runner/errors.rs +++ b/proptest/src/test_runner/errors.rs @@ -86,7 +86,9 @@ impl fmt::Display for TestCaseError { TestCaseError::Reject(ref whence) => { write!(f, "Input rejected at {}", whence) } - TestCaseError::Fail(ref why) => write!(f, "Case failed: {}", why.display_detailed()), + TestCaseError::Fail(ref why) => { + write!(f, "Case failed: {}", why.display_detailed()) + } } } } diff --git a/proptest/src/test_runner/mod.rs b/proptest/src/test_runner/mod.rs index dec9b28a..aff48bac 100644 --- a/proptest/src/test_runner/mod.rs +++ b/proptest/src/test_runner/mod.rs @@ -12,6 +12,7 @@ //! You do not normally need to access things in this module directly except //! when implementing new low-level strategies. +mod backtrace; mod config; mod errors; mod failure_persistence; @@ -22,7 +23,6 @@ mod result_cache; mod rng; mod runner; mod scoped_panic_hook; -mod backtrace; pub use self::config::*; pub use self::errors::*; diff --git a/proptest/src/test_runner/reason.rs b/proptest/src/test_runner/reason.rs index 06126005..e14c7b33 100644 --- a/proptest/src/test_runner/reason.rs +++ b/proptest/src/test_runner/reason.rs @@ -35,10 +35,10 @@ impl Reason { Self(message.into(), Backtrace::empty()) } /// Creates reason from provided message, adding location info as its part - /// + /// /// # Parameters /// * `message` - anything convertible to shared or owned string - /// + /// /// # Returns /// Reason object #[track_caller] @@ -60,7 +60,9 @@ impl Reason { /// Reason object with provided message, augmented with location info, and captured backtrace #[inline(always)] #[track_caller] - pub fn with_location_and_backtrace(message: impl Into>) -> Self { + pub fn with_location_and_backtrace( + message: impl Into>, + ) -> Self { Self(Self::with_location(message).0, Backtrace::capture()) } /// Return the message for this `Reason`. @@ -132,10 +134,13 @@ impl<'a, 'b> From<&'b std::panic::PanicInfo<'a>> for Reason { #[inline(always)] fn from(value: &'b std::panic::PanicInfo<'a>) -> Self { let payload = value.payload(); - let message: String = payload.downcast_ref::<&'static str>() + let message: String = payload + .downcast_ref::<&'static str>() .map(|s| s.to_string()) .or_else(|| payload.downcast_ref::().map(|s| s.clone())) - .or_else(|| payload.downcast_ref::>().map(|s| s.to_string())) + .or_else(|| { + payload.downcast_ref::>().map(|s| s.to_string()) + }) .unwrap_or_else(|| "".to_string()); let message = if let Some(loc) = value.location() { diff --git a/proptest/src/test_runner/runner.rs b/proptest/src/test_runner/runner.rs index 7ec3167a..0cab1084 100644 --- a/proptest/src/test_runner/runner.rs +++ b/proptest/src/test_runner/runner.rs @@ -772,7 +772,7 @@ impl TestRunner { INFO_LOG, "Shrinking disabled by configuration" ); - return None + return None; } #[cfg(feature = "std")] From c21f4186852c890526384e19e8f08d09b34fdbe2 Mon Sep 17 00:00:00 2001 From: Target-san Date: Mon, 7 Oct 2024 09:01:47 +0300 Subject: [PATCH 17/27] Nicer addition of location info to Reason --- proptest/src/test_runner/reason.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/proptest/src/test_runner/reason.rs b/proptest/src/test_runner/reason.rs index e14c7b33..5fd0c754 100644 --- a/proptest/src/test_runner/reason.rs +++ b/proptest/src/test_runner/reason.rs @@ -45,7 +45,10 @@ impl Reason { pub fn with_location(message: impl Into>) -> Self { let message: Cow<'static, str> = message.into(); let loc = core::panic::Location::caller(); - Self(format!("{message} at {loc}").into(), Backtrace::empty()) + Self( + append_location(message.into_owned(), *loc).into(), + Backtrace::empty(), + ) } /// Creates reason from provided message, adding location info as its part, /// and captures backtrace at callsite @@ -144,7 +147,7 @@ impl<'a, 'b> From<&'b std::panic::PanicInfo<'a>> for Reason { .unwrap_or_else(|| "".to_string()); let message = if let Some(loc) = value.location() { - format!("{message} at {loc}") + append_location(message, *loc) } else { message }; @@ -180,3 +183,19 @@ impl<'a> fmt::Display for DisplayReason<'a> { } } } + +fn append_location<'a>( + message: String, + loc: core::panic::Location<'a>, +) -> String { + match message.rfind('\n') { + // Message is multiline and ends with '\n' + Some(pos) if pos == message.len() - '\n'.len_utf8() => { + format!("{message}at {loc}") + } + // Message is multiline and doesn't end with '\n' + Some(_) => format!("{message}\nat {loc}"), + // MEssage is not multiline + _ => format!("{message} at {loc}"), + } +} From 7e1fdd2b8f12515c24268f9cfd1637788c8a1889 Mon Sep 17 00:00:00 2001 From: Target-san Date: Mon, 7 Oct 2024 09:04:20 +0300 Subject: [PATCH 18/27] Spelling --- proptest/src/test_runner/reason.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proptest/src/test_runner/reason.rs b/proptest/src/test_runner/reason.rs index 5fd0c754..ba86fcdd 100644 --- a/proptest/src/test_runner/reason.rs +++ b/proptest/src/test_runner/reason.rs @@ -195,7 +195,7 @@ fn append_location<'a>( } // Message is multiline and doesn't end with '\n' Some(_) => format!("{message}\nat {loc}"), - // MEssage is not multiline + // Message is not multiline _ => format!("{message} at {loc}"), } } From eaa8223ca7fd85a95ffe81fa7b1cc265687233c7 Mon Sep 17 00:00:00 2001 From: Target-san Date: Thu, 12 Dec 2024 20:44:32 +0200 Subject: [PATCH 19/27] Fix test --- proptest/Cargo.toml | 1 + proptest/src/test_runner/runner.rs | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/proptest/Cargo.toml b/proptest/Cargo.toml index bf138539..493e6850 100644 --- a/proptest/Cargo.toml +++ b/proptest/Cargo.toml @@ -128,5 +128,6 @@ all-features = true rustdoc-args = ["--cfg", "docsrs"] [dev-dependencies] +assert_matches = "1.5.0" regex = "1.0" trybuild = "=1.0.0" diff --git a/proptest/src/test_runner/runner.rs b/proptest/src/test_runner/runner.rs index ead55b85..e1511e35 100644 --- a/proptest/src/test_runner/runner.rs +++ b/proptest/src/test_runner/runner.rs @@ -1070,6 +1070,8 @@ mod test { use std::cell::Cell; use std::fs; + use assert_matches::assert_matches; + use super::*; use crate::strategy::Strategy; use crate::test_runner::{FileFailurePersistence, RngAlgorithm, TestRng}; @@ -1127,7 +1129,10 @@ mod test { assert!(v < 5, "not less than 5"); Ok(()) }); - assert_eq!(Err(TestError::Fail("not less than 5".into(), 5)), result); + assert_matches!( + result, + Err(TestError::Fail(reason, 5)) if reason.message().starts_with("not less than 5") + ); } #[test] From cdcb423a3966e30f3134aecbae1217a12efa2449 Mon Sep 17 00:00:00 2001 From: Target-san Date: Thu, 12 Dec 2024 22:44:30 +0200 Subject: [PATCH 20/27] Simplify mod, enable only if handle-panics is enabled --- proptest/src/test_runner/mod.rs | 2 +- proptest/src/test_runner/scoped_panic_hook.rs | 203 ++++++++---------- 2 files changed, 93 insertions(+), 112 deletions(-) diff --git a/proptest/src/test_runner/mod.rs b/proptest/src/test_runner/mod.rs index 34150e8b..d694d417 100644 --- a/proptest/src/test_runner/mod.rs +++ b/proptest/src/test_runner/mod.rs @@ -22,7 +22,7 @@ mod replay; mod result_cache; mod rng; mod runner; -#[cfg(feature = "std")] +#[cfg(feature = "handle-panics")] mod scoped_panic_hook; pub use self::config::*; diff --git a/proptest/src/test_runner/scoped_panic_hook.rs b/proptest/src/test_runner/scoped_panic_hook.rs index 3c916b7a..fc03615f 100644 --- a/proptest/src/test_runner/scoped_panic_hook.rs +++ b/proptest/src/test_runner/scoped_panic_hook.rs @@ -7,127 +7,108 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -#[cfg(feature = "handle-panics")] -mod internal { - //! Implementation of scoped panic hooks - //! - //! 1. `with_hook` serves as entry point, it executes body closure with panic hook closure - //! installed as scoped panic hook - //! 2. Upon first execution, current panic hook is replaced with `scoped_hook_dispatcher` - //! in a thread-safe manner, and original hook is stored for later use - //! 3. When panic occurs, `scoped_hook_dispatcher` either delegates execution to scoped - //! panic hook, if one is installed, or back to original hook stored earlier. - //! This preserves original behavior when scoped hook isn't used - //! 4. When `with_hook` is used, it replaces stored scoped hook pointer with pointer to - //! hook closure passed as parameter. Old hook pointer is set to be restored unconditionally - //! via drop guard. Then, normal body closure is executed. - use std::boxed::Box; - use std::cell::Cell; - use std::panic::{set_hook, take_hook, PanicInfo}; - use std::sync::Once; - use std::{mem, ptr}; +//! Implementation of scoped panic hooks +//! +//! 1. `with_hook` serves as entry point, it executes body closure with panic hook closure +//! installed as scoped panic hook +//! 2. Upon first execution, current panic hook is replaced with `scoped_hook_dispatcher` +//! in a thread-safe manner, and original hook is stored for later use +//! 3. When panic occurs, `scoped_hook_dispatcher` either delegates execution to scoped +//! panic hook, if one is installed, or back to original hook stored earlier. +//! This preserves original behavior when scoped hook isn't used +//! 4. When `with_hook` is used, it replaces stored scoped hook pointer with pointer to +//! hook closure passed as parameter. Old hook pointer is set to be restored unconditionally +//! via drop guard. Then, normal body closure is executed. +use std::boxed::Box; +use std::cell::Cell; +use std::panic::{set_hook, take_hook, PanicInfo}; +use std::sync::Once; +use std::{mem, ptr}; - thread_local! { - /// Pointer to currently installed scoped panic hook, if any - /// - /// NB: pointers to arbitrary fn's are fat, and Rust doesn't allow crafting null pointers - /// to fat objects. So we just store const pointer to tuple with whatever data we need - static SCOPED_HOOK_PTR: Cell<*const (*mut dyn FnMut(&PanicInfo<'_>),)> = Cell::new(ptr::null()); - } - - static INIT_ONCE: Once = Once::new(); - /// Default panic hook, the one which was present before installing scoped one +thread_local! { + /// Pointer to currently installed scoped panic hook, if any /// - /// NB: no need for external sync, value is mutated only once, when init is performed - static mut DEFAULT_HOOK: Option) + Send + Sync>> = - None; - /// Replaces currently installed panic hook with `scoped_hook_dispatcher` once, - /// in a thread-safe manner - fn init() { - INIT_ONCE.call_once(|| { - let old_handler = take_hook(); - set_hook(Box::new(scoped_hook_dispatcher)); - unsafe { - DEFAULT_HOOK = Some(old_handler); - } - }); - } - /// Panic hook which delegates execution to scoped hook, - /// if one installed, or to default hook - fn scoped_hook_dispatcher(info: &PanicInfo<'_>) { - let handler = SCOPED_HOOK_PTR.get(); - if !handler.is_null() { - // It's assumed that if container's ptr is not null, ptr to `FnMut` is non-null too. - // Correctness **must** be ensured by hook switch code in `with_hook` - let hook = unsafe { &mut *(*handler).0 }; - (hook)(info); - return; - } + /// NB: pointers to arbitrary fn's are fat, and Rust doesn't allow crafting null pointers + /// to fat objects. So we just store const pointer to tuple with whatever data we need + static SCOPED_HOOK_PTR: Cell<*const (*mut dyn FnMut(&PanicInfo<'_>),)> = Cell::new(ptr::null()); +} - if let Some(hook) = unsafe { DEFAULT_HOOK.as_ref() } { - (hook)(info); +static INIT_ONCE: Once = Once::new(); +/// Default panic hook, the one which was present before installing scoped one +/// +/// NB: no need for external sync, value is mutated only once, when init is performed +static mut DEFAULT_HOOK: Option) + Send + Sync>> = + None; +/// Replaces currently installed panic hook with `scoped_hook_dispatcher` once, +/// in a thread-safe manner +fn init() { + INIT_ONCE.call_once(|| { + let old_handler = take_hook(); + set_hook(Box::new(scoped_hook_dispatcher)); + unsafe { + DEFAULT_HOOK = Some(old_handler); } + }); +} +/// Panic hook which delegates execution to scoped hook, +/// if one installed, or to default hook +fn scoped_hook_dispatcher(info: &PanicInfo<'_>) { + let handler = SCOPED_HOOK_PTR.get(); + if !handler.is_null() { + // It's assumed that if container's ptr is not null, ptr to `FnMut` is non-null too. + // Correctness **must** be ensured by hook switch code in `with_hook` + let hook = unsafe { &mut *(*handler).0 }; + (hook)(info); + return; } - /// Executes stored closure when dropped - struct Finally(Option); - impl Finally { - fn new(body: F) -> Self { - Self(Some(body)) - } + if let Some(hook) = unsafe { DEFAULT_HOOK.as_ref() } { + (hook)(info); } +} +/// Executes stored closure when dropped +struct Finally(Option); - impl Drop for Finally { - fn drop(&mut self) { - if let Some(body) = self.0.take() { - body(); - } - } - } - /// Executes main closure `body` while installing `guard` as scoped panic hook, - /// for execution duration. - /// - /// Any panics which happen during execution of `body` are passed to `guard` hook - /// to collect any info necessary, although unwind process is **NOT** interrupted. - /// See module documentation for details - /// - /// # Parameters - /// * `panic_hook` - scoped panic hook, functions for the duration of `body` execution - /// * `body` - actual logic covered by `panic_hook` - /// - /// # Returns - /// `body`'s return value - pub fn with_hook( - mut panic_hook: impl FnMut(&PanicInfo<'_>), - body: impl FnOnce() -> R, - ) -> R { - init(); - // Construct scoped hook pointer - let guard_tuple = (unsafe { - // `mem::transmute` is needed due to borrow checker restrictions to erase all lifetimes - mem::transmute(&mut panic_hook as *mut dyn FnMut(&PanicInfo<'_>)) - },); - let old_tuple = SCOPED_HOOK_PTR.replace(&guard_tuple); - // Old scoped hook **must** be restored before leaving function scope to keep it sound - let _undo = Finally::new(|| { - SCOPED_HOOK_PTR.set(old_tuple); - }); - body() +impl Finally { + fn new(body: F) -> Self { + Self(Some(body)) } } -#[cfg(not(feature = "handle-panics"))] -mod internal { - use std::panic::PanicInfo; - - /// Simply executes `body` and returns its execution result. - /// Hook parameter is ignored - pub fn with_hook( - _: impl FnMut(&PanicInfo<'_>), - body: impl FnOnce() -> R, - ) -> R { - body() +impl Drop for Finally { + fn drop(&mut self) { + if let Some(body) = self.0.take() { + body(); + } } } - -pub use internal::with_hook; +/// Executes main closure `body` while installing `guard` as scoped panic hook, +/// for execution duration. +/// +/// Any panics which happen during execution of `body` are passed to `guard` hook +/// to collect any info necessary, although unwind process is **NOT** interrupted. +/// See module documentation for details +/// +/// # Parameters +/// * `panic_hook` - scoped panic hook, functions for the duration of `body` execution +/// * `body` - actual logic covered by `panic_hook` +/// +/// # Returns +/// `body`'s return value +pub fn with_hook( + mut panic_hook: impl FnMut(&PanicInfo<'_>), + body: impl FnOnce() -> R, +) -> R { + init(); + // Construct scoped hook pointer + let guard_tuple = (unsafe { + // `mem::transmute` is needed due to borrow checker restrictions to erase all lifetimes + mem::transmute(&mut panic_hook as *mut dyn FnMut(&PanicInfo<'_>)) + },); + let old_tuple = SCOPED_HOOK_PTR.replace(&guard_tuple); + // Old scoped hook **must** be restored before leaving function scope to keep it sound + let _undo = Finally::new(|| { + SCOPED_HOOK_PTR.set(old_tuple); + }); + body() +} From 670351b822db58714e795a6c9c21d67e0ae4595a Mon Sep 17 00:00:00 2001 From: Target-san Date: Thu, 12 Dec 2024 22:46:29 +0200 Subject: [PATCH 21/27] Allow construct from `dyn Any` --- proptest/src/test_runner/reason.rs | 33 ++++++++++++++++++------------ 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/proptest/src/test_runner/reason.rs b/proptest/src/test_runner/reason.rs index ba86fcdd..c472dc23 100644 --- a/proptest/src/test_runner/reason.rs +++ b/proptest/src/test_runner/reason.rs @@ -7,6 +7,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use core::any::Any; use std::string::ToString; use super::backtrace::Backtrace; @@ -133,26 +134,32 @@ impl From> for Reason { } #[cfg(feature = "std")] -impl<'a, 'b> From<&'b std::panic::PanicInfo<'a>> for Reason { - #[inline(always)] - fn from(value: &'b std::panic::PanicInfo<'a>) -> Self { - let payload = value.payload(); - let message: String = payload +impl<'a> From<&'a (dyn Any + Send)> for Reason { + fn from(value: &'a (dyn Any + Send)) -> Self { + let message: String = value .downcast_ref::<&'static str>() .map(|s| s.to_string()) - .or_else(|| payload.downcast_ref::().map(|s| s.clone())) + .or_else(|| value.downcast_ref::().map(|s| s.clone())) .or_else(|| { - payload.downcast_ref::>().map(|s| s.to_string()) + value.downcast_ref::>().map(|s| s.to_string()) }) .unwrap_or_else(|| "".to_string()); - let message = if let Some(loc) = value.location() { - append_location(message, *loc) - } else { - message - }; + Self(message.into(), Backtrace::empty()) + } +} + +#[cfg(feature = "std")] +impl<'a, 'b> From<&'b std::panic::PanicInfo<'a>> for Reason { + #[inline(always)] + fn from(value: &'b std::panic::PanicInfo<'a>) -> Self { + let Self(mut message, _) = value.payload().into(); + + if let Some(loc) = value.location() { + message = append_location(message.into_owned(), *loc).into(); + } - Self(message.into(), Backtrace::capture()) + Self(message, Backtrace::capture()) } } From 60a14b31418ce20d4ea19de4041061547d9ac46f Mon Sep 17 00:00:00 2001 From: Target-san Date: Thu, 12 Dec 2024 22:47:11 +0200 Subject: [PATCH 22/27] Separate paths ofr handle-panics enabled and disabled --- proptest/src/test_runner/runner.rs | 33 +++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/proptest/src/test_runner/runner.rs b/proptest/src/test_runner/runner.rs index e1511e35..4accd2e6 100644 --- a/proptest/src/test_runner/runner.rs +++ b/proptest/src/test_runner/runner.rs @@ -225,6 +225,28 @@ where { use std::panic::{self, AssertUnwindSafe}; + #[cfg(feature = "handle-panics")] + fn run_case(case: impl FnOnce() -> TestCaseResult) -> TestCaseResult { + let mut reason = None; + unwrap_or!( + super::scoped_panic_hook::with_hook( + |panic_info| { reason = Some(panic_info.into()); }, + || panic::catch_unwind(AssertUnwindSafe(case)) + ), + _panic => Err(TestCaseError::Fail(reason.expect( + "Reason should have been obtained from panic hook" + ))) + ) + } + + #[cfg(not(feature = "handle-panics"))] + fn run_case(case: impl FnOnce() -> TestCaseResult) -> TestCaseResult { + unwrap_or!( + panic::catch_unwind(AssertUnwindSafe(case)), + panic => Err(TestCaseError::Fail(panic.as_ref().into())) + ) + } + #[cfg(feature = "timeout")] let timeout = runner.config.timeout(); @@ -252,16 +274,7 @@ where #[cfg(feature = "timeout")] let time_start = std::time::Instant::now(); - let mut reason = None; - let mut result = unwrap_or!( - super::scoped_panic_hook::with_hook( - |panic_info| { reason = Some(panic_info.into()); }, - || panic::catch_unwind(AssertUnwindSafe(|| test(case))) - ), - _panic => Err(TestCaseError::Fail(reason.expect( - "Reason should have been obtained from panic hook" - ))) - ); + let mut result = run_case(|| test(case)); // If there is a timeout and we exceeded it, fail the test here so we get // consistent behaviour. (The parent process cannot precisely time the test From 9211183d373735f691eb0fe8ccdefeb927a754d5 Mon Sep 17 00:00:00 2001 From: Target-san Date: Thu, 12 Dec 2024 22:48:20 +0200 Subject: [PATCH 23/27] Run tests for handle-panics and backtrace --- .github/workflows/rust.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index c7c4aeb0..67ff3e95 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -46,6 +46,12 @@ jobs: run: cd proptest && cargo build --verbose - name: Run tests run: cd proptest && cargo test --verbose + - name: Run tests handle-panics + run: cd proptest && cargo test --verbose --features handle-panics + - name: Run tests backtrace + run: cd proptest && cargo test --verbose --features backtrace + - name: Run tests handle-panics+backtrace + run: cd proptest && cargo test --verbose --features "handle-panics backtrace" - name: Build coverage no-default-features if: ${{ matrix.build == 'stable' }} env: From d6b3009385f22c6588fadecd078b1213ecadc610 Mon Sep 17 00:00:00 2001 From: Target-san Date: Thu, 12 Dec 2024 23:04:29 +0200 Subject: [PATCH 24/27] Use old-style access to thread-local cells, to allow builds on pinned --- proptest/src/test_runner/scoped_panic_hook.rs | 38 ++++++++----------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/proptest/src/test_runner/scoped_panic_hook.rs b/proptest/src/test_runner/scoped_panic_hook.rs index fc03615f..dccf7190 100644 --- a/proptest/src/test_runner/scoped_panic_hook.rs +++ b/proptest/src/test_runner/scoped_panic_hook.rs @@ -34,37 +34,31 @@ thread_local! { } static INIT_ONCE: Once = Once::new(); -/// Default panic hook, the one which was present before installing scoped one -/// -/// NB: no need for external sync, value is mutated only once, when init is performed -static mut DEFAULT_HOOK: Option) + Send + Sync>> = - None; /// Replaces currently installed panic hook with `scoped_hook_dispatcher` once, /// in a thread-safe manner fn init() { INIT_ONCE.call_once(|| { let old_handler = take_hook(); - set_hook(Box::new(scoped_hook_dispatcher)); - unsafe { - DEFAULT_HOOK = Some(old_handler); - } + set_hook(Box::new(move |panic_info| { + if !scoped_hook_dispatcher(panic_info) { + old_handler(panic_info) + } + })); }); } /// Panic hook which delegates execution to scoped hook, /// if one installed, or to default hook -fn scoped_hook_dispatcher(info: &PanicInfo<'_>) { - let handler = SCOPED_HOOK_PTR.get(); - if !handler.is_null() { - // It's assumed that if container's ptr is not null, ptr to `FnMut` is non-null too. - // Correctness **must** be ensured by hook switch code in `with_hook` - let hook = unsafe { &mut *(*handler).0 }; - (hook)(info); - return; +fn scoped_hook_dispatcher(info: &PanicInfo<'_>) -> bool { + let handler = SCOPED_HOOK_PTR.with(Cell::get); + if handler.is_null() { + return false; } + // It's assumed that if container's ptr is not null, ptr to `FnMut` is non-null too. + // Correctness **must** be ensured by hook switch code in `with_hook` + let hook = unsafe { &mut *(*handler).0 }; + (hook)(info); - if let Some(hook) = unsafe { DEFAULT_HOOK.as_ref() } { - (hook)(info); - } + true } /// Executes stored closure when dropped struct Finally(Option); @@ -105,10 +99,10 @@ pub fn with_hook( // `mem::transmute` is needed due to borrow checker restrictions to erase all lifetimes mem::transmute(&mut panic_hook as *mut dyn FnMut(&PanicInfo<'_>)) },); - let old_tuple = SCOPED_HOOK_PTR.replace(&guard_tuple); + let old_tuple = SCOPED_HOOK_PTR.with(|c| c.replace(&guard_tuple)); // Old scoped hook **must** be restored before leaving function scope to keep it sound let _undo = Finally::new(|| { - SCOPED_HOOK_PTR.set(old_tuple); + SCOPED_HOOK_PTR.with(|c| c.set(old_tuple)); }); body() } From f7337f1c7b552b3357c31b9f8c952b8d3059ce3b Mon Sep 17 00:00:00 2001 From: Target-san Date: Thu, 12 Dec 2024 23:30:54 +0200 Subject: [PATCH 25/27] Fix some import inconsistencies --- proptest/src/arbitrary/_alloc/collections.rs | 1 + proptest/src/test_runner/reason.rs | 15 +++++++-------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/proptest/src/arbitrary/_alloc/collections.rs b/proptest/src/arbitrary/_alloc/collections.rs index 3d0c50bc..c6f3333c 100644 --- a/proptest/src/arbitrary/_alloc/collections.rs +++ b/proptest/src/arbitrary/_alloc/collections.rs @@ -19,6 +19,7 @@ use crate::std_facade::{ binary_heap, btree_map, btree_set, fmt, linked_list, vec, vec_deque, Arc, BTreeMap, BTreeSet, BinaryHeap, Box, LinkedList, Rc, Vec, VecDeque, }; +#[cfg(feature = "std")] use core::hash::Hash; use core::ops::{Bound, RangeInclusive}; diff --git a/proptest/src/test_runner/reason.rs b/proptest/src/test_runner/reason.rs index c472dc23..5db338ce 100644 --- a/proptest/src/test_runner/reason.rs +++ b/proptest/src/test_runner/reason.rs @@ -7,9 +7,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use core::any::Any; -use std::string::ToString; - use super::backtrace::Backtrace; use crate::std_facade::{fmt, Box, Cow, String}; @@ -134,15 +131,16 @@ impl From> for Reason { } #[cfg(feature = "std")] -impl<'a> From<&'a (dyn Any + Send)> for Reason { - fn from(value: &'a (dyn Any + Send)) -> Self { +impl<'a> From<&'a (dyn std::any::Any + Send)> for Reason { + #[inline(always)] + fn from(value: &'a (dyn std::any::Any + Send)) -> Self { + use std::string::ToString; + let message: String = value .downcast_ref::<&'static str>() .map(|s| s.to_string()) .or_else(|| value.downcast_ref::().map(|s| s.clone())) - .or_else(|| { - value.downcast_ref::>().map(|s| s.to_string()) - }) + .or_else(|| value.downcast_ref::>().map(|s| s.to_string())) .unwrap_or_else(|| "".to_string()); Self(message.into(), Backtrace::empty()) @@ -152,6 +150,7 @@ impl<'a> From<&'a (dyn Any + Send)> for Reason { #[cfg(feature = "std")] impl<'a, 'b> From<&'b std::panic::PanicInfo<'a>> for Reason { #[inline(always)] + #[track_caller] fn from(value: &'b std::panic::PanicInfo<'a>) -> Self { let Self(mut message, _) = value.payload().into(); From eb0ed008caed9670383266c00766e6a90a144fa0 Mon Sep 17 00:00:00 2001 From: Target-san Date: Fri, 17 Jan 2025 15:49:06 +0200 Subject: [PATCH 26/27] Forcibly pin transitive dev-depencency's version, to avoid MSRV conflict --- proptest-macro/Cargo.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/proptest-macro/Cargo.toml b/proptest-macro/Cargo.toml index dc47a12a..a38fe870 100644 --- a/proptest-macro/Cargo.toml +++ b/proptest-macro/Cargo.toml @@ -23,3 +23,6 @@ convert_case = "0.6" [dev-dependencies] insta = "1" prettyplease = "0.2" +# Transitive dependency of `insta` +# version 0.15.10 now requires Rust 1.66 which conflicts with `proptest`'s MSRV +console = "=0.15.8" From 3cbb8565f59ce370d5a2176cb214b05c9d15949f Mon Sep 17 00:00:00 2001 From: Target-san Date: Fri, 17 Jan 2025 16:36:23 +0200 Subject: [PATCH 27/27] Pin few more transitive deps, to conform to MSRV 1.65 --- proptest-macro/Cargo.toml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/proptest-macro/Cargo.toml b/proptest-macro/Cargo.toml index a38fe870..121326a2 100644 --- a/proptest-macro/Cargo.toml +++ b/proptest-macro/Cargo.toml @@ -23,6 +23,7 @@ convert_case = "0.6" [dev-dependencies] insta = "1" prettyplease = "0.2" -# Transitive dependency of `insta` -# version 0.15.10 now requires Rust 1.66 which conflicts with `proptest`'s MSRV +# Transitive dependency of `insta`, v0.15.10 requires MSRV 1.66 console = "=0.15.8" +# Transitive dependency of `insta`, v0.1.14+ requires some features unstable on Rust 1.65 +unicode-width = "=0.1.13"