diff options
| -rw-r--r-- | clippy_lints/src/entry.rs | 47 | ||||
| -rw-r--r-- | tests/ui/entry_unfixable.rs | 94 | ||||
| -rw-r--r-- | tests/ui/entry_unfixable.stderr | 41 |
3 files changed, 167 insertions, 15 deletions
diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index f404bc59b3b..dcfee0b6d3c 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -1,5 +1,6 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg}; use clippy_utils::source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context}; +use clippy_utils::ty::is_copy; use clippy_utils::visitors::for_each_expr; use clippy_utils::{ SpanlessEq, can_move_expr_to_closure_no_visit, higher, is_expr_final_block_expr, is_expr_used_or_unified, @@ -84,14 +85,21 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { return; }; + let lint_msg = format!("usage of `contains_key` followed by `insert` on a `{}`", map_ty.name()); let mut app = Applicability::MachineApplicable; let map_str = snippet_with_context(cx, contains_expr.map.span, contains_expr.call_ctxt, "..", &mut app).0; let key_str = snippet_with_context(cx, contains_expr.key.span, contains_expr.call_ctxt, "..", &mut app).0; + let sugg = if let Some(else_expr) = else_expr { let Some(else_search) = find_insert_calls(cx, &contains_expr, else_expr) else { return; }; + if then_search.is_key_used_and_no_copy || else_search.is_key_used_and_no_copy { + span_lint(cx, MAP_ENTRY, expr.span, lint_msg); + return; + } + if then_search.edits.is_empty() && else_search.edits.is_empty() { // No insertions return; @@ -184,15 +192,7 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { } }; - span_lint_and_sugg( - cx, - MAP_ENTRY, - expr.span, - format!("usage of `contains_key` followed by `insert` on a `{}`", map_ty.name()), - "try", - sugg, - app, - ); + span_lint_and_sugg(cx, MAP_ENTRY, expr.span, lint_msg, "try", sugg, app); } } @@ -354,6 +354,8 @@ struct InsertSearcher<'cx, 'tcx> { key: &'tcx Expr<'tcx>, /// The context of the top level block. All insert calls must be in the same context. ctxt: SyntaxContext, + /// The spanless equality utility used to compare expressions. + spanless_eq: SpanlessEq<'cx, 'tcx>, /// Whether this expression can be safely moved into a closure. allow_insert_closure: bool, /// Whether this expression can use the entry api. @@ -364,6 +366,8 @@ struct InsertSearcher<'cx, 'tcx> { is_single_insert: bool, /// If the visitor has seen the map being used. is_map_used: bool, + /// If the visitor has seen the key being used. + is_key_used: bool, /// The locations where changes need to be made for the suggestion. edits: Vec<Edit<'tcx>>, /// A stack of loops the visitor is currently in. @@ -479,11 +483,11 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { } match try_parse_insert(self.cx, expr) { - Some(insert_expr) if SpanlessEq::new(self.cx).eq_expr(self.map, insert_expr.map) => { + Some(insert_expr) if self.spanless_eq.eq_expr(self.map, insert_expr.map) => { self.visit_insert_expr_arguments(&insert_expr); // Multiple inserts, inserts with a different key, and inserts from a macro can't use the entry api. if self.is_map_used - || !SpanlessEq::new(self.cx).eq_expr(self.key, insert_expr.key) + || !self.spanless_eq.eq_expr(self.key, insert_expr.key) || expr.span.ctxt() != self.ctxt { self.can_use_entry = false; @@ -502,9 +506,12 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { self.visit_non_tail_expr(insert_expr.value); self.is_single_insert = is_single_insert; }, - _ if is_any_expr_in_map_used(self.cx, self.map, expr) => { + _ if is_any_expr_in_map_used(self.cx, &mut self.spanless_eq, self.map, expr) => { self.is_map_used = true; }, + _ if self.spanless_eq.eq_expr(self.key, expr) => { + self.is_key_used = true; + }, _ => match expr.kind { ExprKind::If(cond_expr, then_expr, Some(else_expr)) => { self.is_single_insert = false; @@ -568,9 +575,14 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { /// Check if the given expression is used for each sub-expression in the given map. /// For example, in map `a.b.c.my_map`, The expression `a.b.c.my_map`, `a.b.c`, `a.b`, and `a` are /// all checked. -fn is_any_expr_in_map_used<'tcx>(cx: &LateContext<'tcx>, map: &'tcx Expr<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { +fn is_any_expr_in_map_used<'tcx>( + cx: &LateContext<'tcx>, + spanless_eq: &mut SpanlessEq<'_, 'tcx>, + map: &'tcx Expr<'tcx>, + expr: &'tcx Expr<'tcx>, +) -> bool { for_each_expr(cx, map, |e| { - if SpanlessEq::new(cx).eq_expr(e, expr) { + if spanless_eq.eq_expr(e, expr) { return ControlFlow::Break(()); } ControlFlow::Continue(()) @@ -582,6 +594,7 @@ struct InsertSearchResults<'tcx> { edits: Vec<Edit<'tcx>>, allow_insert_closure: bool, is_single_insert: bool, + is_key_used_and_no_copy: bool, } impl<'tcx> InsertSearchResults<'tcx> { fn as_single_insertion(&self) -> Option<Insertion<'tcx>> { @@ -694,11 +707,13 @@ fn find_insert_calls<'tcx>( map: contains_expr.map, key: contains_expr.key, ctxt: expr.span.ctxt(), + spanless_eq: SpanlessEq::new(cx), allow_insert_closure: true, can_use_entry: true, in_tail_pos: true, is_single_insert: true, is_map_used: false, + is_key_used: false, edits: Vec::new(), loops: Vec::new(), locals: HirIdSet::default(), @@ -706,10 +721,12 @@ fn find_insert_calls<'tcx>( s.visit_expr(expr); let allow_insert_closure = s.allow_insert_closure; let is_single_insert = s.is_single_insert; + let is_key_used_and_no_copy = s.is_key_used && !is_copy(cx, cx.typeck_results().expr_ty(contains_expr.key)); let edits = s.edits; s.can_use_entry.then_some(InsertSearchResults { edits, allow_insert_closure, is_single_insert, + is_key_used_and_no_copy, }) } diff --git a/tests/ui/entry_unfixable.rs b/tests/ui/entry_unfixable.rs new file mode 100644 index 00000000000..dbdacf95056 --- /dev/null +++ b/tests/ui/entry_unfixable.rs @@ -0,0 +1,94 @@ +#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![warn(clippy::map_entry)] +//@no-rustfix + +use std::collections::HashMap; +use std::hash::Hash; + +macro_rules! m { + ($e:expr) => {{ $e }}; +} + +macro_rules! insert { + ($map:expr, $key:expr, $val:expr) => { + $map.insert($key, $val) + }; +} + +mod issue13306 { + use std::collections::HashMap; + + struct Env { + enclosing: Option<Box<Env>>, + values: HashMap<String, usize>, + } + + impl Env { + fn assign(&mut self, name: String, value: usize) -> bool { + if !self.values.contains_key(&name) { + //~^ map_entry + self.values.insert(name, value); + true + } else if let Some(enclosing) = &mut self.enclosing { + enclosing.assign(name, value) + } else { + false + } + } + } +} + +fn issue9925(mut hm: HashMap<String, bool>) { + let key = "hello".to_string(); + if hm.contains_key(&key) { + //~^ map_entry + let bval = hm.get_mut(&key).unwrap(); + *bval = false; + } else { + hm.insert(key, true); + } +} + +mod issue9470 { + use std::collections::HashMap; + use std::sync::Mutex; + + struct Interner(i32); + + impl Interner { + const fn new() -> Self { + Interner(0) + } + + fn resolve(&self, name: String) -> Option<String> { + todo!() + } + } + + static INTERNER: Mutex<Interner> = Mutex::new(Interner::new()); + + struct VM { + stack: Vec<i32>, + globals: HashMap<String, i32>, + } + + impl VM { + fn stack_top(&self) -> &i32 { + self.stack.last().unwrap() + } + + fn resolve(&mut self, name: String, value: i32) -> Result<(), String> { + if self.globals.contains_key(&name) { + //~^ map_entry + self.globals.insert(name, value); + } else { + let interner = INTERNER.lock().unwrap(); + return Err(interner.resolve(name).unwrap().to_owned()); + } + + Ok(()) + } + } +} + +fn main() {} diff --git a/tests/ui/entry_unfixable.stderr b/tests/ui/entry_unfixable.stderr new file mode 100644 index 00000000000..9f9956d351b --- /dev/null +++ b/tests/ui/entry_unfixable.stderr @@ -0,0 +1,41 @@ +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> tests/ui/entry_unfixable.rs:28:13 + | +LL | / if !self.values.contains_key(&name) { +LL | | +LL | | self.values.insert(name, value); +LL | | true +... | +LL | | false +LL | | } + | |_____________^ + | + = note: `-D clippy::map-entry` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::map_entry)]` + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> tests/ui/entry_unfixable.rs:43:5 + | +LL | / if hm.contains_key(&key) { +LL | | +LL | | let bval = hm.get_mut(&key).unwrap(); +LL | | *bval = false; +LL | | } else { +LL | | hm.insert(key, true); +LL | | } + | |_____^ + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> tests/ui/entry_unfixable.rs:81:13 + | +LL | / if self.globals.contains_key(&name) { +LL | | +LL | | self.globals.insert(name, value); +LL | | } else { +LL | | let interner = INTERNER.lock().unwrap(); +LL | | return Err(interner.resolve(name).unwrap().to_owned()); +LL | | } + | |_____________^ + +error: aborting due to 3 previous errors + |