Auto merge of rust-lang#11791 - Jacherr:iter_over_hash_type, r=Jarcho

Implement new lint `iter_over_hash_type`

Implements and fixes rust-lang/rust-clippy#11788

This PR adds a new *restriction* lint `iter_over_hash_type` which prevents `Hash`-types (that is, `HashSet` and `HashMap`) from being used as the iterator in `for` loops.

The justification for this is because in `Hash`-based types, the ordering of items is not guaranteed and may vary between executions of the same program on the same hardware. In addition, it reduces readability due to the unclear iteration order.

The implementation of this lint also ensures the following:
- Calls to `HashMap::keys`, `HashMap::values`, and `HashSet::iter` are also denied when used in `for` loops,
- When this expression is used in procedural macros, it is not linted/denied.

changelog: add new `iter_over_hash_type` lint to prevent unordered iterations through hashed data structures

Author: bors

CHANGELOG.md

@@ -5123,6 +5123,7 @@ Released 2018-09-13
 [`iter_on_empty_collections`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_on_empty_collections
 [`iter_on_single_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_on_single_items
 [`iter_out_of_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_out_of_bounds
+[`iter_over_hash_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_over_hash_type
 [`iter_overeager_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_overeager_cloned
 [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next
 [`iter_skip_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_zero

clippy_lints/src/declared_lints.rs

@@ -231,6 +231,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::items_after_statements::ITEMS_AFTER_STATEMENTS_INFO,
     crate::items_after_test_module::ITEMS_AFTER_TEST_MODULE_INFO,
     crate::iter_not_returning_iterator::ITER_NOT_RETURNING_ITERATOR_INFO,
+    crate::iter_over_hash_type::ITER_OVER_HASH_TYPE_INFO,
     crate::iter_without_into_iter::INTO_ITER_WITHOUT_ITER_INFO,
     crate::iter_without_into_iter::ITER_WITHOUT_INTO_ITER_INFO,
     crate::large_const_arrays::LARGE_CONST_ARRAYS_INFO,

clippy_lints/src/iter_over_hash_type.rs

@@ -0,0 +1,78 @@
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::higher::ForLoop;
+use clippy_utils::match_any_def_paths;
+use clippy_utils::paths::{
+    HASHMAP_DRAIN, HASHMAP_ITER, HASHMAP_ITER_MUT, HASHMAP_KEYS, HASHMAP_VALUES, HASHMAP_VALUES_MUT, HASHSET_DRAIN,
+    HASHSET_ITER_TY,
+};
+use clippy_utils::ty::is_type_diagnostic_item;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// This is a restriction lint which prevents the use of hash types (i.e., `HashSet` and `HashMap`) in for loops.
+    ///
+    /// ### Why is this bad?
+    /// Because hash types are unordered, when iterated through such as in a for loop, the values are returned in
+    /// an undefined order. As a result, on redundant systems this may cause inconsistencies and anomalies.
+    /// In addition, the unknown order of the elements may reduce readability or introduce other undesired
+    /// side effects.
+    ///
+    /// ### Example
+    /// ```no_run
+    ///     let my_map = std::collections::HashMap::<i32, String>::new();
+    ///     for (key, value) in my_map { /* ... */ }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    ///     let my_map = std::collections::HashMap::<i32, String>::new();
+    ///     let mut keys = my_map.keys().clone().collect::<Vec<_>>();
+    ///     keys.sort();
+    ///     for key in keys {
+    ///         let value = &my_map[key];
+    ///     }
+    /// ```
+    #[clippy::version = "1.75.0"]
+    pub ITER_OVER_HASH_TYPE,
+    restriction,
+    "iterating over unordered hash-based types (`HashMap` and `HashSet`)"
+}
+
+declare_lint_pass!(IterOverHashType => [ITER_OVER_HASH_TYPE]);
+
+impl LateLintPass<'_> for IterOverHashType {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ rustc_hir::Expr<'_>) {
+        if let Some(for_loop) = ForLoop::hir(expr)
+            && !for_loop.body.span.from_expansion()
+            && let ty = cx.typeck_results().expr_ty(for_loop.arg).peel_refs()
+            && let Some(adt) = ty.ty_adt_def()
+            && let did = adt.did()
+            && (match_any_def_paths(
+                cx,
+                did,
+                &[
+                    &HASHMAP_KEYS,
+                    &HASHMAP_VALUES,
+                    &HASHMAP_VALUES_MUT,
+                    &HASHMAP_ITER,
+                    &HASHMAP_ITER_MUT,
+                    &HASHMAP_DRAIN,
+                    &HASHSET_ITER_TY,
+                    &HASHSET_DRAIN,
+                ],
+            )
+            .is_some()
+                || is_type_diagnostic_item(cx, ty, sym::HashMap)
+                || is_type_diagnostic_item(cx, ty, sym::HashSet))
+        {
+            span_lint(
+                cx,
+                ITER_OVER_HASH_TYPE,
+                expr.span,
+                "iteration over unordered hash-based type",
+            );
+        };
+    }
+}

clippy_lints/src/lib.rs

@@ -164,6 +164,7 @@ mod item_name_repetitions;
 mod items_after_statements;
 mod items_after_test_module;
 mod iter_not_returning_iterator;
+mod iter_over_hash_type;
 mod iter_without_into_iter;
 mod large_const_arrays;
 mod large_enum_variant;
@@ -1064,6 +1065,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     });
     store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(msrv())));
     store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter));
+    store.register_late_pass(|_| Box::new(iter_over_hash_type::IterOverHashType));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_utils/src/paths.rs

@@ -32,7 +32,15 @@ pub const FUTURES_IO_ASYNCREADEXT: [&str; 3] = ["futures_util", "io", "AsyncRead
 pub const FUTURES_IO_ASYNCWRITEEXT: [&str; 3] = ["futures_util", "io", "AsyncWriteExt"];
 pub const HASHMAP_CONTAINS_KEY: [&str; 6] = ["std", "collections", "hash", "map", "HashMap", "contains_key"];
 pub const HASHMAP_INSERT: [&str; 6] = ["std", "collections", "hash", "map", "HashMap", "insert"];
+pub const HASHMAP_ITER: [&str; 5] = ["std", "collections", "hash", "map", "Iter"];
+pub const HASHMAP_ITER_MUT: [&str; 5] = ["std", "collections", "hash", "map", "IterMut"];
+pub const HASHMAP_KEYS: [&str; 5] = ["std", "collections", "hash", "map", "Keys"];
+pub const HASHMAP_VALUES: [&str; 5] = ["std", "collections", "hash", "map", "Values"];
+pub const HASHMAP_DRAIN: [&str; 5] = ["std", "collections", "hash", "map", "Drain"];
+pub const HASHMAP_VALUES_MUT: [&str; 5] = ["std", "collections", "hash", "map", "ValuesMut"];
+pub const HASHSET_ITER_TY: [&str; 5] = ["std", "collections", "hash", "set", "Iter"];
 pub const HASHSET_ITER: [&str; 6] = ["std", "collections", "hash", "set", "HashSet", "iter"];
+pub const HASHSET_DRAIN: [&str; 5] = ["std", "collections", "hash", "set", "Drain"];
 pub const IDENT: [&str; 3] = ["rustc_span", "symbol", "Ident"];
 pub const IDENT_AS_STR: [&str; 4] = ["rustc_span", "symbol", "Ident", "as_str"];
 pub const INSERT_STR: [&str; 4] = ["alloc", "string", "String", "insert_str"];

tests/ui/iter_over_hash_type.rs

@@ -0,0 +1,74 @@
+//@aux-build:proc_macros.rs
+#![feature(rustc_private)]
+#![warn(clippy::iter_over_hash_type)]
+use std::collections::{HashMap, HashSet};
+
+extern crate rustc_data_structures;
+
+extern crate proc_macros;
+
+fn main() {
+    let mut hash_set = HashSet::<i32>::new();
+    let mut hash_map = HashMap::<i32, i32>::new();
+    let mut fx_hash_map = rustc_data_structures::fx::FxHashMap::<i32, i32>::default();
+    let mut fx_hash_set = rustc_data_structures::fx::FxHashMap::<i32, i32>::default();
+    let vec = Vec::<i32>::new();
+
+    // test hashset
+    for x in &hash_set {
+        let _ = x;
+    }
+    for x in hash_set.iter() {
+        let _ = x;
+    }
+    for x in hash_set.clone() {
+        let _ = x;
+    }
+    for x in hash_set.drain() {
+        let _ = x;
+    }
+
+    // test hashmap
+    for (x, y) in &hash_map {
+        let _ = (x, y);
+    }
+    for x in hash_map.keys() {
+        let _ = x;
+    }
+    for x in hash_map.values() {
+        let _ = x;
+    }
+    for x in hash_map.values_mut() {
+        *x += 1;
+    }
+    for x in hash_map.iter() {
+        let _ = x;
+    }
+    for x in hash_map.clone() {
+        let _ = x;
+    }
+    for x in hash_map.drain() {
+        let _ = x;
+    }
+
+    // test type-aliased hashers
+    for x in fx_hash_set {
+        let _ = x;
+    }
+    for x in fx_hash_map {
+        let _ = x;
+    }
+
+    // shouldnt fire
+    for x in &vec {
+        let _ = x;
+    }
+    for x in vec {
+        let _ = x;
+    }
+
+    // should not lint, this comes from an external crate
+    proc_macros::external! {
+      for _ in HashMap::<i32, i32>::new() {}
+    }
+}

tests/ui/iter_over_hash_type.stderr

@@ -0,0 +1,109 @@
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:18:5
+   |
+LL | /     for x in &hash_set {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+   |
+   = note: `-D clippy::iter-over-hash-type` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::iter_over_hash_type)]`
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:21:5
+   |
+LL | /     for x in hash_set.iter() {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:24:5
+   |
+LL | /     for x in hash_set.clone() {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:27:5
+   |
+LL | /     for x in hash_set.drain() {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:32:5
+   |
+LL | /     for (x, y) in &hash_map {
+LL | |         let _ = (x, y);
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:35:5
+   |
+LL | /     for x in hash_map.keys() {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:38:5
+   |
+LL | /     for x in hash_map.values() {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:41:5
+   |
+LL | /     for x in hash_map.values_mut() {
+LL | |         *x += 1;
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:44:5
+   |
+LL | /     for x in hash_map.iter() {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:47:5
+   |
+LL | /     for x in hash_map.clone() {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:50:5
+   |
+LL | /     for x in hash_map.drain() {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:55:5
+   |
+LL | /     for x in fx_hash_set {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+
+error: iteration over unordered hash-based type
+  --> $DIR/iter_over_hash_type.rs:58:5
+   |
+LL | /     for x in fx_hash_map {
+LL | |         let _ = x;
+LL | |     }
+   | |_____^
+
+error: aborting due to 13 previous errors
+