proptest-derive: #[inline] + no_cfg Cargo feature gate

[Centril]

Changes:

• Formatting fix + rewording in proptest-derive/Cargo.toml.

• A default enabled cargo feature no_cfg is introduced in proptest-derive/Cargo.toml.

  • Unless no_cfg is active then #[cfg(test)] will be put on implementations (generated by #[derive(Arbitrary)]).

  • This entails that you can get the #[cfg(test)] behavior with default-features = false.

• #[inline] is put on arbitrary_with generated by #[derive(Arbitrary)]. This change was suggested by eddyb; the idea being that we don't do ahead-of-time codegen of the implementations produced.

• Tests for the no_cfg behavior above; in particular, we set default-features = false and ensure that Arbitrary isn't provided.

One drawback to this approach where cargo is used is that if one of the dependencies use no_cfg then all of them will; but I don't think this should be that much of a problem; I think it is better than the ergonomics hit you get by having to annotate each derive(Arbitrary) location instead.

cc #102, @Nemo157, @Eh2406

[Centril]

The PR is pretty much done and is working; The question now is whether this is the design we want.

( could probably use another test to ensure that when cfg(test) is active it works... )

[Nemo157]

I’ll try and give proptest-derive a spin tonight and see how I like this for my usecase.

[AltSysrq]

if one of the dependencies use no_cfg then all of them will

It does seem like a problem to me, unless I'm misunderstanding something.

Crate dep-a:

[dependencies]
proptest-derive = { version = "xx", default-features = false }

[dev-dependencies]
proptest = "xx"

Crate dep-b:

[dependencies]
proptest-derive = "xx"
proptest = "xx"

Crate bin:

[dependencies]
dep-a = "yy"
dep-b = "zz"

dep-b forces the no_cfg feature to be on everywhere. But now in dep-a #[derive(Arbitrary)] tries to reference proptest in the generated code, but the proptest crate is not available to dep-a.

[Centril]

Aw! bummer :(
You're right, this creates a big problem for the whole setup.

This problem could be fixed with:

#[cfg(all(
    any(test, feature = "prop"),
    accessible(::proptest::prelude::Arbitrary)
    // `dep-a` will not have `::proptest::*` accessible so you get no impl
    // and thus also no error.
)]
impl Arbitrary for Foo { ... }

but unfortunately we don't have accessible(...) yet.

At this point it might be best to just standardize on feature = "prop" and we can perhaps also have #[proptest(cfg)] which will add #[cfg(any(test, feature = "prop"))] on the impl...?

(We should also retain #[inline] from this PR...)

[AltSysrq]

At this point it might be best to just standardize on feature = "prop" and we can perhaps also have #[proptest(cfg)] which will add #[cfg(any(test, feature = "prop"))] on the impl...?

I'd still prefer it to "just work" for crates that don't want to / need to export their Arbitrary implementations, but I think this is going the right direction. (I agree that accessible is the ideal solution, sadly it's still far away).

New proposal:

• We emit #[cfg(any(test, feature = "proptest"))] unconditionally. This gets us "just works" for crates not exporting their implementations since a proptest feature does not ordinarily exist.

• Crates that wish to export their Arbitrary impls put proptest in [dependencies] and mark it optional = true. This brings into existence a proptest feature and means that downstream users of that library don't pay for proptest if they themselves don't use it.

[Centril]

I think the new proposal works even for those who want to do:

#[derive(Debug)]
#[cfg_attr(feature = "prop", derive(Arbitrary))]
struct Foo;

as well... so that is good.

cc @Nemo157 what do you think? does that work for you?

PS: I would not name the feature proptest tho because that would conflict with the crate proptest due to deficiencies in Cargo. prop is shorter and doesn't conflict so it should work; it's also fairly unique I think so that's also good.

[AltSysrq]

I would not name the feature proptest tho because that would conflict with the crate proptest

Is there something I don't know here? I was deliberately using feature == optional crate so that there's less to wire up.

[Nemo157]

@AltSysrq the problem is when you have a dependency that has its own optional proptest feature. You would want to enable it when a user enables your proptest feature, but you can’t override the meaning of features created by optional dependencies.

It may pay to talk to the serde team about this, they should have the same issue but they strongly recommend using the name serde anyway; maybe they have some ideas of how to improve on this.

[Centril]

Is there something I don't know here? I was deliberately using feature == optional crate so that there's less to wire up.

Oh so someone (crate "alpha") would do:

name = "alpha"

[dependencies]
proptest = { version = "x.y", optional = true }

and then someone else (crate "beta") would do:

name = "beta"

[dependencies]
# For the direct stuff we want to derive:
proptest = { version = "x.y", optional = true }

[dependencies.alpha]
version = "x.y"
features = ?
#^ How do we make: proptest imply alpha/proptest?

https://doc.rust-lang.org/cargo/reference/manifest.html#rules

Feature names must not conflict with other package names in the manifest. This is because they are opted into via features = [...], which only has a single namespace.

So we cannot do:

name = "beta"

[dependencies]
# For the direct stuff we want to derive:
proptest = { version = "x.y", optional = true }

[dependencies.alpha]
version = "x.y"

[features]
proptest = ["alpha/proptest"]

[AltSysrq]

Huh, that's pretty awkward. I'd be fine with doing prop for now then.

[Centril]

@AltSysrq yeah haha; we've been running into 3 different Rust language / Cargo walls (limitations) in a very short span of time. =P

I'll try to adapt the PR into the new proposal (but prop => proptest) in a bit.