Nightly detection does not take into account whether features can actually be used

[jonhoo]

Currently, the build script just checks whether the user is using nightly, but that doesn't work when the user's configuration, other flags, or the rustc wrapper, prevent the use of the relevant features:

cargo new cookie-issue
cd cookie-issue
mkdir .cargo
echo '[build]' >> .cargo/config.toml
echo 'rustflags = ["-Zallow-features="]' >> .cargo/config.toml
echo 'cookie = "0.15"' >> Cargo.toml
rustup override set nightly
cargo check

produces

   Compiling cookie v0.15.0
error[E0725]: the feature `proc_macro_span` is not in the list of allowed features
  --> /Users/jongje/.cargo/registry/src/github.com-1ecc6299db9ec823/proc-macro2-1.0.27/src/lib.rs:82:59
   |
82 | #![cfg_attr(any(proc_macro_span, super_unstable), feature(proc_macro_span))]
   |                                                           ^^^^^^^^^^^^^^^

error[E0658]: use of unstable library feature 'proc_macro_span'
   --> /Users/jongje/.cargo/registry/src/github.com-1ecc6299db9ec823/proc-macro2-1.0.27/src/wrapper.rs:517:72
    |
517 |             (Span::Compiler(a), Span::Compiler(b)) => Span::Compiler(a.join(b)?),
    |                                                                        ^^^^
    |
    = note: see issue #54725 <https://github.com/rust-lang/rust/issues/54725> for more information
    = help: add `#![feature(proc_macro_span)]` to the crate attributes to enable
...

The proc-macro2 crate specifically tries to handle this by parsing RUSTFLAGS if set, though that currently also runs into issues (dtolnay/proc-macro2#290). The anyhow crate uses a "compile probe", which is very robust, but currently suffers from lack of information from Cargo (dtolnay/anyhow#156). But once rust-lang/cargo#9601 lands, we'll have access to the necessary configuration information to check for feature support using either of those two approaches.

I filed this here (in addition to proc-macro2) because once proc-macro2 is fixed, cookie will start breaking with the same reproduction steps due to this build.rs:
https://github.com/SergioBenitez/cookie-rs/blob/434721e1f9fdf9bdb6ec6d760918e1c7390c9a93/build.rs#L2-L3

[jonhoo]

This can also be seen if you adjust the above to -Zallow-features=proc_macro_span, in which case you get:

$ cargo check
...
error[E0725]: the feature `doc_cfg` is not in the list of allowed features
  --> /Users/jongje/.cargo/registry/src/github.com-1ecc6299db9ec823/cookie-0.15.0/src/lib.rs:77:30
   |
77 | #![cfg_attr(nightly, feature(doc_cfg))]
   |                              ^^^^^^^

[SergioBenitez]

So this occurs when a crate 1) uses a build of rustc that allows features, but 2) disables all but some of the features, in particular, features that would be required to build its own dependencies, is that right?

This seems...totally weird to me, to the same effect as the RUSTC_BOOTSTRAP=1 variable. That is, to support such a use-case seems ill-advised. Why can't that same project just enable (in this case) doc_cfg? The project is already opting into unstable features, so this doesn't seem like a stretch.

In any case, I don't generally like the idea of probing. Really, there's obviously something very broken here with crates needing to figure out if/when they can do things. Probing for a feature doesn't mean that that feature works the way you expect it to. Feature implementations can change, for example, or the implementation can be broken in one nightly and working in another. We're just trading one set of edge-cases for another.

For this particular issue, we can simply gate enabling the feature on doc so that it's only enabled when docs are being built for this crate. Thus, dependents can build cookie always, with no features.

[jonhoo]

The use-case for this is when consumers want to test a particular feature that's been vetted and risk-assessed, but don't want to opt into all unstable features more broadly. There's a bunch more discussion over in this internals thread. In my case, I am constrained to using stable Rust from an operational risk perspective, but also specifically need the patch-in-config feature and want to provide as much evidence I can to help that feature stabilize. This means allowing that, and only that feature, so RUSTC_BOOTSTRAP=1 + -Zallow-features=patch_in_config. It's not great, but it's what the Rust ecosystem provides at the moment, and for the foreseeable future.

Restricting the use of this feature only to doc helps, but only marginally. It will still prevent users in such environments (which likely means most large-scale commercial code-bases) from generating documentation for cookie, when there's no material reason why they should be prevented from doing so. I believe it will also fail to document any crate that depends on cookie unless the user specifically passes in --no-deps, unless I'm misremembering when cfg(doc) gets set.

I agree with you that there's a bad smell coming from the need for crates to "sniff" for nightly features in their build scripts, but arguably that's just a side-effect of trying to use an unstable feature, which is, inherently, unstable. In my opinion at least, it's up to the build scripts that add such sniffing to do the best they can to make the sniffing as accurate as they can, rather than just blanket checking for "nightly". That's the cost of trying to depend on unstable features. You're right that in practice you can't truly probe for "the feature works exactly the way I need", but there's a far stretch from that to throwing up our collective hands and saying we're not even going to try. A compile probe is much better than just checking for nightly, and much less likely to enable the feature when it cannot be, and I think it's a meaningful improvement.

[SergioBenitez]

Restricting the use of this feature only to doc helps, but only marginally. It will still prevent users in such environments (which likely means most large-scale commercial code-bases) from generating documentation for cookie, when there's no material reason why they should be prevented from doing so.

You can generate documentation for cookie by using the stable or beta compiler instead of a makeshift nightly compiler. (Or, of course, by enabling doc_cfg) To be sure, cookie is certainly not preventing anyone from documenting the crate.

I believe it will also fail to document any crate that depends on cookie unless the user specifically passes in --no-deps, unless I'm misremembering when cfg(doc) gets set.

This might be the case for library dependents, but not for binary dependents.

I agree with you that there's a bad smell coming from the need for crates to "sniff" for nightly features in their build scripts, but arguably that's just a side-effect of trying to use an unstable feature, which is, inherently, unstable. In my opinion at least, it's up to the build scripts that add such sniffing to do the best they can to make the sniffing as accurate as they can, rather than just blanket checking for "nightly". That's the cost of trying to depend on unstable features.

Why doesn't this same logic apply to your environment, and doubly so? Your environment is using a flag, RUSTC_BOOTSTRAP for a completely unintended purpose as well as an unstable command line argument -Zallow-features. It is unreasonable, in my opinion, to expect the entire ecosystem to work around issues created as a result of an unsupported toolchain.

You're right that in practice you can't truly probe for "the feature works exactly the way I need", but there's a far stretch from that to throwing up our collective hands and saying we're not even going to try. A compile probe is much better than just checking for nightly, and much less likely to enable the feature when it cannot be, and I think it's a meaningful improvement.

In a vacuum, I agree. But doing this means that we literally have to compile a program before we even begin to build the library. Every crate that transitively depends on cookie is now passed on this cost. This is expensive, and the gain (for cookie) is that its documentation can be built with a custom compiler with unstable feature flags. This trade-off does not seem worth it to me. It's too easy to invoke cargo +stable doc or add doc_cfg, a feature flag that doesn't effect rustc itself and thus carries little to no risk.

[jonhoo]

I'd like to start by saying that this is definitely a thorny issue in the Rust ecosystem with not a lot of great options available to either party. And that's part of what we're running into here.

You can generate documentation for cookie by using the stable or beta compiler instead of a makeshift nightly compiler. (Or, of course, by enabling doc_cfg) To be sure, cookie is certainly not preventing anyone from documenting the crate.

That is true for anyone with free choice of what toolchain configuration they're using. In many larger organizations that want (for good reasons I would argue) strict control over what toolchains and unstable features are used, that will not be the case. And while the user may have an escape hatch to force a particular toolchain, that would mean working around the toolchain they are "stuck" with. Organizational uses are not the only place where this comes up though — you could also imagine projects that have to pin a nightly for unrelated reasons, and also get tripped up on this nightly check not being able to realize that their nightly can't support the feature in question.

I agree with you that there's a bad smell coming from [..]. That's the cost of trying to depend on unstable features.

Why doesn't this same logic apply to your environment, and doubly so? Your environment is using a flag, RUSTC_BOOTSTRAP for a completely unintended purpose as well as an unstable command line argument -Zallow-features. It is unreasonable, in my opinion, to expect the entire ecosystem to work around issues created as a result of an unsupported toolchain.

I think RUSTC_BOOTSTRAP is actually irrelevant here. It's true that in my case I'm using that, but the argument applies just the same for a regular nightly toolchain. -Zallow-features must, by it very nature, be unstable — it's entire job revolves around limiting what unstable features are permitted, and so it wouldn't make sense to use in a situation where unstable features aren't accessible anyway. It is perma-unstable, even though its interface is (to my knowledge at least) essentially stable. -Zallow-features exists for the express purpose of being able to restrict which unstable features to opt into, rather than being forced into an all or nothing choice, and that is a very valuable feature for anyone concerned about blanket allowing anything and everything unstable just because they need some unstable feature. Which, I assume, is also why proc-macro2 explicitly supports -Zallow-features.

But doing this means that we literally have to compile a program before we even begin to build the library. Every crate that transitively depends on cookie is now passed on this cost.

It adds a cost, that's definitely true. Though given that the crate isn't recompiled for each dependent, the cost is somewhat mitigated. The fact that anyhow does it also suggests that in practice the cost isn't prohibitive, though I also agree with you that we shouldn't introduce features just because they're "perhaps not noticeably expensive". If you consider that a blocking problem, then I would suggest adopting the same path as proc-macro2, which is to explicitly check for -Zallow-features in RUSTFLAGS, as it carries no build-time cost and is still fairly reliable. It adds some cognitive cost, that's true, but it also reduces cognitive cost for anyone running into this issue and having to debug it, so on balance it likely evens out.

This is expensive, and the gain (for cookie) is that its documentation can be built with a custom compiler with unstable feature flags. This trade-off does not seem worth it to me. It's too easy to invoke cargo +stable doc or add doc_cfg, a feature flag that doesn't effect rustc itself and thus carries little to no risk.

We disagree here — the upside is that developers who a) are forced to use particular toolchain configurations; or b) must use nightly and want to restrict their reliance on unstable with -Zallow-features; are still able to build the documentation for cookie or anything that depends on it. That is a decently sized win for a small cost (using the proc-macro2 approach) at least in my mind.

[SergioBenitez]

The fact that anyhow does it also suggests that in practice the cost isn't prohibitive, though I also agree with you that we shouldn't introduce features just because they're "perhaps not noticeably expensive". If you consider that a blocking problem, then I would suggest adopting the same path as proc-macro2, which is to explicitly check for -Zallow-features in RUSTFLAGS, as it carries no build-time cost and is still fairly reliable.

I think this is a fair compromise. Thanks for suggesting it. I'll be implementing it soon.

Edit: Oh, I see, this isn't actually possible until rust-lang/cargo#9601 lands. Well alright. This really seems like whack-a-mole now when the correct solution to this (and all other version query related problems) is for rustc itself to have an interface by which it can be asked support-related questions. Nevertheless, I'll implement support in version_check when these new environment variables land and forward that support here once its available.

[jonhoo]

Edit: Oh, I see, this isn't actually possible until rust-lang/cargo#9601 lands. Well alright. This really seems like whack-a-mole now when the correct solution to this (and all other version query related problems) is for rustc itself to have an interface by which it can be asked support-related questions.

Heh, yeah, it really is. A long dependency chain brought me here 😅 And I actually had the same though as you — it'd be really nice if there was a cargo caniuse "feature(doc_cfg)" that just exited with 0 if you can, and 1 if you can't (and similarly for rustc). Cc @cuviper who has thought about this problem before too.

Nevertheless, I'll implement support in version_check when these new environment variables land and forward that support here once its available.

❤️

[jonhoo]

rust-lang/cargo#9601 has now landed, and the new environment variable CARGO_ENCODED_RUSTFLAGS is now available. It is a \x1f-separated list of flags that can be walked to look for -Zallow-features — see the fix to proc-macro2 in dtolnay/proc-macro2#294.

[RalfJung]

And I actually had the same though as you — it'd be really nice if there was a cargo caniuse "feature(doc_cfg)" that just exited with 0 if you can, and 1 if you can't (and similarly for rustc).

FWIW https://github.com/cuviper/autocfg provides basically that. It doesn't seem to support probing for feature flags, but that should be easy to add?

EDIT: Ah looks like cuviper/autocfg#35 has been sitting around form a while...