From a914f37409f0ff0cdee37bfc959d77f91f3bcb0c Mon Sep 17 00:00:00 2001 From: Obei Sideg Date: Thu, 16 Feb 2023 22:04:59 +0300 Subject: [PATCH 1/5] Add lint against `Iterator::map` receiving a callable that returns `()` --- compiler/rustc_lint/locales/en-US.ftl | 7 ++ compiler/rustc_lint/src/lib.rs | 6 +- compiler/rustc_lint/src/lints.rs | 16 ++++ compiler/rustc_lint/src/map_unit_fn.rs | 104 +++++++++++++++++++++++ library/core/src/iter/mod.rs | 1 + library/core/src/iter/traits/iterator.rs | 1 + 6 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 compiler/rustc_lint/src/map_unit_fn.rs 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`, 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 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 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 @@ fn lint_mod(tcx: TyCtxt<'_>, module_def_id: LocalDefId) { NamedAsmLabels: NamedAsmLabels, OpaqueHiddenInferredBound: OpaqueHiddenInferredBound, MultipleSupertraitUpcastable: MultipleSupertraitUpcastable, + MapUnitFn: MapUnitFn, ] ] ); @@ -298,7 +301,8 @@ macro_rules! add_lint_group { 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 @@ fn add_to_diagnostic_with(self, diag: &mut rustc_errors::Diagnostic, _: F) } } +// 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..53383413690 --- /dev/null +++ b/compiler/rustc_lint/src/map_unit_fn.rs @@ -0,0 +1,104 @@ +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(), + }, + ) + } + } + } + } + } + } +} + +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 9e3e13e7004..5d272ec3522 100644 --- a/library/core/src/iter/traits/iterator.rs +++ b/library/core/src/iter/traits/iterator.rs @@ -777,6 +777,7 @@ fn intersperse_with(self, separator: G) -> IntersperseWith /// println!("{x}"); /// } /// ``` + #[rustc_diagnostic_item = "IteratorMap"] #[inline] #[stable(feature = "rust1", since = "1.0.0")] fn map(self, f: F) -> Map From ddd7d1087998ef692e46520d0ceb56a0eb5fd7a6 Mon Sep 17 00:00:00 2001 From: Obei Sideg Date: Thu, 16 Feb 2023 22:05:35 +0300 Subject: [PATCH 2/5] Add ui test for `map_unit_fn` lint --- tests/ui/lint/lint_map_unit_fn.rs | 11 +++++++++++ tests/ui/lint/lint_map_unit_fn.stderr | 25 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 tests/ui/lint/lint_map_unit_fn.rs create mode 100644 tests/ui/lint/lint_map_unit_fn.stderr 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..1dae47cd4c6 --- /dev/null +++ b/tests/ui/lint/lint_map_unit_fn.rs @@ -0,0 +1,11 @@ +#![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 +} 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..ee300e170b3 --- /dev/null +++ b/tests/ui/lint/lint_map_unit_fn.stderr @@ -0,0 +1,25 @@ +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: aborting due to previous error + From a87443a85999eadc867d261667363137adcb2420 Mon Sep 17 00:00:00 2001 From: Obei Sideg Date: Thu, 16 Feb 2023 22:18:14 +0300 Subject: [PATCH 3/5] Emit `map_unit_fn` lint in closure case --- compiler/rustc_lint/src/map_unit_fn.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/compiler/rustc_lint/src/map_unit_fn.rs b/compiler/rustc_lint/src/map_unit_fn.rs index 53383413690..62e8b4fe9e4 100644 --- a/compiler/rustc_lint/src/map_unit_fn.rs +++ b/compiler/rustc_lint/src/map_unit_fn.rs @@ -72,6 +72,22 @@ fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &Stmt<'_>) { }, ) } + } 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(), + }, + ) + } } } } From b93d54556fd25a704dbba77d9d09f6726f5bce37 Mon Sep 17 00:00:00 2001 From: Obei Sideg Date: Thu, 16 Feb 2023 22:18:36 +0300 Subject: [PATCH 4/5] Add ui test for `map_unit_fn` lint in closure case --- tests/ui/lint/lint_map_unit_fn.rs | 9 ++++++ tests/ui/lint/lint_map_unit_fn.stderr | 43 ++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/tests/ui/lint/lint_map_unit_fn.rs b/tests/ui/lint/lint_map_unit_fn.rs index 1dae47cd4c6..dc1ecbf8424 100644 --- a/tests/ui/lint/lint_map_unit_fn.rs +++ b/tests/ui/lint/lint_map_unit_fn.rs @@ -8,4 +8,13 @@ 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 index ee300e170b3..fbf689c5421 100644 --- a/tests/ui/lint/lint_map_unit_fn.stderr +++ b/tests/ui/lint/lint_map_unit_fn.stderr @@ -21,5 +21,46 @@ help: you might have meant to use `Iterator::for_each` LL | x.iter_mut().for_each(foo); | ~~~~~~~~ -error: aborting due to previous error +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 From 99344a8b3231925bc52190b7e9c18a597c54ca90 Mon Sep 17 00:00:00 2001 From: Obei Sideg Date: Mon, 20 Feb 2023 15:43:55 +0300 Subject: [PATCH 5/5] Add ui test for `E0271` error --- tests/ui/lint/issue-106991.rs | 13 +++++++++++++ tests/ui/lint/issue-106991.stderr | 11 +++++++++++ 2 files changed, 24 insertions(+) create mode 100644 tests/ui/lint/issue-106991.rs create mode 100644 tests/ui/lint/issue-106991.stderr 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) { + 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 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 { + | ^^^^^^^^^^^^^^^^^^^^^^^^^ 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`.