Auto merge of #12341 - y21:issue12337, r=dswij

Check for try blocks in `question_mark` more consistently

Fixes #12337

I split this PR up into two commits since this moves a method out of an `impl`, which makes for a pretty bad diff (the `&self` parameter is now unused, and there isn't a reason for that function to be part of the `impl` now).

The first commit is the actual relevant change and the 2nd commit just moves stuff (github's "hide whitespace" makes the diff easier to look at)

------------
Now for the actual issue:

`?` within `try {}` blocks desugars to a `break` to the block, rather than a `return`, so that changes behavior in those cases.

The lint has multiple patterns to look for and in *some* of them it already does correctly check whether we're in a try block, but this isn't done for all of its patterns.

We could add another `self.inside_try_block()` check to the function that looks for `let-else-return`, but I chose to actually just move those checks out and instead have them in `LintPass::check_{stmt,expr}`. This has the advantage that we can't (easily) accidentally forget to add that check in new patterns that might be added in the future.

(There's also a bit of a subtle interaction between two lints, where `question_mark`'s LintPass calls into `manual_let_else`, so I added a check to make sure we don't avoid linting for something that doesn't have anything to do with `?`)

changelog: [`question_mark`]: avoid linting on try blocks in more cases

4 files changed, 67 insertions, 31 deletions

diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs
index 5d94660ec89..1fb252e3f97 100644
--- a/tests/ui/manual_let_else.rs
+++ b/tests/ui/manual_let_else.rs
@@ -1,3 +1,4 @@
+#![feature(try_blocks)]
 #![allow(unused_braces, unused_variables, dead_code)]
 #![allow(
     clippy::collapsible_else_if,
@@ -446,3 +447,12 @@ struct U<T> {
     w: T,
     x: T,
 }
+
+fn issue12337() {
+    // We want to generally silence question_mark lints within try blocks, since `?` has different
+    // behavior to `return`, and question_mark calls into manual_let_else logic, so make sure that
+    // we still emit a lint for manual_let_else
+    let _: Option<()> = try {
+        let v = if let Some(v_some) = g() { v_some } else { return };
+    };
+}
diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr
index b6433fc460c..7012c6b8891 100644
--- a/tests/ui/manual_let_else.stderr
+++ b/tests/ui/manual_let_else.stderr
@@ -1,5 +1,5 @@
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:27:5
+  --> tests/ui/manual_let_else.rs:28:5
    |
 LL |     let v = if let Some(v_some) = g() { v_some } else { return };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };`
@@ -8,7 +8,7 @@ LL |     let v = if let Some(v_some) = g() { v_some } else { return };
    = help: to override `-D warnings` add `#[allow(clippy::manual_let_else)]`
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:30:5
+  --> tests/ui/manual_let_else.rs:31:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -26,7 +26,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:37:5
+  --> tests/ui/manual_let_else.rs:38:5
    |
 LL | /     let v = if let Some(v) = g() {
 LL | |
@@ -47,25 +47,25 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:49:9
+  --> tests/ui/manual_let_else.rs:50:9
    |
 LL |         let v = if let Some(v_some) = g() { v_some } else { continue };
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { continue };`
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:51:9
+  --> tests/ui/manual_let_else.rs:52:9
    |
 LL |         let v = if let Some(v_some) = g() { v_some } else { break };
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { break };`
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:56:5
+  --> tests/ui/manual_let_else.rs:57:5
    |
 LL |     let v = if let Some(v_some) = g() { v_some } else { panic!() };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { panic!() };`
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:60:5
+  --> tests/ui/manual_let_else.rs:61:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -83,7 +83,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:68:5
+  --> tests/ui/manual_let_else.rs:69:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -101,7 +101,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:76:5
+  --> tests/ui/manual_let_else.rs:77:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -121,7 +121,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:85:5
+  --> tests/ui/manual_let_else.rs:86:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -141,7 +141,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:94:5
+  --> tests/ui/manual_let_else.rs:95:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -168,7 +168,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:110:5
+  --> tests/ui/manual_let_else.rs:111:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -190,7 +190,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:121:5
+  --> tests/ui/manual_let_else.rs:122:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -217,7 +217,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:137:5
+  --> tests/ui/manual_let_else.rs:138:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -239,7 +239,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:148:5
+  --> tests/ui/manual_let_else.rs:149:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -257,7 +257,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:156:5
+  --> tests/ui/manual_let_else.rs:157:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -278,7 +278,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:166:5
+  --> tests/ui/manual_let_else.rs:167:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -299,7 +299,7 @@ LL +     } };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:176:5
+  --> tests/ui/manual_let_else.rs:177:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |
@@ -328,7 +328,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:194:5
+  --> tests/ui/manual_let_else.rs:195:5
    |
 LL | /     let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) {
 LL | |
@@ -346,7 +346,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:202:5
+  --> tests/ui/manual_let_else.rs:203:5
    |
 LL | /     let (w, S { v }) = if let (Some(v_some), w_some) = (g().map(|_| S { v: 0 }), 0) {
 LL | |
@@ -364,7 +364,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:212:13
+  --> tests/ui/manual_let_else.rs:213:13
    |
 LL |             let $n = if let Some(v) = $e { v } else { return };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some($n) = g() else { return };`
@@ -375,19 +375,19 @@ LL |     create_binding_if_some!(w, g());
    = note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:221:5
+  --> tests/ui/manual_let_else.rs:222:5
    |
 LL |     let v = if let Variant::A(a, 0) = e() { a } else { return };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(v, 0) = e() else { return };`
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:225:5
+  --> tests/ui/manual_let_else.rs:226:5
    |
 LL |     let mut v = if let Variant::B(b) = e() { b } else { return };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::B(mut v) = e() else { return };`
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:230:5
+  --> tests/ui/manual_let_else.rs:231:5
    |
 LL | /     let v = if let Ok(Some(Variant::B(b))) | Err(Some(Variant::A(b, _))) = nested {
 LL | |
@@ -405,19 +405,19 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:237:5
+  --> tests/ui/manual_let_else.rs:238:5
    |
 LL |     let v = if let Variant::A(.., a) = e() { a } else { return };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(.., v) = e() else { return };`
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:241:5
+  --> tests/ui/manual_let_else.rs:242:5
    |
 LL |     let w = if let (Some(v), ()) = (g(), ()) { v } else { return };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let (Some(w), ()) = (g(), ()) else { return };`
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:245:5
+  --> tests/ui/manual_let_else.rs:246:5
    |
 LL | /     let w = if let Some(S { v: x }) = Some(S { v: 0 }) {
 LL | |
@@ -435,7 +435,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:253:5
+  --> tests/ui/manual_let_else.rs:254:5
    |
 LL | /     let v = if let Some(S { v: x }) = Some(S { v: 0 }) {
 LL | |
@@ -453,7 +453,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:261:5
+  --> tests/ui/manual_let_else.rs:262:5
    |
 LL | /     let (x, S { v }, w) = if let Some(U { v, w, x }) = None::<U<S<()>>> {
 LL | |
@@ -471,7 +471,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> tests/ui/manual_let_else.rs:378:5
+  --> tests/ui/manual_let_else.rs:379:5
    |
 LL | /     let _ = match ff {
 LL | |
@@ -480,5 +480,11 @@ LL | |         _ => macro_call!(),
 LL | |     };
    | |______^ help: consider writing: `let Some(_) = ff else { macro_call!() };`
 
-error: aborting due to 30 previous errors
+error: this could be rewritten as `let...else`
+  --> tests/ui/manual_let_else.rs:456:9
+   |
+LL |         let v = if let Some(v_some) = g() { v_some } else { return };
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };`
+
+error: aborting due to 31 previous errors
 
diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed
index 2ef006c1419..567472a8af2 100644
--- a/tests/ui/question_mark.fixed
+++ b/tests/ui/question_mark.fixed
@@ -273,3 +273,13 @@ const fn issue9175(option: Option<()>) -> Option<()> {
     //stuff
     Some(())
 }
+
+fn issue12337() -> Option<i32> {
+    let _: Option<i32> = try {
+        let Some(_) = Some(42) else {
+            return None;
+        };
+        123
+    };
+    Some(42)
+}
diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs
index c170669823f..abf8c270de8 100644
--- a/tests/ui/question_mark.rs
+++ b/tests/ui/question_mark.rs
@@ -313,3 +313,13 @@ const fn issue9175(option: Option<()>) -> Option<()> {
     //stuff
     Some(())
 }
+
+fn issue12337() -> Option<i32> {
+    let _: Option<i32> = try {
+        let Some(_) = Some(42) else {
+            return None;
+        };
+        123
+    };
+    Some(42)
+}