cuviper / autocfg

Automatic cfg for Rust compiler features
Apache License 2.0
95 stars 24 forks source link

Probe rustc channel #33

Closed Anders429 closed 3 years ago

Anders429 commented 3 years ago

This PR introduces the ability to probe for the current channel (stable, beta, nightly, dev). It is a solution for issue #28 and also addresses some functionality requested in issue #24.

Firstly, this PR introduces a Channel enum, stored in AutoCfg, denoting which channel is being used. PartialOrd and Ord are also defined on this enum, where ordering is defined by available features. In other words, a channel is defined to be "less than" another channel if it contains a subset of available features. So, the channels would be ordered like Stable < Beta < Nightly < Dev.

Secondly, there are seven methods added to AutoCfg. They are:

This seemed like the best API for interacting with nightly features. If the API addition is too large, it might make sense to just expose set_feature() and unset_feature(), since the real end-goal is for users to be able to probe the functionality provided by the feature, but it seemed like there is some interest in users probing the features, or even the channel itself, directly. I'm definitely up for discussion on this.

Anders429 commented 3 years ago

@cuviper Just a friendly ping. Did you have any thoughts or feedback on this?

cuviper commented 3 years ago

I think I would rather avoid the channel stuff, especially since there are knobs like RUSTC_BOOTSTRAP that can enable features on stable/beta, and that's further complicated by conditions like rust-lang/rust#77802.

If we just support probing features with an actual rustc invocation, I think we'll be on firmer ground. And IMO, responsible use needs to consciously probe not just that a feature exists, but also that it works the way you expect (to the extent that's possible with just a compilation test).

  • set_feature() - Sets a feature to be allowed before each subsequent probe. Basically the same idea as what is being done with no_std in #27. This doesn't do anything on stable/beta channels, which is good as it allows probes to succeed still if running on a later rustc version where the desired feature may have already been stabilized.

I don't think that should be conditional at all -- just do as asked and let further use fail when that feature isn't actually supported.

Anders429 commented 3 years ago

Wow, TIL about RUSTC_BOOTSTRAP. Looks like this approach might be too narrow when it comes to probing feature-gated stuff. With that in mind, I think I agree that avoiding the channel stuff completely would be best.

And IMO, responsible use needs to consciously probe not just that a feature exists, but also that it works the way you expect (to the extent that's possible with just a compilation test).

I completely agree with you on this one. Taking a second look, I don't know if probe_feature() would actually be helpful. The API should probably encourage people to check that code runs correctly when features are enabled, since something being gated behind a feature means it is in development and subject to change.

What if this was simplified to literally just be:

This would drop any need to parse rustc output for channel, and allows for stuff like:

let mut ac = autocfg::new();

ac.set_feature("step_trait");
ac.emit_has_trait("core::iter::Step");
ac.unset_feature("step_trait");
// Other probes that don't require the feature.

Do you think this would be sufficient?

cuviper commented 3 years ago

You can also clone() before trying something with a feature, so we only need the setter -- but I think unset is fine too.

Bikeshedding the name -- is set/unset the best choice? Maybe add/remove or enable/disable? (#27 uses set, but that's controlling a single value rather than a collection.)

Anders429 commented 3 years ago

Changing the names is fine with me. You're right, I think I went with set because #27 used set, but I'm not overly attached to it. I feel like I prefer enable/disable over add/remove in this context, since users will be "enabling" the feature in future probes.

Since this PR includes quite a bit of stuff that isn't going to be included, it seemed easier to open another PR. Hope you don't mind, I'm closing this in favor of #35.