Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Suggest cargo clippy --fix with trailing lint groups/levels arguments #11671

Open
weihanglo opened this issue Feb 3, 2023 · 3 comments
Open

Suggest cargo clippy --fix with trailing lint groups/levels arguments #11671

weihanglo opened this issue Feb 3, 2023 · 3 comments
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-bug Category: bug Command-clippy S-triage Status: This issue is waiting on initial triage.

Comments

@weihanglo
Copy link
Member

Problem

After running cargo clippy -- -W clippy::doc_markdown, Cargo suggests

warning: `cargo` (lib) generated 83 warnings (run `cargo clippy --fix --lib -p cargo` to apply 79 suggestions)

To make the suggestion work, I believe trailing cli arguments should be added. That is

cargo clippy --fix --lib -p cargo -- -W clippy::doc_markdown`.

Steps

  1. Switch the toolchain to the version mentioned below.
  2. Checkout rust-lang/cargo to 0b2c38f.
  3. Run cargo clippy -- -W clippy::doc_markdown.
  4. Copy the suggested commmand and run it.
  5. Nothing happen. Nothing fixed.

Possible Solution(s)

Suggest trailing varargs here? Not sure if it could be a bit overkill though.

cargo/src/cargo/core/compiler/job_queue.rs

Lines 1286 to 1320 in 0b2c38f

if let FixableWarnings::Positive(fixable) = count.fixable {
// `cargo fix` doesnt have an option for custom builds
if !unit.target.is_custom_build() {
// To make sure the correct command is shown for `clippy` we
// check if `RUSTC_WORKSPACE_WRAPPER` is set and pointing towards
// `clippy-driver`.
let clippy = std::ffi::OsStr::new("clippy-driver");
let command = match rustc_workspace_wrapper.as_ref().and_then(|x| x.file_stem())
{
Some(wrapper) if wrapper == clippy => "cargo clippy --fix",
_ => "cargo fix",
};
let mut args = {
let named = unit.target.description_named();
// if its a lib we need to add the package to fix
if unit.target.is_lib() {
format!("{} -p {}", named, unit.pkg.name())
} else {
named
}
};
if unit.mode.is_rustc_test()
&& !(unit.target.is_test() || unit.target.is_bench())
{
args.push_str(" --tests");
}
let mut suggestions = format!("{} suggestion", fixable);
if fixable > 1 {
suggestions.push_str("s")
}
drop(write!(
message,
" (run `{command} --{args}` to apply {suggestions})"
))
}

Notes

cc #11605 and @Muscraft

Version

cargo 1.69.0-nightly (e84a7928d 2023-01-31)
release: 1.69.0-nightly
commit-hash: e84a7928d93a31f284b497c214a2ece69b4d7719
commit-date: 2023-01-31
host: aarch64-apple-darwin
libgit2: 1.5.0 (sys:0.16.0 vendored)
libcurl: 7.79.1 (sys:0.4.59+curl-7.86.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
os: Mac OS 12.6.3 [64-bit]
@weihanglo weihanglo added C-bug Category: bug A-diagnostics Area: Error and warning messages generated by Cargo itself. Command-clippy labels Feb 3, 2023
@epage
Copy link
Contributor

epage commented Feb 3, 2023

With at least this and #11605, I wonder how well we can always produce the exact command. If not, I wonder what is the least confusing suggestion to the user, all the way from "show as much as we can" to "just suggest cargo-fix / --fix generically".

Another downside to the current solution is we can be overly prescriptive. When I run cargo clippy and need to apply fixes, I don't care about running the specialized subset but just run cargo clippy --fix, that is more helpful for tests when you want to compile/run faster and reduce output noise. The specificity is more of line noise to me. It will also fail if you run each suggested specific command as the working tree will be dirty.

@Muscraft
Copy link
Member

Muscraft commented Feb 3, 2023 •
edited
Loading

Finding the right thing to do is hard because of all the possible ways we could show the message.

There are three times to show the message:

  • Once per target (current)
    • This allows for commits to fix new clippy warnings to be per target
  • Once per crate
    • This allows for commits to fix new clippy warnings to be per crate
  • Once
    • This allows for only one commit to fix new clippy

There are two places to show the line:

  • In the summary line (current)
  • On its own line

There are quite a few ways to show the message, but they can be broken down into two categories:

  • Complex: 
    cargo clippy --fix  {extra info} -- -W clippy::doc_markdown
  • Simple: 
    cargo clippy --fix

With the large number of possible combinations, it would be interesting to allow a user to choose the output. That could be with CLI flags or something within config.toml. The only problem would then be to pick defaults. Giving the user choice in the matter could be the best possible path forward.

@epage
Copy link
Contributor

epage commented Feb 3, 2023

but they can be broken down into two categories:

Another visualization style left off is to refer to the feature without valid syntax, e.g. --fix or cargo-fix

As for where we show the message, I feel like the answer to that would be guided by what we show. I don't have strong opinions on what we show but feel we should be finding a good balance of helpful and not confusing.

With the large number of possible combinations, it would be interesting to allow a user to choose the output. T

imo I would want to see strong demand from users before making it configurable unless it was just for nightly to get feedback on different approaches. Configuration adds user and implementation complexity and this seems like an area that doesn't justify it.

@weihanglo weihanglo mentioned this issue Feb 9, 2023
Merged
@epage epage added the S-triage Status: This issue is waiting on initial triage. label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-bug Category: bug Command-clippy S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@epage @weihanglo @Muscraft