-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Stabilize #[cfg(version(...))]
#141137
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?
Stabilize #[cfg(version(...))]
#141137
Conversation
Some changes occurred in compiler/rustc_attr_data_structures Some changes occurred in compiler/rustc_attr_parsing |
This comment has been minimized.
This comment has been minimized.
@@ -107,6 +107,8 @@ declare_features! ( | |||
(accepted, cfg_target_feature, "1.27.0", Some(29717)), | |||
/// Allows `cfg(target_vendor = "...")`. | |||
(accepted, cfg_target_vendor, "1.33.0", Some(29718)), | |||
/// Allow conditional compilation depending on rust version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Allow conditional compilation depending on rust version | |
/// Allow conditional compilation depending on Rust version. |
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: 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. |
Thanks also to @epage for perhaps prodding us along here, in his recent talk, by pointing out the cost to ecosystem compile time performance of not doing this. |
Regarding whether we should hold off merging this stabilization until the change to Cargo is ready, if we do want to do that, we can simply mark this as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not about this file specifically)
I think the example of the stabilization report, let chains, are a particularly bad example here because they're syntax, and syntax is, except for some old legacy stuff, gated pre-expansion, so cfg(version) won't actually help there. It would probably make sense to mention this in the reference that this can't really be used for conditionally making use of syntax, or rather that such uses requires going through an intermediary macro_rules to avoid parsing it on the old version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or putting the syntax in a separate module IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, anything that won't be parsed before the cfg is expanded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the example to use a libs function instead. The example still fails compilation on the old release, because #[cfg(version(...))]
errors on compilers that don't have the stabilization yet, so it's only useful when your MSRV is >= the release that #[cfg(version(...))]
stabilized in.
It's actually maybe something that one can discuss, whether to support #[cfg(version = "...")]
as well. This syntax does not error on older compilers, or on current ones. The disadvantage is two different ways of doing things and possible breakage because it might activate some blocks within #[cfg(version = "...")]
, but I'd say that breakage is rather theoretical.
In the long term, #[cfg(version = "...")]
is not neccessary: it's only helpful for the transitory period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to take the breakage here, I'm not sure why we wouldn't just make #[cfg(version = "..")]
the one way to write this.
The concern I recall hearing was the worry that people might think this was a version equality constraint rather than a ^
or >=
constraint, but that doesn't seem to be a problem for Cargo dependencies, where e.g. time = "0.1.12"
means time = "^0.1.12"
, or for the MSRV field itself, e.g. rust-version = "1.89"
.
If we were still concerned, we could require people to write the ^
explicitly, e.g. #[cfg(version = "^1.89")]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the RFC, the question was considered in the context of cfg(accessible(..))
vs cfg(accessible = ..))
, where yes, the former definitely seems better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re cfg(version = "...")
, see the RFC text. This is actually also listed in the unresolved questions in the tracking issue, and was discussed briefly on the tracking issue.
IMO, I think we should only support cfg(version = "...")
if we emitted all the (hundreds of) valid version="1.0"
, version="1.0.0"
, version="1.1"
, version="1.1.0"
, etc. as part of rustc --print cfg
too.
Unless we want to diverge from cfg(key = "value")
being a static thing? I can see the value in that as well, it'd be useful for many other cfg
attributes.
EDIT: Sorry, GH didn't update, so posted this before I saw TC's response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem at this point that we should probably just stabilize cfg(version(".."))
. We could always decide later about doing more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding cfg(version = "...")
, see #141137 (comment) .
@@ -1,49 +1,35 @@ | |||
#[cfg(version(42))] //~ ERROR: expected a version literal | |||
//~^ ERROR `cfg(version)` is experimental and subject to change | |||
fn foo() {} | |||
#[cfg(version(1.20))] //~ ERROR: expected a version literal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning we don't use this syntax again?
From reading #71314, it seems like it was punted on for later because of implementation difficulty. Maybe that's not true any more?
I'd prefer writing #[cfg(version(1.87.1))]
over #[cfg(version("1.87.1"))]
. Having two levels of grouping, both parentheses and quotes, seems redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes a certain sense to me to quote it as it is a "version string". In attributes, we generally still try to match the general grammar of Rust, and we wouldn't accept an unquoted 1.87.1
as an argument anywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we wouldn't accept an unquoted
1.87.1
as an argument anywhere else.
I can see the argument, though I don't see why that should necessarily be an absolute? Syntactically, you can write let x = 1.87.1
, though that's of course parsed as "literal 1.87" + "field access 1" (which then fails because floats don't have any fields).
Idk., I just think it's unfortunate that we limit ourselves here, since I suspect the vast majority of cases where cfg(version(...))
is useful, you wouldn't need to specify the patch version - looking at the changelog, I actually cannot find a single use-case where you'd want cfg(version(major.minor.patch))
where patch
wouldn't just be 0
?
If we wanted this syntax, an option could be to disallow patch versions to begin with?
A difficulty with major.minor.patch
is on the macro-side, I see a few ways to solve this:
-
The heavy-weight: Introduce a new token literal
LitKind::Version
and change$x:literal
over an edition to contain this as well. -
Define that the token parsing is
cfg(version($major_minor:literal $(. $patch:tt)?))
. -
Specialize parsing of this as one token, but try to hide it from the language. Though that has unclear interactions with macros such as:
macro_rules! split { ($x:tt) => { cfg!(version(1.87 . $x)) }; } macro_rules! split { ($x:tt) => { cfg!(version($x . 87 . 1)) }; }
Maybe we can disallow those? But we'd want a stable way for
proc-macro
s to create the requiredTokenTree::Literal
.
Regardless, I think what I'm saying with this is that while this doesn't have an immediately clean solution, the solution space isn't empty, and I think it's worthwhile it to choose one of these options to get the cleaner syntax. But I guess I'm also motivated here by future attributes with versions in them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @petrochenkov who reviewed the implementation PR that did cfg(version("major.minor.patch"))
(with the quotes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a tokenization perspective, a string fits best. A major/minor version could look like a float which we don't have now but I do want to explore bools, ints, and floats for --cfg
for cargo "globals" though I don't have use cases for #[cfg]
operators for these at this time
I'll hold off on reviewing the impl until some of the design questions are settled (looks like there are unresolved design concerns), please ping me if it's ready for impl review. |
The proposal as originally reviewed and approved was for It's already not going to be functional in any version older than the version that stabilizes I would argue that it's a bug to silently ignore it in older versions, precisely because it'll be non-functional: Pieces of the ecosystem can switch to |
Agreed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small shortenings like #[cfg(version("1.20"))] are allowed, but #[cfg(version("1"))] is not
This is a bit inconsistent with other places where a Cargo user puts in a version/version range.
package.version
requires all fieldspackage.rust-version
has 1-3 fieldsdependencies.*.version
has 1-3 fields
I know version("1")
is pretty meaningless (which so is the same for package.rust-version
).
I guess the question for me is whether package.rust-version
should be
- the same syntax with the same parser implementation
- a different syntax in which we want to make sure our parser is unique to avoid changes in one affecting the other
We think it would be best if we can make sure the change lands in both cargo and rustc in the same release. There is already someone working on it, and I think we are pretty comfortable that we will be able to get that merged in time, so I don't think there is a strong reason to block. I'll try and make sure that everything is ready, though we should be careful about the scheduling in mid-june if the stabilization process here takes a long time. |
15ea1f3
to
b5a7902
Compare
This comment has been minimized.
This comment has been minimized.
b5a7902
to
dd17060
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
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
The first test was added by the original implementation PR #71314 , and is mostly a test for the allowed and not allowed ways to specify the version as part of the attribute. It has seen some changes as the implementation of
cfg_version
has been changed. As of this PR, the test ensures:#[cfg(version(1.20.0))]
is not allowed, deviating from the RFC#[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)The stabilization PR also adds a functional test, which ensures that
cfg_version
actually works.#[cfg(version = "1.20.0")]
acts as if the cfg was false due to the wrong syntax, even if the compiler version is above the specified versioncfg!(version("1.50.4"))
evals as false on1.50.3
, andcfg!(version("1.50.3"))
evals as true.Lastly, there is assume-incomplete.rs using macros instead of
RUSTC_OVERRIDE_VERSION_STRING
.This PR makes
cfg(version)
respectRUSTC_OVERRIDE_VERSION_STRING
, to make it easier to test things, and adds a test based on that.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.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).See also
Earlier stabilization report
TODOs before stabilization
cfg_version
cargo#15531