BREAKING CHANGE fix running path/to/cargo-clippy --fix

[matthiaskrgr]

Previously, we would assume clippy is called as "cargo clippy --arg1 --arg2" so we stripped the first two items and parsed "--arg1" and "--arg2" as cmdline args. The problem is when we run cargo-clippy binary directly as "target/debug/cargo-clippy --arg1 --arg2", we would still remove the first two args which would be "..cargo-clippy" and "--arg1" in this case which is wrong.

Fix this by checking if we run clippy via "cargo clippy" or "../path/to/cargo-clippy" and for the latter, only skip the first arg instead of two.

Spotted by Kraktus because lintcheck --fix was no longer working

This is potentially a breaking change because the "target/debug/cargo-clippy clippy --fix" will no longer work! Use "target/debug/cargo-clippy --fix" directly instead.

---

changelog: Important change: cargo-clippy will now no longer ignore the first argument. cargo-clippy clippy will still work as documented (the second clippy is ignored). However, arguments like --fix will issue a warning for now and will no longer be ignored in a few releases. cargo clippy (without -!) is unaffected by this change. #9461

[rust-highfive]

r? @giraffate

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

[matthiaskrgr]

so to recap, on current master: path/to/cargo-clippy --fix // this doesn't work but I would like it to cargo clippy --fix // this works cargo-clippy clippy --fix // this works but its super weird imo, I personally have no objections to dropping support but its probably better to keep backwards compatibility here :upside_down_face: path/to/cargo-clippy -- --fix // this also works cargo clippy -- --fix this does not work but I don't really care cargo-clippy -- --fix this works

[Jarcho]

I think the least breaking change we could do here is to check if the first argument is one of clippy's argument (currently --no-deps, --fix, -h, --help, -V, --version, --explain). This does leave an inconsistency with -- being ignored if it's the first argument, so it only half fixes the problem. cargo-clippy -- -W some-lint would still not work.

Alternatively, we could just treat anything with a leading dash as an argument. This will break some scripts, but it will make any direct use of cargo-clippy just work. I think this will make everything fail with an invalid argument error, rather than silently running with the wrong arguments.

[flip1995]

Doing a GitHub search I mostly find cargo-clippy clippy things. Or cargo-clippy --version, which works as expected. Also --help and --explain work as the first argument. I would say, as long as we keep supporting cargo-clippy clippy, we can just break every other usage of cargo-clippy where the second argument is used as a separator. I nominate the breakage discussion for the next meeting.

We also see cargo-clippy --all, which we should test if it works.

[flip1995]

Clippy meeting result:

• We don't want to hard break this now.
• We want to keep cargo-clippy clippy as it works now
• We want to emit warnings for all flags that are affected by this bug
  • --fix
  • --no-deps (not sure if this is affected)
  • Find out if other flags are affected
  • Emit a warning if cargo-clippy is followed by one of those flags, but don't change behavior yet
• After 1-3 release cycles, change the behavior and remove the warnings
  • doesn't have to happen in the same release cycle
  • We can remove the warnings a release cycle later

[xFrednet]

Oh, and we should probably add a big disclaimer to the changelog :)

[xFrednet]

I've now updated the changelog entry according to the meeting results listed above :)

[xFrednet]

I'm marking this as waiting on author, until the discussed changes have been implemented. (Correct me, if I'm wrong with my assessment)

@rustbot author

[xFrednet]

Hey @matthiaskrgr, I'd like to announce this change one or two changelogs, before the version that puts this into effect. Can you address the comments and make this PR ready to be merged? Then I could include it in the 1.67 changelog to be added in 1.69 :upside_down_face:

[flip1995]

cargo-clippy now expects clippy to be the first argument.

This is not correct. I was unclear / ambiguous in my summary.

The warning we want to emit for cargo-clippy --*s that exist today is that those flags will have an effect in the future. Currently they don't, because you need something (usually clippy) before the first flag.

We want to keep supporting cargo-clippy clippy as a special case to be backwards compatible. Currently cargo-clippy foo-bar-baz ... and cargo-clippy clippy ... are exactly the same. So what we want to change here is to only remove the first arg after cargo-clippy, if it is actually clippy.

[xFrednet]

That makes a lot of sense, I had the feeling my understanding was not quite correct. Could you take a second look at the suggested changelog entry?

[flip1995]

Updated the changelog.

[xFrednet]

I'll mark this as beta-accepted to make sure, that I include the deprecation message in the next changelog. It seems like I'm the one blocking this rn (Sorry :sweat_smile: )

@rustbot label +I-blocked -S-waiting-on-author +beta-accepted

[xFrednet]

Hey @matthiaskrgr, I'll mention this change in the next changelog, which unblocks this PR from being merged. Can you get this up to speed, with a rebase?

[matthiaskrgr]

Uuh i may need a bit to wrap my head around this again after 1.5 years I fear 😅

[xFrednet]

Very understandable ^^ (Sorry for the delay). The changelog doesn't mention a specific date or release, so there is no pressure from that side.

[flip1995]

In the last meeting we decided to not emit a warning, but rather break the behavior on a edition boundary.

[xFrednet]

Is there already a date for the edition? And what is the best way to find out when it's scheduled to be released?

[flip1995]

I don't think there's a date for the edition. Not sure who's the edition manager though.