diff --git a/compiler/rustc_lint/locales/en-US.ftl b/compiler/rustc_lint/locales/en-US.ftl index b1e7cc69a809b..68e62c9789aed 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`, 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 2070ffea4d99e..35dc533e56c24 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 2d9aa9074be79..20ab0af585651 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 0000000000000..62e8b4fe9e4a2 --- /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) { + /// items.sort(); + /// } + /// + /// fn main() { + /// let mut x: Vec> = 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 156b925de773f..ae00232c12c8a 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 9e3e13e70040e..5d272ec352287 100644 --- a/library/core/src/iter/traits/iterator.rs +++ b/library/core/src/iter/traits/iterator.rs @@ -777,6 +777,7 @@ pub trait Iterator { /// println!("{x}"); /// } /// ``` + #[rustc_diagnostic_item = "IteratorMap"] #[inline] #[stable(feature = "rust1", since = "1.0.0")] fn map(self, f: F) -> Map diff --git a/tests/ui/lint/issue-106991.rs b/tests/ui/lint/issue-106991.rs new file mode 100644 index 0000000000000..e4d7f765b4a8a --- /dev/null +++ b/tests/ui/lint/issue-106991.rs @@ -0,0 +1,13 @@ +fn foo(items: &mut Vec) { + items.sort(); +} + +fn bar() -> impl Iterator { + //~^ ERROR expected `foo` to be a fn item that returns `i32`, but it returns `()` [E0271] + let mut x: Vec> = 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 0000000000000..7b43f0b2ca8f3 --- /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 { + | ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found `i32` + | + = note: required for `Map>, for<'a> fn(&'a mut Vec) {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 0000000000000..dc1ecbf8424d4 --- /dev/null +++ b/tests/ui/lint/lint_map_unit_fn.rs @@ -0,0 +1,20 @@ +#![deny(map_unit_fn)] + +fn foo(items: &mut Vec) { + items.sort(); +} + +fn main() { + let mut x: Vec> = 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| { + 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 0000000000000..fbf689c54219e --- /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) { + | --------------------------- 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`, 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`, 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| { + | --------------------- 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`, 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 +