Skip to content

Commit

Permalink
Rollup merge of #131035 - dingxiangfei2009:tweak-if-let-rescope-lint,…
Browse files Browse the repository at this point in the history 
… r=jieyouxu

Preserve brackets around if-lets and skip while-lets

r? `@jieyouxu`

Tracked by #124085

Fresh out of #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.
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Sep 30, 2024
2 parents b6b43af + ed5443f commit dc1ccc5
Show file tree
Hide file tree
Showing 4 changed files with 145 additions and 7 deletions.
30 changes: 28 additions & 2 deletions compiler/rustc_lint/src/if_let_rescope.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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![];
Expand All @@ -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);
Expand All @@ -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.
Expand Down Expand Up @@ -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))
{
Expand Down
30 changes: 28 additions & 2 deletions tests/ui/drop/lint-if-let-rescope.fixed
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
}
}
30 changes: 28 additions & 2 deletions tests/ui/drop/lint-if-let-rescope.rs
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
}
}
62 changes: 61 additions & 1 deletion tests/ui/drop/lint-if-let-rescope.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit dc1ccc5

Please sign in to comment.