Skip to content

rustc: use more correct span data in for loop desugaring #88214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions compiler/rustc_ast_lowering/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1401,8 +1401,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,
Expand All @@ -1416,7 +1421,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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
22 changes: 22 additions & 0 deletions src/test/ui/borrowck/issue-82462.nll.stderr
Original file line number Diff line number Diff line change
@@ -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`.
21 changes: 21 additions & 0 deletions src/test/ui/borrowck/issue-82462.rs
Original file line number Diff line number Diff line change
@@ -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;
}
}
22 changes: 22 additions & 0 deletions src/test/ui/borrowck/issue-82462.stderr
Original file line number Diff line number Diff line change
@@ -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`.
3 changes: 1 addition & 2 deletions src/tools/clippy/clippy_lints/src/loops/for_kv_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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");
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/clippy_lints/src/loops/iter_next_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
);
Expand Down
8 changes: 4 additions & 4 deletions src/tools/clippy/clippy_lints/src/loops/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -601,15 +601,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 {
Expand All @@ -622,7 +622,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);
},
_ => {},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,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(
Expand All @@ -172,7 +172,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(
Expand Down