Skip to content

Stabilize cfg_boolean_literals #138632

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

Merged
merged 3 commits into from
Apr 17, 2025
Merged

Conversation

clubby789
Copy link
Contributor

@clubby789 clubby789 commented Mar 18, 2025

Closes #131204
@rustbot labels +T-lang +I-lang-nominated
This will end up conflicting with the test in #138293 so whichever doesn't land first will need updating

--

Stabilization Report

General design

What is the RFC for this feature and what changes have occurred to the user-facing design since the RFC was finalized?

RFC 3695, none.

What behavior are we committing to that has been controversial? Summarize the major arguments pro/con.

None

Are there extensions to this feature that remain unstable? How do we know that we are not accidentally committing to those?

None

Has a call-for-testing period been conducted? If so, what feedback was received?

Yes; only positive feedback was received.

Implementation quality

Summarize the major parts of the implementation and provide links into the code (or to PRs)

Implemented in #131034.

Summarize existing test coverage of this feature

What outstanding bugs in the issue tracker involve this feature? Are they stabilization-blocking?

The above mentioned issue; it should not block as it interacts with another unstable feature.

What FIXMEs are still in the code for that feature and why is it ok to leave them there?

None

Summarize contributors to the feature by name for recognition and assuredness that people involved in the feature agree with stabilization

Which tools need to be adjusted to support this feature. Has this work been done?

rustdoc's unstable#[doc(cfg(..)] has been updated to respect it. cargo has been updated with a forward compatibility lint to enable supporting it in cargo once stabilized.

Type system and execution rules

What updates are needed to the reference/specification? (link to PRs when they exist)

A few lines to be added to the reference for configuration predicates, specified in the RFC.

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2025

r? @davidtwco

rustbot has assigned @davidtwco.
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

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 18, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2025

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

@rustbot rustbot added I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Mar 18, 2025
@traviscross traviscross removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 18, 2025
@traviscross
Copy link
Contributor

traviscross commented Mar 18, 2025

@clubby789: If you could, please link in the stabilization report to each test that we would want to see to review the behavior of this feature and the edges of it, and describe what each is intended to demonstrate if that's not already included as a comment in the test itself.

@clubby789
Copy link
Contributor Author

Added a couple of new tests with the edge cases described, and descriptions of the test coverage to the report

@tmandry
Copy link
Member

tmandry commented Mar 19, 2025

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 19, 2025

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

Concerns:

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.

@rfcbot rfcbot added 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 Mar 19, 2025
@traviscross
Copy link
Contributor

traviscross commented Mar 19, 2025

In testing, I notice that this works, as expected:

//@ check-pass
//@ compile-flags: --cfg r#false --check-cfg=cfg(r#false)
fn main() {
    #[cfg(not(r#false))]
    compile_error!("");
}

And this works:

//@ check-pass
//@ compile-flags: --cfg false --check-cfg=cfg(r#false)
fn main() {
    #[cfg(not(r#false))]
    compile_error!("");
}

But that this doesn't work:

//@ check-pass
//@ compile-flags: --cfg false --check-cfg=cfg(false)
fn main() {
    #[cfg(not(r#false))]
    compile_error!("");
}

What's the reason that it's OK for --check-cfg to require r#false when it's not for --cfg?

@traviscross
Copy link
Contributor

@rfcbot concern question-on-check-cfg
@rfcbot reviewed

@joshtriplett
Copy link
Member

We should definitely ensure that --cfg and --check-cfg accept the same things, consistently.

We may wish to add a warning when you apply either of those to a keyword, since in code that would need to be matched with r#, and it would be clearer if the command-line likewise used the r# syntax.

(None of these should ever affect the behavior of cfg(false) or cfg(true), only cfg(r#false) and cfg(r#true).)

@traviscross
Copy link
Contributor

We discussed this today in the lang triage meeting. We believe this test should pass, exactly as it does for cfg(r#while):

//@ check-pass
//@ revisions: r0x0 r0x1 r1x0 r1x1
//@[r0x0] compile-flags: --cfg false --check-cfg=cfg(false)
//@[r0x1] compile-flags: --cfg false --check-cfg=cfg(r#false)
//@[r1x0] compile-flags: --cfg r#false --check-cfg=cfg(false)
//@[r1x1] compile-flags: --cfg r#false --check-cfg=cfg(r#false)
#![deny(unexpected_cfgs)]
fn main() {
    #[cfg(not(r#false))]
    compile_error!("");
}

Please add a test to this effect.

@Urgau
Copy link
Member

Urgau commented Mar 19, 2025

What's the reason that it's OK for --check-cfg to require r#false when it's not for --cfg?

In --cfg false, r#false is the path of the MetaItem.

While in --check-cfg cfg(false), false is the boolean literal false.

And since a path can be a identifier --cfg false becomes r#false, while false is a literal, which can't be an identifier.

@traviscross
Copy link
Contributor

@Urgau: Do you think that false, as a literal, should be treated differently than non-literal keywords like while in this regard? I could maybe see a case for that.

@Urgau
Copy link
Member

Urgau commented Mar 19, 2025

I think we could justify the boolean literals being treated differently and accept them in --check-cfg on the basis of consistency with --cfg.

while and other keywords are also an interesting case, we do accept them without raw-ness in --cfg while and --check-cfg cfg(while) but we only accept them with raw in the attribute #[cfg(r#while)].

We could try to be more consistent in --cfg and --check-cfg by only accepting the same form that will be accepted in the attribute, so only accept r#false and r#while in --cfg and --check-cfg, that would make them consistent with the #[cfg] attribute.

@traviscross
Copy link
Contributor

Yes. Since we already accept while in both --cfg and --check-cfg, it still sounds right then to do the same for true and false. If we ever decide to tighten that up for other keywords over an edition, we'd handle then for this as well.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Mar 30, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 30, 2025

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 30, 2025
@rust-cloud-vms rust-cloud-vms bot force-pushed the stabilize-cfg-boolean-lit branch from 45763c3 to 7c441c4 Compare March 30, 2025 08:24
@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 2, 2025
@bors
Copy link
Collaborator

bors commented Apr 3, 2025

☔ The latest upstream changes (presumably #139137) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-cloud-vms rust-cloud-vms bot force-pushed the stabilize-cfg-boolean-lit branch from 7c441c4 to 21fa62e Compare April 3, 2025 18:12
@rust-log-analyzer

This comment has been minimized.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 3, 2025
Allow boolean literals in `check-cfg`

rust-lang#138632 (comment)
This makes it consistent with `--cfg`

We could alternatively add a forward-compatible lint against `--cfg true/false`
r? `@Urgau`
@rust-cloud-vms rust-cloud-vms bot force-pushed the stabilize-cfg-boolean-lit branch from 21fa62e to 303c1b4 Compare April 3, 2025 21:42
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 4, 2025
Rollup merge of rust-lang#138767 - clubby789:check-cfg-bool, r=Urgau

Allow boolean literals in `check-cfg`

rust-lang#138632 (comment)
This makes it consistent with `--cfg`

We could alternatively add a forward-compatible lint against `--cfg true/false`
r? `@Urgau`
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 9, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 9, 2025

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.

@traviscross
Copy link
Contributor

traviscross commented Apr 17, 2025

On review, everything looks right to me at this point, so let's...

@bors r=davidtwco,Urgau,traviscross rollup

@bors
Copy link
Collaborator

bors commented Apr 17, 2025

📌 Commit 303c1b4 has been approved by davidtwco,Urgau,traviscross

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2025
@traviscross traviscross added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 17, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#138632 (Stabilize `cfg_boolean_literals`)
 - rust-lang#139416 (unstable book; document `macro_metavar_expr_concat`)
 - rust-lang#139782 (Consistent with treating Ctor Call as Struct in liveness analysis)
 - rust-lang#139885 (document RUSTC_BOOTSTRAP, RUSTC_OVERRIDE_VERSION_STRING, and -Z allow-features in the unstable book)
 - rust-lang#139904 (Explicitly annotate edition for `unpretty=expanded` and `unpretty=hir` tests)
 - rust-lang#139932 (transmutability: Refactor tests for simplicity)
 - rust-lang#139944 (Move eager translation to a method on Diag)
 - rust-lang#139948 (git: ignore `60600a6fa403216bfd66e04f948b1822f6450af7` for blame purposes)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0de803c into rust-lang:master Apr 17, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 17, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2025
Rollup merge of rust-lang#138632 - clubby789:stabilize-cfg-boolean-lit, r=davidtwco,Urgau,traviscross

Stabilize `cfg_boolean_literals`

Closes rust-lang#131204
`@rustbot` labels +T-lang +I-lang-nominated
This will end up conflicting with the test in rust-lang#138293 so whichever doesn't land first will need updating

--

# Stabilization Report

## General design

### What is the RFC for this feature and what changes have occurred to the user-facing design since the RFC was finalized?

[RFC 3695](rust-lang/rfcs#3695), none.

### What behavior are we committing to that has been controversial? Summarize the major arguments pro/con.

None

### Are there extensions to this feature that remain unstable? How do we know that we are not accidentally committing to those?

None

## Has a call-for-testing period been conducted? If so, what feedback was received?

Yes; only positive feedback was received.

## Implementation quality

### Summarize the major parts of the implementation and provide links into the code (or to PRs)

Implemented in [rust-lang#131034](rust-lang#131034).

### Summarize existing test coverage of this feature

- [Basic usage, including `#[cfg()]`, `cfg!()` and `#[cfg_attr()]`](https://github.com/rust-lang/rust/blob/6d71251cf9e40326461f90f8ff9a7024706aea87/tests/ui/cfg/true-false.rs)
- [`--cfg=true/false` on the command line being accessible via `r#true/r#false`](https://github.com/rust-lang/rust/blob/6d71251cf9e40326461f90f8ff9a7024706aea87/tests/ui/cfg/raw-true-false.rs)
- [Interaction with the unstable `#[doc(cfg(..))]` feature](https://github.com/rust-lang/rust/tree/6d71251/tests/rustdoc-ui/cfg-boolean-literal.rs)
- [Denying `--check-cfg=cfg(true/false)`](https://github.com/rust-lang/rust/tree/6d71251/tests/ui/check-cfg/invalid-arguments.rs)
- Ensuring `--cfg false` on the command line doesn't change the meaning of `cfg(false)`: `tests/ui/cfg/cmdline-false.rs`
- Ensuring both `cfg(true)` and `cfg(false)` on the same item result in it being disabled: `tests/ui/cfg/both-true-false.rs`

### What outstanding bugs in the issue tracker involve this feature? Are they stabilization-blocking?

The above mentioned issue; it should not block as it interacts with another unstable feature.

### What FIXMEs are still in the code for that feature and why is it ok to leave them there?

None

### Summarize contributors to the feature by name for recognition and assuredness that people involved in the feature agree with stabilization
- `@clubby789` (RFC)
- `@Urgau` (Implementation in rustc)

### Which tools need to be adjusted to support this feature. Has this work been done?

`rustdoc`'s  unstable`#[doc(cfg(..)]` has been updated to respect it. `cargo` has been updated with a forward compatibility lint to enable supporting it in cargo once stabilized.

## Type system and execution rules

### What updates are needed to the reference/specification? (link to PRs when they exist)

A few lines to be added to the reference for configuration predicates, specified in the RFC.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for RFC 3695: Allow boolean literals as cfg predicates