Auto merge of rust-lang#88214 - notriddle:notriddle/for-loop-span-drop-temps-mut, r=nagisa

rustc: use more correct span data in for loop desugaring

Fixes rust-lang#82462

Before:

      help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
         |
      LL |     for x in DroppingSlice(&*v).iter(); {
         |                                       +

After:

      help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
         |
      LL |     };
         |      +

This seems like a reasonable fix: since the desugared "expr_drop_temps_mut" contains the entire desugared loop construct, its span should contain the entire loop construct as well.

Author: bors

compiler/rustc_ast_lowering/src/expr.rs

@@ -1450,8 +1450,13 @@ impl<'hir> LoweringContext<'_, 'hir> {
             )
         };
 
+        // #82462: to correctly diagnose borrow errors, the block that contains
+        // the iter expr needs to have a span that covers the loop body.
+        let desugared_full_span =
+            self.mark_span_with_reason(DesugaringKind::ForLoop(ForLoopLoc::Head), e.span, None);
+
         let match_expr = self.arena.alloc(self.expr_match(
-            desugared_span,
+            desugared_full_span,
             into_iter_expr,
             arena_vec![self; iter_arm],
             hir::MatchSource::ForLoopDesugar,
@@ -1465,7 +1470,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
         // surrounding scope of the `match` since the `match` is not a terminating scope.
         //
         // Also, add the attributes to the outer returned expr node.
-        self.expr_drop_temps_mut(desugared_span, match_expr, attrs.into())
+        self.expr_drop_temps_mut(desugared_full_span, match_expr, attrs.into())
     }
 
     /// Desugar `ExprKind::Try` from: `<expr>?` into:

src/test/mir-opt/remove_storage_markers.main.RemoveStorageMarkers.diff

@@ -80,7 +80,7 @@
 -         StorageDead(_7);                 // scope 3 at $DIR/remove_storage_markers.rs:8:18: 8:19
 -         StorageDead(_6);                 // scope 2 at $DIR/remove_storage_markers.rs:10:5: 10:6
 -         StorageDead(_4);                 // scope 1 at $DIR/remove_storage_markers.rs:10:5: 10:6
--         StorageDead(_2);                 // scope 1 at $DIR/remove_storage_markers.rs:8:18: 8:19
+-         StorageDead(_2);                 // scope 1 at $DIR/remove_storage_markers.rs:10:5: 10:6
 -         StorageDead(_1);                 // scope 0 at $DIR/remove_storage_markers.rs:11:1: 11:2
           return;                          // scope 0 at $DIR/remove_storage_markers.rs:11:2: 11:2
       }

src/test/ui/borrowck/issue-82462.nll.stderr

@@ -0,0 +1,22 @@
+error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
+  --> $DIR/issue-82462.rs:18:9
+   |
+LL |     for x in DroppingSlice(&*v).iter() {
+   |              ------------------
+   |              |               |
+   |              |               immutable borrow occurs here
+   |              a temporary with access to the immutable borrow is created here ...
+LL |         v.push(*x);
+   |         ^ mutable borrow occurs here
+LL |         break;
+LL |     }
+   |     - ... and the immutable borrow might be used here, when that temporary is dropped and runs the `Drop` code for type `DroppingSlice`
+   |
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL |     };
+   |      +
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0502`.

src/test/ui/borrowck/issue-82462.rs

@@ -0,0 +1,21 @@
+struct DroppingSlice<'a>(&'a [i32]);
+
+impl Drop for DroppingSlice<'_> {
+    fn drop(&mut self) {
+        println!("hi from slice");
+    }
+}
+
+impl DroppingSlice<'_> {
+    fn iter(&self) -> std::slice::Iter<'_, i32> {
+        self.0.iter()
+    }
+}
+
+fn main() {
+    let mut v = vec![1, 2, 3, 4];
+    for x in DroppingSlice(&*v).iter() {
+        v.push(*x); //~ERROR
+        break;
+    }
+}

src/test/ui/borrowck/issue-82462.stderr

@@ -0,0 +1,22 @@
+error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
+  --> $DIR/issue-82462.rs:18:9
+   |
+LL |     for x in DroppingSlice(&*v).iter() {
+   |              ------------------
+   |              |               |
+   |              |               immutable borrow occurs here
+   |              a temporary with access to the immutable borrow is created here ...
+LL |         v.push(*x);
+   |         ^^^^^^^^^^ mutable borrow occurs here
+LL |         break;
+LL |     }
+   |     - ... and the immutable borrow might be used here, when that temporary is dropped and runs the `Drop` code for type `DroppingSlice`
+   |
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL |     };
+   |      +
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0502`.

src/tools/clippy/clippy_lints/src/loops/for_kv_map.rs

@@ -15,7 +15,6 @@ pub(super) fn check<'tcx>(
     pat: &'tcx Pat<'_>,
     arg: &'tcx Expr<'_>,
     body: &'tcx Expr<'_>,
-    expr: &'tcx Expr<'_>,
 ) {
     let pat_span = pat.span;
 
@@ -43,7 +42,7 @@ pub(super) fn check<'tcx>(
                 span_lint_and_then(
                     cx,
                     FOR_KV_MAP,
-                    expr.span,
+                    arg_span,
                     &format!("you seem to want to iterate on a map's {}s", kind),
                     |diag| {
                         let map = sugg::Sugg::hir(cx, arg, "map");

src/tools/clippy/clippy_lints/src/loops/iter_next_loop.rs

@@ -5,12 +5,12 @@ use rustc_hir::Expr;
 use rustc_lint::LateContext;
 use rustc_span::sym;
 
-pub(super) fn check(cx: &LateContext<'_>, arg: &Expr<'_>, expr: &Expr<'_>) -> bool {
+pub(super) fn check(cx: &LateContext<'_>, arg: &Expr<'_>) -> bool {
     if is_trait_method(cx, arg, sym::Iterator) {
         span_lint(
             cx,
             ITER_NEXT_LOOP,
-            expr.span,
+            arg.span,
             "you are iterating over `Iterator::next()` which is an Option; this will compile but is \
             probably not what you want",
         );

src/tools/clippy/clippy_lints/src/loops/mod.rs

@@ -616,15 +616,15 @@ fn check_for_loop<'tcx>(
         needless_range_loop::check(cx, pat, arg, body, expr);
         explicit_counter_loop::check(cx, pat, arg, body, expr);
     }
-    check_for_loop_arg(cx, pat, arg, expr);
-    for_kv_map::check(cx, pat, arg, body, expr);
+    check_for_loop_arg(cx, pat, arg);
+    for_kv_map::check(cx, pat, arg, body);
     mut_range_bound::check(cx, arg, body);
     single_element_loop::check(cx, pat, arg, body, expr);
     same_item_push::check(cx, pat, arg, body, expr);
     manual_flatten::check(cx, pat, arg, body, span);
 }
 
-fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>, expr: &Expr<'_>) {
+fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {
     let mut next_loop_linted = false; // whether or not ITER_NEXT_LOOP lint was used
 
     if let ExprKind::MethodCall(method, _, [self_arg], _) = arg.kind {
@@ -637,7 +637,7 @@ fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>, expr:
                 explicit_into_iter_loop::check(cx, self_arg, arg);
             },
             "next" => {
-                next_loop_linted = iter_next_loop::check(cx, arg, expr);
+                next_loop_linted = iter_next_loop::check(cx, arg);
             },
             _ => {},
         }

src/tools/clippy/clippy_lints/src/loops/needless_range_loop.rs

@@ -144,7 +144,7 @@ pub(super) fn check<'tcx>(
                     span_lint_and_then(
                         cx,
                         NEEDLESS_RANGE_LOOP,
-                        expr.span,
+                        arg.span,
                         &format!("the loop variable `{}` is used to index `{}`", ident.name, indexed),
                         |diag| {
                             multispan_sugg(
@@ -170,7 +170,7 @@ pub(super) fn check<'tcx>(
                     span_lint_and_then(
                         cx,
                         NEEDLESS_RANGE_LOOP,
-                        expr.span,
+                        arg.span,
                         &format!("the loop variable `{}` is only used to index `{}`", ident.name, indexed),
                         |diag| {
                             multispan_sugg(