-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Stabilize #[cfg(version(...))]
, take 2
#141766
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
base: master
Are you sure you want to change the base?
Conversation
#[cfg(version(...))]
, take 2
I'll highlight for our awareness that this test passes: //@ run-pass
//@ rustc-env:RUSTC_OVERRIDE_VERSION_STRING=1.86.0
#![feature(cfg_version)]
fn main() {
assert!(cfg!(not(version("1.85.65536"))));
} That is, we're exposing that "1.85.65536 > 1.86.0". I don't really love that, in terms of specifying the language, but I understand the motivation for it. Probably we should make sure to say in the Reference that the behavior when the version string does not conform to the current requirements is unspecified and may change in the future. |
This all still looks right to me. Inclusive of taking the normative position that the behavior of The best day to add @rfcbot fcp merge |
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
What's the reason for this? Is it just an accident of implementation, or is there a use case for it? |
If we change the versioning scheme in the future -- we start naming our versions "25-06" or something -- then you'd want older versions of Rust to just accept those and assume they must be from the future. So it treats parse errors as "must be from the future". It's parsing each segment into a |
yeah, if you do
It's possible to use bignums to fix this but I'd say it's a non-issue: even u16 gives a millenium worth of releases (at the current cadence). And we can always extend it in the future should we anticipate an increase in velocity. |
Seems like something that would deserve at least a warn-by-default lint, if not deny-by-default? (Although that's not a blocker for stabilization, the lint can always be added later.) |
Rather than bignum, what I had nearly proposed (before deciding I was OK with how it is) was to saturate anything matching |
It does warn if the version string literal does not parse. |
Yeah, it's a builtin warning, without a way to opt out. Which is not convenient to deal with if you face it, but this also serves as a good deterrent, better than a warn-by-default lint. IF a new versioning scheme is being introduced, hopefully most of the population will be on newer compilers that either recognize the versioning scheme, or it has been turned into a lint at that point. That at least was my original reasoning why I made it into a warning and not a lint. In any case, warning or lint, we don't lock ourselves in in any way due to this stabilization, even if we turn it into an error-by-default lint let's say. |
@rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
@rfcbot reviewed |
I think this syntax is a mistake.
I'm not excited to see If we use The only counter-arguments for the We tend to think a lot about backwards compatibility in the Rust project. But in this case we need to think about forward compatibility: what will old versions of Rust do with the new syntax? Using any syntax that just immediately errors out on current/old versions makes this feature so much less powerful, to the point where it will be basically useless for the near future: we'd be releasing a Rust 1.89.0 with a new cfg(version()) feature that can only be used if you set your MSRV to 1.89.0. 🙃 It'll only become useful after a long time, once it's acceptable to bump your MSRV to 1.89.0. (Which will take quite some time for some projects.) If instead, we pick a syntax that's already accepted by current/old rust versions, like (Unfortunately, |
Is there a warning if someone does At least it is a compile error when testing on that version. Having a check-cfg-like warning could help catch that earlier. I would consider this non-blocking with the current proposal. This may become more important with |
@epage there is no such warning. In the stabilization report I write about integrating into the greater MSRV system, i.e. making it read @m-ou-se most features introduced by rustc are taking from the error territory, but I understand the desire to make an exception for this one, given its purpose. In any case, the moment right after stabilization is the moment it's least useful, after that it monotonically gains usefulness. In 1-3 years, I think the ecosystem will see a large hit in Edit: also, the further back you go with your MSRV, the more likely folks will want to use features of compilers that don't have |
My concern was less to do with the greater MSRV story and more about correct use of this feature. If someone uses |
The top 10 most-downloaded crates on crates.io range from MSRVs of 1.48 (Nov 2020) to 1.65 (Nov 2022), with the median being 1.59 (Feb 2022), using their latest versions as of today. Most do not use version-based feature gating. The ones I thought of that did (serde and proc-macro2) work more or less like @est31 describes: They detect features as recent as 1.81 (Sep 2024) and 1.79 (June 2024) respectively, and would need a build script as long as their MSRV was below that. It's easy to imagine that more of these crates would use version gating if a build script was not required. A change like adding |
Note that crates like |
Following up on my comment above:
Actually, |
Been thinking over Mara's ideas and finally putting together my thoughts. First, I am sympathetic to how we introduce features to reduce MSRV issues. The MSRV-resolver opt-in was ignored-with-warning on stable to allow people to opt-in more easily without bumping an MSRV. I do have a couple of biases against the proposal
Assuming
I suspect with the way most users interact with On the other hand, seeing I get the framing of " |
I've been thinking about this since the meeting. I think that @tmandry and @m-ou-se argument have a pretty strong argument. It bottoms out to me as: who is this feature for? The answer, is "crates that maintain an MSRV". But crates that maintain an MSRV won't be able to use it, quite possibly for some time. The strongest argument is blunting the existing momentum. But the fact that the target audience won't be able to use it feels like a problem -- how much does it matter if it lands if the target audience can't use it? I'd rather we open an issue, vote on the new design, and stabilize that. I'm not super keen on making last minute changes to syntax but this feature is old and it's not exactly unknown. (This pattern happens over and over in my experience: there is a long blockage that sucks the oxygen out of the discussion; by the time that block is resolved, it turns out a bunch of people have small issues they'd like to discuss, but there seemed to be no point in discussing them while the whole feature was in existential limbo--and then it's been years. It's super duper frustrating.) I guess I'd like to hear from @est31 for their take on whether they'd be up to implement the change in syntax if we decided to go that way. I do see Ed's points about obviousness and the fact that people may be using |
(Apologies if you don't want to hear from folks here) I'm one of the people who maintains things with an MSRV and is keen for this. Notwithstanding that, my suggestion would be to focus on making the long term right decision. My view is very much, "The best time to plant a tree was 20 years ago, the second best time is today". While I might not be able to use it in the release this is in, I can a) get excited, b) look forward to a glorious future, c) drive my MSRV strategy to take advantage as soon as is practical. |
yeah tbh if I had a time machine, I'd suggest we use some other syntax for
definitely, but I don't want to implement it in a haste: the lang team needs to have consensus for a change of the syntax. once that's established, we should probably close this PR and change the implementation. Then, maybe after a certain waiting period as it represents a major change of the feature (6 weeks?), stabilize. |
For what it's worth there was discussion in rust-lang/rfcs#3796 to permit |
@epage The only way
Regarding
|
Except if we release |
Perhaps a silly question, but what is the use case for wanting to gate something on a point release? |
@rfcbot concern let's take rust_version seriously Having given this some thought, I think While I agree that the Previously I had wanted to use something like #[cfg(has_cfg_version)]
#[cfg(version("1.80"))]
fn foo() {}
#[cfg(not(has_cfg_version))]
fn foo() {}
#[cfg(has_cfg_version)]
#[cfg(not(version("1.80")))]
fn foo() {} That's actually quite important, and something I had missed in our earlier discussion on this. It means that there isn't, in my view, a satisfactory way to retrofit something like So as much as I would like to get this feature today, I don't think we should rush it out the door. As this discussion and previous discussions have shown, there have been more design concerns hiding underneath the question of whether we can ship I'm also happy to put time in to writing an RFC, FCP, or whatever's needed and drive consensus on it – and having raised a concern, driving this will be a priority for me. On that note, a huge thanks to @est31 for being willing to roll with the punches as we work out the issues here. |
There is no reason Rust 1.86 wouldn't know about it, when we would do a stable-point release we would backport that knowledge to the current stable/beta/nightly releases, technically This also assumes that we do want to distingues about the point releases, we could also simply have: |
One of the points that's been made in this thread that stands out strongly to me is the potentially limited window of utility for not giving an error in earlier versions. If you're maintaining an MSRV of 1.48 while also using If doing something like this were a big help to people with 1.48 MSRVs today, then that would be one thing. But I sense that it's probably not. In any event, there is, I think, more work for us to eventually do here. E.g., I'd like to see us add a |
I think it's a solid point @traviscross. I haven't done a comprehensive analysis, but in the two crates I looked at we might gain only a year; perhaps less, if we take too long. There are also a couple of things that push me in the other direction:
Certainly there is still time pressure to maximize the value of the thing we ship. I just don't want that to be the overriding theme when there are legitimate design concerns, especially if they can (arguably) have a similarly sized impact. |
Thanks @traviscross for putting it in better words than what I wrote in #141766 (comment) I'd say the major advantage of So |
For me, my overriding sensation isn't time pressure to ship, but simple humility. The tradeoffs we're discussing today were also largely discussed at the time of RFC acceptance and would have been reviewed at the time of the original stabilization attempt. Many good and reasonable points were raised, and the team seems to have acted reasonably in making adjustments to take those into account, leading to what's before us today and what would likely have been stabilized in Rust 1.53 had it not been for the concern about stabilizing It's just not yet clear to me, in this case, that much has changed or that we're certain to do better. It seems within the realm of the possible that we go through another round and come back with the same design. Conversely, the cost of shipping this design today and making any later extensions or adjustments, if we were to decide we wanted them, seems low. My humility extends as well to our team bandwidth. We have a lot of important work stacked up. Perhaps I just don't have that much appetite for putting this one back into our queue. |
If you were to adopt a syntax using |
Stabilization report
This proposes the stabilization of
cfg_version
(tracking issue, RFC 2523).What is being stabilized
Permit users to
cfg
gate code sections based on the currently present rust version.Tests
cfg-version-expand.rs: a functional test that makes rustc pretend to be
1.50.3
, then tries with1.50.0
,1.50.3
, and1.50.4
, as well as other version numbers.syntax.rs: tries various ways to pass wrong syntax to
cfg(version)
:#[cfg(version("1.20.0"))]
#[cfg(version("1.20"))]
are allowed, but#[cfg(version("1"))]
is not#[cfg(version("1.20.0-stable"))]
are not allowed#[cfg(version = "1.20.0")]
is not supported, and there is a warning of theunexpected_cfgs
lint (but no compilation error)assume-incomplete.rs: another functional test, that uses macros. It also tests the
-Z assume-incomplete-release
flag added by #81468.wrong-version-syntax.rs ensures that check_cfg gives a nice suggestion for
#[cfg(version("1.2.3"))]
when someone tries to do#[cfg(version = "abc")]
.Development of the implementation
The initial implementation was added by PR #71314 which used the
version_check
crate.PR #72001 made
cfg(version(1.20))
eval to false onnightly
builds with version1.20.0
, upon request from the lang team. This decision was pushed back on bydtolnay
in this comment, leading tonikomatsakis
reversing his decision.Ultimately, a compromise was agreed upon, in which nightly releases are treated as "complete" i.e.
cfg(version(1.20))
evals to true onnightly
builds with version1.20.0
, but there is a nightly flag-Z assume-incomplete-release
to opt into the behaviour that doesn't do this assumption. This compromise was implemented in PR #81468.PR #81259 made us adopt our own parsing code instead of using the
version_check
crate.PR #141552 pulled out the syntactic checks from the feature gate test into its own dedicated test.
PR #141413 made
#[cfg(version)]
more testable by making it respectRUSTC_OVERRIDE_VERSION_STRING
.Prior stabilization attempts were #64796 (comment) and #141137.
cfg_has_version
In the course of the earlier stabilization attempt, it came up that due to the way
#[cfg(version)]
uses "new" syntax, one can only adopt it if the MSRV includes the version that stabilized#[cfg(version)]
. So it won't be immediately useful: For a long time, many crates will still use the alternatives that#[cfg(version)]
meant to displace, until the stabilization of#[cfg(version)]
was sufficiently long ago.In order to solve this,
cfg_has_version
was proposed: a builtin, always true cfg variable. Ultimately, the lang team decided in #141401 to not immediately includecfg_has_version
into the stabilization (#141137 included it), but go via a proper RFC instead. Implementation wise,cfg_has_version
is not hard to implement, but semantically, a cfg variable is not a small deal, it will be present everywhere, e.g. inrustc --print cfg
.There is no such thing as unstable cfg variables (and even if there were, it would counteract the purpose of
cfg_has_version
), so its addition would have an immediate-stable effect.In a couple of months to a couple of years, this will not be a problem, as the MSRV of even slower moving projects like serde gets bumped every now and then. We probably feel the desire for
cfg_has_version
the strongest directly after the stabilization of#[cfg(version)]
, then it decreases monotonically.Unresolved questions
Should we lint for
cfg(version)
probing for a compiler version below the specified MSRV? Part of a larger discussion on MSRV specific behaviour in the Rust compiler. It feels like it should be a rustc lint though instead of a clippy lint.Future work
The stabilization doesn't close the tracking issue, as the
#[cfg(accessible(...))]
part of the work is still not stabilized, currently requiring an implementation (if an implementation is something we'd want to merge in the first place).We also explicitly opt to treat
cfg_has_version
separately.TODOs before stabilization
cfg_version
cargo#15531 -> decided not to do it