Support -A, -W, -D and -F when running ./x.py clippy

[Milo123459]

Resolves #97059

This PR adds support for -A, -W, -D and -F when running ./x.py clippy.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[jyn514]

Rather than special casing these 4 flags, I wonder if it instead makes sense to add --test-args (or I guess --run-args?) so that we can pass arbitrary flags to clippy / rustc.

I think RUSTFLAGS may already do this? although that breaks a lot of the caching ... I wonder if we should expose RUST_LINT_FLAGS to users.

[Milo123459]

Maybe. Personally, I like having it work near the same as regular cargo, hence me making this PR.

[Mark-Simulacrum]

I am actually surprised to see cargo clippy supports passing these directly (cargo build/check/etc do not, today). I think I would prefer the route @jyn514 suggests here over us piping down arguments. The consistency argument is one I do not fully buy into, because we don't generally support every flag on any other Cargo command, and the full spectrum here does not seem significantly different.

In general given that Clippy is really only present in x.py for local usage and is not really intended to be well-supported beyond that, I prefer to keep its surface area quite minimal.

[jyn514]

I am actually surprised to see cargo clippy supports passing these directly (cargo build/check/etc do not, today).

@Mark-Simulacrum cargo-clippy is it's own binary distributed by clippy, it's not part of cargo itself. It has a few inconsistencies like this, another is that the sysroot detection is kind of hacky and occasionally needs help from bootstrap.

[Mark-Simulacrum]

Yeah, I'm aware. I don't think that changes the rest of my perspectives though.

[JohnCSimon]

Ping from triage:

@Milo123459
From what I can tell about the conversation it looks like this PR isn't something to move forward on - maybe close it?

[jyn514]

Rather than special casing these 4 flags, I wonder if it instead makes sense to add --test-args (or I guess --run-args?) so that we can pass arbitrary flags to clippy / rustc.

I think RUSTFLAGS may already do this? although that breaks a lot of the caching ... I wonder if we should expose RUST_LINT_FLAGS to users.

hmm, so normally this would be MAGIC_EXTRA_RUSTFLAGS, except that clippy bypasses our shim :/ #97443 (comment)

rust/src/bootstrap/bin/rustc.rs

Lines 156 to 160 in 471fa05

	if let Ok(flags) = env::var("MAGIC_EXTRA_RUSTFLAGS") {
	for flag in flags.split(' ') {
	cmd.arg(flag);
	}
	}

[Milo123459]

To be honest, I'm not really sure what to do, as Joshua said clippy doesn't work for the hack of directly passing arguments through so I'm not sure.

[jyn514]

@Mark-Simulacrum I think given that the only purpose of x.py clippy is to show warnings, it's reasonable to have the lint flags at the top-level. The other options are

• use RUSTFLAGS, which breaks caching
• make major changes to clippy to go through the shim; not sure if clippy team is ok with this, they steered me away from it in https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/.60x.2Epy.20clippy.60.20needs.20clippy.20changes/near/287541213
• expose RUST_LINT_FLAGS to users. This works, but seems confusing and hard to discover.

I think A/W/D/F are reasonably low maintenance and consistent with cargo clipppy.

[Mark-Simulacrum]

Okay, I'm not going to block merging this for clippy, but I do want to block any further expansion (i.e., this is not a blessing for it to be carried into check and other modes). Ultimately they are low maintenance, as you say, and consistent with how clippy works elsewhere, even if it's a little unfortunate it's necessary.

[jyn514]

Could you please squash the commits? https://rustc-dev-guide.rust-lang.org/git.html has instructions

[Milo123459]

Should be all good now

[jyn514]

Sorry for the delay. r=me with the comment addressed.

[jyn514]

@bors r+ rollup

[bors]

📌 Commit e3d84b2 has been approved by jyn514

It is now in the queue for this repository.