From 70498b6cdba9d57fdd03dcb000b52b543bf1e0d9 Mon Sep 17 00:00:00 2001 From: Norbert Zeh Date: Wed, 27 Jul 2022 20:44:44 -0300 Subject: [PATCH 1/4] Remove half-open float ranges as Strategies The tests didn't pass, and fixing them reliably seems impossible. --- proptest/src/num.rs | 90 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 73 insertions(+), 17 deletions(-) diff --git a/proptest/src/num.rs b/proptest/src/num.rs index e4a0994a..108815b4 100644 --- a/proptest/src/num.rs +++ b/proptest/src/num.rs @@ -53,7 +53,7 @@ macro_rules! int_any { }; } -macro_rules! numeric_api { +macro_rules! int_api { ($typ:ident, $epsilon:expr) => { impl Strategy for ::core::ops::Range<$typ> { type Tree = BinarySearch; @@ -137,6 +137,40 @@ macro_rules! numeric_api { }; } +macro_rules! float_api { + ($typ:ident, $epsilon:expr) => { + impl Strategy for ::core::ops::Range<$typ> { + type Tree = BinarySearch; + type Value = $typ; + + fn new_tree(&self, runner: &mut TestRunner) -> NewTree { + Ok(BinarySearch::new_clamped( + self.start, + $crate::num::sample_uniform(runner, self.clone()), + self.end - $epsilon, + )) + } + } + + impl Strategy for ::core::ops::RangeInclusive<$typ> { + type Tree = BinarySearch; + type Value = $typ; + + fn new_tree(&self, runner: &mut TestRunner) -> NewTree { + Ok(BinarySearch::new_clamped( + *self.start(), + $crate::num::sample_uniform_incl( + runner, + *self.start(), + *self.end(), + ), + *self.end(), + )) + } + } + }; +} + macro_rules! signed_integer_bin_search { ($typ:ident) => { #[allow(missing_docs)] @@ -234,7 +268,7 @@ macro_rules! signed_integer_bin_search { } } - numeric_api!($typ, 1); + int_api!($typ, 1); } }; } @@ -322,7 +356,7 @@ macro_rules! unsigned_integer_bin_search { } } - numeric_api!($typ, 1); + int_api!($typ, 1); } }; } @@ -842,7 +876,7 @@ macro_rules! float_bin_search { } } - numeric_api!($typ, 0.0); + float_api!($typ, 0.0); } }; } @@ -1032,7 +1066,7 @@ mod test { } mod contract_sanity { - macro_rules! contract_sanity { + macro_rules! contract_sanity_int { ($t:tt) => { mod $t { use crate::strategy::check_strategy_sanity; @@ -1067,18 +1101,40 @@ mod test { } }; } - contract_sanity!(u8); - contract_sanity!(i8); - contract_sanity!(u16); - contract_sanity!(i16); - contract_sanity!(u32); - contract_sanity!(i32); - contract_sanity!(u64); - contract_sanity!(i64); - contract_sanity!(usize); - contract_sanity!(isize); - contract_sanity!(f32); - contract_sanity!(f64); + + macro_rules! contract_sanity_float { + ($t:tt) => { + mod $t { + use crate::strategy::check_strategy_sanity; + + const FOURTY_TWO: $t = 42 as $t; + const FIFTY_SIX: $t = 56 as $t; + + #[test] + fn range() { + check_strategy_sanity(FOURTY_TWO..FIFTY_SIX, None); + } + + #[test] + fn range_inclusive() { + check_strategy_sanity(FOURTY_TWO..=FIFTY_SIX, None); + } + } + }; + } + + contract_sanity_int!(u8); + contract_sanity_int!(i8); + contract_sanity_int!(u16); + contract_sanity_int!(i16); + contract_sanity_int!(u32); + contract_sanity_int!(i32); + contract_sanity_int!(u64); + contract_sanity_int!(i64); + contract_sanity_int!(usize); + contract_sanity_int!(isize); + contract_sanity_float!(f32); + contract_sanity_float!(f64); } #[test] From 5a27dddcbc5fa490512d91916e63a6379f52cf53 Mon Sep 17 00:00:00 2001 From: Norbert Zeh Date: Wed, 27 Jul 2022 20:45:15 -0300 Subject: [PATCH 2/4] Fix test failure in proptest-derive and switch to newest compiletest_rs --- proptest-derive/Cargo.toml | 2 +- proptest-derive/tests/compile-fail/no-arbitrary.rs | 4 ++-- proptest-derive/tests/compiletest.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/proptest-derive/Cargo.toml b/proptest-derive/Cargo.toml index 2a8fff3f..d5c06187 100644 --- a/proptest-derive/Cargo.toml +++ b/proptest-derive/Cargo.toml @@ -27,7 +27,7 @@ proptest = { version = "1.0.0", path = "../proptest" } # features. However, due to # https://github.com/laumann/compiletest-rs/issues/166, the default features of # compiletest-rs fail to compile, but the stable fallback works fine. -compiletest_rs = { version = "0.3.19", features = ["tmp", "stable"] } +compiletest_rs = { version = "0.8.0", features = ["tmp", "stable"] } # criterion is used for benchmarks. criterion = "0.2" diff --git a/proptest-derive/tests/compile-fail/no-arbitrary.rs b/proptest-derive/tests/compile-fail/no-arbitrary.rs index 676c7fa7..320cd7e0 100644 --- a/proptest-derive/tests/compile-fail/no-arbitrary.rs +++ b/proptest-derive/tests/compile-fail/no-arbitrary.rs @@ -13,5 +13,5 @@ fn main() {} struct T0; -#[derive(Debug, Arbitrary)] //~ Arbitrary` is not satisfied [E0277] -struct T1 { f0: T0, } +#[derive(Debug, Arbitrary)] //~ ERROR Arbitrary` is not satisfied [E0277] +struct T1 { f0: T0, } //~ ERROR Arbitrary` is not satisfied [E0277] diff --git a/proptest-derive/tests/compiletest.rs b/proptest-derive/tests/compiletest.rs index f8c50830..1198128a 100644 --- a/proptest-derive/tests/compiletest.rs +++ b/proptest-derive/tests/compiletest.rs @@ -17,7 +17,7 @@ fn run_mode(src: &'static str, mode: &'static str) { config.target_rustcflags = Some("-L ../target/debug/deps --edition=2018".to_owned()); if let Ok(name) = env::var("TESTNAME") { - config.filter = Some(name); + config.filters = vec![name]; } config.src_base = format!("tests/{}", src).into(); From 006a53a135bc8fd84eb4ef259c20878601c00cd9 Mon Sep 17 00:00:00 2001 From: Norbert Zeh Date: Thu, 28 Jul 2022 14:10:27 -0300 Subject: [PATCH 3/4] Implement improved shrinker for prop_flat_map The previous shrinker always shrunk the inner strategy first. This seems to be the wrong thing to do in almost all non-trivial situations (e.g., generate a vector length and then a pair of vectors of this length). The new shrinker shrinks the outer strategy first (e.g., tries to minimize the vector length first and then minimizes the entries in the vectors). Given the current implementation of shrinking via simplify/complicate, I had a hard time doing this without cloning the inner strategy before simplifying it. So there's a breaking change here because prop_flat_map now requires the closure to produce a strategy that implements Clone. This could have been avoided by a change in how simplify/complicate operate, but this has the potential of breaking much more code than adding the requirement that the inner strategy is clonable. --- proptest-derive/Cargo.toml | 4 +- proptest/Cargo.toml | 2 +- proptest/src/strategy/flatten.rs | 80 ++++++++++++++------------------ 3 files changed, 37 insertions(+), 49 deletions(-) diff --git a/proptest-derive/Cargo.toml b/proptest-derive/Cargo.toml index d5c06187..8735149d 100644 --- a/proptest-derive/Cargo.toml +++ b/proptest-derive/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "proptest-derive" -version = "0.3.0" +version = "0.3.1" authors = ["Mazdak Farrokhzad "] license = "MIT/Apache-2.0" readme = "README.md" @@ -22,7 +22,7 @@ edition = "2018" proc-macro = true [dev-dependencies] -proptest = { version = "1.0.0", path = "../proptest" } +proptest = { version = "1.1.0", path = "../proptest" } # We don't actually run the tests on stable since some of them use nightly # features. However, due to # https://github.com/laumann/compiletest-rs/issues/166, the default features of diff --git a/proptest/Cargo.toml b/proptest/Cargo.toml index d308f813..01ea98ee 100644 --- a/proptest/Cargo.toml +++ b/proptest/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "proptest" -version = "1.0.0" +version = "1.1.0" authors = ["Jason Lingle"] license = "MIT/Apache-2.0" readme = "README.md" diff --git a/proptest/src/strategy/flatten.rs b/proptest/src/strategy/flatten.rs index 63ac4160..58b1fc73 100644 --- a/proptest/src/strategy/flatten.rs +++ b/proptest/src/strategy/flatten.rs @@ -8,11 +8,11 @@ // except according to those terms. use crate::std_facade::{fmt, Arc}; -use core::mem; use crate::strategy::fuse::Fuse; use crate::strategy::traits::*; use crate::test_runner::*; +use std::mem; /// Adaptor that flattens a `Strategy` which produces other `Strategy`s into a /// `Strategy` that picks one of those strategies and then picks values from @@ -33,6 +33,7 @@ impl Flatten { impl Strategy for Flatten where S::Value: Strategy, + ::Tree: Clone, { type Tree = FlattenValueTree; type Value = ::Value; @@ -50,19 +51,7 @@ where { meta: Fuse, current: Fuse<::Tree>, - // The final value to produce after successive calls to complicate() on the - // underlying objects return false. - final_complication: Option::Tree>>, - // When `simplify()` or `complicate()` causes a new `Strategy` to be - // chosen, we need to find a new failing input for that case. To do this, - // we implement `complicate()` by regenerating values up to a number of - // times corresponding to the maximum number of test cases. A `simplify()` - // which does not cause a new strategy to be chosen always resets - // `complicate_regen_remaining` to 0. - // - // This does unfortunately depart from the direct interpretation of - // simplify/complicate as binary search, but is still easier to think about - // than other implementations of higher-order strategies. + last_complication: Option::Tree>>, runner: TestRunner, complicate_regen_remaining: u32, } @@ -77,7 +66,7 @@ where FlattenValueTree { meta: self.meta.clone(), current: self.current.clone(), - final_complication: self.final_complication.clone(), + last_complication: self.last_complication.clone(), runner: self.runner.clone(), complicate_regen_remaining: self.complicate_regen_remaining, } @@ -94,7 +83,7 @@ where f.debug_struct("FlattenValueTree") .field("meta", &self.meta) .field("current", &self.current) - .field("final_complication", &self.final_complication) + .field("last_complication", &self.last_complication) .field( "complicate_regen_remaining", &self.complicate_regen_remaining, @@ -112,7 +101,7 @@ where Ok(FlattenValueTree { meta: Fuse::new(meta), current: Fuse::new(current), - final_complication: None, + last_complication: None, runner: runner.partial_clone(), complicate_regen_remaining: 0, }) @@ -122,6 +111,7 @@ where impl ValueTree for FlattenValueTree where S::Value: Strategy, + ::Tree: Clone, { type Value = ::Value; @@ -130,33 +120,28 @@ where } fn simplify(&mut self) -> bool { + self.current.disallow_complicate(); + + if self.meta.simplify() { + if let Ok(v) = self.meta.current().new_tree(&mut self.runner) { + self.last_complication = Some(Fuse::new(v)); + mem::swap( + self.last_complication.as_mut().unwrap(), + &mut self.current, + ); + self.complicate_regen_remaining = self.runner.config().cases; + return true; + } else { + self.meta.disallow_simplify(); + } + } + self.complicate_regen_remaining = 0; + let mut old_current = self.current.clone(); + old_current.disallow_simplify(); if self.current.simplify() { - // Now that we've simplified the derivative value, we can't - // re-complicate the meta value unless it gets simplified again. - // We also mustn't complicate back to whatever's in - // `final_complication` since the new state of `self.current` is - // the most complicated state. - self.meta.disallow_complicate(); - self.final_complication = None; - true - } else if !self.meta.simplify() { - false - } else if let Ok(v) = self.meta.current().new_tree(&mut self.runner) { - // Shift current into final_complication and `v` into - // `current`. We also need to prevent that value from - // complicating beyond the current point in the future - // since we're going to return `true` from `simplify()` - // ourselves. - self.current.disallow_complicate(); - self.final_complication = Some(Fuse::new(v)); - mem::swap( - self.final_complication.as_mut().unwrap(), - &mut self.current, - ); - // Initially complicate by regenerating the chosen value. - self.complicate_regen_remaining = self.runner.config().cases; + self.last_complication = Some(old_current); true } else { false @@ -177,17 +162,20 @@ where } } - if self.current.complicate() { - return true; - } else if self.meta.complicate() { + if self.meta.complicate() { if let Ok(v) = self.meta.current().new_tree(&mut self.runner) { - self.complicate_regen_remaining = self.runner.config().cases; self.current = Fuse::new(v); + self.complicate_regen_remaining = self.runner.config().cases; return true; + } else { } } - if let Some(v) = self.final_complication.take() { + if self.current.complicate() { + return true; + } + + if let Some(v) = self.last_complication.take() { self.current = v; true } else { From 7866f6d67afd9790ec6d7ee085db510cefe1a181 Mon Sep 17 00:00:00 2001 From: Norbert Zeh Date: Thu, 28 Jul 2022 14:58:37 -0300 Subject: [PATCH 4/4] Update CHANGELOG --- proptest/CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/proptest/CHANGELOG.md b/proptest/CHANGELOG.md index 386aabcd..d0fbd6f5 100644 --- a/proptest/CHANGELOG.md +++ b/proptest/CHANGELOG.md @@ -1,3 +1,20 @@ +## Unreleased + +### Breaking Changes + +- `prop_flat_map` can only be used with closures that produce a strategy that + implements `Copy`. + +- Half-bounded ranges (`x..`, `..x`, and `..=x`) are no longer supported for + `f32` and `f64`. + +### New Features + +- The shrink strategy for `prop_flat_map` now shrinks the "outer" strategy + first (the one that produces the values given to `prop_flat_map`) and + shrinks the "inner" strategy" (the one produced by `prop_flat_map`) only + when the outer strategy cannot be shrunk further. + ## 1.0.0 ### Breaking Changes