Repository navigation
lint against unexpectedly late drop for temporaries in match scrutinee expressions #93883
Description
This came up recently on twitter: https://twitter.com/fasterthanlime/status/1491803585385877519
Where @fasterthanlime ran into an unexpected difference in behavior that caused a deadlock.
The following code works fine:
use parking_lot::Mutex;
struct State {}
impl State {
fn foo(&self) -> bool {
true
}
fn bar(&self) {}
}
fn main() {
let mutex = Mutex::new(State {});
let val = mutex.lock().foo();
match val {
true => {
mutex.lock().bar();
}
false => {}
};
println!("All done!");
}But when the mutex locking expression is inlined into the match scrutinee expression it causes deadlock:
match mutex.lock().foo() {
true => {
mutex.lock().bar();
}
false => {}
};Link to playgrounds
- Working: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=cb6f3a378805e4bbff601cee783af7f4
- Deadlock: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=09d0aed9879678812aea960bd87dc263
This deadlock occurs because temporaries created in a match scrutinee automatically have their lifetime extended for the duration of the match expression. This is done to allow match arms to borrow from the scrutinee expressions, but this surprised many of the users in the above twitter thread because foo returns a bool, so there is no borrow taken by any of the match arms. My understanding is that the current behavior was chosen because otherwise drop timing of match temporaries would depend on whether or not you create any borrows.
This issue has generated some discussion around the possibility of changing drop rules in an edition to more aggressively drop temporaries. In this issue I would like to avoid a discussion of that size and instead propose that we introduce a lint that warns when one could encounter potentially surprising drop behaviour. @oli-obk has volunteered to mentor this issue. He suggested that we produce a warning when a temporary with a Drop impl which isn't borrowed by any match arms has its lifetime extended.
Activity
eddyb commentedon Feb 11, 2022
This is a duplicate of #37612 - but I am not inclined to immediately close, since the ideas here might cause more progress.
But I do want to stress that this is an ancient issue. rust-lang/rfcs#210 describes not just the problem (surprising semantics around releasing lock guards - though the culrpit in that RFC is different from what's implemented today), but offers a solution (
NoisyDrop/QuietDropdichotomy, to control which types need linting), all the way back in the summer of 2014.The sad thing is that there really hasn't been much (any?) progress around that kind of categorization, even if it could've come in handy in more than one way (e.g. a semantic
QuietDropmarker could opt into MIR optimizations moving the drop earlier).(A quick search finds some newer mentions of
NoisyDropbut it's mostly a common name for "print onDrop" types in code snippets, not the marker trait concept from that old RFC)cc @nikomatsakis @pnkfelix
oli-obk commentedon Feb 11, 2022
Related clippy issue: rust-lang/rust-clippy#1904
PrestonFrom commentedon Feb 12, 2022
I'm relatively new to rust, but after reading the fasterthanlime blog post about this and the related issues, I'd be interested in taking a swing at it, assuming no one else has already claimed it. Would that be okay?
oli-obk commentedon Feb 12, 2022
Yes! So we've set up some procedure for this to make sure other ppl will know you are working on it. Just write
@rustbot claimon the issue and it will get assigned to you.On the implementation side there are a few ways to go about it, but you'll likely have the best experience by implementing it in clippy first (going from editing your lint to seeing it in action on a test is quicker in the clippy repo).
Generic instructions to get you started are in https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md
I do recommend using https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md#author-lint to get a picture of how some tests that you come up with look like (tho I am biased, as I initiated the author lint xD).
In any case, you'll want to stay simple, so I recommend ignoring the actual match arms for now and only concentrating on the situation where the match scrutinee type has no lifetimes other than
'static. Now, instead of a big info dump, I'd say you look into these first steps and if you hit any point where you wonder "what next?" or have any questions at all, you open a thread on https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy and ping me (@oli) on your question. If you prefer you can also PM me in zulip, though you'll get more eyes on it if it's a public thread.PrestonFrom commentedon Feb 12, 2022
Thank you for the detailed suggestions! It’s very helpful!
PrestonFrom commentedon Feb 12, 2022
@rustbot claim
nikomatsakis commentedon Feb 14, 2022
Cool! I'd love to see this get fixed. I would say that -- implementation wise at least -- we should do it with an attribute placed on the type, analogous to
#[must_use]. This can in fact be a start towards that "noisy drop" distinction that @pnkfelix described oh-so-many-years-ago.23 remaining items
PrestonFrom commentedon Jul 6, 2022
That makes sense! To make sure I don’t forget to come back to this, Would that mean waiting until 1.63 or 1.64? Or am I misunderstanding what a release cycle is?
flip1995 commentedon Jul 6, 2022
The current nightly is 1.64, see https://forge.rust-lang.org/#current-release-versions
This would mean to wait until the current nightly is 1.65, which on 2022-08-11
flip1995 commentedon Jul 6, 2022
However you can start with the uplifting work a bit earlier, so that the uplifted lint will get into 1.65 right away.
PrestonFrom commentedon Jul 6, 2022
Thank you for the explanation!
Auto merge of #94206 - PrestonFrom:significant_drop, r=flip1995
leontoeides commentedon Feb 24, 2024
This lint saved my hide and helped me identify and correct a longstanding issue. Thank you for this lint.
I feel like this lint should be elevated or even be made into a compiler warning/error. Unexpected deadlocks arguably undermine rust's claims of safety and fearless concurrency, especially for something so seemingly innocuous as a
matchon anOptionorResultoconnor663 commentedon Sep 8, 2024
Agreed with @leontoeides. I just ran into this footgun, and I was surprised that rustc and clippy didn't seem to have anything for it. For other folks like me finding this thread late, the lint that @PrestonFrom added is https://rust-lang.github.io/rust-clippy/master/index.html#/significant_drop_in_scrutinee, and it's currently disabled by default. You can try it with something like this:
In my specific case I'm retaking the same lock behind a function call, and I'm especially happy that the lint still fires for me. (For reference here's my corrected code, with comments about how
while letis tempting but broken.) I would love to see this warn in rustc by default. I'm sure I've written "correct" code that still trips this lint, and that might make it feel noisy in some cases, but I'm completely convinced that it's worth it. It is rarer that folks run into this, but when it comes up it feels on par with "unusedResultthat must be used" in terms of how essential it is.PrestonFrom commentedon Sep 8, 2024
Hi,
Sorry, this is on me. Having a second kid really sucked up free time. I'm not sure what needs to be done to get it enabled by default but I'll look into it and get it moving again.
oconnor663 commentedon Sep 8, 2024
Wow that was a lightning fast response :-D If you'd rather someone else take the ball, please let me know! I've been meaning to learn more about how Clippy works anyway :)
PrestonFrom commentedon Sep 8, 2024
I'd like to finish it since I started it, and I'm getting some free time, but if it looks doubtful, I'll ping you here! I appreciate the offer very much though.
flip1995 commentedon Sep 16, 2024
A good starting point to see what needs to be addressed is the PR moving the lint to
nursery: rust-lang/rust-clippy#9302 Some of those issues might already be fixed. But in order to move it back to a warn-by-default group, we need someone to go through all the issues that were raised and address them, either by showing that they were already fixed, fix them, add a test, or explain why this shouldn't be a blocking issue.Running
lintcheckfor the lint is also always helpful. We're doing this on CI now, but I'm not sure, if you'll see any different results by just changing the lint group.PrestonFrom commentedon Oct 31, 2024
@oconnor663 I thought I would have more time to work on this but I haven't yet. If you wanted to take over, I would be happy to step aside. Otherwise, I'm hoping to have more time in a few months.
academician commentedon Feb 21, 2025
It's worth noting that this behavior can now be fixed trivially in the 2024 edition by wrapping the scrutinee in a block scope. This is because of the new tail expression temporary scope behavior. Example:
PrestonFrom commentedon May 30, 2025
@kyle-leonhard Yes! Thank you for offering -- I'm not really in a good position to work on this now. Please feel free to take over.
git-staus commentedon Sep 10, 2026
@PrestonFrom could it be that you pinged the wrong user? @kyle-leonhard has not been involved in this issue at all as far as I can tell.
Also, since
clippy::significant_drop_in_scrutineeis already present in Rust (albeit opt-in), is there any benefit of keeping this issue open?