Add explanations and suggestions to irrefutable_let_patterns lint

[camelid]

Fixes#79716.

[rust-highfive]

r? @varkor

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

[camelid]

Ugh, x.py is rebuilding LLVM for some reason 😩

[ehuss]

My preference would be to not add one-off links like this. Ideally, every warning should (automatically) have some way to get more information. The issue is that it is not clear exactly what that should look like. Some options that were considered:

• Add an HTTP link similar to this PR. This has several drawbacks:
  • Links are release-specific. It could always link to nightly, but that would cause problems with renamed lints.
  • Doesn't work offline.
  • Is very long and wordy, adding quite a bit of characters to the error message.
• Add it to the --explain flag. This has several concerns:
  • It's not really clear if anyone is actually using --explain.
  • Explain has historically been only for Exxxx codes, would it be confusing if it was also for warnings?
  • Not really clear how --explain will work in relation with -Zteach in the future.

There is some history in #48041 and #48337 about trying to reduce the verbosity with --explain. I'm not sure how that would work with warnings, it would need some rewording. In some situations, there can be a lot of warnings, and I'd be concerned that it would spew out way too much text.

[camelid]

I based this off of errors like:

error: unexpected `(` after qualified path
--> $DIR/vec-macro-in-pattern.rs:5:14
|
LL | Some(vec![_x]) => (),
| ^^^^^^^^
| |
| unexpected `(` after qualified path
| the qualified path
| in this macro invocation
| help: use a slice pattern here instead: `[_x]`
|
= help: for more information, see https://doc.rust-lang.org/edition-guide/rust-2018/slice-patterns.html
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: aborting due to previous error

but if we want to wait for a unified approach before adding more, I'm okay with that.

And I have another Con to add to using HTTP links: we currently don't check links produced in error messages, so they're prone to link rot.

[camelid]

Another example:

error: getting the inner pointer of a temporary `CString`
--> $DIR/lint-temporary-cstring-as-param.rs:9:45
|
LL | some_function(CString::new("").unwrap().as_ptr());
| ------------------------- ^^^^^^ this pointer will be invalid
| |
| this `CString` is deallocated at the end of the statement, bind it to a variable to extend its lifetime
|
note: the lint level is defined here
--> $DIR/lint-temporary-cstring-as-param.rs:1:9
|
LL | #![deny(temporary_cstring_as_ptr)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
= note: pointers do not have a lifetime; when calling `as_ptr` the `CString` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned
= help: for more information, see https://doc.rust-lang.org/reference/destructors.html

[varkor]

I think we ought to link to the stable version of the documentation (or, even better, to the relevant version). But that aside, it seems like a link to the documentation is not the best fix for this issue. We should be suggesting the user change the while let into a loop. Can we do that instead?

[camelid]

Starting with a rebase.

[camelid]

@varkor How does this look:

warning: irrefutable while-let pattern
--> $DIR/while-let.rs:27:5
|
LL | / while let _a = 1 {
LL | | println!("irrefutable pattern");
LL | | break;
LL | | }
| |_____^
|
= note: this pattern will always match, so the loop will never exit
= help: consider using a `loop { ... }` and adding a `let` inside it

? I added similar notes and suggestions to if-let and if-let guard as well.

Ideally they would be structured suggestions, but I think it would be tricky to recover the span information. Also, these aren't mechanical fixes which makes them more difficult to automate and not as useful to users.

[camelid]

Note that the current output uses help: for both; I'm in the middle of changing the first one to use note: instead.

[varkor]

Ideally they would be structured suggestions, but I think it would be tricky to recover the span information. Also, these aren't mechanical fixes which makes them more difficult to automate and not as useful to users.

Yes, that's true. This is definitely an improvement as-is.

[camelid]

(I think I need to bless tests from the error message change.)

[camelid]

I can squash before you r+, but I figure it's easier to review as separate commits.

[varkor]

Suggested change

	diag.help("consider replacing the if-let with a let");
	diag.help("consider replacing the if-let with a `let`");

I wonder whether it would be more readable to write

Suggested change

	diag.help("consider replacing the if-let with a let");
	diag.help("consider replacing the `if let` with a `let`");

as well.

[camelid]

I intentionally removed the backticks on let to be consistent with if-let. I thought of doing the second option you suggest, but didn't because then it would be inconsistent with the error message. I'm still open to it, I just want to make sure we don't confuse people. What do you think?

[varkor]

I'd be tempted to replace if-let with `if let` in general (including in the main error message). To me, it's slightly easier to read, and feels more consistent with other uses of code fragments in error messages.

[camelid]

I like the look of if-let, though I do think it's probably clearer as `if let`. However, it seems like if-let and the like are used a lot, not just in the compiler but also in documentation. So can we do that as a separate change so this one doesn't wallow too long?

[varkor]

That works (as long as we don't overlook that let needs to be enclosed in backticks in the message above as well).

[varkor]

r=me after squashing.

[camelid]

Also: blocked on #82215 (not adding S-blocked since that PR will merge very soon).

[camelid]

Starting with a squash to make the rebase easier (and since I need to anyway).

[camelid]

Rebased. Ready for review!

[varkor]

Thanks, looks good!

@bors r+

[bors]

📌 Commit 5d2a2a1 has been approved by varkor