Stabilize #[cfg(version(...))], take 2

[est31]

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.

#[cfg(version("1.87"))]
pub fn from_utf8_unwrap(buf: &[u8]) -> &str {
    str::from_utf8(buf).unwrap()
}

#[cfg(not(version("1.87")))]
pub fn from_utf8_unwrap(buf: &[u8]) -> &str {
    std::str::from_utf8(buf).unwrap()
}

Tests

cfg-version-expand.rs: a functional test that makes rustc pretend to be 1.50.3, then tries with 1.50.0, 1.50.3, and 1.50.4, as well as other version numbers.

syntax.rs: tries various ways to pass wrong syntax to cfg(version):

• The expected syntax is #[cfg(version("1.20.0"))]
• small shortenings like #[cfg(version("1.20"))] are allowed, but #[cfg(version("1"))] is not
• postfixes to the version, like #[cfg(version("1.20.0-stable"))] are not allowed
• #[cfg(version = "1.20.0")] is not supported, and there is a warning of the unexpected_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 on nightly builds with version 1.20.0, upon request from the lang team. This decision was pushed back on by dtolnay in this comment, leading to nikomatsakis 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 on nightly builds with version 1.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 respect RUSTC_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 include cfg_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. in rustc --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

• incompatible_msrv doesn't respect #[cfg(version(...))] rust-clippy#14827 . Not a hard blocker, but the lint is warning by default and people will probably hit it in fairly common combinations.
• should we sync up with the cargo implementation? There is currently nothing implemented in cargo yet. Tracking Issue for cfg_version cargo#15531 -> decided not to do it
• reference PR: Document #[cfg(version(...))] reference#1828

[rustbot]

r? @eholk

rustbot has assigned @eholk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[traviscross]

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.

[traviscross]

This all still looks right to me. Inclusive of taking the normative position that the behavior of cfg(version("..")) with unsupported version literal strings is unspecified and may change in the future, I propose that we do this.

The best day to add cfg(version("..")) was yesterday. The second best day is today.

@rfcbot fcp merge

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

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.
See this document for info about what commands tagged team members can give me.

[eholk]

That is, we're exposing that "1.85.65536 > 1.86.0".

What's the reason for this? Is it just an accident of implementation, or is there a use case for it?

[traviscross]

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 u16, and so "1.85.65536 > 1.86.0".

[est31]

yeah, if you do rust-version = "1.80.10000000000000000000000000000000000000" in Cargo.toml, it will also error.

error: expected a version like "1.32"
  --> Cargo.toml:5:16
   |
11 | rust-version = "1.80.10000000000000000000000000000000000000"
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |

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.

[PoignardAzur]

That is, we're exposing that "1.85.65536 > 1.86.0".

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.)

[traviscross]

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.

Rather than bignum, what I had nearly proposed (before deciding I was OK with how it is) was to saturate anything matching [0-9]+ and greater than u16::MAX.

[traviscross]

Seems like something that would deserve at least a warn-by-default lint, if not deny-by-default?

It does warn-by-default if the version string literal does not parse.