diff --git a/tooling/lints/README.md b/tooling/lints/README.md index 6804a2e909..6e7f69da49 100644 --- a/tooling/lints/README.md +++ b/tooling/lints/README.md @@ -25,6 +25,7 @@ picks them up automatically when run from that directory. - `shared_string_from_str_literal` — `SharedString::new/from` etc where `SharedString::from_static` should be used instead. - `async_block_without_await` — `async { … }` blocks whose body contains no `.await` expression. - `entity_update_in_render` — `Entity::update`/`WeakEntity::update` mutating an entity inside `Render::render`. +- `map_lookup_then_insert` — map value lookups that branch into an insertion for the same key, on any map with an `entry` method (`HashMap`, `BTreeMap`, `FxHashMap`, `hashbrown`, `indexmap`, ...), and the same pattern on `HashSet`/`BTreeSet` (where a bare `insert` already searches only once). Keys are matched up to borrows/derefs, value-preserving conversions (`clone`, `to_owned`, `to_string` on string types, `to_vec` on slices, `String::from` on `&str` — each resolved by definition, not name), and immutable local aliases, so looking up `&key` and inserting `key.clone()` is caught. - `notify_in_render` — `Context::notify()` called inside `Render::render`. - `owned_string_into_shared` — `String::from().into()` / `.to_string().into()` / `.to_owned().into()` whose target is `SharedString`, `Arc`, `Rc`, or `Cow<'_, str>`. - `blocking_io_on_foreground` - Catch blocking IO calls that are called on the main thread (but not on closures or background threads) diff --git a/tooling/lints/src/lib.rs b/tooling/lints/src/lib.rs index bf4289673d..851ffb739b 100644 --- a/tooling/lints/src/lib.rs +++ b/tooling/lints/src/lib.rs @@ -28,12 +28,14 @@ use rustc_span::Span; mod blocking_io_on_foreground; mod entity_update_in_render; +mod map_lookup_then_insert; mod notify_in_render; mod owned_string_into_shared; mod render_helpers; use blocking_io_on_foreground::BLOCKING_IO_ON_FOREGROUND; use entity_update_in_render::ENTITY_UPDATE_IN_RENDER; +use map_lookup_then_insert::MAP_LOOKUP_THEN_INSERT; use notify_in_render::NOTIFY_IN_RENDER; use owned_string_into_shared::OWNED_STRING_INTO_SHARED; @@ -54,6 +56,7 @@ pub fn register_lints(sess: &rustc_session::Session, lint_store: &mut rustc_lint ASYNC_BLOCK_WITHOUT_AWAIT, BLOCKING_IO_ON_FOREGROUND, ENTITY_UPDATE_IN_RENDER, + MAP_LOOKUP_THEN_INSERT, NOTIFY_IN_RENDER, OWNED_STRING_INTO_SHARED, ]); @@ -61,6 +64,7 @@ pub fn register_lints(sess: &rustc_session::Session, lint_store: &mut rustc_lint lint_store.register_late_pass(|_| Box::new(AsyncBlockWithoutAwait)); lint_store.register_late_pass(|_| Box::new(blocking_io_on_foreground::BlockingIoOnForeground)); lint_store.register_late_pass(|_| Box::new(entity_update_in_render::EntityUpdateInRender)); + lint_store.register_late_pass(|_| Box::new(map_lookup_then_insert::MapLookupThenInsert)); lint_store.register_late_pass(|_| Box::new(notify_in_render::NotifyInRender)); lint_store.register_late_pass(|_| Box::new(owned_string_into_shared::OwnedStringIntoShared)); } diff --git a/tooling/lints/src/map_lookup_then_insert.rs b/tooling/lints/src/map_lookup_then_insert.rs new file mode 100644 index 0000000000..7bf2a193b4 --- /dev/null +++ b/tooling/lints/src/map_lookup_then_insert.rs @@ -0,0 +1,450 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::visitors::{Visitable, for_each_expr_without_closures}; +use clippy_utils::{SpanlessEq, expr_or_init, sym}; +use rustc_hir::def_id::DefId; +use rustc_hir::{Expr, ExprKind, LangItem, LetStmt, MatchSource, UnOp}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Ty; +use rustc_span::Symbol; +use std::ops::ControlFlow; + +rustc_session::declare_lint! { + /// ### What it does + /// + /// Flags control flow that branches on map value lookup methods and then + /// inserts into the same map with the same key. + /// + /// The lint applies to any map type with an inherent `entry` method: std's + /// `HashMap` and `BTreeMap`, hasher aliases such as `FxHashMap`, and + /// third-party maps such as `hashbrown::HashMap` or `indexmap::IndexMap`. + /// + /// The lint covers direct `match`, `if let`, `is_some`/`is_none`, and + /// `let ... else` forms using `get`, `get_mut`, or `get_key_value`. + /// + /// Keys are compared after stripping borrows and derefs, peeling + /// conversions that yield an owned key equal to the original (`clone`, + /// `to_owned`, `to_string` on string types, `to_vec` on slices, and + /// `String::from` on `&str`), and resolving immutable local aliases to + /// their initializers. So looking up `&key` and inserting `key.clone()` + /// is recognized as the same key. Each peeled conversion is resolved by + /// definition, not by name, so an unrelated user method that happens to + /// be called `clone` does not count. + /// + /// When such a conversion is peeled, the diagnostic carries a note: the + /// entry API takes the key by value up front, so a conversion the + /// original code performs only on the miss path would run on every call. + /// + /// The same pattern on `HashSet` and `BTreeSet` is also flagged. Sets have + /// no stable entry API, and need none: `insert` alone searches once, skips + /// duplicates, and returns `false` when the value was already present. + /// + /// Forms that Clippy already rewrites are excluded. `contains_key` or + /// `contains` followed by `insert` is `map_entry` or + /// `set_contains_or_insert`, and `get(..).is_some()`/`is_none()` on the + /// std collections is `unnecessary_get_then_check`, whose `contains_key`/ + /// `contains` suggestion feeds the former two. + /// + /// ### Why is this bad? + /// + /// The lookup searches the map once and `insert` searches it again. The + /// map's entry API can reuse the result of one search. + pub MAP_LOOKUP_THEN_INSERT, + Warn, + "a map value lookup is followed by insertion for the same key" +} + +pub(crate) struct MapLookupThenInsert; + +rustc_session::impl_lint_pass!(MapLookupThenInsert => [MAP_LOOKUP_THEN_INSERT]); + +impl<'tcx> LateLintPass<'tcx> for MapLookupThenInsert { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if expr.span.from_expansion() { + return; + } + + let repeated_lookup = match &expr.kind { + ExprKind::If(condition, then_expression, else_expression) => { + lookup_from_condition(cx, condition).and_then(|lookup| { + find_matching_insert(cx, lookup, *then_expression) + .or_else(|| { + else_expression + .and_then(|expression| find_matching_insert(cx, lookup, expression)) + }) + .map(|key_converted| (lookup, key_converted)) + }) + } + ExprKind::Match(scrutinee, arms, MatchSource::Normal) => parse_lookup(cx, scrutinee) + .and_then(|lookup| { + arms.iter() + .find_map(|arm| find_matching_insert(cx, lookup, arm.body)) + .map(|key_converted| (lookup, key_converted)) + }), + _ => None, + }; + + if let Some((lookup, key_converted)) = repeated_lookup { + emit_lint(cx, expr.span, lookup, key_converted); + } + } + + fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx LetStmt<'tcx>) { + if local.span.from_expansion() { + return; + } + + let Some(init) = local.init else { + return; + }; + let Some(else_block) = local.els else { + return; + }; + let Some(lookup) = parse_lookup(cx, init) else { + return; + }; + + if let Some(key_converted) = find_matching_insert(cx, lookup, else_block) { + emit_lint(cx, local.span, lookup, key_converted); + } + } +} + +#[derive(Clone, Copy)] +struct Lookup<'tcx> { + map: &'tcx Expr<'tcx>, + key: &'tcx Expr<'tcx>, +} + +fn lookup_from_condition<'tcx>( + cx: &LateContext<'tcx>, + condition: &'tcx Expr<'tcx>, +) -> Option> { + match &condition.kind { + ExprKind::Let(let_expression) => parse_lookup(cx, let_expression.init), + ExprKind::MethodCall(segment, receiver, [], _) + if matches!(segment.ident.name.as_str(), "is_some" | "is_none") + && !clippy_covers_get_then_check(cx, receiver) => + { + parse_lookup(cx, receiver) + } + _ => None, + } +} + +fn parse_lookup<'tcx>( + cx: &LateContext<'tcx>, + expression: &'tcx Expr<'tcx>, +) -> Option> { + let expression = match &expression.kind { + ExprKind::MethodCall(segment, receiver, [], _) + if matches!(segment.ident.name.as_str(), "copied" | "cloned") => + { + receiver + } + _ => expression, + }; + + let ExprKind::MethodCall(segment, map, [key], _) = &expression.kind else { + return None; + }; + if !matches!( + segment.ident.name.as_str(), + "get" | "get_mut" | "get_key_value" + ) { + return None; + } + + let map_type = cx.typeck_results().expr_ty_adjusted(map).peel_refs(); + let map_definition = map_type.ty_adt_def()?; + if !is_std_set(cx, map_definition.did()) && !has_inherent_entry_method(cx, map_definition.did()) + { + return None; + } + + // The purity check runs on the peeled key so that borrows and + // value-preserving conversions do not count as side effects. + let mut key_converted = false; + let peeled_key = peel_key_expr(cx, key, &mut key_converted); + if map.can_have_side_effects() || peeled_key.can_have_side_effects() { + return None; + } + + Some(Lookup { map, key }) +} + +/// Searches `node` for an insertion into `lookup`'s map under `lookup`'s key. +/// Returns `Some(key_converted)` when one is found, where `key_converted` +/// records whether the match required peeling an owned-key conversion. +fn find_matching_insert<'tcx>( + cx: &LateContext<'tcx>, + lookup: Lookup<'tcx>, + node: impl Visitable<'tcx>, +) -> Option { + for_each_expr_without_closures(node, |expression| { + if let Some(insert) = parse_insert(expression) + && SpanlessEq::new(cx).eq_expr(lookup.map, insert.map) + && let Some(key_converted) = keys_match(cx, lookup.key, insert.key) + { + ControlFlow::Break(key_converted) + } else { + ControlFlow::Continue(()) + } + }) +} + +fn parse_insert<'tcx>(expression: &'tcx Expr<'tcx>) -> Option> { + // Maps insert `(key, value)`; sets insert only the value. + let ExprKind::MethodCall(segment, map, [key] | [key, _], _) = &expression.kind else { + return None; + }; + if segment.ident.name.as_str() != "insert" { + return None; + } + + Some(Lookup { map, key }) +} + +/// Checks whether the two key expressions denote the same key value. Returns +/// `Some(key_converted)` on a match, where `key_converted` records whether an +/// owned-key conversion had to be peeled to see it. +/// +/// Two forms are recognized: +/// * the keys are equal after peeling borrows, derefs, and value-preserving +/// conversions, e.g. looking up `&entry.key` and inserting +/// `entry.key.clone()`; +/// * after additionally resolving immutable local aliases to their +/// initializers, e.g. `let owned = key.clone();` before a lookup of `&key` +/// and an insertion of `owned`. Both resolved forms must be free of side +/// effects: two structurally equal side-effecting initializers (say, two +/// separate `next_key()` calls) are two different key values. +fn keys_match<'tcx>( + cx: &LateContext<'tcx>, + lookup_key: &'tcx Expr<'tcx>, + insert_key: &'tcx Expr<'tcx>, +) -> Option { + let mut key_converted = false; + let lookup_key = peel_key_expr(cx, lookup_key, &mut key_converted); + let insert_key = peel_key_expr(cx, insert_key, &mut key_converted); + if SpanlessEq::new(cx).eq_expr(lookup_key, insert_key) { + return Some(key_converted); + } + + let lookup_key = resolve_local_aliases(cx, lookup_key, &mut key_converted); + let insert_key = resolve_local_aliases(cx, insert_key, &mut key_converted); + if !lookup_key.can_have_side_effects() + && !insert_key.can_have_side_effects() + && SpanlessEq::new(cx).eq_expr(lookup_key, insert_key) + { + return Some(key_converted); + } + + None +} + +/// Follows immutable local bindings back to their initializers, peeling each +/// initializer down to its underlying key expression. `expr_or_init` only +/// resolves bindings that cannot be reassigned, so the binding's value is the +/// initializer's value wherever the binding is referenced. +fn resolve_local_aliases<'tcx>( + cx: &LateContext<'tcx>, + mut expression: &'tcx Expr<'tcx>, + key_converted: &mut bool, +) -> &'tcx Expr<'tcx> { + loop { + let resolved = expr_or_init(cx, expression); + if resolved.hir_id == expression.hir_id { + return expression; + } + expression = peel_key_expr(cx, resolved, key_converted); + } +} + +/// Strips layers that do not change which slot of the map the key selects: +/// borrows, derefs, and owned-key conversions of the underlying expression. +fn peel_key_expr<'tcx>( + cx: &LateContext<'tcx>, + mut expression: &'tcx Expr<'tcx>, + key_converted: &mut bool, +) -> &'tcx Expr<'tcx> { + loop { + match &expression.kind { + ExprKind::AddrOf(_, _, inner) | ExprKind::Unary(UnOp::Deref, inner) => { + expression = inner; + } + _ => match peel_value_preserving_conversion(cx, expression) { + Some(inner) => { + *key_converted = true; + expression = inner; + } + None => return expression, + }, + } + } +} + +/// If `expression` is a conversion that yields an owned key equal to its +/// input — under the same `Eq`/`Hash`/`Ord` contract the map lookup itself +/// relies on — returns the input. +/// +/// Each conversion is resolved by definition, not by name, so a user-defined +/// method that merely happens to be called `clone` or `to_owned` does not +/// count. For the `ToOwned` family, the `Borrow` contract (the owned and +/// borrowed forms hash and compare identically) is the same contract that +/// makes the map lookup itself well-defined. For `Clone`, the original code +/// already inserts the clone under the probed key, so the lint assumes +/// nothing the code did not. +fn peel_value_preserving_conversion<'tcx>( + cx: &LateContext<'tcx>, + expression: &'tcx Expr<'tcx>, +) -> Option<&'tcx Expr<'tcx>> { + match &expression.kind { + ExprKind::MethodCall(segment, receiver, [], _) => { + let method = cx + .typeck_results() + .type_dependent_def_id(expression.hir_id)?; + let preserves_value = match segment.ident.name.as_str() { + "clone" => is_assoc_of_diag_trait(cx, method, sym::Clone), + "to_owned" => is_assoc_of_diag_trait(cx, method, sym::ToOwned), + // `ToString` runs an arbitrary `Display` impl; only the + // string types are guaranteed to format as themselves. + "to_string" => { + is_assoc_of_diag_trait(cx, method, sym::ToString) && { + let receiver_type = + cx.typeck_results().expr_ty_adjusted(receiver).peel_refs(); + receiver_type.is_str() || is_string(cx, receiver_type) + } + } + "to_vec" => cx.tcx.impl_of_assoc(method).is_some_and(|impl_id| { + cx.tcx.type_of(impl_id).instantiate_identity().is_slice() + }), + _ => false, + }; + preserves_value.then_some(receiver) + } + // UFCS forms: `Clone::clone(&key)`, `ToOwned::to_owned(&key)`, and + // `String::from(key)` where `key` is a `&str`. + ExprKind::Call(callee, [argument]) => { + let ExprKind::Path(qpath) = &callee.kind else { + return None; + }; + let function = cx.qpath_res(qpath, callee.hir_id).opt_def_id()?; + let preserves_value = is_assoc_of_diag_trait(cx, function, sym::Clone) + || is_assoc_of_diag_trait(cx, function, sym::ToOwned) + || (is_assoc_of_diag_trait(cx, function, sym::From) + && is_string(cx, cx.typeck_results().expr_ty(expression)) + && cx.typeck_results().expr_ty(argument).peel_refs().is_str()); + preserves_value.then_some(argument) + } + _ => None, + } +} + +/// Whether `item` is an associated item of the trait named by the diagnostic +/// item `trait_name`. Qualified paths can resolve to a trait impl's item +/// rather than the trait's, so impl items are first mapped back to the trait +/// item they implement. +fn is_assoc_of_diag_trait(cx: &LateContext<'_>, item: DefId, trait_name: Symbol) -> bool { + let item = cx + .tcx + .opt_associated_item(item) + .and_then(|associated| associated.trait_item_def_id()) + .unwrap_or(item); + cx.tcx + .trait_of_assoc(item) + .is_some_and(|trait_id| cx.tcx.is_diagnostic_item(trait_name, trait_id)) +} + +fn is_string(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { + ty.ty_adt_def() + .is_some_and(|definition| cx.tcx.is_lang_item(definition.did(), LangItem::String)) +} + +/// Clippy already rewrites `get(..).is_some()`/`is_none()` on the std maps +/// and sets: `unnecessary_get_then_check` turns the condition into +/// `contains_key`/`contains`, which `map_entry`/`set_contains_or_insert` then +/// turn into the final form. Defer to that chain instead of duplicating it. +/// Conditions using `get_mut` or `get_key_value`, and any `get` on a non-std +/// map, are not covered by Clippy and stay ours. +fn clippy_covers_get_then_check(cx: &LateContext<'_>, receiver: &Expr<'_>) -> bool { + let ExprKind::MethodCall(segment, map, [_], _) = &receiver.kind else { + return false; + }; + if segment.ident.name.as_str() != "get" { + return false; + } + cx.typeck_results() + .expr_ty_adjusted(map) + .peel_refs() + .ty_adt_def() + .is_some_and(|definition| { + matches!( + cx.tcx.get_diagnostic_name(definition.did()), + Some(sym::HashMap | sym::BTreeMap | sym::HashSet | sym::BTreeSet) + ) + }) +} + +/// Sets are gated by diagnostic name rather than by an `entry` method: their +/// entry API is unstable (`hash_set_entry`), and the suggested rewrite (plain +/// `insert`) relies on std's no-op-when-present insertion semantics. +fn is_std_set(cx: &LateContext<'_>, definition: DefId) -> bool { + matches!( + cx.tcx.get_diagnostic_name(definition), + Some(sym::HashSet | sym::BTreeSet) + ) +} + +/// A lookup-then-insert pattern is only worth reporting when the map offers an +/// entry API to rewrite it with. Rather than hard-coding `HashMap` and +/// `BTreeMap`, accept any type whose inherent impls provide an `entry` method; +/// this also covers maps like `hashbrown::HashMap` and `indexmap::IndexMap`. +/// Hasher aliases such as `FxHashMap` need no special case because they +/// resolve to the underlying map type. +fn has_inherent_entry_method(cx: &LateContext<'_>, map_definition: DefId) -> bool { + let entry = Symbol::intern("entry"); + cx.tcx.inherent_impls(map_definition).iter().any(|&imp| { + cx.tcx + .associated_items(imp) + .filter_by_name_unhygienic(entry) + .next() + .is_some() + }) +} + +fn emit_lint( + cx: &LateContext<'_>, + span: rustc_span::Span, + lookup: Lookup<'_>, + key_converted: bool, +) { + let is_set = cx + .typeck_results() + .expr_ty_adjusted(lookup.map) + .peel_refs() + .ty_adt_def() + .is_some_and(|definition| is_std_set(cx, definition.did())); + + let (message, help) = if is_set { + ( + "this set lookup and insertion search for the same value twice", + "`insert` alone searches once, skips duplicates, and returns `false` \ + when the value was already present", + ) + } else { + ( + "this map lookup and insertion search for the same key twice", + "to search the map only once, use the entry API: \ + https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html", + ) + }; + span_lint_and_then(cx, MAP_LOOKUP_THEN_INSERT, span, message, |diagnostic| { + diagnostic.help(help); + if key_converted { + diagnostic.note( + "the inserted key is an owned copy of the looked-up key; a rewrite that \ + passes the key by value up front would make that copy on hits as well \ + as misses", + ); + } + }); +} diff --git a/tooling/lints/ui/map_lookup_then_insert.rs b/tooling/lints/ui/map_lookup_then_insert.rs new file mode 100644 index 0000000000..aa80986923 --- /dev/null +++ b/tooling/lints/ui/map_lookup_then_insert.rs @@ -0,0 +1,573 @@ +// Tests for the `map_lookup_then_insert` lint. + +#![allow(unused)] + +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; + +fn main() {} + +// ------------ SHOULD FIRE ----------- + +fn hash_map_if_let(map: &mut HashMap, key: String) -> usize { + if let Some(value) = map.get(&key) { + *value + } else { + map.insert(key, 1); + 1 + } +} + +fn hash_map_match(map: &mut HashMap, key: String) -> usize { + match map.get(&key).copied() { + Some(value) => value, + None => { + map.insert(key, 1); + 1 + } + } +} + +// Fix: `map.entry(key).and_modify(|value| *value += 1).or_insert(1)`. +fn hash_map_get_mut(map: &mut HashMap, key: String) { + match map.get_mut(&key) { + Some(value) => *value += 1, + None => { + map.insert(key, 1); + } + } +} + +fn hash_map_get_key_value(map: &mut HashMap, key: String) -> usize { + match map.get_key_value(&key) { + Some((_, value)) => *value, + None => { + map.insert(key, 1); + 1 + } + } +} + +fn btree_map_match(map: &mut BTreeMap, key: String) -> usize { + match map.get(&key) { + Some(value) => *value, + None => { + map.insert(key, 1); + 1 + } + } +} + +fn btree_map_if_let(map: &mut BTreeMap, key: String) -> usize { + if let Some(value) = map.get(&key) { + *value + } else { + map.insert(key, 1); + 1 + } +} + +// Fix: `map.entry(key).and_modify(|value| *value += 1).or_insert(1)`. +fn btree_map_get_mut(map: &mut BTreeMap, key: String) { + match map.get_mut(&key) { + Some(value) => *value += 1, + None => { + map.insert(key, 1); + } + } +} + +fn btree_map_get_key_value(map: &mut BTreeMap, key: String) -> usize { + match map.get_key_value(&key) { + Some((_, value)) => *value, + None => { + map.insert(key, 1); + 1 + } + } +} + +fn btree_map_let_else(map: &mut BTreeMap, key: String) -> usize { + let Some(value) = map.get(&key) else { + map.insert(key, 1); + return 1; + }; + *value +} + +// `btree_map::Entry` has no `insert_entry`; the fix here is +// `map.entry(key).and_modify(|value| *value = 2)`. +fn btree_map_overwrite_when_present(map: &mut BTreeMap, key: String) { + if let Some(_current) = map.get(&key) { + map.insert(key, 2); + } +} + +// Sets repeat the search too. They need no entry API: the fix is plain +// `set.insert(value)`, which returns `false` when the value was present. +fn hash_set_if_let(set: &mut HashSet, value: String) -> bool { + if let Some(existing) = set.get(&value) { + existing.is_empty() + } else { + set.insert(value) + } +} + +fn btree_set_match(set: &mut BTreeSet, value: String) -> bool { + match set.get(&value) { + Some(_) => false, + None => set.insert(value), + } +} + +fn hash_map_let_else(map: &mut HashMap, key: String) -> usize { + let Some(value) = map.get(&key) else { + map.insert(key, 1); + return 1; + }; + *value +} + +fn expensive() -> usize { + 42 +} + +// Fix: `map.entry(key).or_insert_with(expensive)`. +fn fix_with_or_insert_with(map: &mut HashMap, key: String) { + match map.get(&key) { + Some(_) => {} + None => { + map.insert(key, expensive()); + } + } +} + +// Fix: `map.entry(key).or_insert_with_key(|key| key.len())`. +fn fix_with_or_insert_with_key(map: &mut HashMap, key: String) { + match map.get(&key) { + Some(_) => {} + None => { + let value = key.len(); + map.insert(key, value); + } + } +} + +// Fix: `map.entry(key).or_default()`. +fn fix_with_or_default(map: &mut HashMap, key: String) { + match map.get(&key) { + Some(_) => {} + None => { + map.insert(key, usize::default()); + } + } +} + +// Fix: `map.entry(key).insert_entry(2)`, keeping the occupancy check. +fn fix_with_insert_entry(map: &mut HashMap, key: String) { + if let Some(_current) = map.get(&key) { + map.insert(key, 2); + } +} + +// Hasher aliases like `FxHashMap` resolve to `HashMap`, so they are covered. +type FxLikeMap = HashMap< + String, + usize, + std::hash::BuildHasherDefault, +>; + +fn custom_hasher_alias(map: &mut FxLikeMap, key: String) { + match map.get(&key) { + Some(_) => {} + None => { + map.insert(key, 1); + } + } +} + +// A third-party map (`hashbrown`, `indexmap`, ...) that offers an inherent +// `entry` method is covered. +struct EntryMap; + +impl EntryMap { + fn get(&self, _key: &str) -> Option<&usize> { + None + } + + fn insert(&mut self, _key: String, _value: usize) {} + + fn entry(&mut self, _key: String) -> std::collections::hash_map::Entry<'_, String, usize> { + unimplemented!() + } +} + +// Clippy's `unnecessary_get_then_check` only covers the std collections, so +// the `get(..).is_none()` form on a third-party map is still ours to catch. +fn entry_compatible_custom_map(map: &mut EntryMap, key: String) { + if map.get(&key).is_none() { + map.insert(key, 1); + } +} + +// --- Keys that differ only by a value-preserving conversion --- + +// The shape that motivated conversion peeling: code in the wild looks up +// `&key` and inserts `key.clone()`, which a textual comparison misses. +fn insert_clones_the_key(map: &mut HashMap>, key: String, value: usize) { + if let Some(values) = map.get_mut(&key) { + values.push(value); + } else { + map.insert(key.clone(), vec![value]); + } +} + +fn insert_clones_the_key_ufcs(map: &mut HashMap, key: String) { + match map.get(&key) { + Some(_) => {} + None => { + map.insert(Clone::clone(&key), 1); + } + } +} + +fn insert_key_to_owned(map: &mut HashMap, key: &str) { + match map.get(key) { + Some(_) => {} + None => { + map.insert(key.to_owned(), 1); + } + } +} + +fn insert_key_to_string(map: &mut HashMap, key: &str) { + match map.get(key) { + Some(_) => {} + None => { + map.insert(key.to_string(), 1); + } + } +} + +fn insert_key_string_from(map: &mut HashMap, key: &str) { + match map.get(key) { + Some(_) => {} + None => { + map.insert(String::from(key), 1); + } + } +} + +fn insert_key_to_vec(map: &mut HashMap, usize>, key: &[u8]) { + match map.get(key) { + Some(_) => {} + None => { + map.insert(key.to_vec(), 1); + } + } +} + +// The lookup and the insertion may each add their own borrow/deref layers. +fn insert_clone_of_deref(map: &mut HashMap, key: &String) { + match map.get(key) { + Some(_) => {} + None => { + map.insert((*key).clone(), 1); + } + } +} + +// An immutable local alias of the cloned key is still the same key. +fn insert_through_alias(map: &mut HashMap, key: String) { + let owned = key.clone(); + match map.get(&key) { + Some(_) => {} + None => { + map.insert(owned, 1); + } + } +} + +fn set_insert_clone(set: &mut HashSet, value: String) -> bool { + if let Some(existing) = set.get(&value) { + existing.is_empty() + } else { + set.insert(value.clone()) + } +} + +// ------------- SHOULD NOT FIRE ------------- + +// `contains_key` followed by `insert` is Clippy's `map_entry` lint. +fn clippy_handles_contains_key(map: &mut HashMap, key: String) { + if !map.contains_key(&key) { + map.insert(key, 1); + } +} + +// `get(..).is_none()`/`is_some()` on std collections is Clippy's chain: +// `unnecessary_get_then_check` rewrites the condition to +// `contains_key`/`contains`, which `map_entry`/`set_contains_or_insert` +// rewrite again. +fn clippy_handles_hash_map_get_is_none(map: &mut HashMap, key: String) { + if map.get(&key).is_none() { + map.insert(key, 1); + } +} + +fn clippy_handles_hash_map_get_is_some(map: &mut HashMap, key: String) { + if map.get(&key).is_some() { + map.insert(key, 2); + } +} + +fn clippy_handles_btree_map_get_is_none(map: &mut BTreeMap, key: String) { + if map.get(&key).is_none() { + map.insert(key, 1); + } +} + +fn clippy_handles_hash_set_get_is_none(set: &mut HashSet, value: String) { + if set.get(&value).is_none() { + set.insert(value); + } +} + +fn clippy_handles_btree_set_get_is_none(set: &mut BTreeSet, value: String) { + if set.get(&value).is_none() { + set.insert(value); + } +} + +fn already_uses_entry_or_insert(map: &mut HashMap, key: String) { + map.entry(key).or_insert(1); +} + +fn already_uses_entry_or_insert_with(map: &mut HashMap, key: String) { + map.entry(key).or_insert_with(expensive); +} + +fn already_uses_entry_or_insert_with_key(map: &mut HashMap, key: String) { + map.entry(key).or_insert_with_key(|key| key.len()); +} + +fn already_uses_entry_or_default(map: &mut HashMap, key: String) { + map.entry(key).or_default(); +} + +fn already_uses_entry_and_modify(map: &mut HashMap, key: String) { + map.entry(key).and_modify(|value| *value += 1).or_insert(1); +} + +fn already_uses_entry_insert_entry(map: &mut HashMap, key: String) { + map.entry(key).insert_entry(2); +} + +fn already_uses_entry_key(map: &mut HashMap, key: String) -> usize { + map.entry(key).key().len() +} + +// `btree_map::Entry` offers every method that `hash_map::Entry` does except +// `insert_entry`, so there is no BTreeMap mirror of that case. +fn btree_already_uses_entry_or_insert(map: &mut BTreeMap, key: String) { + map.entry(key).or_insert(1); +} + +fn btree_already_uses_entry_or_insert_with(map: &mut BTreeMap, key: String) { + map.entry(key).or_insert_with(expensive); +} + +fn btree_already_uses_entry_or_insert_with_key(map: &mut BTreeMap, key: String) { + map.entry(key).or_insert_with_key(|key| key.len()); +} + +fn btree_already_uses_entry_or_default(map: &mut BTreeMap, key: String) { + map.entry(key).or_default(); +} + +fn btree_already_uses_entry_and_modify(map: &mut BTreeMap, key: String) { + map.entry(key).and_modify(|value| *value += 1).or_insert(1); +} + +fn btree_already_uses_entry_key(map: &mut BTreeMap, key: String) -> usize { + map.entry(key).key().len() +} + +// `contains` followed by `insert` is Clippy's `set_contains_or_insert` lint. +fn clippy_handles_set_contains(set: &mut HashSet, value: String) { + if !set.contains(&value) { + set.insert(value); + } +} + +// A bare `insert` already searches only once. +fn set_insert_alone(set: &mut HashSet, value: String) -> bool { + set.insert(value) +} + +fn btree_set_insert_alone(set: &mut BTreeSet, value: String) -> bool { + set.insert(value) +} + +fn different_key(map: &mut HashMap, first: String, second: String) { + match map.get(&first) { + Some(_) => {} + None => { + map.insert(second, 1); + } + } +} + +fn different_map( + first: &HashMap, + second: &mut HashMap, + key: String, +) { + match first.get(&key) { + Some(_) => {} + None => { + second.insert(key, 1); + } + } +} + +fn lookup_without_insert(map: &HashMap, key: &str) -> Option { + map.get(key).copied() +} + +fn insertion_in_deferred_closure(map: &mut HashMap, key: String) { + match map.get(&key).copied() { + Some(_) => {} + None => { + let _insert_later = || { + map.insert(key, 1); + }; + } + } +} + +// `CustomMap` has no `entry` method, so there is no entry API to suggest. +struct CustomMap; + +impl CustomMap { + fn get(&self, _key: &str) -> Option<&usize> { + None + } + + fn insert(&mut self, _key: String, _value: usize) {} +} + +fn non_standard_map(map: &mut CustomMap, key: String) { + if map.get(&key).is_none() { + map.insert(key, 1); + } +} + +fn next_key() -> String { + String::new() +} + +fn side_effecting_key(map: &mut HashMap) { + match map.get(&next_key()) { + Some(_) => {} + None => { + map.insert(next_key(), 1); + } + } +} + +fn global_map() -> &'static mut HashMap { + unimplemented!() +} + +fn side_effecting_map(key: String) { + match global_map().get(&key) { + Some(_) => {} + None => { + global_map().insert(key, 1); + } + } +} + +// Conversions are peeled by definition, not by name: `DisguisedKey`'s +// inherent `clone` and `to_owned` are not `Clone::clone`/`ToOwned::to_owned` +// and may return a different key. +#[derive(Clone, PartialEq, Eq, Hash)] +struct DisguisedKey(usize); + +impl DisguisedKey { + fn clone(&self) -> DisguisedKey { + DisguisedKey(self.0 + 1) + } + + fn to_owned(&self) -> DisguisedKey { + DisguisedKey(self.0 * 2) + } +} + +fn inherent_clone_is_a_different_key(map: &mut HashMap, key: DisguisedKey) { + match map.get(&key) { + Some(_) => {} + None => { + map.insert(key.clone(), 1); + } + } +} + +fn inherent_to_owned_is_a_different_key( + map: &mut HashMap, + key: DisguisedKey, +) { + match map.get(&key) { + Some(_) => {} + None => { + map.insert(key.to_owned(), 1); + } + } +} + +// A mutable alias may no longer hold the looked-up key by insertion time. +fn mutated_alias(map: &mut HashMap, key: String) { + let mut owned = key.clone(); + owned.push('!'); + match map.get(&key) { + Some(_) => {} + None => { + map.insert(owned, 1); + } + } +} + +// A clone of a different key is a different key. +fn clone_of_different_key(map: &mut HashMap, first: String, second: String) { + match map.get(&first) { + Some(_) => {} + None => { + map.insert(second.clone(), 1); + } + } +} + +// `to_string` runs an arbitrary `Display` impl; only the string types are +// guaranteed to format as themselves, so a non-string receiver stays a +// side-effecting key and is not matched. +fn to_string_of_non_string_key(map: &mut HashMap, id: u32) { + match map.get(&id.to_string()) { + Some(_) => {} + None => { + map.insert(id.to_string(), 1); + } + } +} + +// Alias resolution must not equate structurally equal but side-effecting +// initializers: the two `next_key()` calls produce two different keys. +fn aliases_of_two_generated_keys(map: &mut HashMap) { + let first = next_key(); + let second = next_key(); + match map.get(&first) { + Some(_) => {} + None => { + map.insert(second, 1); + } + } +} diff --git a/tooling/lints/ui/map_lookup_then_insert.stderr b/tooling/lints/ui/map_lookup_then_insert.stderr new file mode 100644 index 0000000000..2406471f31 --- /dev/null +++ b/tooling/lints/ui/map_lookup_then_insert.stderr @@ -0,0 +1,359 @@ +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:12:5 + | +LL | / if let Some(value) = map.get(&key) { +LL | | *value +LL | | } else { +LL | | map.insert(key, 1); +LL | | 1 +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + = note: `-D map-lookup-then-insert` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(map_lookup_then_insert)]` + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:21:5 + | +LL | / match map.get(&key).copied() { +LL | | Some(value) => value, +LL | | None => { +LL | | map.insert(key, 1); +... | +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:32:5 + | +LL | / match map.get_mut(&key) { +LL | | Some(value) => *value += 1, +LL | | None => { +LL | | map.insert(key, 1); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:41:5 + | +LL | / match map.get_key_value(&key) { +LL | | Some((_, value)) => *value, +LL | | None => { +LL | | map.insert(key, 1); +... | +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:51:5 + | +LL | / match map.get(&key) { +LL | | Some(value) => *value, +LL | | None => { +LL | | map.insert(key, 1); +... | +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:61:5 + | +LL | / if let Some(value) = map.get(&key) { +LL | | *value +LL | | } else { +LL | | map.insert(key, 1); +LL | | 1 +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:71:5 + | +LL | / match map.get_mut(&key) { +LL | | Some(value) => *value += 1, +LL | | None => { +LL | | map.insert(key, 1); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:80:5 + | +LL | / match map.get_key_value(&key) { +LL | | Some((_, value)) => *value, +LL | | None => { +LL | | map.insert(key, 1); +... | +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:90:5 + | +LL | / let Some(value) = map.get(&key) else { +LL | | map.insert(key, 1); +LL | | return 1; +LL | | }; + | |______^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:100:5 + | +LL | / if let Some(_current) = map.get(&key) { +LL | | map.insert(key, 2); +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this set lookup and insertion search for the same value twice + --> $DIR/map_lookup_then_insert.rs:108:5 + | +LL | / if let Some(existing) = set.get(&value) { +LL | | existing.is_empty() +LL | | } else { +LL | | set.insert(value) +LL | | } + | |_____^ + | + = help: `insert` alone searches once, skips duplicates, and returns `false` when the value was already present + +error: this set lookup and insertion search for the same value twice + --> $DIR/map_lookup_then_insert.rs:116:5 + | +LL | / match set.get(&value) { +LL | | Some(_) => false, +LL | | None => set.insert(value), +LL | | } + | |_____^ + | + = help: `insert` alone searches once, skips duplicates, and returns `false` when the value was already present + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:123:5 + | +LL | / let Some(value) = map.get(&key) else { +LL | | map.insert(key, 1); +LL | | return 1; +LL | | }; + | |______^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:136:5 + | +LL | / match map.get(&key) { +LL | | Some(_) => {} +LL | | None => { +LL | | map.insert(key, expensive()); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:146:5 + | +LL | / match map.get(&key) { +LL | | Some(_) => {} +LL | | None => { +LL | | let value = key.len(); +... | +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:157:5 + | +LL | / match map.get(&key) { +LL | | Some(_) => {} +LL | | None => { +LL | | map.insert(key, usize::default()); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:167:5 + | +LL | / if let Some(_current) = map.get(&key) { +LL | | map.insert(key, 2); +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:180:5 + | +LL | / match map.get(&key) { +LL | | Some(_) => {} +LL | | None => { +LL | | map.insert(key, 1); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:207:5 + | +LL | / if map.get(&key).is_none() { +LL | | map.insert(key, 1); +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:217:5 + | +LL | / if let Some(values) = map.get_mut(&key) { +LL | | values.push(value); +LL | | } else { +LL | | map.insert(key.clone(), vec![value]); +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + = note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:225:5 + | +LL | / match map.get(&key) { +LL | | Some(_) => {} +LL | | None => { +LL | | map.insert(Clone::clone(&key), 1); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + = note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:234:5 + | +LL | / match map.get(key) { +LL | | Some(_) => {} +LL | | None => { +LL | | map.insert(key.to_owned(), 1); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + = note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:243:5 + | +LL | / match map.get(key) { +LL | | Some(_) => {} +LL | | None => { +LL | | map.insert(key.to_string(), 1); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + = note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:252:5 + | +LL | / match map.get(key) { +LL | | Some(_) => {} +LL | | None => { +LL | | map.insert(String::from(key), 1); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + = note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:261:5 + | +LL | / match map.get(key) { +LL | | Some(_) => {} +LL | | None => { +LL | | map.insert(key.to_vec(), 1); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + = note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:271:5 + | +LL | / match map.get(key) { +LL | | Some(_) => {} +LL | | None => { +LL | | map.insert((*key).clone(), 1); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + = note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses + +error: this map lookup and insertion search for the same key twice + --> $DIR/map_lookup_then_insert.rs:282:5 + | +LL | / match map.get(&key) { +LL | | Some(_) => {} +LL | | None => { +LL | | map.insert(owned, 1); +LL | | } +LL | | } + | |_____^ + | + = help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html + = note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses + +error: this set lookup and insertion search for the same value twice + --> $DIR/map_lookup_then_insert.rs:291:5 + | +LL | / if let Some(existing) = set.get(&value) { +LL | | existing.is_empty() +LL | | } else { +LL | | set.insert(value.clone()) +LL | | } + | |_____^ + | + = help: `insert` alone searches once, skips duplicates, and returns `false` when the value was already present + = note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses + +error: aborting due to 28 previous errors +