Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix false positive when Drop and Copy involved in field_reassign_with_default lint #7794

Merged
merged 1 commit into from
Oct 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions clippy_lints/src/default.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
64 changes: 64 additions & 0 deletions tests/ui/field_reassign_with_default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}
26 changes: 25 additions & 1 deletion tests/ui/field_reassign_with_default.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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