add a note that some warnings (and/or errors) can be auto-fixed

[Muscraft]

This adds a note that some warnings can be auto-fixed by running cargo --fix. This only supports cargo check as we can't show the same note for cargo clippy since it is an external subcommand.

The message for the note was taken from #10976 (comment).

[rust-highfive]

r? @ehuss

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

[ehuss]

I'm reluctant to add something like this for a few reasons:

• I never run cargo fix. Something like this will just be constant noise for me. I think it is good to be very cautious about being too verbose or too prodding.
• cargo fix often won't work correctly if there are any errors in the code.
• cargo fix won't work with changes in the user's repo. They have to run with --allow-dirty and/or --allow-staged, which can be awkward. Also, there is a moderate risk that cargo fix will damage or otherwise mess up the user's code with no way to revert (the reason --allow-dirty is there). If there is no git repo, then it is doubly more dangerous. Or if the files haven't been added yet, cargo fix may overwrite them without confirmation.
• Even filtering on machine-fixable errors, it is moderately common that the fix fails for one reason or another. I'm not particularly confident in pushing large numbers of people to use it more often. (Minor concern.)
• This doesn't really make it clear to the user which issues will be fixed.

I appreciate it can be difficult to educate people on the existence of tools or options for performing certain tasks. I'm wondering, though, does rust-analyzer not show a quick-fix for fixable suggestions? Perhaps that would be a better route to make it easier for people to apply suggestions, or otherwise have easy access to it?

[Muscraft]

I'm wondering, though, does rust-analyzer not show a quick-fix for fixable suggestions? Perhaps that would be a better route to make it easier for people to apply suggestions, or otherwise have easy access to it?

I haven't used rust-analyzer (in a while) but I know Clion (with the rust extension) gives quick fixes so I would assume RA does as well

I'm reluctant to add something like this for a few reasons:

Given the number of reasons, and the number of problems with cargo fix (which appear to be major blockers), it might be a good idea to close this and continue the discussion in #10976. I could see waiting on this until cargo fix has some issues resolved, or there is a new error/warning variant within rustc that would always be "fixable".

[epage]

Let's leave this open but move this to the Issue

[Muscraft]

I've updated this with feedback from #10976.
Changes:

• The note is added to the summary line as suggested here
• The command to run now includes the exact command to run i.e.

  warning: `foo` (example "ex1") generated 1 warning (run `cargo fix --example "ex1"` to apply suggestions)
  warning: `foo` (bench "bench") generated 1 warning (run `cargo fix --bench "bench" --tests` to apply suggestions)
  warning: `foo` (bin "foo" test) generated 2 warnings (run `cargo fix --bin "foo" --tests` to apply suggestions)
  

Concerns:

• Will adding this suggestion cause there to be too many issues filed and lots of work to fix things?
• Is the output message too verbose/specific
• Should the message only be shown once?

[weihanglo]

• Is the output message too verbose/specific

Looks fine to me, since Cargo already emits such note.

I am uncertain whether we need to make sure the suggested command is always runnable. A bad suggestion could be worse than no suggestion.

• Should the message only be shown once?

I don't get it. Could you expand a bit?

[Muscraft]

I am uncertain whether we need to make sure the suggested command is always runnable. A bad suggestion could be worse than no suggestion.

I agree that a bad suggestion is much worse than no suggestion and it's something that I tried to make sure wouldn't happen. That being said, I think having a specific command to run is much better.
Pros:

• It ensures that cargo fix is only being run on exactly what is giving the warning
• It makes sure that cargo fix is not being run on anything with an error
• Reduces unintended code churn
• Can clearly see what target/package the command is for
• Fixes applied target by target

Cons:

• The lines are very long
• Lots of new output
• Chance to give the wrong command
• Each command would need to be run separately (lots of cargo fix runs)
• Doesn't suggest --allow-dirty so the command would probably always be "wrong"

• Should the message only be shown once?

I don't get it. Could you expand a bit?

There is a chance that the long lines on potentially every summary line could be overwhelming. Having it output once per package(compared to target) would reduce this and make the command less specific. Going further and having it only be suggested once could make it so there is one place command is suggested and not affect every summary line. Having it be on its own line comes with its own problems but might make it easier to suggest a hyper-specific command (check version control status).

[weihanglo]

Looks fine! Thank you for the pull request.

[weihanglo]

Should we hand-pick field to deserialize? I see other places doing so.

Suggested change

	children: Vec<Diagnostic>,
	}
	children: Vec<Diagnostic>,
	}
	#[derive(serde::Deserialize)]
	struct Diagnostic {
	spans: Vec<Span>,
	// `children: Vec<Diagnostic>` if we need to check them recursively
	}
	#[derive(serde::Deserialize)]
	struct Span {
	suggestion_applicability: Option<Applicability>,
	}

BTW, I feel like every existing ad-hoc deserialization should have a comment telling which struct it refers to.

[Muscraft]

Should we hand-pick field to deserialize? I see other places doing so.

I'm not sure what you mean by this

[weihanglo]

I mean deserializing only those fields we need instead of the whole Diagnostic struct

[Muscraft]

We could do that but I was trying to use the available types from rustfix. I figured that since it had the ability to tell what things were it would be kept updated, instead of updating multiple places.

[weihanglo]

If there is an update of types in rustfix crate, either reuse or not we still need to update to some extent.

I was asking because other places deserialize fields-of-interest. Besides, there might be a performance hit if a package start emitting thousands of warnings with full struct fields, though I guess it is rare.

Anyway, I am good with reuse types from rustfix. Let's move on :)

[weihanglo]

This looks good to me.

According to the conclusion of the recent Cargo meeting, we decide to enable rustfix suggestions on nightly to collect feedback. I am going to merge it soon to proceed.

Just note that, one little concern I have is that Cargo now does full deserialization on Diagnostic struct. Not sure how it impacts the real world performance but I assume it is rare.

[weihanglo]

@bors r+

[bors]

📌 Commit 8ff8eaa has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8ff8eaa with merge 2d04bcd...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 2d04bcd to master...