Skip to content

simplify check_cfg_arg impl #147570

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

simplify check_cfg_arg impl #147570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions compiler/rustc_session/src/config/cfg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
//!
//! ## Adding a new cfg
//!
//! Adding a new feature requires two new symbols one for the cfg itself
//! and the second one for the unstable feature gate, those are defined in
//! Adding a new feature requires two new symbols: one for the cfg itself
//! and the second one for the unstable feature gate; those are defined in
//! `rustc_span::symbol`.
//!
//! As well as the following points,
Expand Down Expand Up @@ -107,8 +107,8 @@ pub(crate) fn disallow_cfgs(sess: &Session, user_cfgs: &Cfg) {
//
// The tests are in tests/ui/cfg/disallowed-cli-cfgs.rs.

// By-default all builtin cfgs are disallowed, only those are allowed:
// - test: as it makes sense to the have the `test` cfg active without the builtin
// By default all builtin cfgs are disallowed, but these are allowed:
// - test: as it makes sense to have the `test` cfg active without the builtin
// test harness. See Cargo `harness = false` config.
//
// Cargo `--cfg test`: https://github.com/rust-lang/cargo/blob/bc89bffa5987d4af8f71011c7557119b39e44a65/src/cargo/core/compiler/mod.rs#L1124
Expand Down
4 changes: 2 additions & 2 deletions src/bootstrap/src/core/builder/cargo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,15 +687,15 @@ impl Builder<'_> {

for (restricted_mode, name, values) in EXTRA_CHECK_CFGS {
if restricted_mode.is_none() || *restricted_mode == Some(mode) {
rustflags.arg(&check_cfg_arg(name, *values));
rustflags.arg(&check_cfg_arg(name, values));

if *name == "bootstrap" {
// Cargo doesn't pass RUSTFLAGS to proc_macros:
// https://github.com/rust-lang/cargo/issues/4423
// Thus, if we are on stage 0, we explicitly set `--cfg=bootstrap`.
// We also declare that the flag is expected, which we need to do to not
// get warnings about it being unexpected.
hostflags.arg(check_cfg_arg(name, *values));
hostflags.arg(check_cfg_arg(name, values));
}
}
}
Expand Down
15 changes: 7 additions & 8 deletions src/bootstrap/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,13 @@ const LLD_FILE_NAMES: &[&str] = &["ld.lld", "ld64.lld", "lld-link", "wasm-ld"];

/// Extra `--check-cfg` to add when building the compiler or tools
/// (Mode restriction, config name, config values (if any))
#[expect(clippy::type_complexity)] // It's fine for hard-coded list and type is explained above.
const EXTRA_CHECK_CFGS: &[(Option<Mode>, &str, Option<&[&'static str]>)] = &[
(Some(Mode::Rustc), "bootstrap", None),
(Some(Mode::Codegen), "bootstrap", None),
(Some(Mode::ToolRustcPrivate), "bootstrap", None),
(Some(Mode::ToolStd), "bootstrap", None),
(Some(Mode::ToolRustcPrivate), "rust_analyzer", None),
(Some(Mode::ToolStd), "rust_analyzer", None),
const EXTRA_CHECK_CFGS: &[(Option<Mode>, &str, &[&str])] = &[
(Some(Mode::Rustc), "bootstrap", &[]),
(Some(Mode::Codegen), "bootstrap", &[]),
(Some(Mode::ToolRustcPrivate), "bootstrap", &[]),
(Some(Mode::ToolStd), "bootstrap", &[]),
(Some(Mode::ToolRustcPrivate), "rust_analyzer", &[]),
(Some(Mode::ToolStd), "rust_analyzer", &[]),
Comment on lines +84 to +90
Copy link
Member

@Urgau Urgau Oct 12, 2025

Choose a reason for hiding this comment

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

What do you think about having &[Option<&str>] for representing the expected values? which would make the none() part explicit.

The generation part would then always be like that cfg(name, values(...)).

Copy link
Member Author

@hkBst hkBst Oct 12, 2025

Choose a reason for hiding this comment

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

That would allow &[None, None, None] which seems silly...

Copy link
Member

@Urgau Urgau Oct 12, 2025

Choose a reason for hiding this comment

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

It would indeed be silly to have multiple None, but it's better than not having any None (to represent none()).

// Any library specific cfgs like `target_os`, `target_arch` should be put in
// priority the `[lints.rust.unexpected_cfgs.check-cfg]` table
// in the appropriate `library/{std,alloc,core}/Cargo.toml`
Expand Down
23 changes: 7 additions & 16 deletions src/bootstrap/src/utils/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,22 +490,13 @@ where
})
}

/// Create a `--check-cfg` argument invocation for a given name
/// and it's values.
pub fn check_cfg_arg(name: &str, values: Option<&[&str]>) -> String {
// Creating a string of the values by concatenating each value:
// ',values("tvos","watchos")' or '' (nothing) when there are no values.
let next = match values {
Some(values) => {
let mut tmp = values.iter().flat_map(|val| [",", "\"", val, "\""]).collect::<String>();

tmp.insert_str(1, "values(");
tmp.push(')');
tmp
}
None => "".to_string(),
};
format!("--check-cfg=cfg({name}{next})")
/// Create a `--check-cfg` argument invocation for a given name and values.
pub fn check_cfg_arg(name: &str, values: &[&str]) -> String {
if values.is_empty() {
format!("--check-cfg=cfg({name})")
} else {
format!("--check-cfg=cfg({name},values(\"{}\"))", values.join("\",\""))
}
}
Comment on lines +494 to 500
Copy link
Member

@Urgau Urgau Oct 12, 2025

Choose a reason for hiding this comment

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

This is not technically correct, having an empty values() is valid and has the semantic of defining a cfg that has no expected values (and thus will always warn).

But it doesn't really make sense for bootstrap to be defining such cfgs.

Copy link
Member Author

@hkBst hkBst Oct 12, 2025

Choose a reason for hiding this comment

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

I read about this, but it seems really weird that values() makes the cfg name unusable(?) and what accepts an empty value is values(none()).

Copy link
Member

@Urgau Urgau Oct 12, 2025

Choose a reason for hiding this comment

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

I read about this, but it seems really weird that values() makes the cfg name unusable(?) and what accepts an empty value is values(none()).

cfgs are always a combination of name+value, if one doesn't pass any values the compiler doesn't know which values are expected.

It's useful for cfgs whose values are variable (like Cargo feature cfg). It would be weird to get a expected cfg name warning (when for example there are no Cargo features) while it's actually the value that is unexpected.

Copy link
Member Author

@hkBst hkBst Oct 12, 2025

Choose a reason for hiding this comment

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

I thought cfgs could also have no value, like #[cfg(debug_assertions)], or #[cfg(test)]. But I'm a bit confused now, so let me try and clarify my understanding of what you wrote.

But it doesn't really make sense for bootstrap to be defining such cfgs.

Right, so then perhaps it is good that the function now makes it impossible to define such cfgs? Or did I misunderstand you?

Copy link
Member

@Urgau Urgau Oct 12, 2025

Choose a reason for hiding this comment

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

I thought cfgs could also have no value, like #[cfg(debug_assertions)], or #[cfg(test)].

Yes, of course cfgs can have "no value". I was talking about --check-cfg and the fact that a "no value" cfg still has to represented, that's why a "no value" value is represented as a none() values in --check-cfg.

Right, so then perhaps it is good that the function now makes it impossible to define such cfgs? Or did I misunderstand you?

Your proposed representation indeed makes it impossible to define a cfg in --check-cfg that has no expected values, but it does so by confusing the no expected values case of --check-cfg and the "no value" of a cfg.

I other words I think EXTRA_CHECK_CFGS should be as simple as possible and making the empty slice represent the "no value", is IMO highly problematic as it conflicts with the valid semantic of the empty values values() (no expected values) in --check-cfg.


/// Prepares `BootstrapCommand` that runs git inside the source directory if given.
Expand Down
6 changes: 3 additions & 3 deletions src/bootstrap/src/utils/helpers/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ fn test_string_to_hex_encode() {

#[test]
fn test_check_cfg_arg() {
assert_eq!(check_cfg_arg("bootstrap", None), "--check-cfg=cfg(bootstrap)");
assert_eq!(check_cfg_arg("bootstrap", &[]), "--check-cfg=cfg(bootstrap)");
assert_eq!(
check_cfg_arg("target_arch", Some(&["s360"])),
check_cfg_arg("target_arch", &["s360"]),
"--check-cfg=cfg(target_arch,values(\"s360\"))"
);
assert_eq!(
check_cfg_arg("target_os", Some(&["nixos", "nix2"])),
check_cfg_arg("target_os", &["nixos", "nix2"]),
"--check-cfg=cfg(target_os,values(\"nixos\",\"nix2\"))"
);
}
Expand Down
Loading