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

Implement RFC3695: Allow boolean literals as cfg predicates #14649

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

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Oct 6, 2024

What does this PR try to resolve?

This PR implements rust-lang/rfcs#3695: allow boolean literals as cfg predicates, i.e. cfg(true) and cfg(false).

How should we test and review this PR?

The PR should be reviewed commit by commit and tested by looking at the tests and using [target.'cfg(<true/false>)'] in Cargo.toml/.cargo/config.toml.

Additional information

I had to bump cargo-platform to 0.2.0 has we are changing CfgExpr enum in a semver incompatible change.

I choose a use a Cargo.toml feature (for the manifest) as well as a unstable CLI flag for the .cargo/config.toml part.

Given the very small (two occurrences on Github Search) for cfg(true) and cfg(false), I choose to gate the feature under a error and not a warning.

  • Create and link the tracking issue for the Cargo side

@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
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-cfg-expr Area: Platform cfg expressions A-documenting-cargo-itself Area: Cargo's documentation A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2024
if let Ok(Platform::Cfg(cfg_expr)) = key.parse() {
cfg_expr.walk_expr(|e| match e {
CfgExpr::True | CfgExpr::False => {
if !gctx.cli_unstable().cfg_boolean_literals {
Copy link
Contributor

@epage epage Oct 7, 2024

Choose a reason for hiding this comment

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

Should we have a deprecation/future-incompat warning for Name("true") / Name("false") in existing versions, here and in toml?

If we do this, then no point in moving the if like I mentioned

Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, so long as a user doesn't set --cfg false, the use of #[cfg(false)] won't fundamentally change, so maybe we don't need a future-incompat for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we have a deprecation/future-incompat warning for Name("true") / Name("false") in existing versions, here and in toml?

We could, but I don't think it's worth it, we only have two users and I expect the RFC to become stable relatively quickly.

Strictly speaking, so long as a user doesn't set --cfg false, the use of #[cfg(false)] won't fundamentally change

Technically that's not true for rustc as #[cfg(false)] was never legal before the RFC.

Copy link
Contributor

Choose a reason for hiding this comment

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

Two users on github. That isn't a complete search of all code. I'm just hesitant about turning this into a hard error without any prior notice.

@weihanglo, thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Technically that's not true for rustc as #[cfg(false)] was never legal before the RFC.

If we treat it as a Cargo bug, we can literally ship this breaking change 😬.

Being conservative, I vote for warnings. We could give a “clear timeline” when it will become a hard error, and note that in our CHANGELOG.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder what other cfg value rustc disallows but Cargo is happy with. We may also fix them altogether.

Technically the syntax is called MetaItem and what is allowed inside the #[cfg(...)] attribute is defined by the MetaItemInner syntax, which accepts a MetaItem (paths, idents, ...) or an Expression, which can be a lot of things, the most interesting one is the literals one which has true | false 😄.

All of the other literals seems to be correctly rejected by Cargo.

I didn't test the other kinds of expressions but I suspect they will all be rejected as the Cargo parser doesn't like anything that isn't a ident or string.

However all of the keywords (async, return, while, ...) are NOT rejected by Cargo.
They are just regular idents for Cargo, like are true&false today.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we treat it as a Cargo bug, we can literally ship this breaking change 😬.

That's my position, I would consider the Cargo parser to be in-sufficient and buggy, and just ship the breaking change ...

Being conservative, I vote for warnings. We could give a “clear timeline” when it will become a hard error, and note that in our CHANGELOG.

... but that doesn't seems to be either of your position. 😄

Which leads me to ask, what kind of warning you want? A future-incompatibility warning with no change in behaviour? A warning but with change in behaviour? Or a future-incompatibility warning but with unstable flag and feature to change the behaviour?

That last one would be my preferred one, as it would allow nightly testing, but it would be the most involved as currently the parsing is done via <CfgExpr as FromStr>::from_str(&str) which doesn't take any options, which will would need to configure the parser behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think an unstable-opt-in is fine to change from the future-incompat warning to the new behavior.

Copy link
Member

Choose a reason for hiding this comment

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

We may also want to update the doc to note that some cfg values are not intended to use. Even if they are valid in Cargo.toml or .cargo/config.toml, they are unlikely to be picked by rustc.

There are two categories I can think of people might misuse:

  • [target.'cfg(true)'.dependencies] being a way to activate additional features
    • The [features] table is the right approach of it.
  • target.'cfg(ture).rustflags being a way to set/overwrite rustflags
    • cargo --config should be used instead, or RUSTFLAGS or CARGO_BUILD_RUSTFLAGS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #14671 to address this thread (as this PR is already quite large).

@epage
Copy link
Contributor

epage commented Oct 8, 2024

@bors
Copy link
Collaborator

bors commented Oct 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cfg-expr Area: Platform cfg expressions A-documenting-cargo-itself Area: Cargo's documentation A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants