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

Stabilize cfg_boolean_literals #138632

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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

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.

@clubby789
Copy link
Contributor Author

Please add a test to this effect.

Made a PR to allow this

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stabilisation impl LGTM, r=me after t-lang FCP concludes

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. I-lang-nominated Nominated for discussion during a lang team meeting. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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