Tracking issue for RFC 2523, #[cfg(version(..))]

[Centril]

This is a tracking issue for #[cfg(version(..))] (rust-lang/rfcs#2523).

Steps:

• Implement the RFC (cc @rust-lang/compiler -- can anyone write up mentoring instructions?)
• Adjust documentation (see instructions on rustc-guide)
• Stabilization PR (see instructions on rustc-guide)

Unresolved questions:

• What is cfg(version(...)) relative to in terms of nightly compilers?

  We could check against what rustc --version says, e.g. nightly being 1.40.0, beta being 1.39.0, and stable being 1.38.0. We could also have cfg(version(...)) at most be relative to the beta compiler. See the RFC for a longer discussion.

  • Resolved in Tracking issue for RFC 2523, #[cfg(version(..))] #64796 (comment)

• Should we also support version = "..." so that crates having a MSRV below when version(...) was stabilized can use the flag?

• Dependency updates cause language changes (src tarballs are vendored without respecting lockfile #79010)

[pickfire]

@csmoe Are you working on this? If not I might want to try out this task first since @petrochenkov mentioned that this task is easier than cfg(accessible = ::path).

[csmoe]

@pickfire I didn't have much bandwidth on this, seems you have already made some progress on cfg(accessible = ::path), so feel free to check this :)
(Don't forget to assign yourself with @rustbot claim)

[pickfire]

@csmoe Nice, thanks a lot for telling me that. I did not know such thing exist.

@rustbot claim

[pickfire]

Can anyone please help to write up the mentoring instructions. Based on what I know, this should be similar to #64797 (comment)

1. Syntax:
   
   1. Add a new `sym::accessible` in https://doc.rust-lang.org/nightly/nightly-rustc/src/syntax_pos/symbol.rs.html#22.
   2. Feature gate `accessible` in [`GATED_CFGS`](https://doc.rust-lang.org/nightly/nightly-rustc/syntax/feature_gate/builtin_attrs/constant.GATED_CFGS.html). Also add `cfg_accessible` to `active.rs`: https://doc.rust-lang.org/nightly/nightly-rustc/src/syntax/feature_gate/active.rs.html#530. This will also require a new `sym::cfg_accessible`.
   3. Introduce a match arm for `sym::accessible` in [`cfg_matches`](https://doc.rust-lang.org/nightly/nightly-rustc/syntax/attr/fn.cfg_matches.html). This should look mostly like [the case for `sym::not`](https://doc.rust-lang.org/nightly/nightly-rustc/src/syntax/attr/builtin.rs.html#591).
      Here you need to extract an [`&ast::Path`](https://doc.rust-lang.org/nightly/nightly-rustc/syntax/ast/struct.Path.html) and delegate the interpretation to a function of the rough shape `fn is_accessible(sess: &ParseSess, path: &Path) -> bool { ... }`

2. Implement `fn is_accessible`.
   
   1. First do some validation. We want to emit errors if [`!path.is_global()`](https://doc.rust-lang.org/nightly/nightly-rustc/syntax/ast/struct.Path.html#method.is_global). Use [`sess.span_diagnostic.struct_span_err`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.Handler.html#method.struct_span_err).

I believed step 1 and 2 should be the same. For step 3, based on what I know requires me to modify somewhere around

rust/src/libsyntax/attr/builtin.rs

Lines 566 to 568 in b56b239

	MetaItemKind::List(..) => {
	error(cfg.span, "unexpected parentheses after `cfg` predicate key")
	}

.

Based on fn is_accessible, the next thing to be done should be to implement fn min_version and then somehow reference version_check on how to do the min_version logic by checking the CFG_VERSION environment variable?

[mibac138]

Hi, @pickfire. Are you still working on this?

[pickfire]

No, I was stuck working on this and don't know how to proceed.

[mibac138]

Ok, thanks for quick response. I'll take my shot at this.
@rustbot claim

[pickfire]

@mibac138 Do you want me to publish my progress?

[mibac138]

@pickfire sure, that'd be great!

[pickfire]

@mibac138 Err, no need. You have already done more than me, I don't know how to change the eval_condition. Oh, it didn't seemed that hard after looking at your PR but I didn't know how to work on that.

[petrochenkov]

Status update: #71314 implemented this RFC with a slightly different syntax - #[cfg(version("1.2.3"))] instead of #[cfg(version(1.2.3))].

The reason is that 1.2.3 doesn't fit into some existing attribute-parsing infrastructure in the compiler (MetaItems), so it requires separate work.

Additionally, the version_check crate currently used to parse versions wants a string as an input, so we need to stringify 1.2.3 before passing it, which we can only do imprecisely (up to whitespaces).
So we can turn 1 . 2 .3 into e.g. "1.2.3" during stringification and it may be a problem depending on what exactly we are supposed to accept as a valid version.

[roblabla]

The RFC had an unresolved question about how this feature would interact with nightly/beta. Should #[cfg(version("1.45.0"))] return true or false for nightly/beta 1.45.0?

Currently, the implementation returns true. E.G. for rustc 1.45.0-nightly, the following code prints Yes:

#![feature(cfg_version)]
fn main() {
    test();
}

#[cfg(version("1.45.0"))]
fn test() {
    println!("Yes")
}

#[cfg(not(version("1.45.0")))]
fn test() {
    println!("No")
}

IMO, this is a mistake. The main use-case for cfg(version), at least for me, is to allow use of new features that are known to be stable at a given version. For instance, in the core-error crate, I was planning to use cfg(version) to implement my custom core::error::Error trait on the various error structures of libcore/liballoc automatically, based on cfg(version). Unfortunately, this is not possible as it will certainly cause breakage in the in-between nightly versions that are reported as implementing a version, whilst not having all the types associated with it.

[nikomatsakis]

That sounds like good reasoning to me, but I've not been following this RFC that closely. @joshtriplett -- I feel like you were "liaison'ing" for this before we had a term for it, do you have a take?

[nikomatsakis]

Nominating for lang-team meeting to discuss

[dekellum]

Currently, the implementation returns true

Which is most useful, the way it is, AFAICT.

Isn't there a way to combine this with cfg test(s) for "train", e.g. beta or nightly and unstable feature gates?

[Nemo157]

Isn't there a way to combine this with cfg test(s) for "train", e.g. beta or nightly and unstable feature gates?

Code written for the stable compiler should not have to know about beta or nightly.

[dekellum]

I'm not surprised with that as a goal, but I can think of cases where for practical reasons I nead to dev on 1.45 nightly with expectation that it will then work on 1.45 stable. I'm surprised if I'm the only one.

[joshtriplett]

It looks like cfg(accessible(...)) may be at a state where we could stabilize enough of it to be generally useful, and what's available is enough that I'd consider it to unblock this.

#64797

[alex]

#64797 appears to have stalled out somewhat. I'd like to make an argument for why I believe it'd be appropriate to consider stabilizing this even before cfg(accessible(...)) is stabilized:

1. There are many use cases for version checking beyond core library types being available. A particularly common one I've seen is version checking to see whether a particular piece of functionality can be used in const contexts.
2. 3rd party crates for this functionality are already quite common version_check and autocfg both have more than 100 million downloads from crates.io. Under many circumstances, availability of adequate 3rd party crates would ameliorate the need for an in-language solution, however in this case they have a major downside: they require the use of build scripts, which impose a non-trivial compilation time overhead. Build speed is of course a major area of interest -- in a survey of a crate with 56 items in Cargo.lock, about half the build scripts are solely to do version checks.
3. Because this functionality ultimately is aimed at enabling folks to support multiple versions of Rust more easily, its users won't be able to adopt it until its been stable for quite a while. Therefore getting it out sooner is a big benefit.

[karolherbst]

I hit a bug in Rustc today, where rustc fails to derive Send and Sync automatically in some cyclic type relation context. This is fixed with 1.72, so my use case here would be to #[cfg(version(..))] an unsafe Send block to make sure code compiles with older Rustc.

This is important for the project I'm working on as it's Mesa, the OpenGL/Vulkan/OpenCL/etc.. implementation for the Linux desktop and we need to be mindful of what Rustc version to support.

Edit: Apparently it was Sync stabilized in 1.72 for mpsc::Sender

[mqudsi]

There are many use cases for version checking beyond core library types being available.

+1 to this, because you have to take the entire ecosystem and tooling into account and not just the language itself. For example, in rsconf 0.2, I added an api that uses rustc --check-cfg syntax to inform the compiler of a cfg managed by build.rs, but now building under older versions of rust that were aware of the check-cfg syntax but didn't have support for it enabled without -Z check-cfg are broken (if you error on warnings). This isn't a language level change but a tooling change, and so I would have to use #[cfg(version(1.80))] on the println!("cargo:rustc-check-cfg...") statement to get this to not warn under certain rust versions.

[wesleywiser]

Nominating for lang-team discussion. This was marked as blocked on #64797 two years ago but #64797 still has open design questions and does not appear ready for stabilization anytime soon whereas this feature has no open design questions and is fully implemented. Since #64797 does not address some important use cases such as conditionally using compiler or language features and use of version_check continues to grow (now at >229M all-time downloads and upward-trending daily downloads), I think it makes sense to revisit stabilization of this feature 🙂

@rustbot label +I-nominated

[mqudsi]

Not to add to the pressure or anything, but for a feature with the raison d’être being backwards compatibility this one won't be a viable alternative to the version_check crate until the msrv of the codebase hits whatever version cfg(version) was stabilized under. So it really makes sense to try and get this out the gate on its own without blocking on other tangentially related issues.

[Kixunil]

@mqudsi yes, though if the cfg gets renamed to lang_version as was suggested then people can already start using it in old crates for features that are only available in whichever version it gets stabilized. (Because unknown cfgs are allowed and evaluate to false.)

[traviscross]

We discussed this in the lang call today. We had a question:

What's the situation on stabilizing a minimum version of cfg(accessible(..)), i.e. one that just works for things in core, std, etc.?

We understood there were issues with a more general version of the feature, and we understand there are some issues with paths in attributes, but we were unclear on whether stabilizing the most minimal cfg(accessible(..)) that we could was within the realm of possibility.

If it is within the realm of possibility, the feeling was generally that we'd prefer to do that, and that such a minimum version would resolve the concern here and allow for the stabilization of cfg(version(..)).

On the other hand, if it's not within the realm of short-term possibility for some reason, then people were open to reevaluating the approach here and whether other options might be possible for moving this forward.

[traviscross]

@rustbot labels -I-lang-nominated

Going to unnominate this for now, but please renominate when the answer to the question above is available.

[Skgland]

@mqudsi yes, though if the cfg gets renamed to lang_version as was suggested then people can already start using it in old crates for features that are only available in whichever version it gets stabilized. (Because unknown cfgs are allowed and evaluate to false.)

If stabilizing this soon is out of the question and in case renaming the cfg is not desirable, would it be possible to change the error to a future incompatibility warning now.
That way if this is stabilized some time in the future under the current name, one only needs to wait with using it until the msrv reaches the change from error to future incompatibility warning.

Similar to how cargo switched public/private dependencies to a non-blocking feature gate rust-lang/cargo#13308

[joshtriplett]

Quick note on a potentially useful enhancement to this, once we have it: cargo could pass the crate's MSRV (from the rust-version field) into rustc, and rustc could check if the MSRV means a cfg will always be true or always be false, with an autofix to remove the cfg or remove the entire cfg'd code, respectively.

[bjorn3]

Be aware that the version field of stability attributes isn't always correct. For example if an item has been moved from libstd to libcore and re-exported in libstd, the libcore version has to retain the version when it was stabilized in libstd as re-exports can't override the version in which it was stabilized.

[Lokathor]

I'm unclear when a version cfg will always be false

[joshtriplett]

@Lokathor With not. If you've written #[cfg(not(version(1.123)))] and you change your MSRV to be 1.123 or higher, Rust could offer to delete the code you've applied that conditional to. If you've written #[cfg(version(1.123))], Rust could offer to delete just the conditional while keeping the code you've applied it to.

[joshtriplett]

@bjorn3 wrote:

Be aware that the version field of stability attributes isn't always correct. For example if an item has been moved from libstd to libcore and re-exported in libstd, the libcore version has to retain the version when it was stabilized in libstd as re-exports can't override the version in which it was stabilized.

Yeah, I realize. We won't necessarily be able to use the existing stability markers for library functions without additional care.

[Lokathor]

I think the main time a not would be applied is in a cfg-if context, in which case it would be really annoying to get warnings about that. So we could warn but the detection would have to be really smart

[joshtriplett]

@Lokathor Even in cfg-if, it'd be helpful to get warnings to help you remove unused branches. But yes, we'll have to test and make sure we don't get false positives.