Add details about how significant drop in match scrutinees can cause deadlocks #8981

PrestonFrom · 2022-06-10T07:26:45Z

Adds more details about how a significant drop in a match scrutinee can cause a deadlock and include link to documentation.

changelog: Add more details to significant drop lint to explicitly show how temporaries in match scrutinees can cause deadlocks.

rust-highfive · 2022-06-10T07:26:48Z

r? @Alexendoo

(rust-highfive has picked a reviewer for you, use r? to override)

PrestonFrom · 2022-06-10T07:30:09Z

@oli-obk Apologies for the delay, but I have added more details about the lint, linked to documentation generated as part of clippy, and tried to make the lifetime of the temporary clearer.

I believe you mentioned including wg-diagnostics on the code review. Is that a group I can @ here or do I need to send a link to the PR somewhere?

Thank you!

flip1995 · 2022-06-10T08:36:48Z

clippy_lints/src/matches/significant_drop_in_scrutinee.rs

+                    match, so side effects from dropping such temporaries are delayed. \
+                    Deadlocks can occur if a lock guard is created in the scrutinee and a new \
+                    lock guard for the same mutex is created in a match arm. \
+                    For more information, see https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee");


This link is automatically added to every clippy lint, unless explicitly disabled through an undocumented env var. So no need to add it here.

I don't think adding such a big paragraph to a lint message is what was suggested. We don't add full lint explanations to lint messages, that's what the lint documentation is for. I can't think of a rustc or Clippy lint where something like this is done. Can you link the discussion where adding this was suggested?

@flip1995 Thank you for the review! It looks like I might have misunderstood Oli's suggestion here: rust-lang/rust#93883 (comment)

Particularly this part:

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

I've tried to make the messages smaller, but I did see one PR opened in another project due to this lint where it was mentioned that the current output was unhelpful and confusing. If you think this is still too much, I will keep trying to trim it down.

I had another question actually -- I was able to point to the end of the match, but it's cutting out the middle of the block, and I think the lack of context is going to be confusing. How can I keep the full match block, or at least a few of the last lines?

This link is automatically added to every clippy lint, unless explicitly disabled through an undocumented env var. So no need to add it here.

Oh, I completely missed this! Thank you! The URL has been removed as well.

Ah yeah, we might want to improve the messaging a bit. But just writing a big paragraph won't help here. I think the ideal thing to do would be something like:

error: temporary with drop impl with side effects in match scrutinee lives to end of block --> $DIR/significant_drop_in_scrutinee.rs:59:11 | LL | match mutex.lock().unwrap().foo() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ LL | 42 => mutex.lock().unwrap().bar() | ^^^^^^^^^^^^ note: new side effect temporary created here ... LL | }; | - original temporary lives until here | = note: this might lead to deadlocks or other unexpected behavior help: try moving the temporary above the match | LL ~ let value = mutex.lock().unwrap().foo(); LL ~ match value { |

But this would entail, that we also try to find other significant drop temporaries inside the match block, which might be a bit hard. A first step might be to find any significant drop temp inside the match and point to it. The next step would be to compare if it comes from the same root (i.e. same mutex), which might be way harder.

flip1995 · 2022-06-10T08:38:57Z

I believe you mentioned including wg-diagnostics on the code review. Is that a group I can @ here or do I need to send a link to the PR somewhere?

You only have to include wg-diag once this lint gets on the way to be uplifted. Until then all reviews are done by the Clippy team.

Alexendoo · 2022-06-17T11:40:36Z

tests/ui/significant_drop_in_scrutinee.stderr

 LL |     match counter.temp_increment().len() {
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     };
+   |      - temporary lives until here. If mutex is locked within block, deadlocks can occur.


The message could be confusing when there's no mutexes involved. It would be good to be able to say

if counter is a mutex that is locked within the block, deadlocks can occur

Or something to those effects, if getting the name is tricky

Agreed. See my above suggestion #8981 (comment) Don't mention mutexes at all, but say what the result might be. Which is "deadlocks or other unexpected behavior".

PrestonFrom · 2022-06-21T05:38:05Z

@flip1995 @Alexendoo
Thank you both for the comments! This took me a bit longer than anticipated since I started trying to use MIR to look for the "origin" of temporaries with significant drops, but I wasn't finding a good solution.

The new commit will look for any type in the arms that has a significant drop -- this is not 100% correct, since it's possible that a different "parent" is creating the new temporary. All the types found in the arms are indicated with notes and a final note is added with the message this might lead to deadlocks or other unexpected behavior.
If no other types with significant drops are found in the arms, none of the new messages are emitted. I'm not sure if that's correct though, since the "parent" could be passed to a function that creates a new temporary as well. (E.g., if the match locks a mutex and then that mutex is passed to a function, it could be locked again in that function.)
I think I need to find the "parent" of the temporary in the scrutinee and look for it creating new temporaries or being passed to other functions? Is that possible with MIR? If so, should that be done in this PR or a new PR?

Adds more details about how a significant drop in a match scrutinee can cause a deadlock and include link to documentation. Emits messages indicating temporaries with significant drops in arms of matches and message about possible deadlocks/unexpected behavior. changelog: Add more details to significant drop lint to explicitly show how temporaries in match scrutinees can cause deadlocks/unexpected behavior.

PrestonFrom · 2022-06-21T06:00:40Z

I also had some issues after trying to rebase and ended up force pushing an entirely new squashed commit. I hope I haven't made re-reviewing too annoying for you.

flip1995

I would say: don't overthink it. The current version is definitely good enough. Bringing MIR into this lint would make it way too complicated and pretty unmaintainable. I have a few suggestions on how to improve the wording of the lint a little further.

I'm a little concerned about the perf impact of checking every arm of a match. Clippy has pretty big match blocks, so I should be able to time this change on Clippy itself.

flip1995 · 2022-06-21T16:39:50Z

clippy_lints/src/matches/significant_drop_in_scrutinee.rs

+                    for span in spans {
+                        diag.span_label(span, "significant drop in arm here");
+                    }
+                    diag.note("this might lead to deadlocks or other unexpected behavior");


I would add this note unconditionally.

flip1995 · 2022-06-22T07:43:50Z

tests/ui/significant_drop_in_scrutinee.stderr

 help: try moving the temporary above the match
   |
 LL ~         let value = mutex1.lock().unwrap().s.len();
 LL ~         match (value, true, mutex2.lock().unwrap().s.len()) {
   |

-error: temporary with significant drop in match scrutinee
+error: temporary with drop impl with side effects in match scrutinee lives to end of block


Suggested change

error: temporary with drop impl with side effects in match scrutinee lives to end of block

error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression

flip1995 · 2022-06-22T07:44:34Z

tests/ui/significant_drop_in_scrutinee.stderr

-   |
+...
+LL |                 mutex1.lock().unwrap().s.len();
+   |                 ---------------------- significant drop in arm here


Suggested change

| ---------------------- significant drop in arm here

| ---------------------- another temporary with significant `Drop` created here

Oh, good call, because this could be in a for-loop as well

PrestonFrom · 2022-06-23T04:37:25Z

@flip1995 Thank you for the comments!

My initial assumption was that checking all the arms wouldn't add a lot of overhead because it would only happen when a significant drop was found in the match scrutinee/for-loop condition, but having evidence one way or the other would be great.

Alexendoo

Since it's only run when a candidate is found as you say, the extra visitor should be fine performance wise

When #[allow(clippy::significant_drop_in_scrutinee)] is used it seems the and_then callback isn't triggered which would mean no problem there either. It is called for #[expect] however (cc @xFrednet in case that's a bug)

If you wanted to err on the side of caution you could throw an is_lint_allowed at the top of check, that would allow skipping the initial significant drop checking too

clippy_lints/src/matches/significant_drop_in_scrutinee.rs

flip1995

Thanks! This all LGTM now. I still want to benchmark it though.

xFrednet · 2022-06-28T14:59:34Z

When #[allow(clippy::significant_drop_in_scrutinee)] is used it seems the and_then callback isn't triggered which would mean no problem there either. It is called for #[expect] however (cc @xFrednet in case that's a bug)

Lints should process as usual, if the lint is expected. It sounds like this behaves how it should. Thank you for pinging me 🙃

PrestonFrom · 2022-06-29T03:20:19Z

Thanks everyone for your help!
@flip1995 What needs to be done for benchmarking? Is that something I need to run?

flip1995 · 2022-06-29T13:07:06Z

We don't need "real" benchmark numbers. It would be enough if you run time cargo dev lint . inside Clippy a few times with the master branch and then a few times with the changes in this PR and post the results. I think if you do that it could be faster rather then waiting for me to run it

flip1995 · 2022-06-29T13:20:01Z

Nvm, I just did that myself. I didn't see any significant changes.

Thanks for continuing improving this already awesome lint. Let's get this merged so people can test it in nightly on Friday

@bors r+

bors · 2022-06-29T13:21:50Z

📌 Commit 7bc4096 has been approved by flip1995

PrestonFrom · 2022-06-29T13:21:54Z

@flip1995 My apologies for the delay in responding! I just saw this. Thank you for running the benchmarks and for helping me get this far!

flip1995 · 2022-06-29T13:22:44Z

My apologies for the delay in responding! I just saw this.

No need to apologies. My two messages were in the span of 15 minutes :D

bors · 2022-06-29T13:23:23Z

⌛ Testing commit 7bc4096 with merge 90227c1...

bors · 2022-06-29T13:41:13Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 90227c1 to master...

rust-highfive assigned Alexendoo Jun 10, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 10, 2022

flip1995 reviewed Jun 10, 2022

View reviewed changes

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 10, 2022

Alexendoo reviewed Jun 17, 2022

View reviewed changes

PrestonFrom force-pushed the more_details_for_significant_drop_lint branch from 74f42ae to a97fe06 Compare June 21, 2022 05:08

PrestonFrom force-pushed the more_details_for_significant_drop_lint branch from 68ec83a to 2476100 Compare June 21, 2022 05:59

flip1995 reviewed Jun 22, 2022

View reviewed changes

Update messages for clarity when linting

1f707db

Alexendoo reviewed Jun 27, 2022

View reviewed changes

clippy_lints/src/matches/significant_drop_in_scrutinee.rs Outdated Show resolved Hide resolved

clippy_lints/src/matches/significant_drop_in_scrutinee.rs Outdated Show resolved Hide resolved

clippy_lints/src/matches/significant_drop_in_scrutinee.rs Outdated Show resolved Hide resolved

PrestonFrom added 2 commits June 27, 2022 22:52

Make messages more accurate, check lint enabled

3cfd1e5

Run cargo dev fmt

7bc4096

flip1995 approved these changes Jun 28, 2022

View reviewed changes

Alexendoo approved these changes Jun 28, 2022

View reviewed changes

bors merged commit 90227c1 into rust-lang:master Jun 29, 2022

PrestonFrom mentioned this pull request Jul 6, 2022

lint against unexpectedly late drop for temporaries in match scrutinee expressions rust-lang/rust#93883

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add details about how significant drop in match scrutinees can cause deadlocks #8981

Add details about how significant drop in match scrutinees can cause deadlocks #8981

PrestonFrom commented Jun 10, 2022

rust-highfive commented Jun 10, 2022

PrestonFrom commented Jun 10, 2022

flip1995 Jun 10, 2022

PrestonFrom Jun 15, 2022 •

edited

Loading

flip1995 Jun 15, 2022 •

edited

Loading

flip1995 commented Jun 10, 2022

Alexendoo Jun 17, 2022

flip1995 Jun 17, 2022

PrestonFrom commented Jun 21, 2022

PrestonFrom commented Jun 21, 2022

flip1995 left a comment

flip1995 Jun 21, 2022

PrestonFrom Jun 23, 2022

flip1995 Jun 22, 2022

PrestonFrom Jun 23, 2022

flip1995 Jun 22, 2022

PrestonFrom Jun 23, 2022

PrestonFrom commented Jun 23, 2022

Alexendoo left a comment •

edited

Loading

flip1995 left a comment

xFrednet commented Jun 28, 2022

PrestonFrom commented Jun 29, 2022

flip1995 commented Jun 29, 2022

flip1995 commented Jun 29, 2022

bors commented Jun 29, 2022

PrestonFrom commented Jun 29, 2022

flip1995 commented Jun 29, 2022

bors commented Jun 29, 2022

bors commented Jun 29, 2022

	error: temporary with drop impl with side effects in match scrutinee lives to end of block
	error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression

	\| ---------------------- significant drop in arm here
	\| ---------------------- another temporary with significant `Drop` created here

Add details about how significant drop in match scrutinees can cause deadlocks #8981

Add details about how significant drop in match scrutinees can cause deadlocks #8981

Conversation

PrestonFrom commented Jun 10, 2022

rust-highfive commented Jun 10, 2022

PrestonFrom commented Jun 10, 2022

Choose a reason for hiding this comment

PrestonFrom Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

flip1995 Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

flip1995 commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrestonFrom commented Jun 21, 2022

PrestonFrom commented Jun 21, 2022

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrestonFrom commented Jun 23, 2022

Alexendoo left a comment • edited Loading

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

xFrednet commented Jun 28, 2022

PrestonFrom commented Jun 29, 2022

flip1995 commented Jun 29, 2022

flip1995 commented Jun 29, 2022

bors commented Jun 29, 2022

PrestonFrom commented Jun 29, 2022

flip1995 commented Jun 29, 2022

bors commented Jun 29, 2022

bors commented Jun 29, 2022

PrestonFrom Jun 15, 2022 •

edited

Loading

flip1995 Jun 15, 2022 •

edited

Loading

Alexendoo left a comment •

edited

Loading