rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.96k stars 1.57k forks source link

RFC: Allow boolean literals as `cfg` predicates #3695

Closed clubby789 closed 1 month ago

clubby789 commented 2 months ago

This RFC proposes allowing boolean literals as cfg predicates, e.g. cfg(true) and cfg(false).


Rendered

Tracking:

clarfonthey commented 2 months ago

Also worth noting that, in addition to cfg(any()) meaning cfg(false), cfg(all()) means cfg(true) as well. So, both technically have easy versions, but I agree that true and false care clearer.

joshtriplett commented 2 months ago

This is simple, straightforward, solves an existing problem, and has no compatibility issues.

@rfcbot merge

rfcbot commented 2 months ago

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

kennytm commented 2 months ago

the RFC should mention somewhere that the raw identifiers r#true and r#false continue to work:

rustc --cfg false --check-cfg 'cfg(r#false, values(none()))' 1.rs
#     ^~~~~~~~~~~
// 1.rs
#![deny(warnings)]

#[expect(unexpected_cfgs)]
mod a {
  #[cfg(r#true)]
  pub fn foo() {}
}

mod b {
  #[cfg(r#false)]
  pub fn bar() {}
}

fn main() { 
    b::bar()
}
nikomatsakis commented 2 months ago

Yes please. I'm always surprised I can't do cfg(false).

@rfcbot reviewed

clubby789 commented 2 months ago

Added a note about r#true/r#false

clubby789 commented 2 months ago

Also worth noting that, in addition to cfg(any()) meaning cfg(false), cfg(all()) means cfg(true) as well.

Mentioned this in a couple of places for completeness

lolbinarycat commented 2 months ago

probably should also mention cfg_attr for completeness

traviscross commented 2 months ago

@rfcbot reviewed

In particular, now that we're warning about #[cfg(False)], #[cfg(FALSE)], etc., this is somewhat more pressing.

rfcbot commented 2 months ago

:bell: This is now entering its final comment period, as per the review above. :bell:

kennytm commented 2 months ago

probably should also mention cfg_attr for completeness

speaking of which i suppose these are also available in Cargo.toml's and .cargo/config.toml's [target.<cfg>] sections.

clubby789 commented 2 months ago

Would changing Cargo need a separate RFC? Currently, Cargo seems to accept these:

warning: unused manifest key: target.cfg(false).rustflags

so changing that could maybe break code - not sure if you can pass custom cfg values to Cargo itself?

kennytm commented 2 months ago

not sure if you can pass custom cfg values to Cargo itself?

uh oh. according to rust-lang/cargo#5313 injecting a custom cfg via RUSTFLAGS is supported.

lolbinarycat commented 2 months ago

so changing that could maybe break code

I'm pretty sure adding new manifest keys is not considered a breaking change.

CAD97 commented 2 months ago

Also, it's worth noting that we've introduced and gated new well-known cfg (e.g. cfg(ub_checks)) before without edition gates. That doesn't itself justify doing so again, but in the case of Cargo's cfg not matching rustc's (does Cargo gate everything that rustc does?) I believe fixing the incongruity would be allowed as a bug fix even if it's technically breaking.

workingjubilee commented 2 months ago

I agree we should just send bad news for anyone who was using cfg(false) and cfg(true) for any meaning beyond the obvious one.

kennytm commented 2 months ago

current situation of cfg-like constructs

false r#false
Rust language
(#[cfg], cfg!, etc)
❌ `cfg` predicate key cannot be a literal
rustc --cfg
rustc --check-cfg ❌ invalid `--check-cfg` argument: `false` is a boolean literal
cargo [target.<cfg>] ❌ failed to parse `r#false` as a cfg expression: unexpected content `#false` found after cfg expression

I think cargo should be made to accept raw identifiers, but this is indeed really out-of-scope for this RFC.

tmandry commented 2 months ago

Thanks for this RFC, I've wanted this for awhile!

@rfcbot reviewed

T0mstone commented 2 months ago

Another future possibility that addresses the drawback: There could be an allow-by-default lint for using these, so you could #![deny(cfg_boolean_literal)] (or whatever it'd be called).

lolbinarycat commented 2 months ago

note that lint names should be plural if they are nouns

joshtriplett commented 1 month ago

We discussed this in today's @rust-lang/cargo meeting. We confirmed that we generally want Cargo to implement every cfg that rustc does, to avoid confusion, and that we anticipate this being easy to implement.

rfcbot commented 1 month ago

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

clubby789 commented 1 month ago

Updated the name to cfg_boolean_literals: (https://github.com/rust-lang/rust/pull/131034#discussion_r1780710463)

traviscross commented 1 month ago

The team has accepted this RFC, and it's now been merged.

Thanks to @clubby789 for writing this up and pushing it forward.

For further updates, follow the tracking issue: