Skip to content

Fix iter_kv_map false positive into_keys and into_values suggestion #11757

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 22, 2023
Merged

Fix iter_kv_map false positive into_keys and into_values suggestion #11757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f651da
fix iter_kv_map dont suggest into_keys and into_values if msrv is to low
matthri Nov 3, 2023
a20f61b
add iter_kv_map to msrv config
matthri Nov 4, 2023
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
1 change: 1 addition & 0 deletions book/src/lint_configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio
* [`tuple_array_conversions`](https://rust-lang.github.io/rust-clippy/master/index.html#tuple_array_conversions)
* [`manual_try_fold`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_try_fold)
* [`manual_hash_one`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_hash_one)
* [`iter_kv_map`](https://rust-lang.github.io/rust-clippy/master/index.html#iter_kv_map)


## `cognitive-complexity-threshold`
Expand Down
2 changes: 1 addition & 1 deletion clippy_config/src/conf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ define_Conf! {
///
/// Suppress lints whenever the suggested change would cause breakage for other crates.
(avoid_breaking_exported_api: bool = true),
/// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, OPTION_MAP_UNWRAP_OR, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS, TUPLE_ARRAY_CONVERSIONS, MANUAL_TRY_FOLD, MANUAL_HASH_ONE.
/// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, OPTION_MAP_UNWRAP_OR, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS, TUPLE_ARRAY_CONVERSIONS, MANUAL_TRY_FOLD, MANUAL_HASH_ONE, ITER_KV_MAP.
///
/// The minimum rust version that the project supports. Defaults to the `rust-version` field in `Cargo.toml`
#[default_text = ""]
Expand Down
1 change: 1 addition & 0 deletions clippy_config/src/msrvs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ msrv_aliases! {
1,62,0 { BOOL_THEN_SOME, DEFAULT_ENUM_ATTRIBUTE }
1,58,0 { FORMAT_ARGS_CAPTURE, PATTERN_TRAIT_CHAR_ARRAY }
1,55,0 { SEEK_REWIND }
1,54,0 { INTO_KEYS }
1,53,0 { OR_PATTERNS, MANUAL_BITS, BTREE_MAP_RETAIN, BTREE_SET_RETAIN, ARRAY_INTO_ITERATOR }
1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST }
1,51,0 { BORROW_AS_PTR, SEEK_FROM_CURRENT, UNSIGNED_ABS }
Expand Down
5 changes: 5 additions & 0 deletions clippy_lints/src/methods/iter_kv_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#![allow(unused_imports)]

use super::ITER_KV_MAP;
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::{snippet, snippet_with_applicability};
use clippy_utils::ty::is_type_diagnostic_item;
Expand All @@ -21,7 +22,11 @@ pub(super) fn check<'tcx>(
expr: &'tcx Expr<'tcx>, // .iter().map(|(_, v_| v))
recv: &'tcx Expr<'tcx>, // hashmap
m_arg: &'tcx Expr<'tcx>, // |(_, v)| v
msrv: &Msrv,
) {
if map_type == "into_iter" && !msrv.meets(msrvs::INTO_KEYS) {
return;
}
if !expr.span.from_expansion()
&& let ExprKind::Closure(c) = m_arg.kind
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lint should probably check that the closure doesn't have parameter- or return type annotations since that can help type inference or make the map closure act as a coercion site.
Not your problem though, that could be a PR on its own, unless you also want to try to fix that here ^^.

Examples where the machine applicable suggestion breaks: coercions, inference

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have a try fixing this, but I think I will open a separate PR so this one doesn't get too cluttered ^^

&& let Body {
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/methods/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4255,7 +4255,7 @@ impl Methods {
map_clone::check(cx, expr, recv, m_arg, &self.msrv);
match method_call(recv) {
Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) => {
iter_kv_map::check(cx, map_name, expr, recv2, m_arg);
iter_kv_map::check(cx, map_name, expr, recv2, m_arg, &self.msrv);
},
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(
cx,
Expand Down
43 changes: 43 additions & 0 deletions tests/ui/iter_kv_map.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,46 @@ fn main() {
// Don't let a mut interfere.
let _ = map.clone().into_values().count();
}

#[clippy::msrv = "1.53"]
fn msrv_1_53() {
let map: HashMap<u32, u32> = HashMap::new();

// Don't lint because into_iter is not supported
let _ = map.clone().into_iter().map(|(key, _)| key).collect::<Vec<_>>();
let _ = map.clone().into_iter().map(|(key, _)| key + 2).collect::<Vec<_>>();

let _ = map.clone().into_iter().map(|(_, val)| val).collect::<Vec<_>>();
let _ = map.clone().into_iter().map(|(_, val)| val + 2).collect::<Vec<_>>();

// Lint
let _ = map.keys().collect::<Vec<_>>();
//~^ ERROR: iterating on a map's keys
let _ = map.values().collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values
let _ = map.values().map(|v| v + 2).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values
}

#[clippy::msrv = "1.54"]
fn msrv_1_54() {
// Lint all
let map: HashMap<u32, u32> = HashMap::new();

let _ = map.clone().into_keys().collect::<Vec<_>>();
//~^ ERROR: iterating on a map's keys
let _ = map.clone().into_keys().map(|key| key + 2).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's keys

let _ = map.clone().into_values().collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values
let _ = map.clone().into_values().map(|val| val + 2).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values

let _ = map.keys().collect::<Vec<_>>();
//~^ ERROR: iterating on a map's keys
let _ = map.values().collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values
let _ = map.values().map(|v| v + 2).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values
}
43 changes: 43 additions & 0 deletions tests/ui/iter_kv_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,46 @@ fn main() {
// Don't let a mut interfere.
let _ = map.clone().into_iter().map(|(_, mut val)| val).count();
}

#[clippy::msrv = "1.53"]
fn msrv_1_53() {
let map: HashMap<u32, u32> = HashMap::new();

// Don't lint because into_iter is not supported
let _ = map.clone().into_iter().map(|(key, _)| key).collect::<Vec<_>>();
let _ = map.clone().into_iter().map(|(key, _)| key + 2).collect::<Vec<_>>();

let _ = map.clone().into_iter().map(|(_, val)| val).collect::<Vec<_>>();
let _ = map.clone().into_iter().map(|(_, val)| val + 2).collect::<Vec<_>>();

// Lint
let _ = map.iter().map(|(key, _)| key).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's keys
let _ = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values
let _ = map.iter().map(|(_, v)| v + 2).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values
}

#[clippy::msrv = "1.54"]
fn msrv_1_54() {
// Lint all
let map: HashMap<u32, u32> = HashMap::new();

let _ = map.clone().into_iter().map(|(key, _)| key).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's keys
let _ = map.clone().into_iter().map(|(key, _)| key + 2).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's keys

let _ = map.clone().into_iter().map(|(_, val)| val).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values
let _ = map.clone().into_iter().map(|(_, val)| val + 2).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values

let _ = map.iter().map(|(key, _)| key).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's keys
let _ = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values
let _ = map.iter().map(|(_, v)| v + 2).collect::<Vec<_>>();
//~^ ERROR: iterating on a map's values
}
62 changes: 61 additions & 1 deletion tests/ui/iter_kv_map.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,5 +201,65 @@ error: iterating on a map's values
LL | let _ = map.clone().into_iter().map(|(_, mut val)| val).count();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.clone().into_values()`

error: aborting due to 28 previous errors
error: iterating on a map's keys
--> $DIR/iter_kv_map.rs:109:13
|
LL | let _ = map.iter().map(|(key, _)| key).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.keys()`

error: iterating on a map's values
--> $DIR/iter_kv_map.rs:111:13
|
LL | let _ = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.values()`

error: iterating on a map's values
--> $DIR/iter_kv_map.rs:113:13
|
LL | let _ = map.iter().map(|(_, v)| v + 2).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.values().map(|v| v + 2)`

error: iterating on a map's keys
--> $DIR/iter_kv_map.rs:122:13
|
LL | let _ = map.clone().into_iter().map(|(key, _)| key).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.clone().into_keys()`

error: iterating on a map's keys
--> $DIR/iter_kv_map.rs:124:13
|
LL | let _ = map.clone().into_iter().map(|(key, _)| key + 2).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.clone().into_keys().map(|key| key + 2)`

error: iterating on a map's values
--> $DIR/iter_kv_map.rs:127:13
|
LL | let _ = map.clone().into_iter().map(|(_, val)| val).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.clone().into_values()`

error: iterating on a map's values
--> $DIR/iter_kv_map.rs:129:13
|
LL | let _ = map.clone().into_iter().map(|(_, val)| val + 2).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.clone().into_values().map(|val| val + 2)`

error: iterating on a map's keys
--> $DIR/iter_kv_map.rs:132:13
|
LL | let _ = map.iter().map(|(key, _)| key).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.keys()`

error: iterating on a map's values
--> $DIR/iter_kv_map.rs:134:13
|
LL | let _ = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.values()`

error: iterating on a map's values
--> $DIR/iter_kv_map.rs:136:13
|
LL | let _ = map.iter().map(|(_, v)| v + 2).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `map.values().map(|v| v + 2)`

error: aborting due to 38 previous errors