lint against unexpectedly late drop for temporaries in match scrutinee expressions

[yaahc]

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.

[eddyb]

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/QuietDrop dichotomy, 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 QuietDrop marker could opt into MIR optimizations moving the drop earlier).

(A quick search finds some newer mentions of NoisyDrop but it's mostly a common name for "print on Drop" types in code snippets, not the marker trait concept from that old RFC)

cc @nikomatsakis @pnkfelix

[oli-obk]

Related clippy issue: rust-lang/rust-clippy#1904

[PrestonFrom]

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]

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 claim on 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]

Thank you for the detailed suggestions! It’s very helpful!

[PrestonFrom]

@rustbot claim

[nikomatsakis]

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.

[PrestonFrom]

@nikomatsakis Do you mean that mutexes would need the attribute to trigger the lint or as a solution outside the lint?

[Mark-Simulacrum]

We already have a similar attribute (at least in theory) for the RFC 2229 work, right? #[rustc_insignificant_dtor] https://github.com/rust-lang/rust/blob/master/library/alloc/src/vec/mod.rs#L399

That's obviously perma-unstable under that name, but we could consider a stable version of it.

[nikomatsakis]

@PrestonFrom I mean that the lint would not hard-code particular types, but we would annotate the "guard" types with the attribute. And yes, @Mark-Simulacrum is right that #[rustc_insignificant_dtor] is the same general idea, except that it is biased in the opposite direction (i.e., significant unless annotated otherwise).

[PrestonFrom]

@nikomatsakis Thank you -- to make sure I am correctly following your suggestion:

1. I should create a new attribute to annotate types whose Drop impl has a side effect that would be delayed in a surprising way if it had a static lifetime when it is used as a scrutinee in a match block
2. I should create a new lint to check for types that have the new attribute in match scrutinees and that have a static lifetime (as @oli-obk suggested above)

I think this makes sense -- I wish there were a way to write the lint so that it wouldn't require an "opt-in", but this seems like a good approach to avoid false positives.

[nikomatsakis]

Hmm, I'm unclear on why the lifetime of the scrutinee is significant. I was expecting to just look for cases where a drop is executing and the type being dropped has this flag. (We probably do want to discuss whether to "traverse" the type, e.g., to detect things like a Vec<LockGuard> or something...)

Really this problem is larger than match, I think, but let's just start with match -- I'd like to have tests for if let, but maybe we get that for free because of desugaring? I think so.

[PrestonFrom]

@nikomatsakis This might be confusion on my part. I thought we'd want to look at the lifetime because it would indicate that the object's lifetime was being extended (and thank you for mentioning the if let case, because I'd totally missed that).

Maybe instead of checking the lifetime, it should check that the object is not a ref?

[oli-obk]

The reason I mentioned that we should only lint if scrutinee: 'static is to have something simple to start with where we know that the suggestion will compile, as it cannot possibly be in use within the match arms. This is just an idea for a generic MVP.

If we want to target just lock guards and similar, the plan with an extra attribute (or simply an auto trait?) sgtm. It won't immediately work for lock guard types outside of libstd, but that's ok, we can figure out how to allow crates to register themselves for the lint later.

If we want to look into things like Vec<LockGuard>, too, we should probably go down the auto-trait route so that it automatically bubbles up through aggregates.

indicate that the object's lifetime was being extended

that's the thing, there is no extension happening that is based on types or such, this also happens when the scrutinee type is bool (e.g. in match some_mutex.lock().is_some()). My comment about the 'static lifetime was only based on my idea for doing a general analysis. Doing it just for lock types is much simpler and more robust, so let's go with that.

[PrestonFrom]

@oli-obk Thank you so much for the clarification! I think I understand this better now.

An auto trait sounds like it might be a good way to go -- I will look at that vs an attribute

[PrestonFrom]

@oli-obk Thank you for the initial review on the fork -- I have opened a proper pull request in this repo: #94206

Any feedback/comments/suggestions/general advice would be welcome!

[PrestonFrom]

@oli-obk With #94206 merged and in the nursery, I'm a little unsure what my next steps should be. Should this be implemented for rustc lint next?

[oli-obk]

I'm a little unsure what my next steps should be.

That's because you're exploring new territory for which we don't have procedures yet. A clippy PR against the rustc repo is just one of them. We'll need to debrief with @flip1995 to figure out whether we need to change/improve some things there. Then we can write some docs so other people will have some guidance in the future.

About moving the lint to rustc: Here's a PR where we did this previousy: #75671

Reading through it, a few things come to mind:

1. the clippy team needs to be pinged on such a PR
2. the lint needs to be removed from clippy and added to its deprecation list
3. a compiler team MCP needs to be opened
4. the diagnostic messaging needs to explain why this is a problem, ideally with some links to documentation and examples
   • maybe even add a short span pointing to the end of the match stating that the lock holds until there, so if any arms directly or via function calls lock again, you're in a deadlock
   • ping wg-diagnostics on that PR so we have a look
5. the lint needs to be warn-by-default, irrespective of what it was in clippy, as it is not a guarantee for a bug, but only a footgun that often becomes a bug later.
6. once that PR is through, this list should go into the rustc guide, amended with anything that came up in the PR.

you can do 4. in the clippy repo first if you'd like, then the uplifting PR itself will only be about the uplift.

[flip1995]

Usually lints that are uplifted from Clippy to rustc were in Clippy for a long time and therefore got tested on real world code. Also they were mostly deny-by-default IIRC or at the very least warn-by-default in Clippy.

This lint is currently in nursery, which means it is allow-by-default. I'd move this lint to the suspicious group before uplifting it. Then it is a warn-by-default lint and will get tested by everyone who uses (nightly) Clippy.

you can do 4. in the clippy repo first if you'd like,

I suggest that all modifications and bug fixes to the lint are now done in the Clippy repo. You can r? @flip1995 on those PRs if you want.

If you manage to submit the PR moving this lint to the sus group some time this week, we'll have this lint warn-by-default in nightly Clippy for the whole next release cycle, which should give it enough time for testing. After that period we can re-evaluate if it is ready for uplifting or if some more work has to be done.

---

As for the uplifting process:

2. the lint needs to be removed from clippy and added to its deprecation list

We now mark uplifted lints as "register_renamed", so that the new rustc lint name gets suggested when the old Clippy lint name is found in the code.

5. the lint needs to be warn-by-default, irrespective of what it was in clippy,

In general it doesn't have to be warn-by-default, it could also be allow-by-default. This should probably be decided in the MCP. For this lint warn-by-default sounds right though.

(On the other hand does uplifting a lint to an allow-by-default rustc lint even make sense?)

[oli-obk]

(On the other hand does uplifting a lint to an allow-by-default rustc lint even make sense?)

nope :) I meant more like, even if clippy has it in deny-by-default, we should move it to warn-by-default in rustc

[PrestonFrom]

Thank you both for the overview of next steps! I'll try to get step 4 done this week in the Clippy repo!

[PrestonFrom]

@oli-obk The PR adding more context (rust-lang/rust-clippy#8981) was merged into clippy last week. Is this what you were looking for in terms of adding context or would you like more added?

[oli-obk]

This looks really great now! Thank you

[PrestonFrom]

@oli-obk Great! Is the next step moving uplifting or is this a good time to pause and wait for feedback from the community on how useful/confusing the lint is?

[flip1995]

Great! Is the next step moving uplifting or is this a good time to pause and wait for feedback from the community on how useful/confusing the lint is?

I'd suggest to wait for a release cycle to get feedback on the lint.

[PrestonFrom]

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]

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]

However you can start with the uplifting work a bit earlier, so that the uplifted lint will get into 1.65 right away.

[PrestonFrom]

Thank you for the explanation!

[leontoeides]

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 match on an Option or Result

[oconnor663]

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:

cargo clippy -- -D clippy::significant_drop_in_scrutinee

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 let is 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 "unused Result that must be used" in terms of how essential it is.

[PrestonFrom]

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]

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]

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]

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 lintcheck for 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]

@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.