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

matthiaskrgr · 2022-09-10T22:21:53Z

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 · 2022-09-10T22:21:56Z

r? @giraffate

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

Alexendoo · 2022-09-10T22:26:24Z

src/main.rs

+    // if we run "cargo clippy" (as cargo subcommand), we have to strip "cargo clippy" (first 2 args)
+    // but if we are run via "..../target/debug/cargo-clippy", only ommit the first arg, the second one
+    // might be a normal cmdline arg already (which we don't want to ommit)
+    let args = if let Some(first_arg) = env::args().next() && (first_arg.ends_with("cargo-clippy") || first_arg.ends_with("cargo-clippy.exe")) {


We can avoid a breaking change by checking if the 2nd arg is clippy, rather than the first being an invocation of cargo-clippy[.exe]

ah so that cargo clippy but also cargo-clippy clippy still works? clever! :D

I'll revisit this tomorrow after a good portion of sleep :)

That doesn't avoid the breaking change. Anything that uses cargo-clippy -- would break. Or anything else for that matter.

That is what is happening for lintcheck for example

Ah right since any arg can currently be used as the separator

cargo clippy does execute with the first arg as .../cargo-clippy though, so we'd still need a different approach

execve("/home/alex/.cargo/bin/cargo", ["cargo", "clippy"], 0xffffd242db68 /* 49 vars */) = 0 execve("/home/alex/.rustup/toolchains/stable-aarch64-unknown-linux-gnu/bin/cargo", ["/home/alex/.rustup/toolchains/stable-aarch64-unknown-linux-gnu/bin/cargo", "clippy"], 0xaaaae39b8940 /* 54 vars */) = 0 execve("/home/alex/.cargo/bin/cargo-clippy", ["/home/alex/.cargo/bin/cargo-clippy", "clippy"], 0xaaab0d612f40 /* 57 vars */) = 0 execve("/home/alex/.rustup/toolchains/stable-aarch64-unknown-linux-gnu/bin/cargo-clippy", ["/home/alex/.rustup/toolchains/stable-aarch64-unknown-linux-gnu/bin/cargo-clippy", "clippy"], 0xaaaafea6a490 /* 57 vars */) = 0```

I just checked clippy 1.50 which was the lastest version when I wrote lintcheck 2 years ago and it looks like

RUSTC_BOOTSTRAP=1 ~/.rustup/toolchains/1.50-x86_64-unknown-linux-gnu/bin/cargo-clippy --fix -Zunstable-options

actually never worked as intended 😶 😅 😅

matthiaskrgr · 2022-09-11T11:35:05Z

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 🙃
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 · 2022-10-05T14:25:00Z

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 · 2022-10-15T15:47:49Z

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 · 2022-10-18T15:16:55Z

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 · 2022-10-18T18:49:42Z

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

There were several issues: - `--fix` was ignored as part of rust-lang#9461 - `--filter` in conjunction to `--fix` was broken due to rust-lang#9703 After `lintcheck --fix` is used, crates will be re-extracted since their content has been modified

xFrednet · 2022-12-14T23:06:23Z

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

xFrednet · 2022-12-17T14:18:40Z

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 · 2023-01-14T12:11:44Z

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 🙃

flip1995 · 2023-01-31T13:05:32Z

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 · 2023-02-01T20:11:34Z

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 · 2023-02-01T20:31:27Z

Updated the changelog.

xFrednet · 2024-03-21T15:36:13Z

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

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

xFrednet · 2024-04-29T19:38:56Z

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 · 2024-04-29T19:59:47Z

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

xFrednet · 2024-04-29T20:09:48Z

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 · 2024-05-15T18:24:39Z

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

xFrednet · 2024-05-15T19:30:06Z

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 · 2024-05-15T19:35:57Z

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

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: fix "cargo-clippy --foo" omitting the first flag, break "../cargo-clippy clippy --foo" which will now complain about the "clippy" arg.

…after edition 2024

matthiaskrgr · 2024-05-15T21:43:04Z

src/main.rs

-    if let Err(code) = process(env::args().skip(2)) {
+    // if we run "cargo clippy" (as cargo subcommand), we have to strip "cargo clippy" (first 2 args)
+    // but if we are run via "..../target/debug/cargo-clippy", only ommit the first arg, the second one
+    // might be a normal cmdline arg already (which we don't want to ommit)
+    let args = if let Some(first_arg) = env::args().next()
+        && (first_arg.ends_with("cargo-clippy") || first_arg.ends_with("cargo-clippy.exe"))
+    {
+        // turn this of during the migration
+        // env::args().skip(1)
+        eprintln!(
+            "WARNING: potentially breaking change: 'cargo-clippy arg1 arg2...' will no longer ignore 'arg1' after edition=2024"
+        );
+        env::args().skip(2)
+    } else {
+        env::args().skip(2)
+    };
+
+    if let Err(code) = process(args) {


bors · 2024-08-06T15:08:56Z

☔ The latest upstream changes (presumably #13225) made this pull request unmergeable. Please resolve the merge conflicts.

rust-highfive assigned giraffate Sep 10, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 10, 2022

Alexendoo reviewed Sep 10, 2022

View reviewed changes

kraktus mentioned this pull request Sep 11, 2022

fix clippy arguments when running lintcheck matthiaskrgr/rust-clippy#1

Open

flip1995 added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Oct 15, 2022

Alexendoo mentioned this pull request Oct 28, 2022

improve lintcheck #9731

Closed

flip1995 removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Nov 1, 2022

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 17, 2022

xFrednet removed the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 29, 2024

xFrednet added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 29, 2024

xFrednet mentioned this pull request Apr 29, 2024

Changelog for Clippy 1.78 🪄 #12735

Merged

flip1995 added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label May 2, 2024

flip1995 removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label May 15, 2024

matthiaskrgr added 4 commits May 15, 2024 23:14

fix test fallout

83fa8f8

account for windows .exe

b9b12d4

add warning that "cargo-clippy arg1 arg2" will no longer ignore arg1 …

56d2dd9

…after edition 2024

matthiaskrgr force-pushed the ARGH branch from 72a5201 to 56d2dd9 Compare May 15, 2024 21:42

matthiaskrgr commented May 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

matthiaskrgr commented Sep 10, 2022 •

edited by flip1995

Loading

rust-highfive commented Sep 10, 2022

Alexendoo Sep 10, 2022 •

edited

Loading

matthiaskrgr Sep 10, 2022

Jarcho Sep 10, 2022

kraktus Sep 11, 2022

Alexendoo Sep 11, 2022

Alexendoo Sep 11, 2022

matthiaskrgr Sep 11, 2022

matthiaskrgr commented Sep 11, 2022 •

edited

Loading

Jarcho commented Oct 5, 2022

flip1995 commented Oct 15, 2022 •

edited

Loading

flip1995 commented Oct 18, 2022

xFrednet commented Oct 18, 2022

xFrednet commented Dec 14, 2022

xFrednet commented Dec 17, 2022 •

edited

Loading

xFrednet commented Jan 14, 2023

flip1995 commented Jan 31, 2023 •

edited

Loading

xFrednet commented Feb 1, 2023

flip1995 commented Feb 1, 2023

xFrednet commented Mar 21, 2024

xFrednet commented Apr 29, 2024 •

edited

Loading

matthiaskrgr commented Apr 29, 2024

xFrednet commented Apr 29, 2024

flip1995 commented May 15, 2024

xFrednet commented May 15, 2024

flip1995 commented May 15, 2024

matthiaskrgr May 15, 2024

bors commented Aug 6, 2024

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

Are you sure you want to change the base?

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

Conversation

matthiaskrgr commented Sep 10, 2022 • edited by flip1995 Loading

rust-highfive commented Sep 10, 2022

Alexendoo Sep 10, 2022 • edited Loading

Choose a reason for hiding this comment

matthiaskrgr Sep 10, 2022

Choose a reason for hiding this comment

Jarcho Sep 10, 2022

Choose a reason for hiding this comment

kraktus Sep 11, 2022

Choose a reason for hiding this comment

Alexendoo Sep 11, 2022

Choose a reason for hiding this comment

Alexendoo Sep 11, 2022

Choose a reason for hiding this comment

matthiaskrgr Sep 11, 2022

Choose a reason for hiding this comment

matthiaskrgr commented Sep 11, 2022 • edited Loading

Jarcho commented Oct 5, 2022

flip1995 commented Oct 15, 2022 • edited Loading

flip1995 commented Oct 18, 2022

xFrednet commented Oct 18, 2022

xFrednet commented Dec 14, 2022

xFrednet commented Dec 17, 2022 • edited Loading

xFrednet commented Jan 14, 2023

flip1995 commented Jan 31, 2023 • edited Loading

xFrednet commented Feb 1, 2023

flip1995 commented Feb 1, 2023

xFrednet commented Mar 21, 2024

xFrednet commented Apr 29, 2024 • edited Loading

matthiaskrgr commented Apr 29, 2024

xFrednet commented Apr 29, 2024

flip1995 commented May 15, 2024

xFrednet commented May 15, 2024

flip1995 commented May 15, 2024

matthiaskrgr May 15, 2024

Choose a reason for hiding this comment

bors commented Aug 6, 2024

matthiaskrgr commented Sep 10, 2022 •

edited by flip1995

Loading

Alexendoo Sep 10, 2022 •

edited

Loading

matthiaskrgr commented Sep 11, 2022 •

edited

Loading

flip1995 commented Oct 15, 2022 •

edited

Loading

xFrednet commented Dec 17, 2022 •

edited

Loading

flip1995 commented Jan 31, 2023 •

edited

Loading

xFrednet commented Apr 29, 2024 •

edited

Loading