Rollup merge of #107890 - obeis:mapping-to-unit, r=cjgillot

Lint against `Iterator::map` receiving a callable that returns `()`

Close #106991

10 files changed, 260 insertions, 1 deletions

diff --git a/compiler/rustc_lint/locales/en-US.ftl b/compiler/rustc_lint/locales/en-US.ftl
index b1e7cc69a80..68e62c9789a 100644
--- a/compiler/rustc_lint/locales/en-US.ftl
+++ b/compiler/rustc_lint/locales/en-US.ftl
@@ -24,6 +24,13 @@ lint_for_loops_over_fallibles =
     .use_while_let = to check pattern in a loop use `while let`
     .use_question_mark = consider unwrapping the `Result` with `?` to iterate over its contents
 
+lint_map_unit_fn = `Iterator::map` call that discard the iterator's values
+    .note = `Iterator::map`, like many of the methods on `Iterator`, gets executed lazily, meaning that its effects won't be visible until it is iterated
+    .function_label = this function returns `()`, which is likely not what you wanted
+    .argument_label = called `Iterator::map` with callable that returns `()`
+    .map_label = after this call to map, the resulting iterator is `impl Iterator<Item = ()>`, which means the only information carried by the iterator is the number of items
+    .suggestion = you might have meant to use `Iterator::for_each`
+
 lint_non_binding_let_on_sync_lock =
     non-binding let on a synchronization lock
 
diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs
index 2070ffea4d9..35dc533e56c 100644
--- a/compiler/rustc_lint/src/lib.rs
+++ b/compiler/rustc_lint/src/lib.rs
@@ -63,6 +63,7 @@ mod late;
 mod let_underscore;
 mod levels;
 mod lints;
+mod map_unit_fn;
 mod methods;
 mod multiple_supertrait_upcastable;
 mod non_ascii_idents;
@@ -100,6 +101,7 @@ use for_loops_over_fallibles::*;
 use hidden_unicode_codepoints::*;
 use internal::*;
 use let_underscore::*;
+use map_unit_fn::*;
 use methods::*;
 use multiple_supertrait_upcastable::*;
 use non_ascii_idents::*;
@@ -239,6 +241,7 @@ late_lint_methods!(
             NamedAsmLabels: NamedAsmLabels,
             OpaqueHiddenInferredBound: OpaqueHiddenInferredBound,
             MultipleSupertraitUpcastable: MultipleSupertraitUpcastable,
+            MapUnitFn: MapUnitFn,
         ]
     ]
 );
@@ -298,7 +301,8 @@ fn register_builtins(store: &mut LintStore) {
         UNUSED_LABELS,
         UNUSED_PARENS,
         UNUSED_BRACES,
-        REDUNDANT_SEMICOLONS
+        REDUNDANT_SEMICOLONS,
+        MAP_UNIT_FN
     );
 
     add_lint_group!("let_underscore", LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK);
diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs
index 2d9aa9074be..20ab0af5856 100644
--- a/compiler/rustc_lint/src/lints.rs
+++ b/compiler/rustc_lint/src/lints.rs
@@ -748,6 +748,22 @@ impl AddToDiagnostic for HiddenUnicodeCodepointsDiagSub {
     }
 }
 
+// map_unit_fn.rs
+#[derive(LintDiagnostic)]
+#[diag(lint_map_unit_fn)]
+#[note]
+pub struct MappingToUnit {
+    #[label(lint_function_label)]
+    pub function_label: Span,
+    #[label(lint_argument_label)]
+    pub argument_label: Span,
+    #[label(lint_map_label)]
+    pub map_label: Span,
+    #[suggestion(style = "verbose", code = "{replace}", applicability = "maybe-incorrect")]
+    pub suggestion: Span,
+    pub replace: String,
+}
+
 // internal.rs
 #[derive(LintDiagnostic)]
 #[diag(lint_default_hash_types)]
diff --git a/compiler/rustc_lint/src/map_unit_fn.rs b/compiler/rustc_lint/src/map_unit_fn.rs
new file mode 100644
index 00000000000..62e8b4fe9e4
--- /dev/null
+++ b/compiler/rustc_lint/src/map_unit_fn.rs
@@ -0,0 +1,120 @@
+use crate::lints::MappingToUnit;
+use crate::{LateContext, LateLintPass, LintContext};
+
+use rustc_hir::{Expr, ExprKind, HirId, Stmt, StmtKind};
+use rustc_middle::{
+    query::Key,
+    ty::{self, Ty},
+};
+
+declare_lint! {
+    /// The `map_unit_fn` lint checks for `Iterator::map` receive
+    /// a callable that returns `()`.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// fn foo(items: &mut Vec<u8>) {
+    ///     items.sort();
+    /// }
+    ///
+    /// fn main() {
+    ///     let mut x: Vec<Vec<u8>> = vec![
+    ///         vec![0, 2, 1],
+    ///         vec![5, 4, 3],
+    ///     ];
+    ///     x.iter_mut().map(foo);
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Mapping to `()` is almost always a mistake.
+    pub MAP_UNIT_FN,
+    Warn,
+    "`Iterator::map` call that discard the iterator's values"
+}
+
+declare_lint_pass!(MapUnitFn => [MAP_UNIT_FN]);
+
+impl<'tcx> LateLintPass<'tcx> for MapUnitFn {
+    fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &Stmt<'_>) {
+        if stmt.span.from_expansion() {
+            return;
+        }
+
+        if let StmtKind::Semi(expr) = stmt.kind {
+            if let ExprKind::MethodCall(path, receiver, args, span) = expr.kind {
+                if path.ident.name.as_str() == "map" {
+                    if receiver.span.from_expansion()
+                        || args.iter().any(|e| e.span.from_expansion())
+                        || !is_impl_slice(cx, receiver)
+                        || !is_diagnostic_name(cx, expr.hir_id, "IteratorMap")
+                    {
+                        return;
+                    }
+                    let arg_ty = cx.typeck_results().expr_ty(&args[0]);
+                    if let ty::FnDef(id, _) = arg_ty.kind() {
+                        let fn_ty = cx.tcx.fn_sig(id).skip_binder();
+                        let ret_ty = fn_ty.output().skip_binder();
+                        if is_unit_type(ret_ty) {
+                            cx.emit_spanned_lint(
+                                MAP_UNIT_FN,
+                                span,
+                                MappingToUnit {
+                                    function_label: cx.tcx.span_of_impl(*id).unwrap(),
+                                    argument_label: args[0].span,
+                                    map_label: arg_ty.default_span(cx.tcx),
+                                    suggestion: path.ident.span,
+                                    replace: "for_each".to_string(),
+                                },
+                            )
+                        }
+                    } else if let ty::Closure(id, subs) = arg_ty.kind() {
+                        let cl_ty = subs.as_closure().sig();
+                        let ret_ty = cl_ty.output().skip_binder();
+                        if is_unit_type(ret_ty) {
+                            cx.emit_spanned_lint(
+                                MAP_UNIT_FN,
+                                span,
+                                MappingToUnit {
+                                    function_label: cx.tcx.span_of_impl(*id).unwrap(),
+                                    argument_label: args[0].span,
+                                    map_label: arg_ty.default_span(cx.tcx),
+                                    suggestion: path.ident.span,
+                                    replace: "for_each".to_string(),
+                                },
+                            )
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
+
+fn is_impl_slice(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    if let Some(method_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) {
+        if let Some(impl_id) = cx.tcx.impl_of_method(method_id) {
+            return cx.tcx.type_of(impl_id).skip_binder().is_slice();
+        }
+    }
+    false
+}
+
+fn is_unit_type(ty: Ty<'_>) -> bool {
+    ty.is_unit() || ty.is_never()
+}
+
+fn is_diagnostic_name(cx: &LateContext<'_>, id: HirId, name: &str) -> bool {
+    if let Some(def_id) = cx.typeck_results().type_dependent_def_id(id) {
+        if let Some(item) = cx.tcx.get_diagnostic_name(def_id) {
+            if item.as_str() == name {
+                return true;
+            }
+        }
+    }
+    false
+}
diff --git a/library/core/src/iter/mod.rs b/library/core/src/iter/mod.rs
index 156b925de77..ae00232c12c 100644
--- a/library/core/src/iter/mod.rs
+++ b/library/core/src/iter/mod.rs
@@ -278,6 +278,7 @@
 //!
 //! ```
 //! # #![allow(unused_must_use)]
+//! # #![cfg_attr(not(bootstrap), allow(map_unit_fn))]
 //! let v = vec![1, 2, 3, 4, 5];
 //! v.iter().map(|x| println!("{x}"));
 //! ```
diff --git a/library/core/src/iter/traits/iterator.rs b/library/core/src/iter/traits/iterator.rs
index b3a630d9559..b8e7d0a68da 100644
--- a/library/core/src/iter/traits/iterator.rs
+++ b/library/core/src/iter/traits/iterator.rs
@@ -789,6 +789,7 @@ pub trait Iterator {
     ///     println!("{x}");
     /// }
     /// ```
+    #[rustc_diagnostic_item = "IteratorMap"]
     #[inline]
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_do_not_const_check]
diff --git a/tests/ui/lint/issue-106991.rs b/tests/ui/lint/issue-106991.rs
new file mode 100644
index 00000000000..e4d7f765b4a
--- /dev/null
+++ b/tests/ui/lint/issue-106991.rs
@@ -0,0 +1,13 @@
+fn foo(items: &mut Vec<u8>) {
+    items.sort();
+}
+
+fn bar() -> impl Iterator<Item = i32> {
+    //~^ ERROR expected `foo` to be a fn item that returns `i32`, but it returns `()` [E0271]
+    let mut x: Vec<Vec<u8>> = vec![vec![0, 2, 1], vec![5, 4, 3]];
+    x.iter_mut().map(foo)
+}
+
+fn main() {
+    bar();
+}
diff --git a/tests/ui/lint/issue-106991.stderr b/tests/ui/lint/issue-106991.stderr
new file mode 100644
index 00000000000..7b43f0b2ca8
--- /dev/null
+++ b/tests/ui/lint/issue-106991.stderr
@@ -0,0 +1,11 @@
+error[E0271]: expected `foo` to be a fn item that returns `i32`, but it returns `()`
+  --> $DIR/issue-106991.rs:5:13
+   |
+LL | fn bar() -> impl Iterator<Item = i32> {
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found `i32`
+   |
+   = note: required for `Map<std::slice::IterMut<'_, Vec<u8>>, for<'a> fn(&'a mut Vec<u8>) {foo}>` to implement `Iterator`
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0271`.
diff --git a/tests/ui/lint/lint_map_unit_fn.rs b/tests/ui/lint/lint_map_unit_fn.rs
new file mode 100644
index 00000000000..dc1ecbf8424
--- /dev/null
+++ b/tests/ui/lint/lint_map_unit_fn.rs
@@ -0,0 +1,20 @@
+#![deny(map_unit_fn)]
+
+fn foo(items: &mut Vec<u8>) {
+    items.sort();
+}
+
+fn main() {
+    let mut x: Vec<Vec<u8>> = vec![vec![0, 2, 1], vec![5, 4, 3]];
+    x.iter_mut().map(foo);
+    //~^ ERROR `Iterator::map` call that discard the iterator's values
+    x.iter_mut().map(|items| {
+    //~^ ERROR `Iterator::map` call that discard the iterator's values
+        items.sort();
+    });
+    let f = |items: &mut Vec<u8>| {
+        items.sort();
+    };
+    x.iter_mut().map(f);
+    //~^ ERROR `Iterator::map` call that discard the iterator's values
+}
diff --git a/tests/ui/lint/lint_map_unit_fn.stderr b/tests/ui/lint/lint_map_unit_fn.stderr
new file mode 100644
index 00000000000..fbf689c5421
--- /dev/null
+++ b/tests/ui/lint/lint_map_unit_fn.stderr
@@ -0,0 +1,66 @@
+error: `Iterator::map` call that discard the iterator's values
+  --> $DIR/lint_map_unit_fn.rs:9:18
+   |
+LL | fn foo(items: &mut Vec<u8>) {
+   | --------------------------- this function returns `()`, which is likely not what you wanted
+...
+LL |     x.iter_mut().map(foo);
+   |                  ^^^^---^
+   |                  |   |
+   |                  |   called `Iterator::map` with callable that returns `()`
+   |                  after this call to map, the resulting iterator is `impl Iterator<Item = ()>`, which means the only information carried by the iterator is the number of items
+   |
+   = note: `Iterator::map`, like many of the methods on `Iterator`, gets executed lazily, meaning that its effects won't be visible until it is iterated
+note: the lint level is defined here
+  --> $DIR/lint_map_unit_fn.rs:1:9
+   |
+LL | #![deny(map_unit_fn)]
+   |         ^^^^^^^^^^^
+help: you might have meant to use `Iterator::for_each`
+   |
+LL |     x.iter_mut().for_each(foo);
+   |                  ~~~~~~~~
+
+error: `Iterator::map` call that discard the iterator's values
+  --> $DIR/lint_map_unit_fn.rs:11:18
+   |
+LL |         x.iter_mut().map(|items| {
+   |                      ^   -------
+   |                      |   |
+   |  ____________________|___this function returns `()`, which is likely not what you wanted
+   | |  __________________|
+   | | |
+LL | | |
+LL | | |         items.sort();
+LL | | |     });
+   | | |     -^ after this call to map, the resulting iterator is `impl Iterator<Item = ()>`, which means the only information carried by the iterator is the number of items
+   | | |_____||
+   | |_______|
+   |         called `Iterator::map` with callable that returns `()`
+   |
+   = note: `Iterator::map`, like many of the methods on `Iterator`, gets executed lazily, meaning that its effects won't be visible until it is iterated
+help: you might have meant to use `Iterator::for_each`
+   |
+LL |     x.iter_mut().for_each(|items| {
+   |                  ~~~~~~~~
+
+error: `Iterator::map` call that discard the iterator's values
+  --> $DIR/lint_map_unit_fn.rs:18:18
+   |
+LL |     let f = |items: &mut Vec<u8>| {
+   |             --------------------- this function returns `()`, which is likely not what you wanted
+...
+LL |     x.iter_mut().map(f);
+   |                  ^^^^-^
+   |                  |   |
+   |                  |   called `Iterator::map` with callable that returns `()`
+   |                  after this call to map, the resulting iterator is `impl Iterator<Item = ()>`, which means the only information carried by the iterator is the number of items
+   |
+   = note: `Iterator::map`, like many of the methods on `Iterator`, gets executed lazily, meaning that its effects won't be visible until it is iterated
+help: you might have meant to use `Iterator::for_each`
+   |
+LL |     x.iter_mut().for_each(f);
+   |                  ~~~~~~~~
+
+error: aborting due to 3 previous errors
+