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

RFC: #[cfg(accessible(..) / version(..))] #2523

Open
wants to merge 45 commits into
base: master
from

Conversation

Projects
None yet
@Centril
Contributor

Centril commented Aug 12, 2018

🖼️ Rendered

📝 Summary

Permit users to #[cfg(..)] on whether:

  • they have a certain minimum Rust version (#[cfg(version(1.27))]).
  • a certain external path is accessible (#[cfg(accessible(::std::mem::ManuallyDrop))]).

💖 Thanks

To @eddyb, @rpjohnst, @kennytm, @aturon, and @rkruppe for discussing the feature with me.
To @Mark-Simulacrum, @MajorBreakfast, @alexreg, @alercah, and @joshtriplett for reviewing the draft version of this RFC.

Centril added some commits Aug 10, 2018

@Centril Centril added the T-lang label Aug 12, 2018

@Centril Centril self-assigned this Aug 12, 2018

@Centril Centril changed the title RFC: #[cfg(accessible(..) / version(..) / nightly)] RFC: #[cfg(accessible(..) / version = ".." / nightly)] Aug 12, 2018

@Centril

This comment has been minimized.

Contributor

Centril commented Aug 12, 2018

cc @MajorBreakfast re. use case for futures. :)

@cuviper

This comment has been minimized.

Member

cuviper commented Oct 15, 2018

@fbstj FWIW, I have already been doing probing that like that in a few crates for a while. The autocfg crate is my new attempt to consolidate that build logic. :)

@aturon

This comment has been minimized.

Member

aturon commented Nov 9, 2018

@SimonSapin

In the second case, opting into instability should be a choice for users of the library. The mechanism we have for this today is Cargo feature flags, and it works fine. If a library automatically enables unstable features that might not actually be needed when it detects that the compiler happens to be Nightly, this is an anti-pattern. If I only use the stable subset of the library but still use Nightly for other reasons, when the library breaks in tomorrow’s Nightly it’s making my day worse unnecessarily.

I agree with this, and to me the same argument eliminates much of the argument for having accessible rather than just version: you shouldn't be using either of these until the given feature is stabilized and hence you know the version number.

Personally, I'd like to consider dropping this proposal down to just version numbers, at least to start, and see whether in practice we feed the need for the added complexity of accessibility.

@jrvidal

This comment has been minimized.

jrvidal commented Nov 9, 2018

I would much rather prefer fostering better user-land solutions, such as @cuviper's autocfg.

I don't think a strong case for the advantages of the built-in attributes has been presented, or considered thoroughly in the RFC text.

What including #[cfg(nightly)] and #[cfg(version = "..")] into the language itself does is:

  1. giving us a common language to talk about these things (as opposed to everyone inventing their own flags for the same feature).
  2. making build.rs files unnecessary in many cases.
  3. making it ergonomic to specify these things (the current situation is not ergonomic).
  4. giving us the opportunity to provide ecosystem-wide recommendations. People are going to use the ability provided by version_check anyways. By providing the mechanisms in-language, we can provide a few dos and don'ts for wider distribution.

(1) could be solved if a clear winner emerges in the ecosystem (maybe becoming part of the nursery). That would take care of (4) as well, although that's something could be addressed independently today by (maybe the libs team?) getting involved in those projects to provide such recommendations.

I'd argue that (3) is a statement about the current state of affairs of feature-detection in the ecosystem that ignores potential improvements. I think something like autocfg already makes things (in my opinion) way more ergonomic than they used to be compared to, say, bare version_check.

(I concede point (2) 😕)

Why not wait and see where this road takes us? Isn't this the preferred approach in general (i.e. build a macro before adding syntax; let user-land experiment, then nursery, then (maybe) language/std library)?

(Note how the discussion about stabilizing feature names becomes moot: the community could rally around a repository of commonly used "feature flags" wrapping a curated set of compile-time tests.)


Also: what about having the rustc version injected by Cargo itself? (rust-lang/cargo#2903, rust-lang/cargo#4408)

And a final spitball about point (2) above: what if Cargo itself had a notion of feature-detection, where it attempts to compile every file in .features/<feature_name>.rs and conditionally injects <feature_name> depending on success?

@alexreg

This comment was marked as disruptive content.

alexreg commented Nov 9, 2018

@jrvidal Better? That's very hacky. It's not better in any way.

@aturon

This comment has been minimized.

Member

aturon commented Nov 9, 2018

@alexreg Please keep an eye on your tone here and keep critiques specific and constructive.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 9, 2018

@aturon

I agree with this, and to me the same argument eliminates much of the argument for having accessible rather than just version: you shouldn't be using either of these until the given feature is stabilized and hence you know the version number.

Personally, I'd like to consider dropping this proposal down to just version numbers, at least to start, and see whether in practice we feed the need for the added complexity of accessibility.

I am personally fine with slimming this RFC down to #[cfg(version(..))] -- it does give quite a big improvement as compared to the status quo and the implementation is simpler so we can offer a shorter stabilization period (probably shippable in 1.32 or so...).

However, note that cfg(accessible(...)) has several advantages:

  1. It is more direct; when you are using the mechanism, you will know the path you need but not necessarily the version it was introduced in; To check the version you must go out of your editor and do a lookup in the documentation. This is an additional burden; it's not great, but it exists.

  2. cfg(accessible(...)) works for public dependencies that are not the standard library and the language. For example, something interesting may have been introduced in foo-1.X.Y which you would like to offer some extras atop of.

I'm going to nominate this RFC so that we can discuss how to proceed.

@jrvidal

I would much rather prefer fostering better user-land solutions, such as @cuviper's autocfg.

While autocfg uses a powerful approach, I don't see it as an improvement on version_check; the mechanism seems slow (because you need to invoke the compiler) and unergonomic (because it involves build.rs and you need to emit a cfg per version so it doesn't scale).

Furthermore, when you look into the documentation of autocfg, we find:

  • AutoCfg::probe_path -- this is essentially accessible
  • probe_rustc_version -- this is essentially version(...) but is tied to rustc instead of Rust.

This RFC brings these concepts into the language itself eliminating the need for build.rs in a large number of cases and scales better.

(1) could be solved if a clear winner emerges in the ecosystem (maybe becoming part of the nursery). That would take care of (4) as well, although that's something could be addressed independently today by (maybe the libs team?) getting involved in those projects to provide such recommendations.

Library solutions cannot to my knowledge get around build.rs and they cannot be as clear a "winner" as an in-language solution. This RFC just recognizes mechanisms already existing in version_check and autocfg as proven solutions and bring them into the language.

I'd argue that (3) is a statement about the current state of affairs of feature-detection in the ecosystem that ignores potential improvements. I think something like autocfg already makes things (in my opinion) way more ergonomic than they used to be compared to, say, bare version_check.

I don't agree here and I don't think any library solution can be as ergonomic or scalable as an in-language solution. I'd also note that we are talking about compile time reflection about the language itself, so baking such reflection into the language isn't strange at all.

(I concede point (2) 😕)

I think this point is central and impacts points 1, 3, and 4 to such a degree that in my view, there's not much more to say. While you have to use build.rs, no solution is going to be as good as one in-language.

Why not wait and see where this road takes us? Isn't this the preferred approach in general (i.e. build a macro before adding syntax; let user-land experiment, then nursery, then (maybe) language/std library)?

Not always; and in this case I think we've had quite a long testing period for version checking with version_check and similar mechanisms in other crates. The solution seems proven to me and the limitations of an out-of-language solution are known and problematic.

(Note how the discussion about stabilizing feature names becomes moot: the community could rally around a repository of commonly used "feature flags" wrapping a curated set of compile-time tests.)

The discussion doesn't become moot, the mechanism just moved around. I don't think moving feature gate name tests to be done by autocfg makes it any better.

Also: what about having the rustc version injected by Cargo itself? (rust-lang/cargo#2903, rust-lang/cargo#4408)

This seems like an indirection to me... The compiler already knows its own version... why should it tell Cargo the version so that Cargo can tell the compiler the version? Furthermore, if the structured information is needed for other reasons, then we can offer that atop of version(...). Furthermore, not everyone uses Cargo and we need to ensure that we don't bless rustc too much here.

And a final spitball about point (2) above: what if Cargo itself had a notion of feature-detection, where it attempts to compile every file in .features/<feature_name>.rs and conditionally injects <feature_name> depending on success?

That automates the work of build.rs to a certain degree but is still not nearly as ergonomic as version(...) is nor as scalable and readable. The nice thing about version(...) is that it says directly in the source code what the conditional compilation means; you don't have to look elsewhere.

@Centril Centril added the I-nominated label Nov 9, 2018

@jrvidal

This comment has been minimized.

jrvidal commented Nov 10, 2018

@Centril

the mechanism seems slow [...] and unergonomic (because it involves build.rs and you need to emit a cfg per version so it doesn't scale).

Those are valid points but (minor nit) the scaling concern might be a stretch. If a library writer is at the point where they're managing too many min_rustc_1_xx flags, they have bigger problems to deal with in their code (tons of "branches") which are not relieved by having cfg(version(...)).

Library solutions cannot to my knowledge get around build.rs

While you have to use build.rs, no solution is going to be as good as one in-language.

Someone suggested upthread that cfg(accessible(...)) plus dummy items such as std::core::features::Specialization equals feature detection. Instead of doing this in the standard library, you could have a library with a build.rs to conditionally expose those items. This would remove the need to write your own build script and, modulo duplication in the dependency graph, (most of) the compile-time costs.

Now, I haven't thought much about it but: couldn't a crate achieve a similar effect today (without cfg(accessible(...))) by exposing a conditionally generated proc macro?

I'd also note that we are talking about compile time reflection about the language itself, so baking such reflection into the language isn't strange at all.

I'm all 👍 ❤️ on that and, btw, I 👏 your efforts! But please note that cfg(version(...)) is not language reflection unless we accept that the Rust language (not rustc) is versioned, which is a conversation that might be bigger than this RFC.

we've had quite a long testing period for version checking [...]. The solution seems proven to me and the limitations of an out-of-language solution are known and problematic.

(Emphasis mine) I don't think this is a fair statement at all 😕 autocfg in its current form is 1 month old (!)

The discussion doesn't become moot, the mechanism just moved around. I don't think moving feature gate name tests to be done by autocfg makes it any better.

I meant that the discussion about stability of feature gate names in the compiler becomes moot. If it were in a library, it would follow semver, it could go beyond 1.x, and it would unburden compiler devs from committing to and bike-shedding names.


FWIW I don't have may arguments against cfg(accessible(...)).

@cuviper

This comment has been minimized.

Member

cuviper commented Nov 10, 2018

Even as the author of autocfg, I welcome the idea of letting the compiler handle such things itself. I see autocfg as a pragmatic solution, not the end-all-be-all. Of course, builtin features will only be useful starting in some future version where this RFC is stabilized, so there will be a place for build-script solutions for some time yet.

autocfg in its current form is 1 month old (!)

FWIW, I've been using similar probes in the num crates for i128 support since Rust 1.26. I haven't converted those to use autocfg yet, but I intend to.

Raw version checking has been in practice for quite a long time, e.g. rustc_version has been around since Aug 2015.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 10, 2018

@jrvidal

Those are valid points but (minor nit) the scaling concern might be a stretch. If a library writer is at the point where they're managing too many min_rustc_1_xx flags, they have bigger problems to deal with in their code (tons of "branches") which are not relieved by having cfg(version(...)).

It is my personal experience that it doesn't scale and produces a mess. :)

Someone suggested upthread that cfg(accessible(...)) plus dummy items such as std::core::features::Specialization equals feature detection. Instead of doing this in the standard library, you could have a library with a build.rs to conditionally expose those items. This would remove the need to write your own build script and, modulo duplication in the dependency graph, (most of) the compile-time costs.

Now, I haven't thought much about it but: couldn't a crate achieve a similar effect today (without cfg(accessible(...))) by exposing a conditionally generated proc macro?

Why? To what end? These complications doesn't seem to offer any advantages; having the language introspect about its own version is a simple and effective solution without any contortions.

I'm all 👍 ❤️ on that and, btw, I 👏 your efforts! But please note that cfg(version(...)) is not language reflection unless we accept that the Rust language (not rustc) is versioned, which is a conversation that might be bigger than this RFC.

The Rust language is versioned. When we release new versions of rustc the language team is introducing new things into the "language spec" which did not exist in prior versions; the library team does the same thing with the standard library. All conformant Rust compilers which adhere to a certain Rust version must include what the version of rustc included at that version.

(Emphasis mine) I don't think this is a fair statement at all 😕 autocfg in its current form is 1 month old (!)

I was referring to the limitations of build.rs.

I meant that the discussion about stability of feature gate names in the compiler becomes moot. If it were in a library, it would follow semver, it could go beyond 1.x, and it would unburden compiler devs from committing to and bike-shedding names.

It is mostly RFC authors (e.g myself) and contributors to libstd who invent new feature gates, not compiler devs. I don't want to encourage any bikeshedding in this department whether it be done by some external crate or by compiler devs; it seems like a waste of time.

@alexreg

This comment was marked as disruptive content.

alexreg commented Nov 10, 2018

@aturon Are you having a laugh? Am I not allowed to have an opinion on anything? To be frank, I expressed myself in a perfectly polite way. You could have easily asked, "would you like to expand on that", instead of attacking me. In any case, the author already discussed approaches based on build.rs in his RFC, so I didn't feel a particular need to.

@aturon

This comment has been minimized.

Member

aturon commented Nov 10, 2018

@alexreg We care a lot about how we treat each other in the Rust community, especially in formal spaces like RFC threads. See the code of conduct for some insight into the basic spirit we strive for (and also, on how one should react when being called out). If you'd like to discuss these points further, please do so privately with the moderation team.

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Nov 12, 2018

So, when I originally reviewed this, cfg(accessible(...)) was the part I was excited about, and I was lukewarm on cfg(version(...)). And that remains true today.

I really don't want to see this RFC pared back to only version; I feel like that would do damage to the ecosystem, by propagating version tests and not giving any alternatives.

On the contrary, I'd be thrilled to see this RFC pared back to only accessible, and then a separate RFC advanced for version and another for feature.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 12, 2018

I personally think having both accessible(...) and version(...) in the language is a good idea as they are optimal for different use cases and honestly, the complexity here isn't all that much... But I would also be OK with having one or the other as a start -- it's an improvement! even if it doesn't get us fully to the goal.

With respect to accessible(...) I previously wrote:

  1. cfg(accessible(...)) works for public dependencies that are not the standard library and the language. For example, something interesting may have been introduced in foo-1.X.Y which you would like to offer some extras atop of.

And then I further elaborated:

Furthermore, when you look into the documentation of autocfg, we find:

  • AutoCfg::probe_path -- this is essentially accessible

When working on AltSysrq/proptest#106, I realized that I needed accessible(...) for precisely the sort of situation as noted in 2. In other words, I would have liked to generate the following in a procedural macro (proptest_derive):

#[derive(Debug, Arbitrary)]
struct MyType { ... }

// == generates ==v

#[cfg(all(
    any(test, feature = "prop"),
    accessible(::proptest::prelude::Arbitrary)
)]
impl Arbitrary for MyType { ... }

Here, version(...) would not help at all because this accessible(::proptest::prelude::Arbitrary) is referring to something outside of the standard library; not even autocfg could help because we cannot inject build.rs files into the crate that invokes a procedural macro.

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Nov 12, 2018

The problem with how cfg(accessible) is presented in this RFC is that it starts with an example of referring to an unstable path, and continues to present that as the primary raison d’être. This has the same issues as cfg(feature) in that it pushes crates into assuming unstable features will not change before stabilisation.

If cfg(accessible) were instead positioned as a tool for adding conditional support for new stable compilers, and never showed unstable paths as accessible—even when their feature is active—then I think it’s a much easier sell.

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Nov 12, 2018

@Nemo157 I don't feel like that's an unreasonable example. Code that touches unstable features may break. accessible doesn't mean it'll never break. It means it's possible to write code that might have a hope of not breaking.

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Nov 12, 2018

Maybe allowing cfg(accessible) to detect unstable paths could still be allowed then, but I strongly feel it should not be presented as the primary reason for it. A better example would be a feature that has already been stabilised, e.g. say you have a crate that wants to continue compiling with Rust 1.24 but also wants to include some trait implementations for i128 which was stabilised in Rust 1.26.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 12, 2018

@Nemo157 If I'm understanding you correctly you are mostly concerned about what the RFC presents as idiomatic usage to some learner who might read the RFC at some point (and that we should pick examples that are not as risky...)?

I could definitely introduce better examples + leave notes that doing this on unstable features carries risks; that is an utterly fair thing to ask.

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Nov 12, 2018

Oh, and the problem with that example is not that it may break for a user of the unstable feature if the iterator_flatten feature changes, the problem is it may break in the future on a newer stable compiler for a user that is not using the unstable feature. If during stabilisation the iterator flatten API changes (silly example, but maybe the method changes to flatten2), but std::iter::Flatten remains an accessible path, then this code will attempt to use this newer API it was never tested with.

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Nov 12, 2018

Adding another example, even a primary example, that applies to stable seems completely reasonable.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 29, 2018

For greater flexibility, usefulness, and user facing simplicity, I have specified that accessible(...) will consider macros and attributes.

@rfcbot resolve macros-and-attributes

@Nemo157 I have changed the language in the RFC and examples to favor uses of accessible(..) which are less risky and I have also left a note about the risks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment