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

mem_replace_with_default: recognize some std library ctors #6820

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
36 changes: 35 additions & 1 deletion clippy_lints/src/mem_replace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::utils::{
};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::def_id::DefId;
use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, QPath};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
Expand All @@ -12,6 +13,8 @@ use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::source_map::Span;
use rustc_span::symbol::sym;

use clippy_utils::is_diagnostic_assoc_item;

declare_clippy_lint! {
/// **What it does:** Checks for `mem::replace()` on an `Option` with
/// `None`.
Expand Down Expand Up @@ -194,13 +197,44 @@ fn check_replace_with_uninit(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'
}
}

/// Returns true if the `def_id` associated with the `path` is recognized as a "default-equivalent"
/// constructor from the std library
fn is_default_equivalent_ctor(cx: &LateContext<'_>, def_id: DefId, path: &QPath<'_>) -> bool {
let std_types_symbols = &[
sym::string_type,
sym::vec_type,
sym::vecdeque_type,
sym::LinkedList,
sym::hashmap_type,
sym::BTreeMap,
sym::hashset_type,
sym::BTreeSet,
sym::BinaryHeap,
];

if std_types_symbols
.iter()
.any(|symbol| is_diagnostic_assoc_item(cx, def_id, *symbol))
{
if let QPath::TypeRelative(_, ref method) = path {
if method.ident.name == sym::new {
return true;
}
}
}

false
}

fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
if let ExprKind::Call(ref repl_func, _) = src.kind {
if_chain! {
if !in_external_macro(cx.tcx.sess, expr_span);
if let ExprKind::Path(ref repl_func_qpath) = repl_func.kind;
if let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id();
if match_def_path(cx, repl_def_id, &paths::DEFAULT_TRAIT_METHOD);
if is_diagnostic_assoc_item(cx, repl_def_id, sym::Default)
|| is_default_equivalent_ctor(cx, repl_def_id, repl_func_qpath);

then {
span_lint_and_then(
cx,
Expand Down
2 changes: 1 addition & 1 deletion clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ pub fn match_trait_method(cx: &LateContext<'_>, expr: &Expr<'_>, path: &[&str])
trt_id.map_or(false, |trt_id| match_def_path(cx, trt_id, path))
}

/// Checks if the method call given in `expr` belongs to a trait or other container with a given
/// Checks if the method call given in `def_id` belongs to a trait or other container with a given
/// diagnostic item
pub fn is_diagnostic_assoc_item(cx: &LateContext<'_>, def_id: DefId, diag_item: Symbol) -> bool {
cx.tcx
Expand Down
29 changes: 29 additions & 0 deletions tests/ui/mem_replace.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
clippy::mem_replace_with_default
)]

use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList, VecDeque};
use std::mem;

fn replace_option_with_none() {
Expand All @@ -19,9 +20,37 @@ fn replace_option_with_none() {
fn replace_with_default() {
let mut s = String::from("foo");
let _ = std::mem::take(&mut s);

let s = &mut String::from("foo");
let _ = std::mem::take(s);
let _ = std::mem::take(s);

let mut v = vec![123];
let _ = std::mem::take(&mut v);
let _ = std::mem::take(&mut v);
let _ = std::mem::take(&mut v);
let _ = std::mem::take(&mut v);

let mut hash_map: HashMap<i32, i32> = HashMap::new();
let _ = std::mem::take(&mut hash_map);

let mut btree_map: BTreeMap<i32, i32> = BTreeMap::new();
let _ = std::mem::take(&mut btree_map);

let mut vd: VecDeque<i32> = VecDeque::new();
let _ = std::mem::take(&mut vd);

let mut hash_set: HashSet<&str> = HashSet::new();
let _ = std::mem::take(&mut hash_set);

let mut btree_set: BTreeSet<&str> = BTreeSet::new();
let _ = std::mem::take(&mut btree_set);

let mut list: LinkedList<i32> = LinkedList::new();
let _ = std::mem::take(&mut list);

let mut binary_heap: BinaryHeap<i32> = BinaryHeap::new();
let _ = std::mem::take(&mut binary_heap);
}

fn main() {
Expand Down
29 changes: 29 additions & 0 deletions tests/ui/mem_replace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
clippy::mem_replace_with_default
)]

use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList, VecDeque};
use std::mem;

fn replace_option_with_none() {
Expand All @@ -19,9 +20,37 @@ fn replace_option_with_none() {
fn replace_with_default() {
let mut s = String::from("foo");
let _ = std::mem::replace(&mut s, String::default());

let s = &mut String::from("foo");
let _ = std::mem::replace(s, String::default());
let _ = std::mem::replace(s, Default::default());

let mut v = vec![123];
let _ = std::mem::replace(&mut v, Vec::default());
let _ = std::mem::replace(&mut v, Default::default());
let _ = std::mem::replace(&mut v, Vec::new());
let _ = std::mem::replace(&mut v, vec![]);

let mut hash_map: HashMap<i32, i32> = HashMap::new();
let _ = std::mem::replace(&mut hash_map, HashMap::new());

let mut btree_map: BTreeMap<i32, i32> = BTreeMap::new();
let _ = std::mem::replace(&mut btree_map, BTreeMap::new());

let mut vd: VecDeque<i32> = VecDeque::new();
let _ = std::mem::replace(&mut vd, VecDeque::new());

let mut hash_set: HashSet<&str> = HashSet::new();
let _ = std::mem::replace(&mut hash_set, HashSet::new());

let mut btree_set: BTreeSet<&str> = BTreeSet::new();
let _ = std::mem::replace(&mut btree_set, BTreeSet::new());

let mut list: LinkedList<i32> = LinkedList::new();
let _ = std::mem::replace(&mut list, LinkedList::new());

let mut binary_heap: BinaryHeap<i32> = BinaryHeap::new();
let _ = std::mem::replace(&mut binary_heap, BinaryHeap::new());
}

fn main() {
Expand Down
78 changes: 72 additions & 6 deletions tests/ui/mem_replace.stderr
Original file line number Diff line number Diff line change
@@ -1,36 +1,102 @@
error: replacing an `Option` with `None`
--> $DIR/mem_replace.rs:14:13
--> $DIR/mem_replace.rs:15:13
|
LL | let _ = mem::replace(&mut an_option, None);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `an_option.take()`
|
= note: `-D clippy::mem-replace-option-with-none` implied by `-D warnings`

error: replacing an `Option` with `None`
--> $DIR/mem_replace.rs:16:13
--> $DIR/mem_replace.rs:17:13
|
LL | let _ = mem::replace(an_option, None);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `an_option.take()`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:21:13
--> $DIR/mem_replace.rs:22:13
|
LL | let _ = std::mem::replace(&mut s, String::default());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut s)`
|
= note: `-D clippy::mem-replace-with-default` implied by `-D warnings`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:23:13
--> $DIR/mem_replace.rs:25:13
|
LL | let _ = std::mem::replace(s, String::default());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(s)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:24:13
--> $DIR/mem_replace.rs:26:13
|
LL | let _ = std::mem::replace(s, Default::default());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(s)`

error: aborting due to 5 previous errors
error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:29:13
|
LL | let _ = std::mem::replace(&mut v, Vec::default());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut v)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:30:13
|
LL | let _ = std::mem::replace(&mut v, Default::default());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut v)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:31:13
|
LL | let _ = std::mem::replace(&mut v, Vec::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut v)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:32:13
|
LL | let _ = std::mem::replace(&mut v, vec![]);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut v)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:35:13
|
LL | let _ = std::mem::replace(&mut hash_map, HashMap::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut hash_map)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:38:13
|
LL | let _ = std::mem::replace(&mut btree_map, BTreeMap::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut btree_map)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:41:13
|
LL | let _ = std::mem::replace(&mut vd, VecDeque::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut vd)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:44:13
|
LL | let _ = std::mem::replace(&mut hash_set, HashSet::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut hash_set)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:47:13
|
LL | let _ = std::mem::replace(&mut btree_set, BTreeSet::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut btree_set)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:50:13
|
LL | let _ = std::mem::replace(&mut list, LinkedList::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut list)`

error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:53:13
|
LL | let _ = std::mem::replace(&mut binary_heap, BinaryHeap::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut binary_heap)`

error: aborting due to 16 previous errors