Skip to content
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

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

Open
2 of 6 tasks
Centril opened this issue Sep 26, 2019 · 92 comments
Open
2 of 6 tasks

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

Centril opened this issue Sep 26, 2019 · 92 comments
Labels
B-RFC-approved Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-cfg_version `#![feature(cfg_version)]` S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Sep 26, 2019

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

Steps:

Unresolved questions:

@Centril Centril added B-RFC-approved Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Sep 26, 2019
@Centril Centril added the F-cfg_version `#![feature(cfg_version)]` label Sep 26, 2019
csmoe added a commit to csmoe/rust that referenced this issue Sep 29, 2019
@pickfire
Copy link
Contributor

pickfire commented Nov 15, 2019

@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
Copy link
Member

csmoe commented Nov 16, 2019

@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
Copy link
Contributor

pickfire commented Nov 16, 2019

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

@rustbot claim

@rustbot rustbot self-assigned this Nov 16, 2019
@pickfire
Copy link
Contributor

pickfire commented Nov 24, 2019

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

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
Copy link
Contributor

mibac138 commented Apr 18, 2020

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

@pickfire
Copy link
Contributor

pickfire commented Apr 18, 2020

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

@mibac138
Copy link
Contributor

mibac138 commented Apr 18, 2020

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

@rustbot rustbot assigned rustbot and unassigned rustbot Apr 18, 2020
@pickfire
Copy link
Contributor

pickfire commented Apr 18, 2020

@mibac138 Do you want me to publish my progress?

@mibac138
Copy link
Contributor

mibac138 commented Apr 18, 2020

@pickfire sure, that'd be great!

@pickfire
Copy link
Contributor

pickfire commented Apr 20, 2020

@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
Copy link
Contributor

petrochenkov commented May 2, 2020

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.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 3, 2020
Implement RFC 2523, `#[cfg(version(..))]`

Hi! This is my first contribution to rust, I hope I didn't miss anything. I tried to implement this feature so that `#[cfg(version(1.44.0))]` works but the parser was printing an error that I wasn't sure how to fix so I just opted for implementing `#[cfg(version("1.44.0"))]` (note the quotes).

Tracking issue: rust-lang#64796
@roblabla
Copy link
Contributor

roblabla commented May 6, 2020

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
Copy link
Contributor

nikomatsakis commented May 6, 2020

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
Copy link
Contributor

nikomatsakis commented May 6, 2020

Nominating for lang-team meeting to discuss

@dekellum
Copy link

dekellum commented May 6, 2020

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
Copy link
Member

Nemo157 commented May 6, 2020

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
Copy link

dekellum commented May 6, 2020

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.

@est31
Copy link
Contributor

est31 commented May 19, 2021

It is not entirely clear to me what is being passed into lib_feature/lang_feature as discussed in the last few comments; if those are feature names it does seem unfortunate to stabilize those. It might make sense to have a dedicated set of names that we add as part of the stabilization process; this would avoid problems where parts of an existing feature are stabilized, for example. It does increase the stabilization overhead though.

I've been wondering about writing an RFC for lib_feature. Part of the RFC's work would be to research about the current behaviour, whether features exist that are half stabilized, half not stabilized, how often this happens in practice, and whether a policy of having to split off features before partially stabilizing them makes sense or not. As for the feature names, yeah maybe there needs to be some review of them. Maybe it would make sense to stabilize them piece by piece. cpp apparently has a similar process to Rust by having proposals denote the feature name. This is a list of features I could find. Last, right now features outside of core are prefixed like proc_macro_something, but it would be useful if that prefix got dropped and turned into a pseudo-path, so proc_macro::something instead, as then the compiler can explicitly look in the proc_macro crate for presence of that, and doesn't have to execute a scan first whether someone has said extern crate proc_macro or not. Generally it'd be something for the RFC's discussion to determine.

It would be nice if the lang team could give a fundamental feedback whether it would be a good idea to open an RFC on this, or whether it's not a good path forward.

@joshtriplett
Copy link
Member

joshtriplett commented May 19, 2021

Rather than using library "feature names", I personally think for a first pass it would suffice to have a cfg(lib_has(std::some::path)), where the path inside lib_has ignores any local name bindings and just checks std/alloc/core/prelude. That would completely sidestep name resolution difficulties and feature naming issues. That'd be quite sufficient to allow feature-detection of the standard library.

@Lokathor
Copy link
Contributor

Lokathor commented May 19, 2021

Could it just use the normal pathing system and people can write ::std if they absolutely want to be sure they're checking the standard library?

@est31
Copy link
Contributor

est31 commented May 19, 2021

Yeah, that wouldn't need lib_has actually. It could be exposed directly as part of cfg(accessible) with an absolute path. After the ::, at least on edition 2018 and beyond, there has to follow a crate name. If it doesn't find the crate as part of the prelude or std/alloc/etc it can just error, regardless of whether anything beyond is accessible or not. Even on edition 2015 you are not allowed to shadow crates from the prelude.

@joshtriplett
Copy link
Member

joshtriplett commented May 20, 2021

@HTG-YT
Copy link
Contributor

HTG-YT commented Jun 9, 2021

Stabilization report

I propose that the cfg_version feature be stabilized. The feature guards the cfg(version) predicate, which takes a version number like cfg(version("1.50.0")) and evaluates to true if the compiler's/language's version is newer or equal to the contained version number. Notation wise, "1.50" is allowed too, but any postfixes like -stable or -beta.2 are not allowed.

#[cfg(version("1.50.0"))]
fn foo() {}

#[cfg(version("1.27.0"))]
fn bar() {}

The predicate is one of the two predicates proposed by RFC 2523, the other being cfg(accessible(path)), tracked by #64797. The main alteration from the RFC was to surround the version number with "" to better fit into the compiler's internal data structures.

The implementation issues a warning if invalid version syntax is contained in the string literal. This is done to provide forward compatibility. Literal-free invocations like version(1.50) do cause errors just as version(foo) does.

version on the nightly channel

In the post-RFC discussion there has been a conversation about which version the nightly compiler should use as its supported version. Unlike beta or stable compilers, nightly compilers can still gain new features, and un-stabilizations are possible. So technically it's possible that newer code can rely on a feature that someone's pinned or not updated nightly does not support yet. On the other hand, during the stabilized-in-nightly phase, features often attain their largest extent of exposure before being shipped as part of a stable release.

The proposed compromise is to treat as a nightly's version as complete, but to add the -Z assume-incomplete-release flag added by #81468 . Users who pin their nightlies can enable it to back-date the supported version to the prior release. Intent is to keep it perma-unstable as it targets nightly users only. As this question only affects nightly compilers, the decision can also be revisited in the future.

Implementation history

Documentation

Tests in the compiler

See also

See also the cfg(accessible) feature (issue #64797).

I propose to add the specific nightly compiler commit to the attribute, as in:

#[cfg(version("1.54.0-nightly (ed597e7e1 2021-06-08)"))]

Such that nightly builds earlier than that (or after than that) would consider the code annotated with that attribute as "conditionally disabled code" even if it is exactly 1.54.0-nightly, but not the same commit? Not sure would that be too specific; but that might solve the problem.

@jhpratt
Copy link
Member

jhpratt commented Jun 9, 2021

Setting aside the fact that I don't believe this is the appropriate time or place to discuss that (a new issue should be opened imo), that would effectively preclude an alternative implementation of Rust from ever existing. Matching the behavior of an exact SHA and date would not be worth the effort to say the least.

@HTG-YT
Copy link
Contributor

HTG-YT commented Jun 9, 2021

That is true though

@joshtriplett
Copy link
Member

joshtriplett commented Jun 15, 2021

Setting aside the fact that I don't believe this is the appropriate time or place to discuss that (a new issue should be opened imo), that would effectively preclude an alternative implementation of Rust from ever existing.

Not inherently; it would just allow Rust code to detect the exact compiler version it's running on. That might be useful for code that intends to be pinned to a specific compiler version.

@roblabla
Copy link
Contributor

roblabla commented Jun 15, 2021

I expect alternative implementations to target stable rust, not nightly. So a feature to pin a particular nightly would not need to be implemented by alternative rust compilers (or they could even be implemented with alternative semantics).

@Kixunil
Copy link
Contributor

Kixunil commented Oct 9, 2021

Regarding:

Should we also support version = "..."

Yes, please! I have a bunch of projects (and contribute to a bunch of others) that want to support at least the version available in Debian stable. However additional features would be sometimes useful.

Unfortunately, this would not work as-is. Let's say I want to do this:

#cfg(version= "1.60")
struct Foo;
// ...

If I write such code it'll behave strangely:

  • It compiles just fine for old Rust versions (e.g. 1.41)
  • It errors in newer versions (e.g. 1.48)
  • It starts to compile in even newer version again (e.g. let's say it's stabilized in 1.58)
  • It turns on the additional functionality in the given version (1.60)

To an uninformed observer this would look like certain versions had regression.

To solve this I propose renaming version to rust_version in the same release this gets stabilized in. (In other words renaming would be mandatory part of the stabilization commit.) I find that name more readable anyway and it's in line with similar cargo feature. An alternative is just to get this in ASAP but that incentivizes rushed decisions. (Maybe not too big issue given this is a very simple feature?)

@est31
Copy link
Contributor

est31 commented Oct 12, 2021

@Kixunil yeah the same behaviour occurs with edition = "..." in Cargo.toml which is ignored by older cargos, then newer cargos error about it, until cargo 1.31.0 onwards accepts it.

I think the best solution to the problem is to just wait it out, because even if this feature exists, projects will increase their MSRV eventually to one that supports #[cfg(version)]. I think that's better than adding the trademarked rust term to even more places in the language (this can be problematic for forks for example, or people who don't want to/can't abide by the trademark policy the foundation sets).

@Kixunil
Copy link
Contributor

Kixunil commented Oct 12, 2021

@est31 WTF, "Rust" is name of the programming language so if you make a fork it's reasonable to keep the name of the language. Does the trademark disallow people forking?

IMO version is not clear - is it crate version? Dep version? std version? cargo version? Rust version? Renaming kills two birds with one stone. (Or if we want to avoid trademark discussion, lang-version is fine.)

@Lokathor
Copy link
Contributor

Lokathor commented Oct 12, 2021

I'd agree that lang_version is better than just version.

@est31
Copy link
Contributor

est31 commented Oct 12, 2021

Does the trademark disallow people forking?

The copyright doesn't but the trademark rights are separate and it's up to the owner of the trademark to set the policy. When the trademarks were owned by Mozilla, they were under basically the same rules as the Firefox trademark. Some distros were forced to create different branding for their Firefox builds even though they only added a few patches. Not familiar enough with what the foundation's been doing to tell whether there has been any change of the trademark policy. The website still treats the trademarks as if they were owned by Mozilla. Even if, such policies can change any minute.

Also, generally it's a good idea to rename a project when forking it to avoid confusion, even if you are not required to do so.

I feel that fairness owes it that if possible, it would be best to not refer to the name of the language in proper language features.

IMO version is not clear - is it crate version? Dep version? std version? cargo version? Rust version? Renaming kills two birds with one stone. (Or if we want to avoid trademark discussion, lang-version is fine.)

I like lang_version.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 12, 2021

@rfcbot cancel

Hi all. This has been in FCP for a very long time and so I am going to cancel the FCP (my personal philosophy is that FCPs should not last this long). I believe it is currently blocked waiting on progress around some kind of subset of cfg-accessible, as described by @joshtriplett here (or perhaps in comments that @joshtriplett is referencing there).

If somebody would like to move this issue forward: The next step would be to open a separate issue regarding that proposal and link it from here, and perhaps we can try to drive that work forward with mentorship or other means?

@rfcbot
Copy link

rfcbot commented Oct 12, 2021

@nikomatsakis proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 12, 2021
@joshtriplett
Copy link
Member

joshtriplett commented Jun 22, 2022

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

@joshtriplett joshtriplett added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. label Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-cfg_version `#![feature(cfg_version)]` S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests