From f3f86d8fd96dfa5986ffe10d0837169e473b9b83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sat, 16 Jul 2022 23:07:06 +0200 Subject: [PATCH] Move iter_once and iter_empty to methods as a late pass This enables more thorough checking of types to avoid triggering on custom Some and None enum variants --- clippy_lints/src/iter_once_empty.rs | 164 -------------------- clippy_lints/src/lib.register_lints.rs | 4 +- clippy_lints/src/lib.register_pedantic.rs | 2 - clippy_lints/src/lib.rs | 2 - clippy_lints/src/methods/iter_once_empty.rs | 70 +++++++++ clippy_lints/src/methods/mod.rs | 81 ++++++++++ tests/ui/iter_empty.fixed | 23 +++ tests/ui/iter_empty.rs | 23 +++ tests/ui/iter_once.fixed | 23 +++ tests/ui/iter_once.rs | 23 +++ 10 files changed, 245 insertions(+), 170 deletions(-) delete mode 100644 clippy_lints/src/iter_once_empty.rs create mode 100644 clippy_lints/src/methods/iter_once_empty.rs diff --git a/clippy_lints/src/iter_once_empty.rs b/clippy_lints/src/iter_once_empty.rs deleted file mode 100644 index 098960011123..000000000000 --- a/clippy_lints/src/iter_once_empty.rs +++ /dev/null @@ -1,164 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet; -use rustc_ast::ast::{Expr, ExprKind}; -use rustc_errors::Applicability; -use rustc_lint::{EarlyContext, EarlyLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; - -declare_clippy_lint! { - /// ### What it does - /// - /// Checks for usage of: - /// - /// - `[foo].iter()` - /// - `[foo].iter_mut()` - /// - `[foo].into_iter()` - /// - `Some(foo).iter()` - /// - `Some(foo).iter_mut()` - /// - `Some(foo).into_iter()` - /// - /// ### Why is this bad? - /// - /// It is simpler to use the once function from the standard library: - /// - /// ### Example - /// - /// ```rust - /// let a = [123].iter(); - /// let b = Some(123).into_iter(); - /// ``` - /// Use instead: - /// ```rust - /// use std::iter; - /// let a = iter::once(&123); - /// let b = iter::once(123); - /// ``` - /// - /// ### Known problems - /// - /// The type of the resulting iterator might become incompatible with its usage - #[clippy::version = "1.64.0"] - pub ITER_ONCE, - nursery, - "Iterator for array of length 1" -} - -declare_clippy_lint! { - /// ### What it does - /// - /// Checks for usage of: - /// - /// - `[].iter()` - /// - `[].iter_mut()` - /// - `[].into_iter()` - /// - `None.iter()` - /// - `None.iter_mut()` - /// - `None.into_iter()` - /// - /// ### Why is this bad? - /// - /// It is simpler to use the empty function from the standard library: - /// - /// ### Example - /// - /// ```rust - /// use std::{slice, option}; - /// let a: slice::Iter = [].iter(); - /// let f: option::IntoIter = None.into_iter(); - /// ``` - /// Use instead: - /// ```rust - /// use std::iter; - /// let a: iter::Empty = iter::empty(); - /// let b: iter::Empty = iter::empty(); - /// ``` - /// - /// ### Known problems - /// - /// The type of the resulting iterator might become incompatible with its usage - #[clippy::version = "1.64.0"] - pub ITER_EMPTY, - nursery, - "Iterator for empty array" -} - -declare_lint_pass!(IterOnceEmpty => [ITER_ONCE, ITER_EMPTY]); - -impl EarlyLintPass for IterOnceEmpty { - fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) { - if expr.span.from_expansion() { - // Don't lint match expressions present in - // macro_rules! block - return; - } - - let (method_name, args) = if let ExprKind::MethodCall(seg, args, _) = &expr.kind { - (seg.ident.as_str(), args) - } else { - return; - }; - let arg = if args.len() == 1 { - &args[0] - } else { - return; - }; - - let item = match &arg.kind { - ExprKind::Array(v) if v.len() <= 1 => v.first(), - ExprKind::Path(None, p) => { - if p.segments.len() == 1 && p.segments[0].ident.name == rustc_span::sym::None { - None - } else { - return; - } - }, - ExprKind::Call(f, some_args) if some_args.len() == 1 => { - if let ExprKind::Path(None, p) = &f.kind { - if p.segments.len() == 1 && p.segments[0].ident.name == rustc_span::sym::Some { - Some(&some_args[0]) - } else { - return; - } - } else { - return; - } - }, - _ => return, - }; - - if let Some(i) = item { - let (sugg, msg) = match method_name { - "iter" => ( - format!("std::iter::once(&{})", snippet(cx, i.span, "...")), - "this `iter` call can be replaced with std::iter::once", - ), - "iter_mut" => ( - format!("std::iter::once(&mut {})", snippet(cx, i.span, "...")), - "this `iter_mut` call can be replaced with std::iter::once", - ), - "into_iter" => ( - format!("std::iter::once({})", snippet(cx, i.span, "...")), - "this `into_iter` call can be replaced with std::iter::once", - ), - _ => return, - }; - span_lint_and_sugg(cx, ITER_ONCE, expr.span, msg, "try", sugg, Applicability::Unspecified); - } else { - let msg = match method_name { - "iter" => "this `iter call` can be replaced with std::iter::empty", - "iter_mut" => "this `iter_mut` call can be replaced with std::iter::empty", - "into_iter" => "this `into_iter` call can be replaced with std::iter::empty", - _ => return, - }; - span_lint_and_sugg( - cx, - ITER_EMPTY, - expr.span, - msg, - "try", - "std::iter::empty()".to_string(), - Applicability::Unspecified, - ); - } - } -} diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 49fc3a895274..e49321d83fa5 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -200,8 +200,6 @@ store.register_lints(&[ invalid_utf8_in_unchecked::INVALID_UTF8_IN_UNCHECKED, items_after_statements::ITEMS_AFTER_STATEMENTS, iter_not_returning_iterator::ITER_NOT_RETURNING_ITERATOR, - iter_once_empty::ITER_EMPTY, - iter_once_empty::ITER_ONCE, large_const_arrays::LARGE_CONST_ARRAYS, large_enum_variant::LARGE_ENUM_VARIANT, large_include_file::LARGE_INCLUDE_FILE, @@ -314,9 +312,11 @@ store.register_lints(&[ methods::ITERATOR_STEP_BY_ZERO, methods::ITER_CLONED_COLLECT, methods::ITER_COUNT, + methods::ITER_EMPTY, methods::ITER_NEXT_SLICE, methods::ITER_NTH, methods::ITER_NTH_ZERO, + methods::ITER_ONCE, methods::ITER_OVEREAGER_CLONED, methods::ITER_SKIP_NEXT, methods::ITER_WITH_DRAIN, diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs index 6063fb00a7b6..4250ee055e6c 100644 --- a/clippy_lints/src/lib.register_pedantic.rs +++ b/clippy_lints/src/lib.register_pedantic.rs @@ -41,8 +41,6 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![ LintId::of(invalid_upcast_comparisons::INVALID_UPCAST_COMPARISONS), LintId::of(items_after_statements::ITEMS_AFTER_STATEMENTS), LintId::of(iter_not_returning_iterator::ITER_NOT_RETURNING_ITERATOR), - LintId::of(iter_once_empty::ITER_EMPTY), - LintId::of(iter_once_empty::ITER_ONCE), LintId::of(large_stack_arrays::LARGE_STACK_ARRAYS), LintId::of(let_underscore::LET_UNDERSCORE_DROP), LintId::of(literal_representation::LARGE_DIGIT_GROUPS), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 78279db9ad54..88c1a727f8dc 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -258,7 +258,6 @@ mod invalid_upcast_comparisons; mod invalid_utf8_in_unchecked; mod items_after_statements; mod iter_not_returning_iterator; -mod iter_once_empty; mod large_const_arrays; mod large_enum_variant; mod large_include_file; @@ -932,7 +931,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(invalid_utf8_in_unchecked::InvalidUtf8InUnchecked)); store.register_late_pass(|| Box::new(std_instead_of_core::StdReexports::default())); store.register_late_pass(|| Box::new(manual_instant_elapsed::ManualInstantElapsed)); - store.register_early_pass(|| Box::new(iter_once_empty::IterOnceEmpty)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/methods/iter_once_empty.rs b/clippy_lints/src/methods/iter_once_empty.rs new file mode 100644 index 000000000000..d45dfc67880e --- /dev/null +++ b/clippy_lints/src/methods/iter_once_empty.rs @@ -0,0 +1,70 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_lang_ctor; +use clippy_utils::source::snippet; + +use rustc_errors::Applicability; +use rustc_hir::LangItem::{OptionNone, OptionSome}; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; + +use super::{ITER_EMPTY, ITER_ONCE}; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, method_name: &str, recv: &Expr<'_>) { + let item = match &recv.kind { + ExprKind::Array(v) if v.len() <= 1 => v.first(), + ExprKind::Path(p) => { + if is_lang_ctor(cx, p, OptionNone) { + None + } else { + return; + } + }, + ExprKind::Call(f, some_args) if some_args.len() == 1 => { + if let ExprKind::Path(p) = &f.kind { + if is_lang_ctor(cx, p, OptionSome) { + Some(&some_args[0]) + } else { + return; + } + } else { + return; + } + }, + _ => return, + }; + + if let Some(i) = item { + let (sugg, msg) = match method_name { + "iter" => ( + format!("std::iter::once(&{})", snippet(cx, i.span, "...")), + "this `iter` call can be replaced with std::iter::once", + ), + "iter_mut" => ( + format!("std::iter::once(&mut {})", snippet(cx, i.span, "...")), + "this `iter_mut` call can be replaced with std::iter::once", + ), + "into_iter" => ( + format!("std::iter::once({})", snippet(cx, i.span, "...")), + "this `into_iter` call can be replaced with std::iter::once", + ), + _ => return, + }; + span_lint_and_sugg(cx, ITER_ONCE, expr.span, msg, "try", sugg, Applicability::Unspecified); + } else { + let msg = match method_name { + "iter" => "this `iter call` can be replaced with std::iter::empty", + "iter_mut" => "this `iter_mut` call can be replaced with std::iter::empty", + "into_iter" => "this `into_iter` call can be replaced with std::iter::empty", + _ => return, + }; + span_lint_and_sugg( + cx, + ITER_EMPTY, + expr.span, + msg, + "try", + "std::iter::empty()".to_string(), + Applicability::Unspecified, + ); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 5ac6b09f0aa2..e449ae0e4247 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -33,6 +33,7 @@ mod iter_count; mod iter_next_slice; mod iter_nth; mod iter_nth_zero; +mod iter_once_empty; mod iter_overeager_cloned; mod iter_skip_next; mod iter_with_drain; @@ -2304,6 +2305,83 @@ declare_clippy_lint! { more clearly with `if .. else ..`" } +declare_clippy_lint! { + /// ### What it does + /// + /// Checks for usage of: + /// + /// - `[foo].iter()` + /// - `[foo].iter_mut()` + /// - `[foo].into_iter()` + /// - `Some(foo).iter()` + /// - `Some(foo).iter_mut()` + /// - `Some(foo).into_iter()` + /// + /// ### Why is this bad? + /// + /// It is simpler to use the once function from the standard library: + /// + /// ### Example + /// + /// ```rust + /// let a = [123].iter(); + /// let b = Some(123).into_iter(); + /// ``` + /// Use instead: + /// ```rust + /// use std::iter; + /// let a = iter::once(&123); + /// let b = iter::once(123); + /// ``` + /// + /// ### Known problems + /// + /// The type of the resulting iterator might become incompatible with its usage + #[clippy::version = "1.64.0"] + pub ITER_ONCE, + nursery, + "Iterator for array of length 1" +} + +declare_clippy_lint! { + /// ### What it does + /// + /// Checks for usage of: + /// + /// - `[].iter()` + /// - `[].iter_mut()` + /// - `[].into_iter()` + /// - `None.iter()` + /// - `None.iter_mut()` + /// - `None.into_iter()` + /// + /// ### Why is this bad? + /// + /// It is simpler to use the empty function from the standard library: + /// + /// ### Example + /// + /// ```rust + /// use std::{slice, option}; + /// let a: slice::Iter = [].iter(); + /// let f: option::IntoIter = None.into_iter(); + /// ``` + /// Use instead: + /// ```rust + /// use std::iter; + /// let a: iter::Empty = iter::empty(); + /// let b: iter::Empty = iter::empty(); + /// ``` + /// + /// ### Known problems + /// + /// The type of the resulting iterator might become incompatible with its usage + #[clippy::version = "1.64.0"] + pub ITER_EMPTY, + nursery, + "Iterator for empty array" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Option, @@ -2406,6 +2484,8 @@ impl_lint_pass!(Methods => [ NEEDLESS_OPTION_TAKE, NO_EFFECT_REPLACE, OBFUSCATED_IF_ELSE, + ITER_ONCE, + ITER_EMPTY ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -2708,6 +2788,7 @@ impl Methods { ("is_digit", [radix]) => is_digit_ascii_radix::check(cx, expr, recv, radix, self.msrv), ("is_none", []) => check_is_some_is_none(cx, expr, recv, false), ("is_some", []) => check_is_some_is_none(cx, expr, recv, true), + ("iter" | "iter_mut" | "into_iter", []) => iter_once_empty::check(cx, expr, name, recv), ("join", [join_arg]) => { if let Some(("collect", _, span)) = method_call(recv) { unnecessary_join::check(cx, expr, recv, join_arg, span); diff --git a/tests/ui/iter_empty.fixed b/tests/ui/iter_empty.fixed index fb7118f0d4a0..690da5a87615 100644 --- a/tests/ui/iter_empty.fixed +++ b/tests/ui/iter_empty.fixed @@ -26,7 +26,30 @@ macro_rules! in_macros { }; } +// Don't trigger on a `None` that isn't std's option +mod custom_option { + #[allow(unused)] + enum CustomOption { + Some(i32), + None, + } + + impl CustomOption { + fn iter(&self) {} + fn iter_mut(&mut self) {} + fn into_iter(self) {} + } + use CustomOption::*; + + pub fn custom_option() { + None.iter(); + None.iter_mut(); + None.into_iter(); + } +} + fn main() { array(); + custom_option::custom_option(); in_macros!(); } diff --git a/tests/ui/iter_empty.rs b/tests/ui/iter_empty.rs index bb192fe16d1f..f8b56898f2d5 100644 --- a/tests/ui/iter_empty.rs +++ b/tests/ui/iter_empty.rs @@ -26,7 +26,30 @@ macro_rules! in_macros { }; } +// Don't trigger on a `None` that isn't std's option +mod custom_option { + #[allow(unused)] + enum CustomOption { + Some(i32), + None, + } + + impl CustomOption { + fn iter(&self) {} + fn iter_mut(&mut self) {} + fn into_iter(self) {} + } + use CustomOption::*; + + pub fn custom_option() { + None.iter(); + None.iter_mut(); + None.into_iter(); + } +} + fn main() { array(); + custom_option::custom_option(); in_macros!(); } diff --git a/tests/ui/iter_once.fixed b/tests/ui/iter_once.fixed index 7247e34d7e86..0c82ab20be1c 100644 --- a/tests/ui/iter_once.fixed +++ b/tests/ui/iter_once.fixed @@ -26,7 +26,30 @@ macro_rules! in_macros { }; } +// Don't trigger on a `Some` that isn't std's option +mod custom_option { + #[allow(unused)] + enum CustomOption { + Some(i32), + None, + } + + impl CustomOption { + fn iter(&self) {} + fn iter_mut(&mut self) {} + fn into_iter(self) {} + } + use CustomOption::*; + + pub fn custom_option() { + Some(3).iter(); + Some(3).iter_mut(); + Some(3).into_iter(); + } +} + fn main() { array(); + custom_option::custom_option(); in_macros!(); } diff --git a/tests/ui/iter_once.rs b/tests/ui/iter_once.rs index 3a2b9c95cc53..d561bf27c1df 100644 --- a/tests/ui/iter_once.rs +++ b/tests/ui/iter_once.rs @@ -26,7 +26,30 @@ macro_rules! in_macros { }; } +// Don't trigger on a `Some` that isn't std's option +mod custom_option { + #[allow(unused)] + enum CustomOption { + Some(i32), + None, + } + + impl CustomOption { + fn iter(&self) {} + fn iter_mut(&mut self) {} + fn into_iter(self) {} + } + use CustomOption::*; + + pub fn custom_option() { + Some(3).iter(); + Some(3).iter_mut(); + Some(3).into_iter(); + } +} + fn main() { array(); + custom_option::custom_option(); in_macros!(); }