Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Send bound for Arbitrary? #262

Closed
benwr opened this issue Dec 5, 2020 · 2 comments
Closed

Remove Send bound for Arbitrary? #262

benwr opened this issue Dec 5, 2020 · 2 comments
Labels

Comments

@benwr
Copy link

benwr commented Dec 5, 2020

In my current project, I've got a complicated data structure I'd like to make an Arbitrary instance for; unfortunately the implementation relies on Rc for performance reasons, so it isn't Send.

I've considered using an ad-hoc structure, like a Vec or something, and generating the data structure in the test from that, but interpreting results of this test would be really annoying.

My other option at the moment is generating two versions of the data structure, one using Rc and the other using Arc. But this is also annoying to do nicely in Rust, and everything that depends on this data structure will need to do the "cfg(test)" dance to choose whether to use the thread-safe version or the other one.

So my question is: Instead of Sending generated values to threads, could quickcheck send a random seed, and then have the thread itself generate the value? I imagine this has significant tradeoffs, but it would make it much easier to test libraries that otherwise don't want to be thread-safe.

Edited to add: I might be willing to spend a significant amount of time working on this if it turns out to be compatible with your goals for the project.

BurntSushi added a commit that referenced this issue Dec 5, 2020
The Send bound is a relic from the past. Indeed, the docs for the
Arbitrary trait have been outdated for quite some time. quickcheck
stopped running each test in a separate thread once
`std::panic::catch_unwind` was stabilized many moons ago. With
`catch_unwind`, the `Send` bound is no longer necessary.

We do need to retain the `'static` bound though. Without that,
implementing shrink seems implausible.

Fixes #262
@BurntSushi
Copy link
Owner

@benwr So it turns out that the Send bound is not necessary at all. I should have removed it long ago, but didn't because I overlooked it. QuickCheck stopped running tests in separate threads when std::panic::catch_unwind became available. It only used separate threads at the time because it was the only way to recover from a panic.

I opened #263 that removes the Send bound. Quite an easy change! The only thing preventing me from merging it at the moment is that it's a breaking change. I'd like to incorporate a few other breaking changes before the next semver release, but I'm not sure when I'll get to that unfortunately...

@benwr
Copy link
Author

benwr commented Dec 5, 2020

Ah, that's awesome! Thanks for quick response! Excited to see it land!

@BurntSushi BurntSushi added the bug label Dec 27, 2020
BurntSushi added a commit that referenced this issue Dec 27, 2020
The Send bound is a relic from the past. Indeed, the docs for the
Arbitrary trait have been outdated for quite some time. quickcheck
stopped running each test in a separate thread once
`std::panic::catch_unwind` was stabilized many moons ago. With
`catch_unwind`, the `Send` bound is no longer necessary.

We do need to retain the `'static` bound though. Without that,
implementing shrink seems implausible.

Fixes #262, Closes #263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants