collapsible_if suggestions are not safe

[jonnius]

Consider this example of using a tokio watch channel. Clippy suggests to collapse the nested if blocks into this form.

Following clippy's suggestion leads to a dead-lock. This is because watch::Sender::borrow() locks the sender untils its return value goes out of scope. Having two separate if blocks, the lock is released before calling watch::Sender::send() and everything is fine. Having both calls in one statement means one scope, so the lock is still there and blocks the second call.

[yawara]

@jonnius I tried running the code in the playground, but I couldn't confirm a deadlock. Is it necessary to specify an MSRV?

[jonnius]

It turns out that the underlying issue was rust-lang/rust#103107. With rust-lang/rust#103293 the deadlock is gone, effective since Rust 1.67.0.

Before that change, temporaries from the first condition were dropped after temporaries from the second condition, temporaries being locks in this case. Effectively, the first lock had to outlive the second, causing a deadlock.

Note that the linked item only discusses drop order, not whether temporaries from lhs conditions are dropped, before rhs conditions are even evaluated. Only if we can rely on that, the collapsing_if suggestion is safe. This seems to be the behaviour with current Rust compilers. I just couldn't find it specified anywhere.

[est31]

That's actually an interesting point. The current behavior seems most reasonable to me: the condition expressions only "return" a bool, they shouldn't hold on to anything for any time. Dropping immediately makes the most sense.

Note that your example would have started to work also on older Rust versions if you'd added a true && to the start, because even before rust-lang/rust#103107, we'd have put any rhs into a terminating scope. This seems to be the case at least since rust-lang/rust@419ac4a , at least the code that my PR changed comes from that time.

I think it shouldn't be hard to add a test for it to the Rust compiler: drop order isn't just specified wrt to other drops, but also in terms of execution order.

[est31]

hmmm looking at the tests we have, probably this one relies that it's actually dropping before the execution. but not sure:

https://github.com/rust-lang/rust/blob/a59a2d3f6a80001b0610f03cfc7a6452b63f8935/src/test/ui/drop/issue-103107.rs

[jonatanzeidler]

hmmm looking at the tests we have, probably this one relies that it's actually dropping before the execution. but not sure:

https://github.com/rust-lang/rust/blob/a59a2d3f6a80001b0610f03cfc7a6452b63f8935/src/test/ui/drop/issue-103107.rs

At least the borrow checker seems to think so. Otherwise you wouldn't be allowed to mutate in the second condition, because the lifetime of the temporary Foo expands to the drop. There is just no hint that the test intents to check this, so it could be refactored in a way that it does not cover the execution vs drop order anymore.