Rollup merge of rust-lang#131035 - dingxiangfei2009:tweak-if-let-rescope-lint, r=jieyouxu

matthiaskrgr · web-flow · commit dc1ccc526435 · 2024-09-30T18:25:14.000+02:00
Preserve brackets around if-lets and skip while-lets r? `@jieyouxu` Tracked by rust-lang#124085 Fresh out of rust-lang#129466, we have discovered 9 crates that the lint did not successfully migrate because the span of `if let` includes the surrounding brackets `(..)` like the following, which surprised me a bit. ```rust if (if let .. { .. } else { .. }) { // ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // the span somehow includes the surrounding brackets } ``` There is one crate that failed the migration because some suggestion spans cross the macro expansion boundaries. Surely there is no way to patch them with `match` rewrite. To handle this case, we will instead require all spans to be tested for admissibility as suggestion spans. Besides, there are 4 false negative cases discovered with desugared-`while let`. We don't need to lint them, because the `else` branch surely contains exactly one statement because the drop order is not changed whatsoever in this case. ```rust while let Some(value) = droppy().get() { .. } // is desugared into loop { if let Some(value) = droppy().get() { .. } else { break; // here can be nothing observable in this block } } ``` I believe this is the one and only false positive that I have found. I think we have finally nailed all the corner cases this time.
diff --git a/compiler/rustc_lint/src/if_let_rescope.rs b/compiler/rustc_lint/src/if_let_rescope.rs
@@ -122,7 +122,11 @@ impl IfLetRescope {
         }
         let tcx = cx.tcx;
         let source_map = tcx.sess.source_map();
-        let expr_end = expr.span.shrink_to_hi();
+        let expr_end = match expr.kind {
+            hir::ExprKind::If(_cond, conseq, None) => conseq.span.shrink_to_hi(),
+            hir::ExprKind::If(_cond, _conseq, Some(alt)) => alt.span.shrink_to_hi(),
+            _ => return,
+        };
         let mut add_bracket_to_match_head = match_head_needs_bracket(tcx, expr);
         let mut significant_droppers = vec![];
         let mut lifetime_ends = vec![];
@@ -145,7 +149,10 @@ impl IfLetRescope {
                 recovered: Recovered::No,
             }) = cond.kind
             {
-                let if_let_pat = expr.span.shrink_to_lo().between(init.span);
+                // Peel off round braces
+                let if_let_pat = source_map
+                    .span_take_while(expr.span, |&ch| ch == '(' || ch.is_whitespace())
+                    .between(init.span);
                 // The consequent fragment is always a block.
                 let before_conseq = conseq.span.shrink_to_lo();
                 let lifetime_end = source_map.end_point(conseq.span);
@@ -159,6 +166,8 @@ impl IfLetRescope {
                     if ty_ascription.is_some()
                         || !expr.span.can_be_used_for_suggestions()
                         || !pat.span.can_be_used_for_suggestions()
+                        || !if_let_pat.can_be_used_for_suggestions()
+                        || !before_conseq.can_be_used_for_suggestions()
                     {
                         // Our `match` rewrites does not support type ascription,
                         // so we just bail.
@@ -240,6 +249,23 @@ impl<'tcx> LateLintPass<'tcx> for IfLetRescope {
         if let (Level::Allow, _) = cx.tcx.lint_level_at_node(IF_LET_RESCOPE, expr.hir_id) {
             return;
         }
+        if let hir::ExprKind::Loop(block, _label, hir::LoopSource::While, _span) = expr.kind
+            && let Some(value) = block.expr
+            && let hir::ExprKind::If(cond, _conseq, _alt) = value.kind
+            && let hir::ExprKind::Let(..) = cond.kind
+        {
+            // Recall that `while let` is lowered into this:
+            // ```
+            // loop {
+            //     if let .. { body } else { break; }
+            // }
+            // ```
+            // There is no observable change in drop order on the overall `if let` expression
+            // given that the `{ break; }` block is trivial so the edition change
+            // means nothing substantial to this `while` statement.
+            self.skip.insert(value.hir_id);
+            return;
+        }
         if expr_parent_is_stmt(cx.tcx, expr.hir_id)
             && matches!(expr.kind, hir::ExprKind::If(_cond, _conseq, None))
         {
diff --git a/tests/ui/drop/lint-if-let-rescope.fixed b/tests/ui/drop/lint-if-let-rescope.fixed
@@ -1,8 +1,8 @@
 //@ run-rustfix
 
 #![deny(if_let_rescope)]
-#![feature(if_let_rescope)]
-#![allow(irrefutable_let_patterns)]
+#![feature(if_let_rescope, stmt_expr_attributes)]
+#![allow(irrefutable_let_patterns, unused_parens)]
 
 fn droppy() -> Droppy {
     Droppy
@@ -68,4 +68,30 @@ fn main() {
         //~| HELP: the value is now dropped here in Edition 2024
         //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
     }
+
+    #[rustfmt::skip]
+    if (match droppy().get() { Some(_value) => { true } _ => { false }}) {
+        //~^ ERROR: `if let` assigns a shorter lifetime since Edition 2024
+        //~| WARN: this changes meaning in Rust 2024
+        //~| HELP: the value is now dropped here in Edition 2024
+        //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
+        // do something
+    } else if (((match droppy().get() { Some(_value) => { true } _ => { false }}))) {
+        //~^ ERROR: `if let` assigns a shorter lifetime since Edition 2024
+        //~| WARN: this changes meaning in Rust 2024
+        //~| HELP: the value is now dropped here in Edition 2024
+        //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
+    }
+
+    while let Some(_value) = droppy().get() {
+        // Should not lint
+        break;
+    }
+
+    while (match droppy().get() { Some(_value) => { false } _ => { true }}) {
+        //~^ ERROR: `if let` assigns a shorter lifetime since Edition 2024
+        //~| WARN: this changes meaning in Rust 2024
+        //~| HELP: the value is now dropped here in Edition 2024
+        //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
+    }
 }
diff --git a/tests/ui/drop/lint-if-let-rescope.rs b/tests/ui/drop/lint-if-let-rescope.rs
@@ -1,8 +1,8 @@
 //@ run-rustfix
 
 #![deny(if_let_rescope)]
-#![feature(if_let_rescope)]
-#![allow(irrefutable_let_patterns)]
+#![feature(if_let_rescope, stmt_expr_attributes)]
+#![allow(irrefutable_let_patterns, unused_parens)]
 
 fn droppy() -> Droppy {
     Droppy
@@ -68,4 +68,30 @@ fn main() {
         //~| HELP: the value is now dropped here in Edition 2024
         //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
     }
+
+    #[rustfmt::skip]
+    if (if let Some(_value) = droppy().get() { true } else { false }) {
+        //~^ ERROR: `if let` assigns a shorter lifetime since Edition 2024
+        //~| WARN: this changes meaning in Rust 2024
+        //~| HELP: the value is now dropped here in Edition 2024
+        //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
+        // do something
+    } else if (((if let Some(_value) = droppy().get() { true } else { false }))) {
+        //~^ ERROR: `if let` assigns a shorter lifetime since Edition 2024
+        //~| WARN: this changes meaning in Rust 2024
+        //~| HELP: the value is now dropped here in Edition 2024
+        //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
+    }
+
+    while let Some(_value) = droppy().get() {
+        // Should not lint
+        break;
+    }
+
+    while (if let Some(_value) = droppy().get() { false } else { true }) {
+        //~^ ERROR: `if let` assigns a shorter lifetime since Edition 2024
+        //~| WARN: this changes meaning in Rust 2024
+        //~| HELP: the value is now dropped here in Edition 2024
+        //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
+    }
 }
diff --git a/tests/ui/drop/lint-if-let-rescope.stderr b/tests/ui/drop/lint-if-let-rescope.stderr
@@ -131,5 +131,65 @@ help: a `match` with a single arm can preserve the drop order up to Edition 2021
 LL |     if let () = { match Droppy.get() { Some(_value) => {} _ => {}} } {
    |                   ~~~~~              +++++++++++++++++    ++++++++
 
-error: aborting due to 5 previous errors
+error: `if let` assigns a shorter lifetime since Edition 2024
+  --> $DIR/lint-if-let-rescope.rs:73:12
+   |
+LL |     if (if let Some(_value) = droppy().get() { true } else { false }) {
+   |            ^^^^^^^^^^^^^^^^^^^--------^^^^^^
+   |                               |
+   |                               this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
+   |
+   = warning: this changes meaning in Rust 2024
+   = note: for more information, see issue #124085 <https://github.com/rust-lang/rust/issues/124085>
+help: the value is now dropped here in Edition 2024
+  --> $DIR/lint-if-let-rescope.rs:73:53
+   |
+LL |     if (if let Some(_value) = droppy().get() { true } else { false }) {
+   |                                                     ^
+help: a `match` with a single arm can preserve the drop order up to Edition 2021
+   |
+LL |     if (match droppy().get() { Some(_value) => { true } _ => { false }}) {
+   |         ~~~~~                +++++++++++++++++          ~~~~          +
+
+error: `if let` assigns a shorter lifetime since Edition 2024
+  --> $DIR/lint-if-let-rescope.rs:79:21
+   |
+LL |     } else if (((if let Some(_value) = droppy().get() { true } else { false }))) {
+   |                     ^^^^^^^^^^^^^^^^^^^--------^^^^^^
+   |                                        |
+   |                                        this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
+   |
+   = warning: this changes meaning in Rust 2024
+   = note: for more information, see issue #124085 <https://github.com/rust-lang/rust/issues/124085>
+help: the value is now dropped here in Edition 2024
+  --> $DIR/lint-if-let-rescope.rs:79:62
+   |
+LL |     } else if (((if let Some(_value) = droppy().get() { true } else { false }))) {
+   |                                                              ^
+help: a `match` with a single arm can preserve the drop order up to Edition 2021
+   |
+LL |     } else if (((match droppy().get() { Some(_value) => { true } _ => { false }}))) {
+   |                  ~~~~~                +++++++++++++++++          ~~~~          +
+
+error: `if let` assigns a shorter lifetime since Edition 2024
+  --> $DIR/lint-if-let-rescope.rs:91:15
+   |
+LL |     while (if let Some(_value) = droppy().get() { false } else { true }) {
+   |               ^^^^^^^^^^^^^^^^^^^--------^^^^^^
+   |                                  |
+   |                                  this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
+   |
+   = warning: this changes meaning in Rust 2024
+   = note: for more information, see issue #124085 <https://github.com/rust-lang/rust/issues/124085>
+help: the value is now dropped here in Edition 2024
+  --> $DIR/lint-if-let-rescope.rs:91:57
+   |
+LL |     while (if let Some(_value) = droppy().get() { false } else { true }) {
+   |                                                         ^
+help: a `match` with a single arm can preserve the drop order up to Edition 2021
+   |
+LL |     while (match droppy().get() { Some(_value) => { false } _ => { true }}) {
+   |            ~~~~~                +++++++++++++++++           ~~~~         +
+
+error: aborting due to 8 previous errors
 
