false positive for if-let-rescope where if let statements without else are also linted

[m4rch3n1ng]

Code

remove the else from the example at https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/if_let_rescope/static.IF_LET_RESCOPE.html

#![warn(if_let_rescope)]
#![allow(unused_variables)]

struct Droppy;
impl Drop for Droppy {
	fn drop(&mut self) {
		// Custom destructor, including this `drop` implementation, is considered
		// significant.
		// Rust does not check whether this destructor emits side-effects that can
		// lead to observable change in program semantics, when the drop order changes.
		// Rust biases to be on the safe side, so that you can apply discretion whether
		// this change indeed breaches any contract or specification that your code needs
		// to honour.
		println!("dropped");
	}
}
impl Droppy {
	fn get(&self) -> Option<u8> {
		None
	}
}

fn main() {
	if let Some(value) = Droppy.get() {
		// do something
	}
}

Current output

warning: `if let` assigns a shorter lifetime since Edition 2024
  --> src/main.rs:24:5
   |
24 |     if let Some(value) = Droppy.get() {
   |        ^^^^^^^^^^^^^^^^^^------^^^^^^
   |                          |
   |                          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
  --> src/main.rs:26:2
   |
26 |     }
   |     ^
note: the lint level is defined here
  --> src/main.rs:1:9
   |
1  | #![warn(if_let_rescope)]
   |         ^^^^^^^^^^^^^^
help: a `match` with a single arm can preserve the drop order up to Edition 2021
   |
24 ~     match Droppy.get() { Some(value) => {
25 |         // do something
26 ~     } _ => {}}
   |

Desired output

Rationale and extra context

as far as i know, the new if-let-rescope is only relevant, when you have an else block, like in the actual example, because it cannot change to be dropped before the else, as there is no else, so (as far as i know) the place where it is dropped does not change.

more info

if you change the code to print stuff

#![warn(if_let_rescope)]
#![allow(unused_variables)]

struct Droppy;
impl Drop for Droppy {
	fn drop(&mut self) {
		dbg!("drop");
	}
}
impl Droppy {
	fn get(&self) -> Option<u8> {
		Some(0)
	}
}

fn main() {
	if let Some(value) = Droppy.get() {
		dbg!("one");
	}
	dbg!("two");
}

and then run it with both edition 2024 and edition 2021

$ rustup run nightly -- rustc --edition 2024 -Zunstable-options src/main.rs 
$ ./main
[src/main.rs:18:3] "one" = "one"
[src/main.rs:7:3] "drop" = "drop"
[src/main.rs:20:2] "two" = "two"
$ rustup run nightly -- rustc --edition 2021 src/main.rs 
$ ./main
[src/main.rs:18:3] "one" = "one"
[src/main.rs:7:3] "drop" = "drop"
[src/main.rs:20:2] "two" = "two"

they both do the same

Other cases

while testing i found out that if you modify the original code to include anything after the if let statement it doesn't lint it anymore

#![warn(if_let_rescope)]
#![allow(unused_variables)]

struct Droppy;
impl Drop for Droppy {
	fn drop(&mut self) {
		println!("dropped");
	}
}
impl Droppy {
	fn get(&self) -> Option<u8> {
		None
	}
}

fn main() {
	if let Some(value) = Droppy.get() {
		// do something
	}
	// this code is still affected by the lint
}

#![warn(if_let_rescope)]
#![allow(unused_variables)]

struct Droppy;
impl Drop for Droppy {
	fn drop(&mut self) {
		println!("dropped");
	}
}
impl Droppy {
	fn get(&self) -> Option<u8> {
		None
	}
}

fn main() {
	if let Some(value) = Droppy.get() {
		// do something
	}
	println!("with this line it doesn't trigger the lint anymore");
}

Rust Version

rustc 1.84.0-nightly (5ec7d6eee 2024-11-17)
binary: rustc
commit-hash: 5ec7d6eee7e0f5236ec1559499070eaf836bc608
commit-date: 2024-11-17
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.3

Anything else?

No response

[jieyouxu]

cc @dingxiangfei2009 as you may know more about the intention.

[jieyouxu]

Marking this as C-discussion for now but please change it to C-bug if more suitable.

[Turbo87]

FWIW I've just followed https://blog.rust-lang.org/2024/11/27/Rust-2024-public-testing.html to test out the Edition 2024 on the https://github.com/rust-lang/crates.io/ codebase and it looks like literally all of the suggested fixes were false positives. if I run cargo +nightly check without those fixes applied it passes without any warnings. luckily crates.io is a relatively small codebase, but for larger projects this could cause quite a bit of busy work.

[jieyouxu]

FYI cc @traviscross

[traviscross]

We talked about this extensively in the edition call yesterday. Note that, while in simple cases like this it is a false positive, there are cases where the behavior does change for if let expressions without else branches due to the fact that the temporaries now do not live throughout the statement.

[jieyouxu]

We talked about this extensively in the edition call yesterday. Note that, while in simple cases like this it is a false positive, there are cases where the behavior does change for if let expressions without else branches due to the fact that the temporaries now do not live throughout the statement.

If the distinction between such cases is not tractable to get right, maybe add a note for no-else cases that indicates the reason you explained, and that it requires additional attention?

[Turbo87]

IMHO false-negatives in this case seem better than this large number of false-positives. if there are false-negatives, wouldn't the compiler complain with a helpful message anyway?

[ehuss]

IMHO false-negatives in this case seem better than this large number of false-positives. if there are false-negatives, wouldn't the compiler complain with a helpful message anyway?

Not if the temporary is holding a lock that other code was assuming is being held, then it would be a runtime issue. Arguably such code would be best to not rely on temporary extensions, but we cannot know if that is the case.

[m4rch3n1ng]

fwiw, if i run cargo +nightly fix --edition in one of my projects, then clippy will give me 28 warnings for "single match".

and also, the lint is only triggered if the if let statement is the last statement of the block that the if let was initiated in, if i put anything after the closing brackets (like i showed in the original post) the lint is not triggered at all, even with the code after the block not affecting the if let at all (unless i'm misunderstanding what is being said here).

so i think the best solution is to either lint every single if let statement (which would make the output extremely noisy and immediately result in a whole lot of clippy warnings) or to make this lint less aggressive (which might then result in a few runtime errors).

[Aloso]

Here's an example I got (minimized):

fn foo(mut iter: impl Iterator<Item = i32>) {
    if let Some(item) = iter.next() {
        _ = item;
    }
}

It says that iter has a significant drop implementation which may observe a major change in drop order.

This is obviously wrong: iter is not a temporary, it gets dropped at the end of the function in all editions.

But when I tried to verify, the warning went away when I inserted a println! statement after the if let statement. It only warns in this instance when the if let is the last statement within the function or block 🤨

[mitsuhiko]

Is there a way to opt out a type from having a significant Drop implementation? I just ran this through minijinja and from what I can tell every single if let that it has changed into a match is a false positive.

[vi]

Maybe there should be an option to explicitly exclude the if let migration, like cargo fix --edition --except-of-if-let-to-match, so it would handle things like rng.gen() to r#gen, but avoid the match patchbomb.

[rnestler]

A workaround is to run cargo clippy --fix --allow-dirty after running cargo fix --edition to revert the if let migration.

[mitsuhiko]

I ran this on a few more projects now and the false positive rate is 100% from what I can tell. I really think it would be good for projects to be able to opt out somehow from this migration lint.

[traviscross]

We're hopeful this will be improved by #133753 which is in the process of being merged.

[traviscross]

Now that #133753 is merged, the examples here, and others that we've tried, such as in #133605, no longer produce false positives, so we'll go ahead and close this.