Add cargo_cfg_target_family_multivalued FCW

[madsmtm]

Add cargo_cfg_target_family_multivalued lint which detects simple comparisons like == or match on the CARGO_CFG_TARGET_FAMILY environment variable. These are wrong, since CARGO_CFG_TARGET_FAMILY is multi-valued (currently only on wasm32-unknown-emscripten and wasm32-wali-linux-musl, so admittedly pretty rare).

This lint is inherently incomplete, because we can't track a value like this through arbitrary control flow. Regardless, I've found that it works pretty well in practice, from the various repos I took a look at that used CARGO_CFG_TARGET_FAMILY, I'd guess at least 80% of them would hit this lint.

This should unblock at some point adding #[cfg(target_family = "darwin")] as a replacement for #[cfg(target_vendor = "apple")], which in turns unblocks #100343.

There exist other multi-valued cfgs such as CARGO_CFG_TARGET_HAS_ATOMIC or CARGO_CFG_TARGET_FEATURE, but these were very clearly multi-valued from the beginning, and thus doesn't make sense to lint for.

This is my first time adding a lint, so beware that I'm a bit unsure if I did things right. Especially unsure if I should feature-gate the lint to start with, or if we should insta-stabilize it? I'm pretty sure it needs a lang FCP though.

r? compiler
CC @thomcc @workingjubilee

[madsmtm]

What do you think about the name? cargo_cfg_target_family_multivalued is kinda long, but I felt that shortening it risked loosing meaning. _multivalued isn't the best descriptor though, perhaps _direct_comparison would be better?

[madsmtm]

We could suggest .contains("unix") instead, that'd work as well. I felt .split(',').any(|x| x == "unix") was more principled, but I'd be fine with either.

[madsmtm]

Unsure if this is the intended use of a FCW? I would like it to trigger in dependents some day, but there wasn't precedent for using FutureReleaseSemanticsChange anywhere.

[madsmtm]

Just to summarize, I think the main questions for lang is:

• Do you still wish to deprecate target_vendor, even if the replacement involves making target_family multi-valued, which would break build scripts? (reaffirm).
• Do you think linting against this problem is enough?
• Are you happy with the name of the lint (cargo_cfg_target_family_multivalued)?

[joshtriplett]

We talked about this in today's @rust-lang/lang meeting.

This is a novel category of lint!

In the future if we have a framework for custom lints, this should use a custom lint. But in the meantime, we're willing to consider this.

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

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.

Finally, we'd like to get confirmation from @rust-lang/cargo that they would turn this lint on once it exists.

[joshtriplett]

(Please renominate for lang when ready per the above comment.)

[epage]

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.

[workingjubilee]

While I understand that it may have a weird "vibe", the purpose of this lint is for powering a migration that would affect usage of Rust in any platform-sensitive way. Thus it would be odd for it to require you to have anything but the minimal profile in order to detect it.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[madsmtm]

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.

[epage]

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).

Cargo has an unstable feature for multiple build scripts which is a stepping stone to having an artifact build-dependency that users tell Cargo to run as a build script. In both cases, the build script crate name is no longer build_script_build.

See rust-lang/cargo#14903

[joshtriplett]

@madsmtm We talked about this a bit more in today's meeting. We felt that:

1. While we're willing to do a lint for a purpose like this, it doesn't seem appropriate to check every Rust program for this when it's a special case for Cargo build scripts.
2. Eventually, we'd like this kind of thing to be the domain of custom lints, and in that case it would likewise need to be applied to specific code.

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.

In practice, I'd expect that it isn't going to be that common to move something from [dependencies] to [build-dependencies]. If Cargo wants to apply it to every single build it does, it could, but I think in practice it might be workable to turn it on specifically for build dependencies. That decision isn't any easier if lang makes it than if cargo does, and cargo has the option of being more scoped, if we provide an allow-by-default lint (and eventually a custom lint mechanism).