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

fix: functions are now unique types and require casting to function pointers #46

Merged
merged 1 commit into from
Dec 24, 2014

Conversation

japaric
Copy link
Contributor

@japaric japaric commented Dec 23, 2014

See rust-lang/rust#19891


To test locally I have used this fork of collect-rs (Gankra/collect-rs#39), since the original one is still broken with today's nightly.

The macro source code could probably use better naming...

@BurntSushi
Copy link
Owner

OMG. Casting to the right function type for every property call is a major pain. Is there really no other way?

@japaric
Copy link
Contributor Author

japaric commented Dec 23, 2014

@BurntSushi Actually, I think there is another way. If we do impl Testable for F where F: Fn(..) -> bool, etc. I think this may work without any casting, let me try.

@japaric
Copy link
Contributor Author

japaric commented Dec 23, 2014

Good news and bad news.

The good news is that, in theory, if we add this implementation:

impl<T, Args, Result, F> Testable for F where
    F: Fn<Args, Result>,
    Args: AShow,
    Result: Testable,
{
    fn result<G: Gen>(&self, g: &mut G) -> TestResult {
        // this definition may not be 100% correct
        let args = arby(g);
        safe(|| self.call(args)).result(g)
    }
}

And because of auto-tupling, that should let us use functions of "any" arity with quickcheck without casting to a function pointer (i.e. without the as fn() -> bool), furthermore quickcheck would also work with unboxed closures: quickcheck(|| TestResult::Passed()).

(Actually, this "any" arity is bounded by the implementations of Arbitratry on tuples, so right now up to function with 12 arguments)

The bad news is that because of rust-lang/rust#18835, that blanket implementation collides with the other "specific" implementations (e.g. impl Testable for bool), therefore this is not doable right now :-(.

Would it make sense to land this PR as an interim solution until rust-lang/rust#18835 gets fixed? That issue is a stdlib stabilization blocker so should get fixed relatively soon. Also the ergonomics of the #[quickcheck] macro remains unchanged, and I think that's way more used that the quickcheck function. (As a data point, I exclusively use the macro in my libraries, and that's 1K+ unit tests)

@BurntSushi
Copy link
Owner

That's some really good news---especially the bit about it working with functions of arbitrary arity! Assuming that bug gets fixed. :-)

Would it make sense to land this PR as an interim solution until rust-lang/rust#18835 gets fixed? That issue is a stdlib stabilization blocker so should get fixed relatively soon. Also the ergonomics of the #[quickcheck] macro remains unchanged, and I think that's way more used that the quickcheck function. (As a data point, I exclusively use the macro in my libraries, and that's 1K+ unit tests)

Agreed. We should definitely keep the build working. Did the travis test fail because the nightly hasn't been updated yet?

Also, with respect to the #[quickcheck] macro, I've basically been ignoring it because it won't work on Rust 1.0. It's a syntax extension, which won't be stable at Rust 1.0. So I've converted all my uses of QuickCheck to go without the macro. :-(

@BurntSushi
Copy link
Owner

Also, in the spirit of rustc's convention, could you add a [breaking-change] message to the commit? Thanks. :-)

…ointers

This is a [breaking-change] for users of the `quickcheck` function, you'll need
to explicitly cast the bare function to a function pointer. For example:

``` rust
fn prop(xs: Vec<int>) -> bool { /* .. */ }

// Change this line
quickcheck(prop)
// into this
quickcheck(prop as fn(Vec<int>) -> bool)
```
@japaric
Copy link
Contributor Author

japaric commented Dec 23, 2014

Did the travis test fail because the nightly hasn't been updated yet?

It failed because the collect-rs dependencency hasn't been updated to work with today nightly (see Gankra/collect-rs#39)

Also, in the spirit of rustc's convention, could you add a [breaking-change] message to the commit?

Done!

BurntSushi added a commit that referenced this pull request Dec 24, 2014
fix: functions are now unique types and require casting to function pointers
@BurntSushi BurntSushi merged commit 9a97e07 into BurntSushi:master Dec 24, 2014
@BurntSushi
Copy link
Owner

@japaric Thank you so much! And extra thanks for investigating the function cast issue. :-)

@BurntSushi
Copy link
Owner

I'll publish a new release as soon as @gankro publishes a new release of collect-rs. :-)

@Gankra
Copy link

Gankra commented Dec 24, 2014

@BurntSushi Done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants