Add cargo_cfg_target_family_multivalued FCW by madsmtm · Pull Request #147545 · rust-lang/rust (original) (raw)

The name should describe the behavior it catches, something like suspicious_cargo_cfg_target_family_comparisons.

I've changed the name to what you suggest. Do note that the lint also catches match env::var("CARGO_CFG_TARGET_FAMILY") { "unix" => ... }, not sure if you want to call that a "comparison", or if there's a more suitable name for that?

We feel that this shouldn't be warn-by-default; instead, it should be allow-by-default, and Cargo should change it to warn when building build.rs.

I considered going that route, but there exist build script helper crates such as cc-rs which we'd want to enable the lint on, but where neither rustc nor Cargo can know whether the crate will be used in a build script in the end. To see this, consider putting cc = "1.0" in the [dependencies] table, run cargo build, and then later move it to [build-dependencies]. Due to dependency sharing for host builds, cc-rs wouldn't be recompiled.

If we really only want this to only trigger in build scripts, there is precedent elsewhere in rustc for changing lints based on the crate name being "build_script_build", so I'd probably go that route instead (I also noted this in a code comment).

For that reason, I'll de-nominate t-cargo, and re-nominate t-lang (while this lints on a Cargo environment variable, this is ultimately a lang issue, Cargo "just" forwards the cfgs the language exposes).

Whats the line for a lint like this to be in rustc vs clippy? In seeing this, it feels like something I would see in clippy.

Clippy can't have FCWs, and we would like this to trigger for dependents because we would like to change this in the future.

If we didn't intend to add further cfg(target_family = "...") values, I'd agree that it fits better in Clippy.