Add lint for holding RefCell Ref across an await #6029

Daniel-B-Smith · 2020-09-10T20:07:31Z

This introduces the lint await_holding_refcell_ref. For async functions, we iterate
over all types in generator_interior_types and look for core::cell::Ref or core::cell::RefMut. If we find one then we emit a lint.

Heavily cribs from: #5439

changelog: introduce the await_holding_refcell_ref lint

rust-highfive · 2020-09-10T20:07:35Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bors · 2020-09-13T16:25:43Z

☔ The latest upstream changes (presumably #6036) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Daniel-B-Smith · 2020-09-15T18:05:49Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

rustbot · 2020-09-15T18:05:51Z

Error: Label S-waiting-on-review can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

yaahc

looks great, just one test I think we should add before we can merge this. Also, I went ahead and created a zulip topic to discuss the lint group this should go in.

yaahc · 2020-09-15T22:55:22Z

clippy_lints/src/await_holding_refcell_ref.rs

+    /// }
+    /// ```
+    pub AWAIT_HOLDING_REFCELL_REF,
+    pedantic,


I think this might be worth putting in correctness, I notice that the mutex PR you linked is also pedantic. We should definitely be consistent, but that might mean creating a diff PR to move both lints from pedantic to correctness after this PR merges.

My vote would be to make a separate PR changing both at the same time as that would give us time to do a crater run and would be easier to roll back in case of false positives.

tests/ui/await_holding_refcell_ref.rs

flip1995

This LGTM. Regarding moving this to correctness: It doesn't really matter if we do it in a separate PR or just in a separate commit. I would even prefer to just add a commit including everything that is necessary for moving it to correctness in this PR.

flip1995 · 2020-10-01T21:09:02Z

clippy_lints/src/await_holding_refcell_ref.rs

+fn is_refcell_ref(cx: &LateContext<'_>, def_id: DefId) -> bool {
+    match_def_path(cx, def_id, &paths::REFCELL_REF) || match_def_path(cx, def_id, &paths::REFCELL_REFMUT)
+}


Since this and the error messages are the only things that differ from the await_holding_lock lint, I think we should implement those two lints in the same module. You can choose the name of the module ;).

flip1995 · 2020-10-01T21:11:32Z

r? @flip1995

reassigning this, since @yaahc stepped down as a reviewer to focus on the error handling project group

flip1995 · 2020-10-10T06:37:04Z

ping from triage @Daniel-B-Smith. Only few changes are left to implement here. Is there anything I can help you with to get this over the finish line?

Daniel-B-Smith · 2020-10-13T17:20:56Z

Thanks for the ping. I got distracted with some other work. I've merged the lints into one module and marked them as correctness in a separate commit.

flip1995

Thanks! A little clean up left. Also, could you please squash the two commits 4ea9af3 and 4a3084e into one?

flip1995 · 2020-10-17T14:40:25Z

clippy_lints/src/await_holding_invalid.rs

+        use AsyncGeneratorKind::{Block, Closure, Fn};
+        if let Some(GeneratorKind::Async(Block | Closure | Fn)) = body.generator_kind {
+            let body_id = BodyId {
+                hir_id: body.value.hir_id,
+            };
+            let def_id = cx.tcx.hir().body_owner_def_id(body_id);
+            let typeck_results = cx.tcx.typeck(def_id);


this is exactly the same code as in line 55. You can just combine the two lint passes.

flip1995 · 2020-10-17T14:40:41Z

clippy_lints/src/await_holding_invalid.rs

@@ -90,3 +90,79 @@ fn is_mutex_guard(cx: &LateContext<'_>, def_id: DefId) -> bool {
        || match_def_path(cx, def_id, &paths::PARKING_LOT_RWLOCK_READ_GUARD)
        || match_def_path(cx, def_id, &paths::PARKING_LOT_RWLOCK_WRITE_GUARD)
 }
+
+declare_clippy_lint! {


NIT: usually lint declarations are on top of the file

flip1995 · 2020-10-17T14:41:12Z

tests/ui/await_holding_invalid.stderr

   |
 LL | /         let guard = x.lock().unwrap();
 LL | |         baz().await
 LL | |     }
   | |_____^

-error: aborting due to 4 previous errors
+error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await.


It would be nice to keep the tests separated.

Done. Unfortunately, I don't think I was able to preserve the git history when they were split. I could attempt to squash the commit where I restore the split into the commit where I merged the two files.

Daniel-B-Smith

Squashed the two correctness commits into one

Daniel-B-Smith · 2020-10-19T15:51:37Z

clippy_lints/src/await_holding_invalid.rs

@@ -90,3 +90,79 @@ fn is_mutex_guard(cx: &LateContext<'_>, def_id: DefId) -> bool {
        || match_def_path(cx, def_id, &paths::PARKING_LOT_RWLOCK_READ_GUARD)
        || match_def_path(cx, def_id, &paths::PARKING_LOT_RWLOCK_WRITE_GUARD)
 }
+
+declare_clippy_lint! {


Daniel-B-Smith · 2020-10-19T15:51:49Z

clippy_lints/src/await_holding_invalid.rs

+        use AsyncGeneratorKind::{Block, Closure, Fn};
+        if let Some(GeneratorKind::Async(Block | Closure | Fn)) = body.generator_kind {
+            let body_id = BodyId {
+                hir_id: body.value.hir_id,
+            };
+            let def_id = cx.tcx.hir().body_owner_def_id(body_id);
+            let typeck_results = cx.tcx.typeck(def_id);


Daniel-B-Smith · 2020-10-19T18:42:47Z

tests/ui/await_holding_invalid.stderr

   |
 LL | /         let guard = x.lock().unwrap();
 LL | |         baz().await
 LL | |     }
   | |_____^

-error: aborting due to 4 previous errors
+error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await.


Done. Unfortunately, I don't think I was able to preserve the git history when they were split. I could attempt to squash the commit where I restore the split into the commit where I merged the two files.

flip1995

Thanks! Can you rebase on top of master, to get rid of the merge commit? You will have to force push, but this is ok.

If you want you can squash some commits, but that is not mandatory (make sure to keep 6b682db in a separate commit though).

Daniel-B-Smith · 2020-10-21T15:36:08Z

Rebased

Outdated

flip1995 · 2020-10-25T08:54:22Z

@bors r+

Thanks!

bors · 2020-10-25T08:54:23Z

📌 Commit 4d33225 has been approved by flip1995

bors · 2020-10-25T08:54:32Z

⌛ Testing commit 4d33225 with merge a675778...

bors · 2020-10-25T09:15:16Z

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

rust-highfive assigned yaahc Sep 10, 2020

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

yaahc previously requested changes Sep 15, 2020

View reviewed changes

flip1995 reviewed Oct 1, 2020

View reviewed changes

rust-highfive assigned flip1995 and unassigned yaahc Oct 1, 2020

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 Oct 1, 2020

flip1995 reviewed Oct 17, 2020

View reviewed changes

Daniel-B-Smith force-pushed the refcell_ref_await branch from 4a3084e to 6b682db Compare October 19, 2020 15:42

Daniel-B-Smith commented Oct 19, 2020

View reviewed changes

flip1995 approved these changes Oct 19, 2020

View reviewed changes

Daniel Smith added 10 commits October 21, 2020 11:04

Add lint for holding RefCell Ref across an await

57bf80f

fmt

8727169

update_lints

070a751

Better naming post copy/paste

0f4abbf

Add another test case

b3a427d

Move existing lint into shared file

3ed69cd

Move refcell lint into shared module

ee20eba

Convert the await holding lints to correctness

d8c6bce

Merge lints into one pass

86f2b29

Separate tests for each lint

4d33225

Daniel-B-Smith force-pushed the refcell_ref_await branch from b46fff5 to 4d33225 Compare October 21, 2020 15:04

bors merged commit a675778 into rust-lang:master Oct 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint for holding RefCell Ref across an await #6029

Add lint for holding RefCell Ref across an await #6029

Daniel-B-Smith commented Sep 10, 2020

rust-highfive commented Sep 10, 2020

bors commented Sep 13, 2020

Daniel-B-Smith commented Sep 15, 2020

rustbot commented Sep 15, 2020

yaahc left a comment

yaahc Sep 15, 2020 •

edited

Loading

Daniel-B-Smith Sep 28, 2020

flip1995 left a comment •

edited

Loading

flip1995 Oct 1, 2020

flip1995 commented Oct 1, 2020

flip1995 commented Oct 10, 2020

Daniel-B-Smith commented Oct 13, 2020

flip1995 left a comment

flip1995 Oct 17, 2020

Daniel-B-Smith Oct 19, 2020

flip1995 Oct 17, 2020

Daniel-B-Smith Oct 19, 2020

flip1995 Oct 17, 2020

Daniel-B-Smith Oct 19, 2020

Daniel-B-Smith left a comment

Daniel-B-Smith Oct 19, 2020

Daniel-B-Smith Oct 19, 2020

Daniel-B-Smith Oct 19, 2020

flip1995 left a comment

Daniel-B-Smith commented Oct 21, 2020

flip1995 commented Oct 25, 2020

bors commented Oct 25, 2020

bors commented Oct 25, 2020

bors commented Oct 25, 2020

Add lint for holding RefCell Ref across an await #6029

Add lint for holding RefCell Ref across an await #6029

Conversation

Daniel-B-Smith commented Sep 10, 2020

rust-highfive commented Sep 10, 2020

bors commented Sep 13, 2020

Daniel-B-Smith commented Sep 15, 2020

rustbot commented Sep 15, 2020

yaahc left a comment

Choose a reason for hiding this comment

yaahc Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 commented Oct 1, 2020

flip1995 commented Oct 10, 2020

Daniel-B-Smith commented Oct 13, 2020

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

Daniel-B-Smith 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

flip1995 left a comment

Choose a reason for hiding this comment

Daniel-B-Smith commented Oct 21, 2020

flip1995 commented Oct 25, 2020

bors commented Oct 25, 2020

bors commented Oct 25, 2020

bors commented Oct 25, 2020

yaahc Sep 15, 2020 •

edited

Loading

flip1995 left a comment •

edited

Loading