Add lint to check for temporary where dropping has side effects. #3066

PieterPenninckx · 2018-08-20T11:58:31Z

Closes #1904 .
There are still some things unclear to me; I indicated this in the source code with questions for the reviewer.

The "dogfood" test also gives one error on the existing code:

error: Expression results in an anonymous temporary variable that will have side effects when being dropped.
   --> clippy_lints/src/consts.rs:444:33
    |
444 |                       let alloc = tcx
    |  _________________________________^
445 | |                         .alloc_map
446 | |                         .lock()
    | |_______________________________^
    |
    = note: #[deny(dropping_temporary_with_side_effect)] on by default
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#dropping_temporary_with_side_effect

I don't know enough about the compiler to see if this uncovered an actual bug or not (I guess not). If this does not indicate a real bug in clippy_lints/src/consts.rs, I can fix it easily.

Thank to @oli-obk for his help on this lint at RustFest Paris. As you can see, it took a lot longer than one workshop to write this lint.

phansch

I only have a small suggestion to improve the docs, someone else should probably have a look, too. In general the code looks good to me 👍

phansch · 2018-08-20T16:39:00Z

clippy_lints/src/temporary_dropped_with_sideeffects.rs

+
+/// **What it does:**
+/// Checks for temporaries containing values where dropping has a side effect.
+///


I think it would be good to include a list of links to the stdlib to the different types this lint checks.

phansch · 2018-08-20T16:50:32Z

tests/ui/temporary_dropped_with_sideeffects.rs

@@ -0,0 +1,152 @@
+// #![cfg_attr(feature = "clippy", deny(dropping_temporary_with_side_effect))]


leftover comment

oli-obk · 2018-08-21T07:41:29Z

I don't know enough about the compiler to see if this uncovered an actual bug or not (I guess not). If this does not indicate a real bug in clippy_lints/src/consts.rs, I can fix it easily.

It's not a bug. A lock() function is always something that is just meant to create a short temporary handle.

I have a suggestion that makes this lint clearer: print the type in the diagnostic so the user knows what we are complaining about, not just where.

oli-obk

All my comments are probably me misunderstanding something about the nature of the wrongness. Can you elaborate on them (maybe even with a comment in the test as to why these are wrong and should be linted?)

oli-obk · 2018-08-21T07:42:54Z

clippy_lints/src/temporary_dropped_with_sideeffects.rs

+
+declare_clippy_lint! {
+    pub DROPPING_TEMPORARY_WITH_SIDE_EFFECT,
+    correctness,


we should have this in the nursery first and run it on a bunch of large crates (especially ones with paralellism)

oli-obk · 2018-08-21T07:49:15Z

clippy_lints/src/temporary_dropped_with_sideeffects.rs

+		TypeVariants::TyForeign(_)
+		  | TypeVariants::TyFnDef(_, _)
+		  | TypeVariants::TyDynamic(_, _)
+		    => false, // Type of a function, trait or a forein type 


nit: foreign

oli-obk · 2018-08-21T07:51:22Z

clippy_lints/src/utils/conf.rs

+                            {
+                                let mut errors = crate::utils::conf::ERRORS.lock()
+                                                    .expect("no threading here");
+                                errors.push(crate::utils::conf::Error::Toml(e.to_string()));


the previous code seemed fine

oli-obk · 2018-08-21T07:51:46Z

clippy_lints/src/utils/conf.rs

+
+	{
+        let global_errors = ERRORS.lock().expect("no threading -> mutex always safe");
+        assert!(global_errors.is_empty());


original code seemed fine, too

oli-obk · 2018-08-21T07:52:05Z

clippy_lints/src/utils/conf.rs

+            {
+                let mut global_errors = ERRORS.lock().expect("no threading -> mutex always safe");
+                errors = global_errors.split_off(0);
+            } // Release the lock for global_errors


was also fine

oli-obk · 2018-08-21T07:54:35Z

tests/ui/temporary_dropped_with_sideeffects.stderr

+error: Expression results in an anonymous temporary variable that will have side effects when being dropped.
+  --> $DIR/temporary_dropped_with_sideeffects.rs:78:30
+   |
+78 |     let _a4 = MutexIndexable{}[&mutex.lock().unwrap()]; // Should trigger


oli-obk · 2018-08-21T07:54:59Z

tests/ui/temporary_dropped_with_sideeffects.stderr

+error: Expression results in an anonymous temporary variable that will have side effects when being dropped.
+  --> $DIR/temporary_dropped_with_sideeffects.rs:85:6
+   |
+85 |     if *mutex_bool.lock().unwrap() { // Should trigger


this is ok code

oli-obk · 2018-08-21T07:55:08Z

tests/ui/temporary_dropped_with_sideeffects.stderr

+error: Expression results in an anonymous temporary variable that will have side effects when being dropped.
+  --> $DIR/temporary_dropped_with_sideeffects.rs:89:8
+   |
+89 |     match mutex_bool.lock() { // should trigger


oli-obk · 2018-08-21T07:55:18Z

tests/ui/temporary_dropped_with_sideeffects.stderr

+error: Expression results in an anonymous temporary variable that will have side effects when being dropped.
+   --> $DIR/temporary_dropped_with_sideeffects.rs:107:9
+    |
+107 |         match cache.read() { // Should trigger


oli-obk · 2018-08-21T07:56:04Z

clippy_lints/src/temporary_dropped_with_sideeffects.rs

+		TypeVariants::TyClosure(_, _)
+		  | TypeVariants::TyFnPtr(_)
+		  | TypeVariants::TyGenerator(_, _, _)
+		  | TypeVariants::TyGeneratorWitness(_) // Question for the reviewer: is this correct?


yea let's ignore those cases

flip1995 · 2018-12-13T13:49:01Z

Ping from triage @PieterPenninckx. Do you want to continue working on this?

PieterPenninckx · 2018-12-18T07:41:00Z

@flip1995 Thanks for asking. I am now busy with other things and I will probably not work on this in the coming months. Should I close this PR and re-open it when (and if) I resume work on it?

flip1995 · 2018-12-18T13:22:22Z

No problem! Thanks for your contribution. Feel free to reopen, whenever you want to continue to work on this.

Add lint to check for temporary where dropping has side effects.

15c8276

phansch reviewed Aug 20, 2018

View reviewed changes

oli-obk suggested changes Aug 21, 2018

View reviewed changes

phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Sep 2, 2018

flip1995 closed this Dec 18, 2018

flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint to check for temporary where dropping has side effects. #3066

Add lint to check for temporary where dropping has side effects. #3066

PieterPenninckx commented Aug 20, 2018

phansch left a comment

phansch Aug 20, 2018

phansch Aug 20, 2018

oli-obk commented Aug 21, 2018

oli-obk left a comment

oli-obk Aug 21, 2018

oli-obk Aug 21, 2018

oli-obk Aug 21, 2018

oli-obk Aug 21, 2018

oli-obk Aug 21, 2018

oli-obk Aug 21, 2018

oli-obk Aug 21, 2018

oli-obk Aug 21, 2018

oli-obk Aug 21, 2018

oli-obk Aug 21, 2018

flip1995 commented Dec 13, 2018

PieterPenninckx commented Dec 18, 2018

flip1995 commented Dec 18, 2018

		@@ -0,0 +1,152 @@
		// #![cfg_attr(feature = "clippy", deny(dropping_temporary_with_side_effect))]

Add lint to check for temporary where dropping has side effects. #3066

Add lint to check for temporary where dropping has side effects. #3066

Conversation

PieterPenninckx commented Aug 20, 2018

phansch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Aug 21, 2018

oli-obk 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

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 commented Dec 13, 2018

PieterPenninckx commented Dec 18, 2018

flip1995 commented Dec 18, 2018