Tracking issue for proptest_derive

[Centril]

This issue tracks things we need to do before shipping proptest_derive 0.1:

• Setup workspace
• Test proptest_derive with (Travis) CI
• Invite @AltSysrq as crate owner on crates.io
• Implement #[proptest(regex = "...")]
  • PR in Implement #[proptest(regex = "...")] #103.
• Review the base function of the macro... is there anything we want to change?
  • Should the base attribute be proptest or prop? or something else?
  • Are the "sub-attributes" appropriately named and do they function well?
    • weight (shorthand: w)
    • strategy
    • value
    • params
    • no_params
    • skip
    • no_bound
    • filter
• Write user facing documentation in the form of an mdBook akin to https://serde.rs/ or https://mcarton.github.io/rust-derivative/ (example: https://rust-lang-nursery.github.io/edition-guide/introduction.html).
  • Flesh out basic structure; should we include all of proptest or just proptest_derive for now?
  • Write the guide
• Issues
  • #[proptest(value = "4a")] is accepted by the macro but of course is ultimately invalid syntax.

[AltSysrq]

Thanks for putting this together.

I do think it'd make sense to move some of the current lib.rs docs to mdbook since that documentation has gotten a bit unweildy. I'll take a stab at that this weekend.

[Centril]

:)

I'll see if I can work on the base structure and some docs for the derive bits (particularly the error index...).

[AltSysrq]

Actually, would you mind letting me do the derive docs? I've found that writing docs is one of the better ways for me to fully learn a system.

[Centril]

Sure :)

I started on the "Error Index" a bit: https://gist.github.com/Centril/5be8f6383c888a16d75b8c46f4310e82
Maybe you could integrate that?

For writing the mdBook, I've been using mdbook watch --open on Firefox; It works really well.
I basically pasted the error messages in error.rs and started converting them one by one to markdown...

[AltSysrq]

One thing that occurred to me: It's fairly common to have proptest as a dev-dependency (especially for binaries) to not pull the extra weight in. This is possible with proptest-derive as it is right now, but ends up being a bit cumbersome and is something users would need to learn right out the gate. To illustrate with the code I was testing with:

#[derive(Debug)]
#[cfg_attr(test, derive(Arbitrary))]
struct _TheStruct {
    #[cfg_attr(test, proptest(value = "NonArbitrary"))]
    _a: NonArbitrary,
}

#[derive(Debug)]
struct NonArbitrary;

I'm wondering it it'd make sense to have the macro put #[cfg(test)] on every item it emits by default and have an attribute to disable that if desired, so we'd get something like

// In a crate that doesn't have a hard dependency on proptest
// It "just works"
#[derive(Debug, Arbitrary)]
struct _TheStruct {
  #[proptest(value = "NonArbitrary"))]
  _a: NonArbitrary,
}

#[derive(Debug)]
struct NonArbitrary;

// In a crate that wants to unconditionally export its `Arbitrary` implementation
// Need to opt out but otherwise fairly easy
#[derive(Debug, Arbitrary)]
#[proptest(always_derive)]
struct _TheStruct {
  #[proptest(value = "NonArbitrary"))]
  _a: NonArbitrary,
}

#[derive(Debug)]
struct NonArbitrary;

// In a crate that wants to export its `Arbitrary` implementation based on its own feature
// No explicit help for this, but you're in "advanced" territory anyway.
// It's a bit more cumbersome than in the status quo since you need to conditionally
// opt out of the implicit #[cfg(test)].
#[derive(Debug)]
#[cfg_attr(feature = "use_proptest", derive(Arbitrary))]
#[cfg_attr(feature = "use_proptest", proptest(always_derive))]
struct _TheStruct {
    #[cfg_attr(feature = "use_proptest", proptest(value = "NonArbitrary"))]
    _a: NonArbitrary,
}

#[derive(Debug)]
struct NonArbitrary;

[AltSysrq]

It looks like #[proptest(value = "4a")] is initially accepted but causes a compilation error later. I added this as a TODO to the description, but I haven't looked into how to address it yet or whether there's anything reasonable we can do about it.

[AltSysrq]

Started on the book: #104

[Centril]

One thing that occurred to me: It's fairly common to have proptest as a dev-dependency (especially for binaries) to not pull the extra weight in. This is possible with proptest-derive as it is right now, but ends up being a bit cumbersome and is something users would need to learn right out the gate. To illustrate with the code I was testing with:

I've been sorta thinking about this too; but I don't know if it can be made any better...

I'm wondering it it'd make sense to have the macro put #[cfg(test)] on every item it emits by default and have an attribute to disable that if desired, so we'd get something like

// In a crate that doesn't have a hard dependency on proptest
// It "just works"
#[derive(Debug, Arbitrary)]
struct _TheStruct {
  #[proptest(value = "NonArbitrary"))]
  _a: NonArbitrary,
}

#[derive(Debug)]
struct NonArbitrary;

So the problem here is phase ordering...
The compiler must be aware of #[proptest] so the dependency (not dev-dependency) on proptest_derive has to be there or otherwise the code won't compile at all (unknown attribute proptest etc. etc.). So I don't think it's possible to avoid #[cfg_attr(test, ...)] in this case if you want to avoid proptest_derive as a dependency in your crate (and the added compile time that incurs) :(
Once you use #[cfg_attr(test, derive(Arbitrary))] you can be sure that the implementation is also cfg(test) so then there's no added value in putting the attribute on the implementation.

The only thing I could think of that would technically work right now is to have some cargo feature for proptest_derive that when activated will strip out all of proptest_derive's dependencies in cargo and then have a dummy proptest_derive crate that does nothing (besides registering itself and the attribute proptest).

Another thing worth trying is whether you can detect cfg(test) in a build.rs file (in proptest_derive) and then do the stripping. That might be more automatic, but that also makes it more difficult to intentionally "always_derive".

[Centril]

It looks like #[proptest(value = "4a")] is initially accepted but causes a compilation error later. I added this as a TODO to the description, but I haven't looked into how to address it yet or whether there's anything reasonable we can do about it.

I went and checked the source of this and it appears that in extract_expr we have:

    match lit {
        Lit::Str(lit) => lit.parse().ok(),
        ...
    }

I made a file src/tests/sandbox.rs:

#[macro_use]
extern crate proptest_derive;

#[derive(Debug, Arbitrary)]
struct Foo {
    #[proptest(value = "4a")]
    bar: u8,
}

and changed the above match variant to:

    match lit {
        Lit::Str(lit) => {

            println!("{:#?}", lit);
            println!("{:#?}", lit.parse::<Expr>());

            lit.parse().ok()
        },
        ...
    }

and ran cargo test --test sandbox and then I got:

λ cargo test --test sandbox
   Compiling proptest-derive v0.1.0 (D:\programming\proptest\proptest\proptest-derive)
LitStr {
    token: Literal {
        lit: Str_(
            4a
        ),
        suffix: None,
        span: #0 bytes(467..471)
    }
}
Ok(
    Lit(
        ExprLit {
            attrs: [],
            lit: Int(
                LitInt {
                    token: Literal {
                        lit: Integer(
                            4
                        ),
                        suffix: Some(
                            a
                        ),
                        span: #0 bytes(467..471)
                    }
                }
            )
        }
    )
)
error: invalid suffix `a` for numeric literal
  --> proptest-derive\tests\sandbox.rs:14:24
   |
14 |     #[proptest(value = "4a")]
   |                        ^^^^
   |
   = help: the suffix must be one of the integral types (`u32`, `isize`, etc)

error: proc-macro derive produced unparseable tokens
  --> proptest-derive\tests\sandbox.rs:12:17
   |
12 | #[derive(Debug, Arbitrary)]
   |                 ^^^^^^^^^

error: aborting due to 2 previous errors

error: Could not compile `proptest-derive`.

To learn more, run the command again with --verbose.

We could check for this situation by seeing if we get IntSuffix::None from LitInt::suffix.

However, it seems to me that the error message:

error: invalid suffix `a` for numeric literal
  --> proptest-derive\tests\sandbox.rs:14:24
   |
14 |     #[proptest(value = "4a")]
   |                        ^^^^
   |
   = help: the suffix must be one of the integral types (`u32`, `isize`, etc)

is pretty good... It points the user to the source of the error and explains it well... and so the added compile time to check this may not be worth it (it's not likely to occur often also...)?

[AltSysrq]

The only thing I could think of that would technically work right now is to have some cargo feature for proptest_derive that when activated will strip out all of proptest_derive's dependencies in cargo and then have a dummy proptest_derive crate that does nothing (besides registering itself and the attribute proptest).

Do proc macros and their dependencies actually get included in the output binary? I assumed they were just loaded into the compiler and so having proptest-dirve in [dependencies] wouldn't be an issue.

[AltSysrq]

However, it seems to me that the error message:
…
is pretty good... It points the user to the source of the error and explains it well... and so the added compile time to check this may not be worth it (it's not likely to occur often also...)?

Yeah, I'm fine with leaving it. I mainly wasn't sure if "leaking" the invalid syntax pointed to some other kind of problem.

[Centril]

@AltSysrq I don't think they get included in the binary no, but proptest itself will tho. The main problem with proptest_derive in this situation is the added compile times for compiling proptest_derive itself. What the stripping would do is make the macro compile quickly and then there are no references to proptest in the code of the person using it, so maybe the compiler can eliminate the code size hit (not sure tho).

[AltSysrq]

The proptest crate wouldn't need to get included since it could be in [dev-dependencies] and any extern crate declarations for it could also have #[cfg(test)].

The compile time point doesn't seem like a huge deal to me personally since the dependencies are fairly light (the whole thing takes 16s to build on my machine), but anyone who disagrees can still do the #[cfg_attr(test, derive(Arbitrary))] stuff themselves. (Also, the set of people who care about the compile time but don't run the tests seems like it is probably quite small.)

[Centril]

The proptest crate wouldn't need to get included since it could be in [dev-dependencies] and any extern crate declarations for it could also have #[cfg(test)].

(extern crate isn't necessary anymore with Rust 2018)

Oh I see; this could work I think.

In this case I think perhaps we should use a cargo feature tho (because it is simplest / least work needed to flick the switch) to either opt into gating on cfg(test) behavior or opt out of it. I think my preference is opt-in right now because the behavior could be surprising / it's not what I think people might expect.

The cargo feature seems like something you could do by just changing a few lines in Cargo.toml + proptest-derive/src/lib.rs (i.e. if cfg(test) isn't enabled, we just replace the proc macro function with one that outputs literally an empty TokenStream). This should also help with the compile times, at least I think so (because you can eliminate the dependencies of proptest_derive in Cargo.toml).

[Eh2406]

Just a thought, we should keep in mind that one use of proptest_derive will be adding Arbitrary to types that get exported of a library. So we want it to be possible to impl Arbitrary even when we are not the crate under test. (witch is what cfg(test) tells you.)

[Nemo157]

So we want it to be possible to impl Arbitrary even when we are not the crate under test.

This is exactly the situation I'm currently in as I'm using proptest for integration tests. I think it would be worth having a section in the book you're planning on how to do this effectively, and how to ensure it composes well with other crates that also support proptest (i.e. standardising on a feature name for enabling it, preferably just proptest but I don't know if there's a reason it would need to be a configurable feature like use_proptest).

[AltSysrq]

proptest-derive 0.1.0 is now available. There's still some outstanding things we'd like to improve, such as #106, but I think it makes sense to consider this particular issue complete.