about summary refs log tree commit diff
diff options
context:
space:
mode:
authorbors <bors@rust-lang.org>2021-10-09 13:44:36 +0000
committerbors <bors@rust-lang.org>2021-10-09 13:44:36 +0000
commit8f1555f123be0c58390679c8aa99dca9b79c1bf3 (patch)
treec25dbc594df1d9cefb282c33aab7c3e06c796040
parent22144c02c2d790c2e3b74dc0363000511284f6d8 (diff)
parent7aaed02b8c4cad724506a44a53b5205132ff4d20 (diff)
downloadrust-8f1555f123be0c58390679c8aa99dca9b79c1bf3.tar.gz
rust-8f1555f123be0c58390679c8aa99dca9b79c1bf3.zip
Auto merge of #7794 - ThibsG:FieldReassignDefault6312, r=llogiq
Fix false positive when `Drop` and `Copy` involved in `field_reassign_with_default` lint

Fix FP in `field_reassign_with_default` lint when type implements `Drop` but not all fields are `Copy`.

fixes: #6312

changelog: [`field_reassign_with_default`] Fix FP when `Drop` and `Copy` are involved
-rw-r--r--clippy_lints/src/default.rs8
-rw-r--r--tests/ui/field_reassign_with_default.rs64
-rw-r--r--tests/ui/field_reassign_with_default.stderr26
3 files changed, 97 insertions, 1 deletions
diff --git a/clippy_lints/src/default.rs b/clippy_lints/src/default.rs
index db8f2171348..cde27d3ad2a 100644
--- a/clippy_lints/src/default.rs
+++ b/clippy_lints/src/default.rs
@@ -1,5 +1,6 @@
 use clippy_utils::diagnostics::{span_lint_and_note, span_lint_and_sugg};
 use clippy_utils::source::snippet_with_macro_callsite;
+use clippy_utils::ty::{has_drop, is_copy};
 use clippy_utils::{any_parent_is_automatically_derived, contains_name, in_macro, match_def_path, paths};
 use if_chain::if_chain;
 use rustc_data_structures::fx::FxHashSet;
@@ -139,6 +140,13 @@ impl LateLintPass<'_> for Default {
                     .fields
                     .iter()
                     .all(|field| field.vis.is_accessible_from(module_did, cx.tcx));
+                let all_fields_are_copy = variant
+                    .fields
+                    .iter()
+                    .all(|field| {
+                        is_copy(cx, cx.tcx.type_of(field.did))
+                    });
+                if !has_drop(cx, binding_type) || all_fields_are_copy;
                 then {
                     (local, variant, ident.name, binding_type, expr.span)
                 } else {
diff --git a/tests/ui/field_reassign_with_default.rs b/tests/ui/field_reassign_with_default.rs
index 787053fb000..7367910eaa1 100644
--- a/tests/ui/field_reassign_with_default.rs
+++ b/tests/ui/field_reassign_with_default.rs
@@ -183,3 +183,67 @@ struct WrapperMulti<T, U> {
     i: T,
     j: U,
 }
+
+mod issue6312 {
+    use std::sync::atomic::AtomicBool;
+    use std::sync::Arc;
+
+    // do not lint: type implements `Drop` but not all fields are `Copy`
+    #[derive(Clone, Default)]
+    pub struct ImplDropNotAllCopy {
+        name: String,
+        delay_data_sync: Arc<AtomicBool>,
+    }
+
+    impl Drop for ImplDropNotAllCopy {
+        fn drop(&mut self) {
+            self.close()
+        }
+    }
+
+    impl ImplDropNotAllCopy {
+        fn new(name: &str) -> Self {
+            let mut f = ImplDropNotAllCopy::default();
+            f.name = name.to_owned();
+            f
+        }
+        fn close(&self) {}
+    }
+
+    // lint: type implements `Drop` and all fields are `Copy`
+    #[derive(Clone, Default)]
+    pub struct ImplDropAllCopy {
+        name: usize,
+        delay_data_sync: bool,
+    }
+
+    impl Drop for ImplDropAllCopy {
+        fn drop(&mut self) {
+            self.close()
+        }
+    }
+
+    impl ImplDropAllCopy {
+        fn new(name: &str) -> Self {
+            let mut f = ImplDropAllCopy::default();
+            f.name = name.len();
+            f
+        }
+        fn close(&self) {}
+    }
+
+    // lint: type does not implement `Drop` though all fields are `Copy`
+    #[derive(Clone, Default)]
+    pub struct NoDropAllCopy {
+        name: usize,
+        delay_data_sync: bool,
+    }
+
+    impl NoDropAllCopy {
+        fn new(name: &str) -> Self {
+            let mut f = NoDropAllCopy::default();
+            f.name = name.len();
+            f
+        }
+    }
+}
diff --git a/tests/ui/field_reassign_with_default.stderr b/tests/ui/field_reassign_with_default.stderr
index b56db08ec8a..3ce4b91a548 100644
--- a/tests/ui/field_reassign_with_default.stderr
+++ b/tests/ui/field_reassign_with_default.stderr
@@ -107,5 +107,29 @@ note: consider initializing the variable with `WrapperMulti::<i32, i64> { i: 42,
 LL |     let mut a: WrapperMulti<i32, i64> = Default::default();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 9 previous errors
+error: field assignment outside of initializer for an instance created with Default::default()
+  --> $DIR/field_reassign_with_default.rs:229:13
+   |
+LL |             f.name = name.len();
+   |             ^^^^^^^^^^^^^^^^^^^^
+   |
+note: consider initializing the variable with `issue6312::ImplDropAllCopy { name: name.len(), ..Default::default() }` and removing relevant reassignments
+  --> $DIR/field_reassign_with_default.rs:228:13
+   |
+LL |             let mut f = ImplDropAllCopy::default();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: field assignment outside of initializer for an instance created with Default::default()
+  --> $DIR/field_reassign_with_default.rs:245:13
+   |
+LL |             f.name = name.len();
+   |             ^^^^^^^^^^^^^^^^^^^^
+   |
+note: consider initializing the variable with `issue6312::NoDropAllCopy { name: name.len(), ..Default::default() }` and removing relevant reassignments
+  --> $DIR/field_reassign_with_default.rs:244:13
+   |
+LL |             let mut f = NoDropAllCopy::default();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 11 previous errors