From 828ddbe414d77d61292a72ffb9ade85cd6e72cda Mon Sep 17 00:00:00 2001 From: Elliot Bobrow Date: Wed, 29 Dec 2021 08:52:40 -0800 Subject: [PATCH 01/47] fix [`redundant_closure`] fp with `Arc` --- clippy_lints/src/eta_reduction.rs | 5 +++++ tests/ui/eta.fixed | 11 +++++++++++ tests/ui/eta.rs | 11 +++++++++++ 3 files changed, 27 insertions(+) diff --git a/clippy_lints/src/eta_reduction.rs b/clippy_lints/src/eta_reduction.rs index 5a4b42471044..b22515a39079 100644 --- a/clippy_lints/src/eta_reduction.rs +++ b/clippy_lints/src/eta_reduction.rs @@ -1,6 +1,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; use clippy_utils::higher::VecArgs; use clippy_utils::source::snippet_opt; +use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::usage::local_used_after_expr; use clippy_utils::{get_enclosing_loop_or_closure, higher, path_to_local, path_to_local_id}; use if_chain::if_chain; @@ -12,6 +13,7 @@ use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow}; use rustc_middle::ty::subst::Subst; use rustc_middle::ty::{self, ClosureKind, Ty, TypeFoldable}; use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::sym; declare_clippy_lint! { /// ### What it does @@ -113,6 +115,9 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction { // A type param function ref like `T::f` is not 'static, however // it is if cast like `T::f as fn()`. This seems like a rustc bug. if !substs.types().any(|t| matches!(t.kind(), ty::Param(_))); + let callee_ty_unadjusted = cx.typeck_results().expr_ty(callee).peel_refs(); + if !is_type_diagnostic_item(cx, callee_ty_unadjusted, sym::Arc); + if !is_type_diagnostic_item(cx, callee_ty_unadjusted, sym::Rc); then { span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| { if let Some(mut snippet) = snippet_opt(cx, callee.span) { diff --git a/tests/ui/eta.fixed b/tests/ui/eta.fixed index 1de79667f55f..f938f7106884 100644 --- a/tests/ui/eta.fixed +++ b/tests/ui/eta.fixed @@ -248,3 +248,14 @@ mod type_param_bound { take(X::fun as fn()); } } + +// #8073 Don't replace closure with `Arc` or `Rc` +fn arc_fp() { + let rc = std::rc::Rc::new(|| 7); + let arc = std::sync::Arc::new(|n| n + 1); + let ref_arc = &std::sync::Arc::new(|_| 5); + + true.then(|| rc()); + (0..5).map(|n| arc(n)); + Some(4).map(|n| ref_arc(n)); +} diff --git a/tests/ui/eta.rs b/tests/ui/eta.rs index 86abd347baa7..075bbc74922f 100644 --- a/tests/ui/eta.rs +++ b/tests/ui/eta.rs @@ -248,3 +248,14 @@ mod type_param_bound { take(X::fun as fn()); } } + +// #8073 Don't replace closure with `Arc` or `Rc` +fn arc_fp() { + let rc = std::rc::Rc::new(|| 7); + let arc = std::sync::Arc::new(|n| n + 1); + let ref_arc = &std::sync::Arc::new(|_| 5); + + true.then(|| rc()); + (0..5).map(|n| arc(n)); + Some(4).map(|n| ref_arc(n)); +} From 65d1f83d2c593c7997b4fe781814eb0fb69272e9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 27 Dec 2021 11:17:17 -0500 Subject: [PATCH 02/47] Extend [`unused_io_amount`] to cover AsyncRead and AsyncWrite. Clippy helpfully warns about code like this, telling you that you probably meant "write_all": fn say_hi(w: &mut W) { w.write(b"hello").unwrap(); } This patch attempts to extend the lint so it also covers this case: async fn say_hi(w: &mut W) { w.write(b"hello").await.unwrap(); } (I've run into this second case several times in my own programming, and so have my coworkers, so unless we're especially accident-prone in this area, it's probably worth addressing?) This patch covers the Async{Read,Write}Ext traits in futures-rs, and in tokio, since both are quite widely used. changelog: [`unused_io_amount`] now supports AsyncReadExt and AsyncWriteExt. --- Cargo.toml | 2 + clippy_lints/src/unused_io_amount.rs | 75 ++++++++++++++++++++++++---- clippy_utils/src/paths.rs | 8 +++ tests/compile-test.rs | 6 +++ tests/ui/unused_io_amount.rs | 53 ++++++++++++++++++++ tests/ui/unused_io_amount.stderr | 58 +++++++++++++++++---- 6 files changed, 181 insertions(+), 21 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 8661a8677588..79a7ec92071c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -47,7 +47,9 @@ itertools = "0.10" quote = "1.0" serde = { version = "1.0", features = ["derive"] } syn = { version = "1.0", features = ["full"] } +futures = "0.3" parking_lot = "0.11.2" +tokio = { version = "1", features = ["io-util"] } [build-dependencies] rustc_tools_util = { version = "0.2", path = "rustc_tools_util" } diff --git a/clippy_lints/src/unused_io_amount.rs b/clippy_lints/src/unused_io_amount.rs index 004530db0861..8afd8472db7a 100644 --- a/clippy_lints/src/unused_io_amount.rs +++ b/clippy_lints/src/unused_io_amount.rs @@ -17,10 +17,17 @@ declare_clippy_lint! { /// partial-write/read, use /// `write_all`/`read_exact` instead. /// + /// When working with asynchronous code (either with the `futures` + /// crate or with `tokio`), a similar issue exists for + /// `AsyncWriteExt::write()` and `AsyncReadExt::read()` : these + /// functions are also not guaranteed to process the entire + /// buffer. Your code should either handle partial-writes/reads, or + /// call the `write_all`/`read_exact` methods on those traits instead. + /// /// ### Known problems /// Detects only common patterns. /// - /// ### Example + /// ### Examples /// ```rust,ignore /// use std::io; /// fn foo(w: &mut W) -> io::Result<()> { @@ -68,6 +75,23 @@ impl<'tcx> LateLintPass<'tcx> for UnusedIoAmount { } } +/// If `expr` is an (e).await, return the inner expression "e" that's being +/// waited on. Otherwise return None. +fn try_remove_await<'a>(expr: &'a hir::Expr<'a>) -> Option<&hir::Expr<'a>> { + if let hir::ExprKind::Match(expr, _, hir::MatchSource::AwaitDesugar) = expr.kind { + if let hir::ExprKind::Call(func, [ref arg_0, ..]) = expr.kind { + if matches!( + func.kind, + hir::ExprKind::Path(hir::QPath::LangItem(hir::LangItem::IntoFutureIntoFuture, ..)) + ) { + return Some(arg_0); + } + } + } + + None +} + fn check_map_error(cx: &LateContext<'_>, call: &hir::Expr<'_>, expr: &hir::Expr<'_>) { let mut call = call; while let hir::ExprKind::MethodCall(path, _, args, _) = call.kind { @@ -77,30 +101,61 @@ fn check_map_error(cx: &LateContext<'_>, call: &hir::Expr<'_>, expr: &hir::Expr< break; } } - check_method_call(cx, call, expr); + + if let Some(call) = try_remove_await(call) { + check_method_call(cx, call, expr, true); + } else { + check_method_call(cx, call, expr, false); + } } -fn check_method_call(cx: &LateContext<'_>, call: &hir::Expr<'_>, expr: &hir::Expr<'_>) { +fn check_method_call(cx: &LateContext<'_>, call: &hir::Expr<'_>, expr: &hir::Expr<'_>, is_await: bool) { if let hir::ExprKind::MethodCall(path, _, _, _) = call.kind { let symbol = path.ident.as_str(); - let read_trait = match_trait_method(cx, call, &paths::IO_READ); - let write_trait = match_trait_method(cx, call, &paths::IO_WRITE); + let read_trait = if is_await { + match_trait_method(cx, call, &paths::FUTURES_IO_ASYNCREADEXT) + || match_trait_method(cx, call, &paths::TOKIO_IO_ASYNCREADEXT) + } else { + match_trait_method(cx, call, &paths::IO_READ) + }; + let write_trait = if is_await { + match_trait_method(cx, call, &paths::FUTURES_IO_ASYNCWRITEEXT) + || match_trait_method(cx, call, &paths::TOKIO_IO_ASYNCWRITEEXT) + } else { + match_trait_method(cx, call, &paths::IO_WRITE) + }; - match (read_trait, write_trait, symbol) { - (true, _, "read") => span_lint( + match (read_trait, write_trait, symbol, is_await) { + (true, _, "read", false) => span_lint( cx, UNUSED_IO_AMOUNT, expr.span, "read amount is not handled. Use `Read::read_exact` instead", ), - (true, _, "read_vectored") => span_lint(cx, UNUSED_IO_AMOUNT, expr.span, "read amount is not handled"), - (_, true, "write") => span_lint( + (true, _, "read", true) => span_lint( + cx, + UNUSED_IO_AMOUNT, + expr.span, + "read amount is not handled. Use `AsyncReadExt::read_exact` instead", + ), + (true, _, "read_vectored", _) => { + span_lint(cx, UNUSED_IO_AMOUNT, expr.span, "read amount is not handled"); + }, + (_, true, "write", false) => span_lint( cx, UNUSED_IO_AMOUNT, expr.span, "written amount is not handled. Use `Write::write_all` instead", ), - (_, true, "write_vectored") => span_lint(cx, UNUSED_IO_AMOUNT, expr.span, "written amount is not handled"), + (_, true, "write", true) => span_lint( + cx, + UNUSED_IO_AMOUNT, + expr.span, + "written amount is not handled. Use `AsyncWriteExt::write_all` instead", + ), + (_, true, "write_vectored", _) => { + span_lint(cx, UNUSED_IO_AMOUNT, expr.span, "written amount is not handled"); + }, _ => (), } } diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 6171823abbbd..aa3b3af23567 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -64,6 +64,10 @@ pub const FROM_ITERATOR: [&str; 5] = ["core", "iter", "traits", "collect", "From pub const FROM_ITERATOR_METHOD: [&str; 6] = ["core", "iter", "traits", "collect", "FromIterator", "from_iter"]; pub const FROM_STR_METHOD: [&str; 5] = ["core", "str", "traits", "FromStr", "from_str"]; pub const FUTURE_FROM_GENERATOR: [&str; 3] = ["core", "future", "from_generator"]; +#[allow(clippy::invalid_paths)] // internal lints do not know about all external crates +pub const FUTURES_IO_ASYNCREADEXT: [&str; 3] = ["futures_util", "io", "AsyncReadExt"]; +#[allow(clippy::invalid_paths)] // internal lints do not know about all external crates +pub const FUTURES_IO_ASYNCWRITEEXT: [&str; 3] = ["futures_util", "io", "AsyncWriteExt"]; pub const HASH: [&str; 3] = ["core", "hash", "Hash"]; pub const HASHMAP_CONTAINS_KEY: [&str; 6] = ["std", "collections", "hash", "map", "HashMap", "contains_key"]; pub const HASHMAP_ENTRY: [&str; 5] = ["std", "collections", "hash", "map", "Entry"]; @@ -194,6 +198,10 @@ pub const SYM_MODULE: [&str; 3] = ["rustc_span", "symbol", "sym"]; pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"]; pub const TO_OWNED_METHOD: [&str; 4] = ["alloc", "borrow", "ToOwned", "to_owned"]; pub const TO_STRING_METHOD: [&str; 4] = ["alloc", "string", "ToString", "to_string"]; +#[allow(clippy::invalid_paths)] // internal lints do not know about all external crates +pub const TOKIO_IO_ASYNCREADEXT: [&str; 5] = ["tokio", "io", "util", "async_read_ext", "AsyncReadExt"]; +#[allow(clippy::invalid_paths)] // internal lints do not know about all external crates +pub const TOKIO_IO_ASYNCWRITEEXT: [&str; 5] = ["tokio", "io", "util", "async_write_ext", "AsyncWriteExt"]; pub const TRY_FROM: [&str; 4] = ["core", "convert", "TryFrom", "try_from"]; pub const VEC_AS_MUT_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_mut_slice"]; pub const VEC_AS_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_slice"]; diff --git a/tests/compile-test.rs b/tests/compile-test.rs index a2d58491872b..d602d2042ee7 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -21,6 +21,7 @@ const RUN_INTERNAL_TESTS: bool = cfg!(feature = "internal-lints"); static TEST_DEPENDENCIES: &[&str] = &[ "clippy_utils", "derive_new", + "futures", "if_chain", "itertools", "quote", @@ -28,6 +29,7 @@ static TEST_DEPENDENCIES: &[&str] = &[ "serde", "serde_derive", "syn", + "tokio", "parking_lot", ]; @@ -38,6 +40,8 @@ extern crate clippy_utils; #[allow(unused_extern_crates)] extern crate derive_new; #[allow(unused_extern_crates)] +extern crate futures; +#[allow(unused_extern_crates)] extern crate if_chain; #[allow(unused_extern_crates)] extern crate itertools; @@ -47,6 +51,8 @@ extern crate parking_lot; extern crate quote; #[allow(unused_extern_crates)] extern crate syn; +#[allow(unused_extern_crates)] +extern crate tokio; /// Produces a string with an `--extern` flag for all UI test crate /// dependencies. diff --git a/tests/ui/unused_io_amount.rs b/tests/ui/unused_io_amount.rs index 8b141e25942d..4b059558173f 100644 --- a/tests/ui/unused_io_amount.rs +++ b/tests/ui/unused_io_amount.rs @@ -1,6 +1,8 @@ #![allow(dead_code)] #![warn(clippy::unused_io_amount)] +extern crate futures; +use futures::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt}; use std::io::{self, Read}; fn question_mark(s: &mut T) -> io::Result<()> { @@ -61,4 +63,55 @@ fn combine_or(file: &str) -> Result<(), Error> { Ok(()) } +async fn bad_async_write(w: &mut W) { + w.write(b"hello world").await.unwrap(); +} + +async fn bad_async_read(r: &mut R) { + let mut buf = [0u8; 0]; + r.read(&mut buf[..]).await.unwrap(); +} + +async fn io_not_ignored_async_write(mut w: W) { + // Here we're forgetting to await the future, so we should get a + // warning about _that_ (or we would, if it were enabled), but we + // won't get one about ignoring the return value. + w.write(b"hello world"); +} + +fn bad_async_write_closure(w: W) -> impl futures::Future> { + let mut w = w; + async move { + w.write(b"hello world").await?; + Ok(()) + } +} + +async fn async_read_nested_or(r: &mut R, do_it: bool) -> Result<[u8; 1], Error> { + let mut buf = [0u8; 1]; + if do_it { + r.read(&mut buf[..]).await.or(Err(Error::Kind))?; + } + Ok(buf) +} + +use tokio::io::{AsyncRead as TokioAsyncRead, AsyncReadExt as _, AsyncWrite as TokioAsyncWrite, AsyncWriteExt as _}; + +async fn bad_async_write_tokio(w: &mut W) { + w.write(b"hello world").await.unwrap(); +} + +async fn bad_async_read_tokio(r: &mut R) { + let mut buf = [0u8; 0]; + r.read(&mut buf[..]).await.unwrap(); +} + +async fn undetected_bad_async_write(w: &mut W) { + // It would be good to detect this case some day, but the current lint + // doesn't handle it. (The documentation says that this lint "detects + // only common patterns".) + let future = w.write(b"Hello world"); + future.await.unwrap(); +} + fn main() {} diff --git a/tests/ui/unused_io_amount.stderr b/tests/ui/unused_io_amount.stderr index d8dfc0e5a798..3852d0622d6a 100644 --- a/tests/ui/unused_io_amount.stderr +++ b/tests/ui/unused_io_amount.stderr @@ -1,5 +1,5 @@ error: written amount is not handled. Use `Write::write_all` instead - --> $DIR/unused_io_amount.rs:7:5 + --> $DIR/unused_io_amount.rs:9:5 | LL | s.write(b"test")?; | ^^^^^^^^^^^^^^^^^ @@ -7,55 +7,55 @@ LL | s.write(b"test")?; = note: `-D clippy::unused-io-amount` implied by `-D warnings` error: read amount is not handled. Use `Read::read_exact` instead - --> $DIR/unused_io_amount.rs:9:5 + --> $DIR/unused_io_amount.rs:11:5 | LL | s.read(&mut buf)?; | ^^^^^^^^^^^^^^^^^ error: written amount is not handled. Use `Write::write_all` instead - --> $DIR/unused_io_amount.rs:14:5 + --> $DIR/unused_io_amount.rs:16:5 | LL | s.write(b"test").unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: read amount is not handled. Use `Read::read_exact` instead - --> $DIR/unused_io_amount.rs:16:5 + --> $DIR/unused_io_amount.rs:18:5 | LL | s.read(&mut buf).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: read amount is not handled - --> $DIR/unused_io_amount.rs:20:5 + --> $DIR/unused_io_amount.rs:22:5 | LL | s.read_vectored(&mut [io::IoSliceMut::new(&mut [])])?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: written amount is not handled - --> $DIR/unused_io_amount.rs:21:5 + --> $DIR/unused_io_amount.rs:23:5 | LL | s.write_vectored(&[io::IoSlice::new(&[])])?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: read amount is not handled. Use `Read::read_exact` instead - --> $DIR/unused_io_amount.rs:28:5 + --> $DIR/unused_io_amount.rs:30:5 | LL | reader.read(&mut result).ok()?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: read amount is not handled. Use `Read::read_exact` instead - --> $DIR/unused_io_amount.rs:37:5 + --> $DIR/unused_io_amount.rs:39:5 | LL | reader.read(&mut result).or_else(|err| Err(err))?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: read amount is not handled. Use `Read::read_exact` instead - --> $DIR/unused_io_amount.rs:49:5 + --> $DIR/unused_io_amount.rs:51:5 | LL | reader.read(&mut result).or(Err(Error::Kind))?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: read amount is not handled. Use `Read::read_exact` instead - --> $DIR/unused_io_amount.rs:56:5 + --> $DIR/unused_io_amount.rs:58:5 | LL | / reader LL | | .read(&mut result) @@ -64,5 +64,41 @@ LL | | .or(Err(Error::Kind)) LL | | .expect("error"); | |________________________^ -error: aborting due to 10 previous errors +error: written amount is not handled. Use `AsyncWriteExt::write_all` instead + --> $DIR/unused_io_amount.rs:67:5 + | +LL | w.write(b"hello world").await.unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: read amount is not handled. Use `AsyncReadExt::read_exact` instead + --> $DIR/unused_io_amount.rs:72:5 + | +LL | r.read(&mut buf[..]).await.unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: written amount is not handled. Use `AsyncWriteExt::write_all` instead + --> $DIR/unused_io_amount.rs:85:9 + | +LL | w.write(b"hello world").await?; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: read amount is not handled. Use `AsyncReadExt::read_exact` instead + --> $DIR/unused_io_amount.rs:93:9 + | +LL | r.read(&mut buf[..]).await.or(Err(Error::Kind))?; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: written amount is not handled. Use `AsyncWriteExt::write_all` instead + --> $DIR/unused_io_amount.rs:101:5 + | +LL | w.write(b"hello world").await.unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: read amount is not handled. Use `AsyncReadExt::read_exact` instead + --> $DIR/unused_io_amount.rs:106:5 + | +LL | r.read(&mut buf[..]).await.unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 16 previous errors From b6bcf0c51b0d719cfd141c1c010b41ebe74f2abb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 31 Dec 2021 12:20:02 -0500 Subject: [PATCH 03/47] unused_io_amount: Use span_lint_and_help. This improves the quality of the genrated output and makes it more in line with other lint messages. changelog: [`unused_io_amount`]: Improve help text --- clippy_lints/src/unused_io_amount.rs | 26 ++++++++----- tests/ui/unused_io_amount.stderr | 55 +++++++++++++++++++++------- 2 files changed, 58 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/unused_io_amount.rs b/clippy_lints/src/unused_io_amount.rs index 8afd8472db7a..287ac5b4a908 100644 --- a/clippy_lints/src/unused_io_amount.rs +++ b/clippy_lints/src/unused_io_amount.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::span_lint; +use clippy_utils::diagnostics::{span_lint, span_lint_and_help}; use clippy_utils::{is_try, match_trait_method, paths}; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; @@ -126,32 +126,40 @@ fn check_method_call(cx: &LateContext<'_>, call: &hir::Expr<'_>, expr: &hir::Exp }; match (read_trait, write_trait, symbol, is_await) { - (true, _, "read", false) => span_lint( + (true, _, "read", false) => span_lint_and_help( cx, UNUSED_IO_AMOUNT, expr.span, - "read amount is not handled. Use `Read::read_exact` instead", + "read amount is not handled", + None, + "use `Read::read_exact` instead, or handle partial reads", ), - (true, _, "read", true) => span_lint( + (true, _, "read", true) => span_lint_and_help( cx, UNUSED_IO_AMOUNT, expr.span, - "read amount is not handled. Use `AsyncReadExt::read_exact` instead", + "read amount is not handled", + None, + "use `AsyncReadExt::read_exact` instead, or handle partial reads", ), (true, _, "read_vectored", _) => { span_lint(cx, UNUSED_IO_AMOUNT, expr.span, "read amount is not handled"); }, - (_, true, "write", false) => span_lint( + (_, true, "write", false) => span_lint_and_help( cx, UNUSED_IO_AMOUNT, expr.span, - "written amount is not handled. Use `Write::write_all` instead", + "written amount is not handled", + None, + "use `Write::write_all` instead, or handle partial writes", ), - (_, true, "write", true) => span_lint( + (_, true, "write", true) => span_lint_and_help( cx, UNUSED_IO_AMOUNT, expr.span, - "written amount is not handled. Use `AsyncWriteExt::write_all` instead", + "written amount is not handled", + None, + "use `AsyncWriteExt::write_all` instead, or handle partial writes", ), (_, true, "write_vectored", _) => { span_lint(cx, UNUSED_IO_AMOUNT, expr.span, "written amount is not handled"); diff --git a/tests/ui/unused_io_amount.stderr b/tests/ui/unused_io_amount.stderr index 3852d0622d6a..e5bdd993aa1a 100644 --- a/tests/ui/unused_io_amount.stderr +++ b/tests/ui/unused_io_amount.stderr @@ -1,28 +1,35 @@ -error: written amount is not handled. Use `Write::write_all` instead +error: written amount is not handled --> $DIR/unused_io_amount.rs:9:5 | LL | s.write(b"test")?; | ^^^^^^^^^^^^^^^^^ | = note: `-D clippy::unused-io-amount` implied by `-D warnings` + = help: use `Write::write_all` instead, or handle partial writes -error: read amount is not handled. Use `Read::read_exact` instead +error: read amount is not handled --> $DIR/unused_io_amount.rs:11:5 | LL | s.read(&mut buf)?; | ^^^^^^^^^^^^^^^^^ + | + = help: use `Read::read_exact` instead, or handle partial reads -error: written amount is not handled. Use `Write::write_all` instead +error: written amount is not handled --> $DIR/unused_io_amount.rs:16:5 | LL | s.write(b"test").unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Write::write_all` instead, or handle partial writes -error: read amount is not handled. Use `Read::read_exact` instead +error: read amount is not handled --> $DIR/unused_io_amount.rs:18:5 | LL | s.read(&mut buf).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Read::read_exact` instead, or handle partial reads error: read amount is not handled --> $DIR/unused_io_amount.rs:22:5 @@ -36,25 +43,31 @@ error: written amount is not handled LL | s.write_vectored(&[io::IoSlice::new(&[])])?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: read amount is not handled. Use `Read::read_exact` instead +error: read amount is not handled --> $DIR/unused_io_amount.rs:30:5 | LL | reader.read(&mut result).ok()?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Read::read_exact` instead, or handle partial reads -error: read amount is not handled. Use `Read::read_exact` instead +error: read amount is not handled --> $DIR/unused_io_amount.rs:39:5 | LL | reader.read(&mut result).or_else(|err| Err(err))?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Read::read_exact` instead, or handle partial reads -error: read amount is not handled. Use `Read::read_exact` instead +error: read amount is not handled --> $DIR/unused_io_amount.rs:51:5 | LL | reader.read(&mut result).or(Err(Error::Kind))?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Read::read_exact` instead, or handle partial reads -error: read amount is not handled. Use `Read::read_exact` instead +error: read amount is not handled --> $DIR/unused_io_amount.rs:58:5 | LL | / reader @@ -63,42 +76,56 @@ LL | | .or(Err(Error::Kind)) LL | | .or(Err(Error::Kind)) LL | | .expect("error"); | |________________________^ + | + = help: use `Read::read_exact` instead, or handle partial reads -error: written amount is not handled. Use `AsyncWriteExt::write_all` instead +error: written amount is not handled --> $DIR/unused_io_amount.rs:67:5 | LL | w.write(b"hello world").await.unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `AsyncWriteExt::write_all` instead, or handle partial writes -error: read amount is not handled. Use `AsyncReadExt::read_exact` instead +error: read amount is not handled --> $DIR/unused_io_amount.rs:72:5 | LL | r.read(&mut buf[..]).await.unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `AsyncReadExt::read_exact` instead, or handle partial reads -error: written amount is not handled. Use `AsyncWriteExt::write_all` instead +error: written amount is not handled --> $DIR/unused_io_amount.rs:85:9 | LL | w.write(b"hello world").await?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `AsyncWriteExt::write_all` instead, or handle partial writes -error: read amount is not handled. Use `AsyncReadExt::read_exact` instead +error: read amount is not handled --> $DIR/unused_io_amount.rs:93:9 | LL | r.read(&mut buf[..]).await.or(Err(Error::Kind))?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `AsyncReadExt::read_exact` instead, or handle partial reads -error: written amount is not handled. Use `AsyncWriteExt::write_all` instead +error: written amount is not handled --> $DIR/unused_io_amount.rs:101:5 | LL | w.write(b"hello world").await.unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `AsyncWriteExt::write_all` instead, or handle partial writes -error: read amount is not handled. Use `AsyncReadExt::read_exact` instead +error: read amount is not handled --> $DIR/unused_io_amount.rs:106:5 | LL | r.read(&mut buf[..]).await.unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `AsyncReadExt::read_exact` instead, or handle partial reads error: aborting due to 16 previous errors From 3d41358a554cb70e23e001f6ac92cf79d805b671 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 31 Dec 2021 23:39:40 -0500 Subject: [PATCH 04/47] wrong_self_convention: Match `SelfKind::No` more restrictively The `wrong_self_convention` lint uses a `SelfKind` type to decide whether a method has the right kind of "self" for its name, or whether the kind of "self" it has makes its name confusable for a method in a common trait. One possibility is `SelfKind::No`, which is supposed to mean "No `self`". Previously, SelfKind::No matched everything _except_ Self, including references to Self. This patch changes it to match Self, &Self, &mut Self, Box, and so on. For example, this kind of method was allowed before: ``` impl S { // Should trigger the lint, because // "methods called `is_*` usually take `self` by reference or no `self`" fn is_foo(&mut self) -> bool { todo!() } } ``` But since SelfKind::No matched "&mut self", no lint was triggered (see #8142). With this patch, the code above now gives a lint as expected. Fixes #8142 changelog: [`wrong_self_convention`] rejects `self` references in more cases --- clippy_lints/src/methods/mod.rs | 8 +++++++- clippy_lints/src/redundant_clone.rs | 4 ++-- tests/ui/issue_4266.stderr | 11 ++++++++++- tests/ui/wrong_self_convention.rs | 21 +++++++++++++++++++++ tests/ui/wrong_self_convention.stderr | 10 +++++++++- 5 files changed, 49 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 4e33b2ff14cd..8a2a468c8523 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2535,11 +2535,17 @@ impl SelfKind { implements_trait(cx, ty, trait_def_id, &[parent_ty.into()]) } + fn matches_none<'a>(cx: &LateContext<'a>, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool { + !matches_value(cx, parent_ty, ty) + && !matches_ref(cx, hir::Mutability::Not, parent_ty, ty) + && !matches_ref(cx, hir::Mutability::Mut, parent_ty, ty) + } + match self { Self::Value => matches_value(cx, parent_ty, ty), Self::Ref => matches_ref(cx, hir::Mutability::Not, parent_ty, ty) || ty == parent_ty && is_copy(cx, ty), Self::RefMut => matches_ref(cx, hir::Mutability::Mut, parent_ty, ty), - Self::No => ty != parent_ty, + Self::No => matches_none(cx, parent_ty, ty), } } diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 1cf349f8aa7c..1991a01fb60b 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -220,7 +220,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone { continue; } else if let Some(loc) = clone_usage.cloned_consume_or_mutate_loc { // cloned value is mutated, and the clone is alive. - if possible_borrower.is_alive_at(ret_local, loc) { + if possible_borrower.local_is_alive_at(ret_local, loc) { continue; } } @@ -767,7 +767,7 @@ impl PossibleBorrowerMap<'_, '_> { self.bitset.0 == self.bitset.1 } - fn is_alive_at(&mut self, local: mir::Local, at: mir::Location) -> bool { + fn local_is_alive_at(&mut self, local: mir::Local, at: mir::Location) -> bool { self.maybe_live.seek_after_primary_effect(at); self.maybe_live.contains(local) } diff --git a/tests/ui/issue_4266.stderr b/tests/ui/issue_4266.stderr index 20419457b47f..e5042aaa776b 100644 --- a/tests/ui/issue_4266.stderr +++ b/tests/ui/issue_4266.stderr @@ -12,5 +12,14 @@ error: explicit lifetimes given in parameter types where they could be elided (o LL | async fn one_to_one<'a>(s: &'a str) -> &'a str { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 2 previous errors +error: methods called `new` usually take no `self` + --> $DIR/issue_4266.rs:27:22 + | +LL | pub async fn new(&mut self) -> Self { + | ^^^^^^^^^ + | + = note: `-D clippy::wrong-self-convention` implied by `-D warnings` + = help: consider choosing a less ambiguous name + +error: aborting due to 3 previous errors diff --git a/tests/ui/wrong_self_convention.rs b/tests/ui/wrong_self_convention.rs index 1b9da8a55e53..f8fee4b3ab2d 100644 --- a/tests/ui/wrong_self_convention.rs +++ b/tests/ui/wrong_self_convention.rs @@ -188,3 +188,24 @@ mod issue6727 { } } } + +pub mod issue8142 { + struct S; + + impl S { + // Should lint: is_ methods should only take &self, or no self at all. + fn is_still_buggy(&mut self) -> bool { + false + } + + // Should not lint: "no self at all" is allowed. + fn is_forty_two(x: u32) -> bool { + x == 42 + } + + // Should not lint: &self is allowed. + fn is_test_code(&self) -> bool { + true + } + } +} diff --git a/tests/ui/wrong_self_convention.stderr b/tests/ui/wrong_self_convention.stderr index 590ee6d9c529..5493a99572e0 100644 --- a/tests/ui/wrong_self_convention.stderr +++ b/tests/ui/wrong_self_convention.stderr @@ -191,5 +191,13 @@ LL | fn to_u64(self) -> u64 { | = help: consider choosing a less ambiguous name -error: aborting due to 24 previous errors +error: methods called `is_*` usually take `self` by reference or no `self` + --> $DIR/wrong_self_convention.rs:197:27 + | +LL | fn is_still_buggy(&mut self) -> bool { + | ^^^^^^^^^ + | + = help: consider choosing a less ambiguous name + +error: aborting due to 25 previous errors From 262b148d8830efa4773f05b27403f94e47d27f52 Mon Sep 17 00:00:00 2001 From: bors Date: Sat, 1 Jan 2022 13:16:49 +0000 Subject: [PATCH 05/47] return_self_not_must_use document `#[must_use]` on the type Inspired by a discussion in rust-lang/rust-clippy#8197 --- r? `@llogiq` changelog: none The lint is this on nightly, therefore no changelog entry for you xD --- clippy_lints/src/return_self_not_must_use.rs | 36 ++++++++++++++++---- tests/ui/return_self_not_must_use.stderr | 5 +++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/return_self_not_must_use.rs b/clippy_lints/src/return_self_not_must_use.rs index b57ec96bc7e6..935bbc37d75d 100644 --- a/clippy_lints/src/return_self_not_must_use.rs +++ b/clippy_lints/src/return_self_not_must_use.rs @@ -1,5 +1,6 @@ +use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::ty::is_must_use_ty; -use clippy_utils::{diagnostics::span_lint, nth_arg, return_ty}; +use clippy_utils::{nth_arg, return_ty}; use rustc_hir::def_id::LocalDefId; use rustc_hir::intravisit::FnKind; use rustc_hir::{Body, FnDecl, HirId, TraitItem, TraitItemKind}; @@ -13,25 +14,46 @@ declare_clippy_lint! { /// This lint warns when a method returning `Self` doesn't have the `#[must_use]` attribute. /// /// ### Why is this bad? - /// It prevents to "forget" to use the newly created value. + /// Methods returning `Self` often create new values, having the `#[must_use]` attribute + /// prevents users from "forgetting" to use the newly created value. + /// + /// The `#[must_use]` attribute can be added to the type itself to ensure that instances + /// are never forgotten. Functions returning a type marked with `#[must_use]` will not be + /// linted, as the usage is already enforced by the type attribute. /// /// ### Limitations /// This lint is only applied on methods taking a `self` argument. It would be mostly noise /// if it was added on constructors for example. /// /// ### Example + /// Missing attribute /// ```rust /// pub struct Bar; - /// /// impl Bar { /// // Bad /// pub fn bar(&self) -> Self { /// Self /// } + /// } + /// ``` /// - /// // Good + /// It's better to have the `#[must_use]` attribute on the method like this: + /// ```rust + /// pub struct Bar; + /// impl Bar { /// #[must_use] - /// pub fn foo(&self) -> Self { + /// pub fn bar(&self) -> Self { + /// Self + /// } + /// } + /// ``` + /// + /// Or on the type definition like this: + /// ```rust + /// #[must_use] + /// pub struct Bar; + /// impl Bar { + /// pub fn bar(&self) -> Self { /// Self /// } /// } @@ -65,11 +87,13 @@ fn check_method(cx: &LateContext<'tcx>, decl: &'tcx FnDecl<'tcx>, fn_def: LocalD if !is_must_use_ty(cx, ret_ty); then { - span_lint( + span_lint_and_help( cx, RETURN_SELF_NOT_MUST_USE, span, "missing `#[must_use]` attribute on a method returning `Self`", + None, + "consider adding the `#[must_use]` attribute to the method or directly to the `Self` type" ); } } diff --git a/tests/ui/return_self_not_must_use.stderr b/tests/ui/return_self_not_must_use.stderr index 3793a5559ba5..8af10cb65c40 100644 --- a/tests/ui/return_self_not_must_use.stderr +++ b/tests/ui/return_self_not_must_use.stderr @@ -5,6 +5,7 @@ LL | fn what(&self) -> Self; | ^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::return-self-not-must-use` implied by `-D warnings` + = help: consider adding the `#[must_use]` attribute to the method or directly to the `Self` type error: missing `#[must_use]` attribute on a method returning `Self` --> $DIR/return_self_not_must_use.rs:17:5 @@ -13,6 +14,8 @@ LL | / pub fn foo(&self) -> Self { LL | | Self LL | | } | |_____^ + | + = help: consider adding the `#[must_use]` attribute to the method or directly to the `Self` type error: missing `#[must_use]` attribute on a method returning `Self` --> $DIR/return_self_not_must_use.rs:20:5 @@ -21,6 +24,8 @@ LL | / pub fn bar(self) -> Self { LL | | self LL | | } | |_____^ + | + = help: consider adding the `#[must_use]` attribute to the method or directly to the `Self` type error: aborting due to 3 previous errors From e8b6b2ac0c115e7208d725b4f6340e68194d0cf3 Mon Sep 17 00:00:00 2001 From: Wigy Date: Fri, 31 Dec 2021 09:37:39 +0100 Subject: [PATCH 06/47] erasing_op lint ignored when output type is different from the non-const one --- clippy_lints/src/erasing_op.rs | 26 +++++++++++++++++++------- tests/ui/erasing_op.rs | 34 ++++++++++++++++++++++++++++++++++ tests/ui/erasing_op.stderr | 20 ++++++++++++++++---- tests/ui/identity_op.rs | 13 +++++++++++++ tests/ui/identity_op.stderr | 26 +++++++++++++------------- 5 files changed, 95 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/erasing_op.rs b/clippy_lints/src/erasing_op.rs index d49cec26be5f..bb6acd8c5dd4 100644 --- a/clippy_lints/src/erasing_op.rs +++ b/clippy_lints/src/erasing_op.rs @@ -1,9 +1,11 @@ use clippy_utils::consts::{constant_simple, Constant}; use clippy_utils::diagnostics::span_lint; +use clippy_utils::ty::same_type_and_consts; + use rustc_hir::{BinOpKind, Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::TypeckResults; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::source_map::Span; declare_clippy_lint! { /// ### What it does @@ -35,24 +37,34 @@ impl<'tcx> LateLintPass<'tcx> for ErasingOp { return; } if let ExprKind::Binary(ref cmp, left, right) = e.kind { + let tck = cx.typeck_results(); match cmp.node { BinOpKind::Mul | BinOpKind::BitAnd => { - check(cx, left, e.span); - check(cx, right, e.span); + check(cx, tck, left, right, e); + check(cx, tck, right, left, e); }, - BinOpKind::Div => check(cx, left, e.span), + BinOpKind::Div => check(cx, tck, left, right, e), _ => (), } } } } -fn check(cx: &LateContext<'_>, e: &Expr<'_>, span: Span) { - if constant_simple(cx, cx.typeck_results(), e) == Some(Constant::Int(0)) { +fn different_types(tck: &TypeckResults<'tcx>, input: &'tcx Expr<'_>, output: &'tcx Expr<'_>) -> bool { + let input_ty = tck.expr_ty(input).peel_refs(); + let output_ty = tck.expr_ty(output).peel_refs(); + !same_type_and_consts(input_ty, output_ty) +} + +fn check(cx: &LateContext<'cx>, tck: &TypeckResults<'cx>, op: &Expr<'cx>, other: &Expr<'cx>, parent: &Expr<'cx>) { + if constant_simple(cx, tck, op) == Some(Constant::Int(0)) { + if different_types(tck, other, parent) { + return; + } span_lint( cx, ERASING_OP, - span, + parent.span, "this operation will always return zero. This is likely not the intended outcome", ); } diff --git a/tests/ui/erasing_op.rs b/tests/ui/erasing_op.rs index 1540062a4bc3..ae2fad0086da 100644 --- a/tests/ui/erasing_op.rs +++ b/tests/ui/erasing_op.rs @@ -1,3 +1,34 @@ +struct Length(u8); +struct Meter; + +impl core::ops::Mul for u8 { + type Output = Length; + fn mul(self, _: Meter) -> Length { + Length(self) + } +} + +#[derive(Clone, Default, PartialEq, Eq, Hash)] +struct Vec1 { + x: i32, +} + +impl core::ops::Mul for i32 { + type Output = Vec1; + fn mul(self, mut right: Vec1) -> Vec1 { + right.x *= self; + right + } +} + +impl core::ops::Mul for Vec1 { + type Output = Vec1; + fn mul(mut self, right: i32) -> Vec1 { + self.x *= right; + self + } +} + #[allow(clippy::no_effect)] #[warn(clippy::erasing_op)] fn main() { @@ -6,4 +37,7 @@ fn main() { x * 0; 0 & x; 0 / x; + 0 * Meter; // no error: Output type is different from the non-zero argument + 0 * Vec1 { x: 5 }; + Vec1 { x: 5 } * 0; } diff --git a/tests/ui/erasing_op.stderr b/tests/ui/erasing_op.stderr index e54ce85f98ec..165ed9bfe58b 100644 --- a/tests/ui/erasing_op.stderr +++ b/tests/ui/erasing_op.stderr @@ -1,5 +1,5 @@ error: this operation will always return zero. This is likely not the intended outcome - --> $DIR/erasing_op.rs:6:5 + --> $DIR/erasing_op.rs:37:5 | LL | x * 0; | ^^^^^ @@ -7,16 +7,28 @@ LL | x * 0; = note: `-D clippy::erasing-op` implied by `-D warnings` error: this operation will always return zero. This is likely not the intended outcome - --> $DIR/erasing_op.rs:7:5 + --> $DIR/erasing_op.rs:38:5 | LL | 0 & x; | ^^^^^ error: this operation will always return zero. This is likely not the intended outcome - --> $DIR/erasing_op.rs:8:5 + --> $DIR/erasing_op.rs:39:5 | LL | 0 / x; | ^^^^^ -error: aborting due to 3 previous errors +error: this operation will always return zero. This is likely not the intended outcome + --> $DIR/erasing_op.rs:41:5 + | +LL | 0 * Vec1 { x: 5 }; + | ^^^^^^^^^^^^^^^^^ + +error: this operation will always return zero. This is likely not the intended outcome + --> $DIR/erasing_op.rs:42:5 + | +LL | Vec1 { x: 5 } * 0; + | ^^^^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs index 2ed4b5db574d..12bbda71f434 100644 --- a/tests/ui/identity_op.rs +++ b/tests/ui/identity_op.rs @@ -11,6 +11,17 @@ impl std::ops::Shl for A { self } } + +struct Length(u8); +struct Meter; + +impl core::ops::Mul for u8 { + type Output = Length; + fn mul(self, _: Meter) -> Length { + Length(self) + } +} + #[allow( clippy::eq_op, clippy::no_effect, @@ -53,4 +64,6 @@ fn main() { let mut a = A("".into()); let b = a << 0; // no error: non-integer + + 1 * Meter; // no error: non-integer } diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr index ff34b38db015..0103cf5457e8 100644 --- a/tests/ui/identity_op.stderr +++ b/tests/ui/identity_op.stderr @@ -1,5 +1,5 @@ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:26:5 + --> $DIR/identity_op.rs:37:5 | LL | x + 0; | ^^^^^ @@ -7,73 +7,73 @@ LL | x + 0; = note: `-D clippy::identity-op` implied by `-D warnings` error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:27:5 + --> $DIR/identity_op.rs:38:5 | LL | x + (1 - 1); | ^^^^^^^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:29:5 + --> $DIR/identity_op.rs:40:5 | LL | 0 + x; | ^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:32:5 + --> $DIR/identity_op.rs:43:5 | LL | x | (0); | ^^^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:35:5 + --> $DIR/identity_op.rs:46:5 | LL | x * 1; | ^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:36:5 + --> $DIR/identity_op.rs:47:5 | LL | 1 * x; | ^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:42:5 + --> $DIR/identity_op.rs:53:5 | LL | -1 & x; | ^^^^^^ error: the operation is ineffective. Consider reducing it to `u` - --> $DIR/identity_op.rs:45:5 + --> $DIR/identity_op.rs:56:5 | LL | u & 255; | ^^^^^^^ error: the operation is ineffective. Consider reducing it to `42` - --> $DIR/identity_op.rs:48:5 + --> $DIR/identity_op.rs:59:5 | LL | 42 << 0; | ^^^^^^^ error: the operation is ineffective. Consider reducing it to `1` - --> $DIR/identity_op.rs:49:5 + --> $DIR/identity_op.rs:60:5 | LL | 1 >> 0; | ^^^^^^ error: the operation is ineffective. Consider reducing it to `42` - --> $DIR/identity_op.rs:50:5 + --> $DIR/identity_op.rs:61:5 | LL | 42 >> 0; | ^^^^^^^ error: the operation is ineffective. Consider reducing it to `&x` - --> $DIR/identity_op.rs:51:5 + --> $DIR/identity_op.rs:62:5 | LL | &x >> 0; | ^^^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:52:5 + --> $DIR/identity_op.rs:63:5 | LL | x >> &0; | ^^^^^^^ From 181d4773a33b1e3a202208ff0214b3c4b94d7d69 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Thu, 30 Dec 2021 22:39:53 -0800 Subject: [PATCH 07/47] New line: cloned_next --- clippy_lints/src/lib.register_nursery.rs | 1 + clippy_lints/src/methods/cloned_last.rs | 36 +++++++++++++++++++++++ clippy_lints/src/methods/mod.rs | 37 +++++++++++++++++++++++- tests/ui/cloned_last.fixed | 9 ++++++ tests/ui/cloned_last.rs | 9 ++++++ tests/ui/cloned_last.stderr | 18 ++++++++++++ 6 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/methods/cloned_last.rs create mode 100644 tests/ui/cloned_last.fixed create mode 100644 tests/ui/cloned_last.rs create mode 100644 tests/ui/cloned_last.stderr diff --git a/clippy_lints/src/lib.register_nursery.rs b/clippy_lints/src/lib.register_nursery.rs index e3cf06700183..2e05c1842473 100644 --- a/clippy_lints/src/lib.register_nursery.rs +++ b/clippy_lints/src/lib.register_nursery.rs @@ -15,6 +15,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(future_not_send::FUTURE_NOT_SEND), LintId::of(index_refutable_slice::INDEX_REFUTABLE_SLICE), LintId::of(let_if_seq::USELESS_LET_IF_SEQ), + LintId::of(methods::CLONED_LAST), LintId::of(missing_const_for_fn::MISSING_CONST_FOR_FN), LintId::of(mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL), LintId::of(mutex_atomic::MUTEX_INTEGER), diff --git a/clippy_lints/src/methods/cloned_last.rs b/clippy_lints/src/methods/cloned_last.rs new file mode 100644 index 000000000000..42ada6ebd48f --- /dev/null +++ b/clippy_lints/src/methods/cloned_last.rs @@ -0,0 +1,36 @@ +use clippy_utils::diagnostics::{span_lint_and_sugg}; +use clippy_utils::source::snippet; +use clippy_utils::ty::implements_trait; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_lint::LateContext; +use rustc_span::sym; + +use super::CLONED_LAST; + +/// lint use of `cloned().next()` for `Iterators` +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'_>, + recv: &'tcx hir::Expr<'_>, +) { + // lint if caller of `.filter().next()` is an Iterator + let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { + implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) + }); + if recv_impls_iterator { + let msg = "called `cloned().last()` on an `Iterator`. It may be more efficient to call + `.last.cloned()` instead"; + let iter_snippet = snippet(cx, recv.span, ".."); + // add note if not multi-line + span_lint_and_sugg( + cx, + CLONED_LAST, + expr.span, + msg, + "try this", + format!("{}.last().cloned()", iter_snippet), + Applicability::MachineApplicable, + ); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8a2a468c8523..ae502939bc60 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -9,6 +9,7 @@ mod chars_next_cmp_with_unwrap; mod clone_on_copy; mod clone_on_ref_ptr; mod cloned_instead_of_copied; +mod cloned_last; mod expect_fun_call; mod expect_used; mod extend_with_drain; @@ -107,6 +108,29 @@ declare_clippy_lint! { "used `cloned` where `copied` could be used instead" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `_.cloned().last()`. + /// + /// ### Why is this bad? + /// Performance, clone only need to be executed once. + /// + /// ### Example + /// ```rust + /// # let vec = vec!["string".to_string()]; + /// vec.iter().cloned().last(); + /// ``` + /// Could be written as + /// ```rust + /// # let vec = vec!["string".to_string()]; + /// vec.iter().last().cloned(); + /// ``` + #[clippy::version = "1.59.0"] + pub CLONED_LAST, + complexity, + "using `cloned().last()`, which is less efficient than `.last().cloned()`" +} + declare_clippy_lint! { /// ### What it does /// Checks for usages of `Iterator::flat_map()` where `filter_map()` could be @@ -1946,6 +1970,7 @@ impl_lint_pass!(Methods => [ CLONE_ON_COPY, CLONE_ON_REF_PTR, CLONE_DOUBLE_REF, + CLONED_LAST, CLONED_INSTEAD_OF_COPIED, FLAT_MAP_OPTION, INEFFICIENT_TO_STRING, @@ -2232,7 +2257,9 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("as_mut", []) => useless_asref::check(cx, expr, "as_mut", recv), ("as_ref", []) => useless_asref::check(cx, expr, "as_ref", recv), ("assume_init", []) => uninit_assumed_init::check(cx, expr, recv), - ("cloned", []) => cloned_instead_of_copied::check(cx, expr, recv, span, msrv), + ("cloned", []) => { + cloned_instead_of_copied::check(cx, expr, recv, span, msrv); + }, ("collect", []) => match method_call!(recv) { Some((name @ ("cloned" | "copied"), [recv2], _)) => { iter_cloned_collect::check(cx, name, expr, recv2); @@ -2285,6 +2312,14 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("is_file", []) => filetype_is_file::check(cx, expr, recv), ("is_none", []) => check_is_some_is_none(cx, expr, recv, false), ("is_some", []) => check_is_some_is_none(cx, expr, recv, true), + ("last", []) => { + if let Some((name, [recv, args @ ..], _)) = method_call!(recv) { + match (name, args) { + ("cloned", []) => cloned_last::check(cx, expr, recv), + _ => {}, + } + } + }, ("map", [m_arg]) => { if let Some((name, [recv2, args @ ..], span2)) = method_call!(recv) { match (name, args) { diff --git a/tests/ui/cloned_last.fixed b/tests/ui/cloned_last.fixed new file mode 100644 index 000000000000..1253c5782b7a --- /dev/null +++ b/tests/ui/cloned_last.fixed @@ -0,0 +1,9 @@ +// run-rustfix +#![warn(clippy::nursery)] + +fn main() { + + #[rustfmt::skip] + let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter().last().cloned(); +} diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs new file mode 100644 index 000000000000..6bdbb136e7f4 --- /dev/null +++ b/tests/ui/cloned_last.rs @@ -0,0 +1,9 @@ +// run-rustfix +#![warn(clippy::nursery)] + +fn main() { + + #[rustfmt::skip] + let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter().cloned().last(); +} diff --git a/tests/ui/cloned_last.stderr b/tests/ui/cloned_last.stderr new file mode 100644 index 000000000000..a8f8a9b26fa4 --- /dev/null +++ b/tests/ui/cloned_last.stderr @@ -0,0 +1,18 @@ +error: called `cloned().last()` on an `Iterator`. It may be more efficient to call + `.last.cloned()` instead + --> $DIR/cloned_last.rs:7:29 + | +LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _____________________________^ +LL | | .iter().cloned().last(); + | |_______________________________^ + | + = note: `-D clippy::cloned-last` implied by `-D warnings` +help: try this + | +LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ .iter().last().cloned(); + | + +error: aborting due to previous error + From 8b7108b85666eebfffeca26b7b5e4447d1732319 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Thu, 30 Dec 2021 23:35:53 -0800 Subject: [PATCH 08/47] Update lints --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_complexity.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_nursery.rs | 1 - 5 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27bac4718b6c..67ba73fd998b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2911,6 +2911,7 @@ Released 2018-09-13 [`clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy [`clone_on_ref_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_ref_ptr [`cloned_instead_of_copied`]: https://rust-lang.github.io/rust-clippy/master/index.html#cloned_instead_of_copied +[`cloned_last`]: https://rust-lang.github.io/rust-clippy/master/index.html#cloned_last [`cmp_nan`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_nan [`cmp_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_null [`cmp_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 944411087e95..2a489a8903fa 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -138,6 +138,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(methods::BYTES_NTH), LintId::of(methods::CHARS_LAST_CMP), LintId::of(methods::CHARS_NEXT_CMP), + LintId::of(methods::CLONED_LAST), LintId::of(methods::CLONE_DOUBLE_REF), LintId::of(methods::CLONE_ON_COPY), LintId::of(methods::EXPECT_FUN_CALL), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index a21ddf73a115..21de9d65a8d1 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -32,6 +32,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(matches::MATCH_SINGLE_BINDING), LintId::of(matches::WILDCARD_IN_OR_PATTERNS), LintId::of(methods::BIND_INSTEAD_OF_MAP), + LintId::of(methods::CLONED_LAST), LintId::of(methods::CLONE_ON_COPY), LintId::of(methods::FILTER_MAP_IDENTITY), LintId::of(methods::FILTER_NEXT), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 002122793f3b..b94ddac924c7 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -262,6 +262,7 @@ store.register_lints(&[ methods::CHARS_LAST_CMP, methods::CHARS_NEXT_CMP, methods::CLONED_INSTEAD_OF_COPIED, + methods::CLONED_LAST, methods::CLONE_DOUBLE_REF, methods::CLONE_ON_COPY, methods::CLONE_ON_REF_PTR, diff --git a/clippy_lints/src/lib.register_nursery.rs b/clippy_lints/src/lib.register_nursery.rs index 2e05c1842473..e3cf06700183 100644 --- a/clippy_lints/src/lib.register_nursery.rs +++ b/clippy_lints/src/lib.register_nursery.rs @@ -15,7 +15,6 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(future_not_send::FUTURE_NOT_SEND), LintId::of(index_refutable_slice::INDEX_REFUTABLE_SLICE), LintId::of(let_if_seq::USELESS_LET_IF_SEQ), - LintId::of(methods::CLONED_LAST), LintId::of(missing_const_for_fn::MISSING_CONST_FOR_FN), LintId::of(mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL), LintId::of(mutex_atomic::MUTEX_INTEGER), From 8898cd61de8f6d158a1b9394c08d1fe80726512f Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Thu, 30 Dec 2021 23:49:17 -0800 Subject: [PATCH 09/47] clippy --- clippy_lints/src/methods/cloned_last.rs | 8 ++------ clippy_lints/src/methods/mod.rs | 7 ++----- rust-toolchain | 1 + 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/methods/cloned_last.rs b/clippy_lints/src/methods/cloned_last.rs index 42ada6ebd48f..61574eefaa6f 100644 --- a/clippy_lints/src/methods/cloned_last.rs +++ b/clippy_lints/src/methods/cloned_last.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::{span_lint_and_sugg}; +use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet; use clippy_utils::ty::implements_trait; use rustc_errors::Applicability; @@ -9,11 +9,7 @@ use rustc_span::sym; use super::CLONED_LAST; /// lint use of `cloned().next()` for `Iterators` -pub(super) fn check<'tcx>( - cx: &LateContext<'tcx>, - expr: &'tcx hir::Expr<'_>, - recv: &'tcx hir::Expr<'_>, -) { +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, recv: &'tcx hir::Expr<'_>) { // lint if caller of `.filter().next()` is an Iterator let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index ae502939bc60..1955903ef2aa 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2313,11 +2313,8 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("is_none", []) => check_is_some_is_none(cx, expr, recv, false), ("is_some", []) => check_is_some_is_none(cx, expr, recv, true), ("last", []) => { - if let Some((name, [recv, args @ ..], _)) = method_call!(recv) { - match (name, args) { - ("cloned", []) => cloned_last::check(cx, expr, recv), - _ => {}, - } + if let ("cloned", []) = (name, args) { + cloned_last::check(cx, expr, recv) } }, ("map", [m_arg]) => { diff --git a/rust-toolchain b/rust-toolchain index 471ae40f1ac7..7abb945484c1 100644 --- a/rust-toolchain +++ b/rust-toolchain @@ -1,3 +1,4 @@ [toolchain] channel = "nightly-2021-12-30" components = ["cargo", "llvm-tools-preview", "rust-src", "rust-std", "rustc", "rustc-dev", "rustfmt"] +targets = [ "wasm32-unknown-unknown" ] From b3351a671e98bd48d2c3ccf86bd8845fcaedda09 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Thu, 30 Dec 2021 23:53:00 -0800 Subject: [PATCH 10/47] clippy --- tests/ui/cloned_last.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs index 6bdbb136e7f4..1eb3410318a9 100644 --- a/tests/ui/cloned_last.rs +++ b/tests/ui/cloned_last.rs @@ -2,7 +2,6 @@ #![warn(clippy::nursery)] fn main() { - #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().cloned().last(); From 6baa42530be7afe12a724fb393ce81a7eaedd276 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 00:20:07 -0800 Subject: [PATCH 11/47] Apply feedback --- clippy_lints/src/lib.register_nursery.rs | 1 + clippy_lints/src/methods/cloned_last.rs | 4 ++-- clippy_lints/src/methods/mod.rs | 6 ++++-- tests/ui/cloned_last.fixed | 3 +-- tests/ui/cloned_last.rs | 4 +++- tests/ui/cloned_last.stderr | 12 +++++++----- 6 files changed, 18 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/lib.register_nursery.rs b/clippy_lints/src/lib.register_nursery.rs index e3cf06700183..aababaa0245e 100644 --- a/clippy_lints/src/lib.register_nursery.rs +++ b/clippy_lints/src/lib.register_nursery.rs @@ -18,6 +18,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(missing_const_for_fn::MISSING_CONST_FOR_FN), LintId::of(mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL), LintId::of(mutex_atomic::MUTEX_INTEGER), + LintId::of(methods::CLONED_LAST), LintId::of(non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY), LintId::of(nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES), LintId::of(option_if_let_else::OPTION_IF_LET_ELSE), diff --git a/clippy_lints/src/methods/cloned_last.rs b/clippy_lints/src/methods/cloned_last.rs index 61574eefaa6f..a9fccfbd0b4d 100644 --- a/clippy_lints/src/methods/cloned_last.rs +++ b/clippy_lints/src/methods/cloned_last.rs @@ -16,7 +16,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, rec }); if recv_impls_iterator { let msg = "called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last.cloned()` instead"; + `.last().clone()` instead"; let iter_snippet = snippet(cx, recv.span, ".."); // add note if not multi-line span_lint_and_sugg( @@ -25,7 +25,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, rec expr.span, msg, "try this", - format!("{}.last().cloned()", iter_snippet), + format!("{}.last().clone()", iter_snippet), Applicability::MachineApplicable, ); } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 1955903ef2aa..599b83345464 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2313,8 +2313,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("is_none", []) => check_is_some_is_none(cx, expr, recv, false), ("is_some", []) => check_is_some_is_none(cx, expr, recv, true), ("last", []) => { - if let ("cloned", []) = (name, args) { - cloned_last::check(cx, expr, recv) + if let Some((name, [recv2, args @ ..], _span2)) = method_call!(recv) { + if let ("cloned", []) = (name, args) { + cloned_last::check(cx, expr, recv2) + } } }, ("map", [m_arg]) => { diff --git a/tests/ui/cloned_last.fixed b/tests/ui/cloned_last.fixed index 1253c5782b7a..6d1712ca7cfa 100644 --- a/tests/ui/cloned_last.fixed +++ b/tests/ui/cloned_last.fixed @@ -2,8 +2,7 @@ #![warn(clippy::nursery)] fn main() { - #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter().last().cloned(); + .iter().last().clone(); } diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs index 1eb3410318a9..8e05ac3cb03a 100644 --- a/tests/ui/cloned_last.rs +++ b/tests/ui/cloned_last.rs @@ -4,5 +4,7 @@ fn main() { #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter().cloned().last(); + .iter() + .cloned() + .last(); } diff --git a/tests/ui/cloned_last.stderr b/tests/ui/cloned_last.stderr index a8f8a9b26fa4..3e4612dd9627 100644 --- a/tests/ui/cloned_last.stderr +++ b/tests/ui/cloned_last.stderr @@ -1,17 +1,19 @@ error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last.cloned()` instead - --> $DIR/cloned_last.rs:7:29 + `.last().clone()` instead + --> $DIR/cloned_last.rs:6:29 | LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] | _____________________________^ -LL | | .iter().cloned().last(); - | |_______________________________^ +LL | | .iter() +LL | | .cloned() +LL | | .last(); + | |_______________^ | = note: `-D clippy::cloned-last` implied by `-D warnings` help: try this | LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] -LL ~ .iter().last().cloned(); +LL ~ .iter().last().clone(); | error: aborting due to previous error From edc20665862681a0c9b5eb8762e209a3d66dd76e Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 00:26:25 -0800 Subject: [PATCH 12/47] Apply feedback --- clippy_lints/src/methods/cloned_last.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/cloned_last.rs b/clippy_lints/src/methods/cloned_last.rs index a9fccfbd0b4d..0fcd071f81cf 100644 --- a/clippy_lints/src/methods/cloned_last.rs +++ b/clippy_lints/src/methods/cloned_last.rs @@ -10,7 +10,7 @@ use super::CLONED_LAST; /// lint use of `cloned().next()` for `Iterators` pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, recv: &'tcx hir::Expr<'_>) { - // lint if caller of `.filter().next()` is an Iterator + // lint if caller of `.cloned().next()` is an Iterator let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) }); @@ -18,7 +18,6 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, rec let msg = "called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().clone()` instead"; let iter_snippet = snippet(cx, recv.span, ".."); - // add note if not multi-line span_lint_and_sugg( cx, CLONED_LAST, From c09063df326e5ce129bc5c41f86ca46a1ab42499 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 00:27:58 -0800 Subject: [PATCH 13/47] Fix doc --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 599b83345464..acde0a0a71d0 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -123,7 +123,7 @@ declare_clippy_lint! { /// Could be written as /// ```rust /// # let vec = vec!["string".to_string()]; - /// vec.iter().last().cloned(); + /// vec.iter().last().clone(); /// ``` #[clippy::version = "1.59.0"] pub CLONED_LAST, From ad8f970f9769354ae8c58839d6cebc626d511e95 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 00:28:33 -0800 Subject: [PATCH 14/47] Remove targets --- rust-toolchain | 1 - 1 file changed, 1 deletion(-) diff --git a/rust-toolchain b/rust-toolchain index 7abb945484c1..471ae40f1ac7 100644 --- a/rust-toolchain +++ b/rust-toolchain @@ -1,4 +1,3 @@ [toolchain] channel = "nightly-2021-12-30" components = ["cargo", "llvm-tools-preview", "rust-src", "rust-std", "rustc", "rustc-dev", "rustfmt"] -targets = [ "wasm32-unknown-unknown" ] From 5d0aefb829beaeafad9a2b405c082d1d33960d4d Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 00:33:26 -0800 Subject: [PATCH 15/47] Fix CI --- clippy_lints/src/lib.register_nursery.rs | 1 - clippy_lints/src/methods/cloned_last.rs | 4 ++-- clippy_lints/src/methods/mod.rs | 2 +- tests/ui/cloned_last.fixed | 4 ++-- tests/ui/cloned_last.rs | 2 +- tests/ui/cloned_last.stderr | 4 ++-- 6 files changed, 8 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/lib.register_nursery.rs b/clippy_lints/src/lib.register_nursery.rs index aababaa0245e..e3cf06700183 100644 --- a/clippy_lints/src/lib.register_nursery.rs +++ b/clippy_lints/src/lib.register_nursery.rs @@ -18,7 +18,6 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(missing_const_for_fn::MISSING_CONST_FOR_FN), LintId::of(mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL), LintId::of(mutex_atomic::MUTEX_INTEGER), - LintId::of(methods::CLONED_LAST), LintId::of(non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY), LintId::of(nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES), LintId::of(option_if_let_else::OPTION_IF_LET_ELSE), diff --git a/clippy_lints/src/methods/cloned_last.rs b/clippy_lints/src/methods/cloned_last.rs index 0fcd071f81cf..09f2de656411 100644 --- a/clippy_lints/src/methods/cloned_last.rs +++ b/clippy_lints/src/methods/cloned_last.rs @@ -16,7 +16,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, rec }); if recv_impls_iterator { let msg = "called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().clone()` instead"; + `.last().cloned()` instead"; let iter_snippet = snippet(cx, recv.span, ".."); span_lint_and_sugg( cx, @@ -24,7 +24,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, rec expr.span, msg, "try this", - format!("{}.last().clone()", iter_snippet), + format!("{}.last().cloned()", iter_snippet), Applicability::MachineApplicable, ); } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index acde0a0a71d0..599b83345464 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -123,7 +123,7 @@ declare_clippy_lint! { /// Could be written as /// ```rust /// # let vec = vec!["string".to_string()]; - /// vec.iter().last().clone(); + /// vec.iter().last().cloned(); /// ``` #[clippy::version = "1.59.0"] pub CLONED_LAST, diff --git a/tests/ui/cloned_last.fixed b/tests/ui/cloned_last.fixed index 6d1712ca7cfa..a8e9996f3fcb 100644 --- a/tests/ui/cloned_last.fixed +++ b/tests/ui/cloned_last.fixed @@ -1,8 +1,8 @@ // run-rustfix -#![warn(clippy::nursery)] +#![warn(clippy::all)] fn main() { #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter().last().clone(); + .iter().last().cloned(); } diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs index 8e05ac3cb03a..348840c249b2 100644 --- a/tests/ui/cloned_last.rs +++ b/tests/ui/cloned_last.rs @@ -1,5 +1,5 @@ // run-rustfix -#![warn(clippy::nursery)] +#![warn(clippy::all)] fn main() { #[rustfmt::skip] diff --git a/tests/ui/cloned_last.stderr b/tests/ui/cloned_last.stderr index 3e4612dd9627..52da6923639b 100644 --- a/tests/ui/cloned_last.stderr +++ b/tests/ui/cloned_last.stderr @@ -1,5 +1,5 @@ error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().clone()` instead + `.last().cloned()` instead --> $DIR/cloned_last.rs:6:29 | LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] @@ -13,7 +13,7 @@ LL | | .last(); help: try this | LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] -LL ~ .iter().last().clone(); +LL ~ .iter().last().cloned(); | error: aborting due to previous error From 9ae3a043d0110179a1372d027c52cbb1b4f070af Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 00:41:00 -0800 Subject: [PATCH 16/47] Fix clippy --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 599b83345464..0576b7e3c4d3 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2315,7 +2315,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("last", []) => { if let Some((name, [recv2, args @ ..], _span2)) = method_call!(recv) { if let ("cloned", []) = (name, args) { - cloned_last::check(cx, expr, recv2) + cloned_last::check(cx, expr, recv2); } } }, From 2220f144fb762fc8be172202797cf0beb49d4736 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 01:59:43 -0800 Subject: [PATCH 17/47] Fix comment in cloned_last --- clippy_lints/src/methods/cloned_last.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/cloned_last.rs b/clippy_lints/src/methods/cloned_last.rs index 09f2de656411..f3aa71dab7f9 100644 --- a/clippy_lints/src/methods/cloned_last.rs +++ b/clippy_lints/src/methods/cloned_last.rs @@ -8,9 +8,9 @@ use rustc_span::sym; use super::CLONED_LAST; -/// lint use of `cloned().next()` for `Iterators` +/// lint use of `cloned().last()` for `Iterators` pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, recv: &'tcx hir::Expr<'_>) { - // lint if caller of `.cloned().next()` is an Iterator + // lint if caller of `.cloned().last()` is an Iterator let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) }); From 9025bad22f4a1ff84da11ddef094477cb4f49980 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 03:30:53 -0800 Subject: [PATCH 18/47] Implement PR for `.cloned().next()` and `.cloned().count()` --- clippy_lints/src/methods/cloned_last.rs | 8 ++++-- clippy_lints/src/methods/mod.rs | 28 ++++++++++--------- tests/ui/cloned_last.fixed | 8 ++++++ tests/ui/cloned_last.rs | 12 +++++++++ tests/ui/cloned_last.stderr | 36 ++++++++++++++++++++++++- 5 files changed, 76 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/methods/cloned_last.rs b/clippy_lints/src/methods/cloned_last.rs index f3aa71dab7f9..6e97407e23c6 100644 --- a/clippy_lints/src/methods/cloned_last.rs +++ b/clippy_lints/src/methods/cloned_last.rs @@ -9,7 +9,7 @@ use rustc_span::sym; use super::CLONED_LAST; /// lint use of `cloned().last()` for `Iterators` -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, recv: &'tcx hir::Expr<'_>) { +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, recv: &'tcx hir::Expr<'_>, name: &str) { // lint if caller of `.cloned().last()` is an Iterator let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) @@ -24,7 +24,11 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, rec expr.span, msg, "try this", - format!("{}.last().cloned()", iter_snippet), + if name == "count" { + format!("{}.{}()", iter_snippet, name) + } else { + format!("{}.{}().cloned()", iter_snippet, name) + }, Applicability::MachineApplicable, ); } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 0576b7e3c4d3..4a10c47dd09f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2274,11 +2274,12 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio }, _ => {}, }, - ("count", []) => match method_call!(recv) { - Some((name @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => { - iter_count::check(cx, expr, recv2, name); + (name @ "count", []) => match method_call!(recv) { + Some((name2 @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => { + iter_count::check(cx, expr, recv2, name2); }, Some(("map", [_, arg], _)) => suspicious_map::check(cx, expr, recv, arg), + Some(("cloned", [recv2], _)) => cloned_last::check(cx, expr, recv2, name), _ => {}, }, ("expect", [_]) => match method_call!(recv) { @@ -2313,9 +2314,9 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("is_none", []) => check_is_some_is_none(cx, expr, recv, false), ("is_some", []) => check_is_some_is_none(cx, expr, recv, true), ("last", []) => { - if let Some((name, [recv2, args @ ..], _span2)) = method_call!(recv) { - if let ("cloned", []) = (name, args) { - cloned_last::check(cx, expr, recv2); + if let Some((name2, [recv2, args @ ..], _span2)) = method_call!(recv) { + if let ("cloned", []) = (name2, args) { + cloned_last::check(cx, expr, recv2, name); } } }, @@ -2334,14 +2335,15 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio map_identity::check(cx, expr, recv, m_arg, span); }, ("map_or", [def, map]) => option_map_or_none::check(cx, expr, recv, def, map), - ("next", []) => { - if let Some((name, [recv, args @ ..], _)) = method_call!(recv) { - match (name, args) { - ("filter", [arg]) => filter_next::check(cx, expr, recv, arg), - ("filter_map", [arg]) => filter_map_next::check(cx, expr, recv, arg, msrv), - ("iter", []) => iter_next_slice::check(cx, expr, recv), - ("skip", [arg]) => iter_skip_next::check(cx, expr, recv, arg), + (name @ "next", []) => { + if let Some((name2, [recv2, args @ ..], _)) = method_call!(recv) { + match (name2, args) { + ("filter", [arg]) => filter_next::check(cx, expr, recv2, arg), + ("filter_map", [arg]) => filter_map_next::check(cx, expr, recv2, arg, msrv), + ("iter", []) => iter_next_slice::check(cx, expr, recv2), + ("skip", [arg]) => iter_skip_next::check(cx, expr, recv2, arg), ("skip_while", [_]) => skip_while_next::check(cx, expr), + ("cloned", []) => cloned_last::check(cx, expr, recv2, name), _ => {}, } } diff --git a/tests/ui/cloned_last.fixed b/tests/ui/cloned_last.fixed index a8e9996f3fcb..9e59ffb4665b 100644 --- a/tests/ui/cloned_last.fixed +++ b/tests/ui/cloned_last.fixed @@ -5,4 +5,12 @@ fn main() { #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().last().cloned(); + + #[rustfmt::skip] + let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter().next().cloned(); + + #[rustfmt::skip] + let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter().count(); } diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs index 348840c249b2..6afa0537d5c7 100644 --- a/tests/ui/cloned_last.rs +++ b/tests/ui/cloned_last.rs @@ -7,4 +7,16 @@ fn main() { .iter() .cloned() .last(); + + #[rustfmt::skip] + let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter() + .cloned() + .next(); + + #[rustfmt::skip] + let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter() + .cloned() + .count(); } diff --git a/tests/ui/cloned_last.stderr b/tests/ui/cloned_last.stderr index 52da6923639b..8a6a1bee2a1b 100644 --- a/tests/ui/cloned_last.stderr +++ b/tests/ui/cloned_last.stderr @@ -16,5 +16,39 @@ LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_s LL ~ .iter().last().cloned(); | -error: aborting due to previous error +error: called `cloned().last()` on an `Iterator`. It may be more efficient to call + `.last().cloned()` instead + --> $DIR/cloned_last.rs:12:33 + | +LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _________________________________^ +LL | | .iter() +LL | | .cloned() +LL | | .next(); + | |_______________^ + | +help: try this + | +LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ .iter().next().cloned(); + | + +error: called `cloned().last()` on an `Iterator`. It may be more efficient to call + `.last().cloned()` instead + --> $DIR/cloned_last.rs:18:24 + | +LL | let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | ________________________^ +LL | | .iter() +LL | | .cloned() +LL | | .count(); + | |________________^ + | +help: try this + | +LL ~ let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ .iter().count(); + | + +error: aborting due to 3 previous errors From 653314360be4c264bf7057581e9364e7a4e159c9 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 03:56:39 -0800 Subject: [PATCH 19/47] Implement PR for `skip`, `take`, `nth` --- clippy_lints/src/methods/cloned_last.rs | 18 +++++--- clippy_lints/src/methods/mod.rs | 37 ++++++++++----- tests/ui/cloned_last.fixed | 33 ++++++++++++-- tests/ui/cloned_last.rs | 21 +++++++++ tests/ui/cloned_last.stderr | 60 +++++-------------------- 5 files changed, 102 insertions(+), 67 deletions(-) diff --git a/clippy_lints/src/methods/cloned_last.rs b/clippy_lints/src/methods/cloned_last.rs index 6e97407e23c6..e4656836a9f1 100644 --- a/clippy_lints/src/methods/cloned_last.rs +++ b/clippy_lints/src/methods/cloned_last.rs @@ -9,7 +9,9 @@ use rustc_span::sym; use super::CLONED_LAST; /// lint use of `cloned().last()` for `Iterators` -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, recv: &'tcx hir::Expr<'_>, name: &str) { +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, recv: &'tcx hir::Expr<'_>, name: &str, + map_arg: &[hir::Expr<'_>], +) { // lint if caller of `.cloned().last()` is an Iterator let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) @@ -24,10 +26,16 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, rec expr.span, msg, "try this", - if name == "count" { - format!("{}.{}()", iter_snippet, name) - } else { - format!("{}.{}().cloned()", iter_snippet, name) + match name { + "count" => { + format!("{}.{}()", iter_snippet, name) + }, + "next" | "last" => { + format!("{}.{}().cloned()", iter_snippet, name) + }, + _ => { + format!("{}.{}({}).cloned()", iter_snippet, name, snippet(cx, map_arg[0].span, "..")) + }, }, Applicability::MachineApplicable, ); diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 4a10c47dd09f..a12a972a6385 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2274,12 +2274,12 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio }, _ => {}, }, - (name @ "count", []) => match method_call!(recv) { + (name @ "count", args@ []) => match method_call!(recv) { Some((name2 @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => { iter_count::check(cx, expr, recv2, name2); }, Some(("map", [_, arg], _)) => suspicious_map::check(cx, expr, recv, arg), - Some(("cloned", [recv2], _)) => cloned_last::check(cx, expr, recv2, name), + Some(("cloned", [recv2], _)) => cloned_last::check(cx, expr, recv2, name, args), _ => {}, }, ("expect", [_]) => match method_call!(recv) { @@ -2313,10 +2313,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("is_file", []) => filetype_is_file::check(cx, expr, recv), ("is_none", []) => check_is_some_is_none(cx, expr, recv, false), ("is_some", []) => check_is_some_is_none(cx, expr, recv, true), - ("last", []) => { - if let Some((name2, [recv2, args @ ..], _span2)) = method_call!(recv) { - if let ("cloned", []) = (name2, args) { - cloned_last::check(cx, expr, recv2, name); + ("last", args @ []) => { + if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call!(recv) { + if let ("cloned", []) = (name2, args2) { + cloned_last::check(cx, expr, recv2, name, args); } } }, @@ -2335,23 +2335,24 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio map_identity::check(cx, expr, recv, m_arg, span); }, ("map_or", [def, map]) => option_map_or_none::check(cx, expr, recv, def, map), - (name @ "next", []) => { - if let Some((name2, [recv2, args @ ..], _)) = method_call!(recv) { - match (name2, args) { + (name @ "next", args @ []) => { + if let Some((name2, [recv2, args2 @ ..], _)) = method_call!(recv) { + match (name2, args2) { ("filter", [arg]) => filter_next::check(cx, expr, recv2, arg), ("filter_map", [arg]) => filter_map_next::check(cx, expr, recv2, arg, msrv), ("iter", []) => iter_next_slice::check(cx, expr, recv2), ("skip", [arg]) => iter_skip_next::check(cx, expr, recv2, arg), ("skip_while", [_]) => skip_while_next::check(cx, expr), - ("cloned", []) => cloned_last::check(cx, expr, recv2, name), + ("cloned", []) => cloned_last::check(cx, expr, recv2, name, args), _ => {}, } } }, - ("nth", [n_arg]) => match method_call!(recv) { + ("nth", args @ [n_arg]) => match method_call!(recv) { Some(("bytes", [recv2], _)) => bytes_nth::check(cx, expr, recv2, n_arg), Some(("iter", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false), Some(("iter_mut", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true), + Some(("cloned", [recv2], _)) => cloned_last::check(cx, expr, recv2, name, args), _ => iter_nth_zero::check(cx, expr, recv, n_arg), }, ("ok_or_else", [arg]) => unnecessary_lazy_eval::check(cx, expr, recv, arg, "ok_or"), @@ -2360,6 +2361,13 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio unnecessary_lazy_eval::check(cx, expr, recv, arg, "or"); } }, + ("skip", args) => { + if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call!(recv) { + if let ("cloned", []) = (name2, args2) { + cloned_last::check(cx, expr, recv2, name, args); + } + } + }, ("splitn" | "rsplitn", [count_arg, pat_arg]) => { if let Some((Constant::Int(count), _)) = constant(cx, cx.typeck_results(), count_arg) { suspicious_splitn::check(cx, name, expr, recv, count); @@ -2377,6 +2385,13 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio } }, ("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg), + ("take", args) => { + if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call!(recv) { + if let ("cloned", []) = (name2, args2) { + cloned_last::check(cx, expr, recv2, name, args); + } + } + }, ("to_os_string" | "to_owned" | "to_path_buf" | "to_vec", []) => { implicit_clone::check(cx, name, expr, recv, span); }, diff --git a/tests/ui/cloned_last.fixed b/tests/ui/cloned_last.fixed index 9e59ffb4665b..5cb4a56cd44e 100644 --- a/tests/ui/cloned_last.fixed +++ b/tests/ui/cloned_last.fixed @@ -4,13 +4,40 @@ fn main() { #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter().last().cloned(); + .iter() + .cloned() + .last(); #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter().next().cloned(); + .iter() + .cloned() + .next(); #[rustfmt::skip] let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter().count(); + .iter() + .cloned() + .count(); + + #[rustfmt::skip] + let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter() + .cloned() + .take(2) + .collect(); + + #[rustfmt::skip] + let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter() + .cloned() + .skip(2) + .collect(); + + #[rustfmt::skip] + let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter() + .cloned() + .nth(2) + .collect(); } diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs index 6afa0537d5c7..5cb4a56cd44e 100644 --- a/tests/ui/cloned_last.rs +++ b/tests/ui/cloned_last.rs @@ -19,4 +19,25 @@ fn main() { .iter() .cloned() .count(); + + #[rustfmt::skip] + let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter() + .cloned() + .take(2) + .collect(); + + #[rustfmt::skip] + let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter() + .cloned() + .skip(2) + .collect(); + + #[rustfmt::skip] + let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter() + .cloned() + .nth(2) + .collect(); } diff --git a/tests/ui/cloned_last.stderr b/tests/ui/cloned_last.stderr index 8a6a1bee2a1b..5b9026e5a3ef 100644 --- a/tests/ui/cloned_last.stderr +++ b/tests/ui/cloned_last.stderr @@ -1,54 +1,18 @@ -error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().cloned()` instead - --> $DIR/cloned_last.rs:6:29 +error[E0599]: the method `collect` exists for enum `std::option::Option`, but its trait bounds were not satisfied + --> $DIR/cloned_last.rs:42:10 | -LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - | _____________________________^ -LL | | .iter() -LL | | .cloned() -LL | | .last(); - | |_______________^ +LL | .collect(); + | ^^^^^^^ method cannot be called on `std::option::Option` due to unsatisfied trait bounds | - = note: `-D clippy::cloned-last` implied by `-D warnings` -help: try this + ::: /home/pmnoxx/.rustup/toolchains/nightly-2021-12-30-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:515:1 | -LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] -LL ~ .iter().last().cloned(); +LL | pub enum Option { + | ------------------ doesn't satisfy `_: std::iter::Iterator` | + = note: the following trait bounds were not satisfied: + `std::option::Option: std::iter::Iterator` + which is required by `&mut std::option::Option: std::iter::Iterator` -error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().cloned()` instead - --> $DIR/cloned_last.rs:12:33 - | -LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - | _________________________________^ -LL | | .iter() -LL | | .cloned() -LL | | .next(); - | |_______________^ - | -help: try this - | -LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] -LL ~ .iter().next().cloned(); - | - -error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().cloned()` instead - --> $DIR/cloned_last.rs:18:24 - | -LL | let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] - | ________________________^ -LL | | .iter() -LL | | .cloned() -LL | | .count(); - | |________________^ - | -help: try this - | -LL ~ let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] -LL ~ .iter().count(); - | - -error: aborting due to 3 previous errors +error: aborting due to previous error +For more information about this error, try `rustc --explain E0599`. From ba104d6e650f03c1ecb4a2a69084c50c78319a88 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 03:57:52 -0800 Subject: [PATCH 20/47] Clippy --- clippy_lints/src/methods/cloned_last.rs | 15 ++++++++++++--- clippy_lints/src/methods/mod.rs | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods/cloned_last.rs b/clippy_lints/src/methods/cloned_last.rs index e4656836a9f1..b62a405c452f 100644 --- a/clippy_lints/src/methods/cloned_last.rs +++ b/clippy_lints/src/methods/cloned_last.rs @@ -9,8 +9,12 @@ use rustc_span::sym; use super::CLONED_LAST; /// lint use of `cloned().last()` for `Iterators` -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, recv: &'tcx hir::Expr<'_>, name: &str, - map_arg: &[hir::Expr<'_>], +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'_>, + recv: &'tcx hir::Expr<'_>, + name: &str, + map_arg: &[hir::Expr<'_>], ) { // lint if caller of `.cloned().last()` is an Iterator let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { @@ -34,7 +38,12 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, rec format!("{}.{}().cloned()", iter_snippet, name) }, _ => { - format!("{}.{}({}).cloned()", iter_snippet, name, snippet(cx, map_arg[0].span, "..")) + format!( + "{}.{}({}).cloned()", + iter_snippet, + name, + snippet(cx, map_arg[0].span, "..") + ) }, }, Applicability::MachineApplicable, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index a12a972a6385..37a4735e4813 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2274,7 +2274,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio }, _ => {}, }, - (name @ "count", args@ []) => match method_call!(recv) { + (name @ "count", args @ []) => match method_call!(recv) { Some((name2 @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => { iter_count::check(cx, expr, recv2, name2); }, From b8efdaef00116454fd0530e0f90e46b2bd75096e Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 03:59:53 -0800 Subject: [PATCH 21/47] Fix test --- tests/ui/cloned_last.fixed | 27 +++------ tests/ui/cloned_last.rs | 5 +- tests/ui/cloned_last.stderr | 111 ++++++++++++++++++++++++++++++++---- 3 files changed, 108 insertions(+), 35 deletions(-) diff --git a/tests/ui/cloned_last.fixed b/tests/ui/cloned_last.fixed index 5cb4a56cd44e..6a330d4e0899 100644 --- a/tests/ui/cloned_last.fixed +++ b/tests/ui/cloned_last.fixed @@ -4,40 +4,27 @@ fn main() { #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .last(); + .iter().last().cloned(); #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .next(); + .iter().next().cloned(); #[rustfmt::skip] let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .count(); + .iter().count(); #[rustfmt::skip] let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .take(2) + .iter().take(2).cloned() .collect(); #[rustfmt::skip] let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .skip(2) + .iter().skip(2).cloned() .collect(); #[rustfmt::skip] - let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .nth(2) - .collect(); + let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] + .iter().nth(2).cloned(); } diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs index 5cb4a56cd44e..179bce5c2849 100644 --- a/tests/ui/cloned_last.rs +++ b/tests/ui/cloned_last.rs @@ -35,9 +35,8 @@ fn main() { .collect(); #[rustfmt::skip] - let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() - .nth(2) - .collect(); + .nth(2); } diff --git a/tests/ui/cloned_last.stderr b/tests/ui/cloned_last.stderr index 5b9026e5a3ef..f630edf8837c 100644 --- a/tests/ui/cloned_last.stderr +++ b/tests/ui/cloned_last.stderr @@ -1,18 +1,105 @@ -error[E0599]: the method `collect` exists for enum `std::option::Option`, but its trait bounds were not satisfied - --> $DIR/cloned_last.rs:42:10 +error: called `cloned().last()` on an `Iterator`. It may be more efficient to call + `.last().cloned()` instead + --> $DIR/cloned_last.rs:6:29 | -LL | .collect(); - | ^^^^^^^ method cannot be called on `std::option::Option` due to unsatisfied trait bounds +LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _____________________________^ +LL | | .iter() +LL | | .cloned() +LL | | .last(); + | |_______________^ | - ::: /home/pmnoxx/.rustup/toolchains/nightly-2021-12-30-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:515:1 + = note: `-D clippy::cloned-last` implied by `-D warnings` +help: try this | -LL | pub enum Option { - | ------------------ doesn't satisfy `_: std::iter::Iterator` +LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ .iter().last().cloned(); | - = note: the following trait bounds were not satisfied: - `std::option::Option: std::iter::Iterator` - which is required by `&mut std::option::Option: std::iter::Iterator` -error: aborting due to previous error +error: called `cloned().last()` on an `Iterator`. It may be more efficient to call + `.last().cloned()` instead + --> $DIR/cloned_last.rs:12:33 + | +LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _________________________________^ +LL | | .iter() +LL | | .cloned() +LL | | .next(); + | |_______________^ + | +help: try this + | +LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ .iter().next().cloned(); + | + +error: called `cloned().last()` on an `Iterator`. It may be more efficient to call + `.last().cloned()` instead + --> $DIR/cloned_last.rs:18:24 + | +LL | let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | ________________________^ +LL | | .iter() +LL | | .cloned() +LL | | .count(); + | |________________^ + | +help: try this + | +LL ~ let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ .iter().count(); + | + +error: called `cloned().last()` on an `Iterator`. It may be more efficient to call + `.last().cloned()` instead + --> $DIR/cloned_last.rs:24:25 + | +LL | let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _________________________^ +LL | | .iter() +LL | | .cloned() +LL | | .take(2) + | |________________^ + | +help: try this + | +LL ~ let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL + .iter().take(2).cloned() + | + +error: called `cloned().last()` on an `Iterator`. It may be more efficient to call + `.last().cloned()` instead + --> $DIR/cloned_last.rs:31:25 + | +LL | let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _________________________^ +LL | | .iter() +LL | | .cloned() +LL | | .skip(2) + | |________________^ + | +help: try this + | +LL ~ let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL + .iter().skip(2).cloned() + | + +error: called `cloned().last()` on an `Iterator`. It may be more efficient to call + `.last().cloned()` instead + --> $DIR/cloned_last.rs:38:17 + | +LL | let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _________________^ +LL | | .iter() +LL | | .cloned() +LL | | .nth(2); + | |_______________^ + | +help: try this + | +LL ~ let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ .iter().nth(2).cloned(); + | + +error: aborting due to 6 previous errors -For more information about this error, try `rustc --explain E0599`. From 6cda20da7e48c1cb5f999ddaed709850741d1a2d Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 04:01:52 -0800 Subject: [PATCH 22/47] Apply feedback --- tests/ui/cloned_last.fixed | 6 ------ tests/ui/cloned_last.rs | 6 ------ tests/ui/cloned_last.stderr | 12 ++++++------ 3 files changed, 6 insertions(+), 18 deletions(-) diff --git a/tests/ui/cloned_last.fixed b/tests/ui/cloned_last.fixed index 6a330d4e0899..86d7001d3360 100644 --- a/tests/ui/cloned_last.fixed +++ b/tests/ui/cloned_last.fixed @@ -2,29 +2,23 @@ #![warn(clippy::all)] fn main() { - #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().last().cloned(); - #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().next().cloned(); - #[rustfmt::skip] let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().count(); - #[rustfmt::skip] let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().take(2).cloned() .collect(); - #[rustfmt::skip] let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().skip(2).cloned() .collect(); - #[rustfmt::skip] let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().nth(2).cloned(); } diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs index 179bce5c2849..dbfd0499811b 100644 --- a/tests/ui/cloned_last.rs +++ b/tests/ui/cloned_last.rs @@ -2,39 +2,33 @@ #![warn(clippy::all)] fn main() { - #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() .last(); - #[rustfmt::skip] let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() .next(); - #[rustfmt::skip] let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() .count(); - #[rustfmt::skip] let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() .take(2) .collect(); - #[rustfmt::skip] let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() .skip(2) .collect(); - #[rustfmt::skip] let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() diff --git a/tests/ui/cloned_last.stderr b/tests/ui/cloned_last.stderr index f630edf8837c..0aaa58fddfaa 100644 --- a/tests/ui/cloned_last.stderr +++ b/tests/ui/cloned_last.stderr @@ -1,6 +1,6 @@ error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:6:29 + --> $DIR/cloned_last.rs:5:29 | LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] | _____________________________^ @@ -18,7 +18,7 @@ LL ~ .iter().last().cloned(); error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:12:33 + --> $DIR/cloned_last.rs:10:33 | LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] | _________________________________^ @@ -35,7 +35,7 @@ LL ~ .iter().next().cloned(); error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:18:24 + --> $DIR/cloned_last.rs:15:24 | LL | let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] | ________________________^ @@ -52,7 +52,7 @@ LL ~ .iter().count(); error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:24:25 + --> $DIR/cloned_last.rs:20:25 | LL | let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] | _________________________^ @@ -69,7 +69,7 @@ LL + .iter().take(2).cloned() error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:31:25 + --> $DIR/cloned_last.rs:26:25 | LL | let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] | _________________________^ @@ -86,7 +86,7 @@ LL + .iter().skip(2).cloned() error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:38:17 + --> $DIR/cloned_last.rs:32:17 | LL | let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] | _________________^ From 67f0d929d2b768f8afb3dc038e39c73410ed63f9 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 04:09:42 -0800 Subject: [PATCH 23/47] Cleanup --- clippy_lints/src/methods/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 37a4735e4813..12cd966d0cd0 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2275,11 +2275,11 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio _ => {}, }, (name @ "count", args @ []) => match method_call!(recv) { + Some(("cloned", [recv2], _)) => cloned_last::check(cx, expr, recv2, name, args), Some((name2 @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => { iter_count::check(cx, expr, recv2, name2); }, Some(("map", [_, arg], _)) => suspicious_map::check(cx, expr, recv, arg), - Some(("cloned", [recv2], _)) => cloned_last::check(cx, expr, recv2, name, args), _ => {}, }, ("expect", [_]) => match method_call!(recv) { @@ -2338,21 +2338,21 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio (name @ "next", args @ []) => { if let Some((name2, [recv2, args2 @ ..], _)) = method_call!(recv) { match (name2, args2) { + ("cloned", []) => cloned_last::check(cx, expr, recv2, name, args), ("filter", [arg]) => filter_next::check(cx, expr, recv2, arg), ("filter_map", [arg]) => filter_map_next::check(cx, expr, recv2, arg, msrv), ("iter", []) => iter_next_slice::check(cx, expr, recv2), ("skip", [arg]) => iter_skip_next::check(cx, expr, recv2, arg), ("skip_while", [_]) => skip_while_next::check(cx, expr), - ("cloned", []) => cloned_last::check(cx, expr, recv2, name, args), _ => {}, } } }, ("nth", args @ [n_arg]) => match method_call!(recv) { Some(("bytes", [recv2], _)) => bytes_nth::check(cx, expr, recv2, n_arg), + Some(("cloned", [recv2], _)) => cloned_last::check(cx, expr, recv2, name, args), Some(("iter", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false), Some(("iter_mut", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true), - Some(("cloned", [recv2], _)) => cloned_last::check(cx, expr, recv2, name, args), _ => iter_nth_zero::check(cx, expr, recv, n_arg), }, ("ok_or_else", [arg]) => unnecessary_lazy_eval::check(cx, expr, recv, arg, "ok_or"), From abef1d2098c2722bcd7cdbdbe42a04d3ed63b6a6 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 04:22:31 -0800 Subject: [PATCH 24/47] cargo dev fmt --- tests/ui/cloned_last.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs index dbfd0499811b..8a89e73b9b8d 100644 --- a/tests/ui/cloned_last.rs +++ b/tests/ui/cloned_last.rs @@ -7,29 +7,29 @@ fn main() { .cloned() .last(); - let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() .next(); - let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() .count(); - let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() .take(2) .collect(); - let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() .skip(2) .collect(); - let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter() .cloned() .nth(2); From 47d58f3c428e7ef473291adb107a88eecdde94f4 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 04:46:38 -0800 Subject: [PATCH 25/47] Run formatter --- tests/ui/cloned_last.fixed | 10 +++++----- tests/ui/cloned_last.stderr | 40 ++++++++++++++++++------------------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/tests/ui/cloned_last.fixed b/tests/ui/cloned_last.fixed index 86d7001d3360..43044e47ed51 100644 --- a/tests/ui/cloned_last.fixed +++ b/tests/ui/cloned_last.fixed @@ -5,20 +5,20 @@ fn main() { let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().last().cloned(); - let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().next().cloned(); - let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().count(); - let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().take(2).cloned() .collect(); - let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().skip(2).cloned() .collect(); - let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] .iter().nth(2).cloned(); } diff --git a/tests/ui/cloned_last.stderr b/tests/ui/cloned_last.stderr index 0aaa58fddfaa..0d2e162ef299 100644 --- a/tests/ui/cloned_last.stderr +++ b/tests/ui/cloned_last.stderr @@ -18,10 +18,10 @@ LL ~ .iter().last().cloned(); error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:10:33 + --> $DIR/cloned_last.rs:10:29 | -LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - | _________________________________^ +LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _____________________________^ LL | | .iter() LL | | .cloned() LL | | .next(); @@ -29,16 +29,16 @@ LL | | .next(); | help: try this | -LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] LL ~ .iter().next().cloned(); | error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:15:24 + --> $DIR/cloned_last.rs:15:20 | -LL | let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] - | ________________________^ +LL | let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | ____________________^ LL | | .iter() LL | | .cloned() LL | | .count(); @@ -46,16 +46,16 @@ LL | | .count(); | help: try this | -LL ~ let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] LL ~ .iter().count(); | error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:20:25 + --> $DIR/cloned_last.rs:20:21 | -LL | let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] - | _________________________^ +LL | let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _____________________^ LL | | .iter() LL | | .cloned() LL | | .take(2) @@ -63,16 +63,16 @@ LL | | .take(2) | help: try this | -LL ~ let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] LL + .iter().take(2).cloned() | error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:26:25 + --> $DIR/cloned_last.rs:26:21 | -LL | let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] - | _________________________^ +LL | let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _____________________^ LL | | .iter() LL | | .cloned() LL | | .skip(2) @@ -80,16 +80,16 @@ LL | | .skip(2) | help: try this | -LL ~ let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] LL + .iter().skip(2).cloned() | error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:32:17 + --> $DIR/cloned_last.rs:32:13 | -LL | let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] - | _________________^ +LL | let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] + | _____________^ LL | | .iter() LL | | .cloned() LL | | .nth(2); @@ -97,7 +97,7 @@ LL | | .nth(2); | help: try this | -LL ~ let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] LL ~ .iter().nth(2).cloned(); | From 9d641ec3ca17fbcc0dc9bd0fde13ea0c7a7a9186 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 06:03:39 -0800 Subject: [PATCH 26/47] Cleanup --- tests/ui/cloned_last.fixed | 14 ++++++++------ tests/ui/cloned_last.rs | 34 ++++++++-------------------------- tests/ui/cloned_last.stderr | 36 ++++++++++++++++++------------------ 3 files changed, 34 insertions(+), 50 deletions(-) diff --git a/tests/ui/cloned_last.fixed b/tests/ui/cloned_last.fixed index 43044e47ed51..c516e047a80f 100644 --- a/tests/ui/cloned_last.fixed +++ b/tests/ui/cloned_last.fixed @@ -2,23 +2,25 @@ #![warn(clippy::all)] fn main() { - let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()]; + + let _: Option = vec .iter().last().cloned(); - let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: Option = vec .iter().next().cloned(); - let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: usize = vec .iter().count(); - let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: Vec<_> = vec .iter().take(2).cloned() .collect(); - let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _: Vec<_> = vec .iter().skip(2).cloned() .collect(); - let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] + let _ = vec .iter().nth(2).cloned(); } diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs index 8a89e73b9b8d..33d56d66c774 100644 --- a/tests/ui/cloned_last.rs +++ b/tests/ui/cloned_last.rs @@ -2,35 +2,17 @@ #![warn(clippy::all)] fn main() { - let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .last(); + let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()]; - let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .next(); + let _: Option = vec.iter().cloned().last(); - let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .count(); + let _: Option = vec.iter().cloned().next(); - let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .take(2) - .collect(); + let _: usize = vec.iter().cloned().count(); - let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .skip(2) - .collect(); + let _: Vec<_> = vec.iter().cloned().take(2).collect(); - let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] - .iter() - .cloned() - .nth(2); + let _: Vec<_> = vec.iter().cloned().skip(2).collect(); + + let _ = vec.iter().cloned().nth(2); } diff --git a/tests/ui/cloned_last.stderr b/tests/ui/cloned_last.stderr index 0d2e162ef299..4651607bc682 100644 --- a/tests/ui/cloned_last.stderr +++ b/tests/ui/cloned_last.stderr @@ -1,8 +1,8 @@ error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:5:29 + --> $DIR/cloned_last.rs:7:29 | -LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL | let _: Option = vec | _____________________________^ LL | | .iter() LL | | .cloned() @@ -12,15 +12,15 @@ LL | | .last(); = note: `-D clippy::cloned-last` implied by `-D warnings` help: try this | -LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _: Option = vec LL ~ .iter().last().cloned(); | error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:10:29 + --> $DIR/cloned_last.rs:12:29 | -LL | let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL | let _: Option = vec | _____________________________^ LL | | .iter() LL | | .cloned() @@ -29,15 +29,15 @@ LL | | .next(); | help: try this | -LL ~ let _: Option = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _: Option = vec LL ~ .iter().next().cloned(); | error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:15:20 + --> $DIR/cloned_last.rs:17:20 | -LL | let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL | let _: usize = vec | ____________________^ LL | | .iter() LL | | .cloned() @@ -46,15 +46,15 @@ LL | | .count(); | help: try this | -LL ~ let _: usize = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _: usize = vec LL ~ .iter().count(); | error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:20:21 + --> $DIR/cloned_last.rs:22:21 | -LL | let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL | let _: Vec<_> = vec | _____________________^ LL | | .iter() LL | | .cloned() @@ -63,15 +63,15 @@ LL | | .take(2) | help: try this | -LL ~ let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _: Vec<_> = vec LL + .iter().take(2).cloned() | error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:26:21 + --> $DIR/cloned_last.rs:28:21 | -LL | let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL | let _: Vec<_> = vec | _____________________^ LL | | .iter() LL | | .cloned() @@ -80,15 +80,15 @@ LL | | .skip(2) | help: try this | -LL ~ let _: Vec<_> = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _: Vec<_> = vec LL + .iter().skip(2).cloned() | error: called `cloned().last()` on an `Iterator`. It may be more efficient to call `.last().cloned()` instead - --> $DIR/cloned_last.rs:32:13 + --> $DIR/cloned_last.rs:34:13 | -LL | let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL | let _ = vec | _____________^ LL | | .iter() LL | | .cloned() @@ -97,7 +97,7 @@ LL | | .nth(2); | help: try this | -LL ~ let _ = vec!["1".to_string(), "2".to_string(), "3".to_string()] +LL ~ let _ = vec LL ~ .iter().nth(2).cloned(); | From db9b13937c9a9f33a5df1f95948aa2b5b5f0a0dc Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 07:13:11 -0800 Subject: [PATCH 27/47] Update documentation --- CHANGELOG.md | 2 +- clippy_lints/src/lib.register_all.rs | 2 +- clippy_lints/src/lib.register_complexity.rs | 2 +- clippy_lints/src/lib.register_lints.rs | 2 +- ...loned_last.rs => iter_overeager_cloned.rs} | 35 +++++- clippy_lints/src/methods/mod.rs | 44 +++++--- tests/ui/cloned_last.fixed | 26 ----- tests/ui/cloned_last.rs | 18 --- tests/ui/cloned_last.stderr | 105 ------------------ tests/ui/iter_overeager_cloned.fixed | 33 ++++++ tests/ui/iter_overeager_cloned.rs | 33 ++++++ tests/ui/iter_overeager_cloned.stderr | 48 ++++++++ 12 files changed, 174 insertions(+), 176 deletions(-) rename clippy_lints/src/methods/{cloned_last.rs => iter_overeager_cloned.rs} (59%) delete mode 100644 tests/ui/cloned_last.fixed delete mode 100644 tests/ui/cloned_last.rs delete mode 100644 tests/ui/cloned_last.stderr create mode 100644 tests/ui/iter_overeager_cloned.fixed create mode 100644 tests/ui/iter_overeager_cloned.rs create mode 100644 tests/ui/iter_overeager_cloned.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 67ba73fd998b..10e6fa74a79a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2911,7 +2911,6 @@ Released 2018-09-13 [`clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy [`clone_on_ref_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_ref_ptr [`cloned_instead_of_copied`]: https://rust-lang.github.io/rust-clippy/master/index.html#cloned_instead_of_copied -[`cloned_last`]: https://rust-lang.github.io/rust-clippy/master/index.html#cloned_last [`cmp_nan`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_nan [`cmp_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_null [`cmp_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned @@ -3049,6 +3048,7 @@ Released 2018-09-13 [`iter_not_returning_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_not_returning_iterator [`iter_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth [`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero +[`iter_overeager_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_overager_cloned [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next [`iterator_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iterator_step_by_zero [`just_underscores_and_digits`]: https://rust-lang.github.io/rust-clippy/master/index.html#just_underscores_and_digits diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 2a489a8903fa..af2bcf3d8c01 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -138,7 +138,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(methods::BYTES_NTH), LintId::of(methods::CHARS_LAST_CMP), LintId::of(methods::CHARS_NEXT_CMP), - LintId::of(methods::CLONED_LAST), + LintId::of(methods::ITER_OVEREAGER_CLONED), LintId::of(methods::CLONE_DOUBLE_REF), LintId::of(methods::CLONE_ON_COPY), LintId::of(methods::EXPECT_FUN_CALL), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index 21de9d65a8d1..e4afa5cc0c35 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -32,7 +32,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(matches::MATCH_SINGLE_BINDING), LintId::of(matches::WILDCARD_IN_OR_PATTERNS), LintId::of(methods::BIND_INSTEAD_OF_MAP), - LintId::of(methods::CLONED_LAST), + LintId::of(methods::ITER_OVEREAGER_CLONED), LintId::of(methods::CLONE_ON_COPY), LintId::of(methods::FILTER_MAP_IDENTITY), LintId::of(methods::FILTER_NEXT), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index b94ddac924c7..9a2300b073a8 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -262,7 +262,7 @@ store.register_lints(&[ methods::CHARS_LAST_CMP, methods::CHARS_NEXT_CMP, methods::CLONED_INSTEAD_OF_COPIED, - methods::CLONED_LAST, + methods::ITER_OVEREAGER_CLONED, methods::CLONE_DOUBLE_REF, methods::CLONE_ON_COPY, methods::CLONE_ON_REF_PTR, diff --git a/clippy_lints/src/methods/cloned_last.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs similarity index 59% rename from clippy_lints/src/methods/cloned_last.rs rename to clippy_lints/src/methods/iter_overeager_cloned.rs index b62a405c452f..0133dbc74abf 100644 --- a/clippy_lints/src/methods/cloned_last.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -6,7 +6,7 @@ use rustc_hir as hir; use rustc_lint::LateContext; use rustc_span::sym; -use super::CLONED_LAST; +use super::ITER_OVEREAGER_CLONED; /// lint use of `cloned().last()` for `Iterators` pub(super) fn check<'tcx>( @@ -21,14 +21,39 @@ pub(super) fn check<'tcx>( implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) }); if recv_impls_iterator { - let msg = "called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().cloned()` instead"; + let (lint, msg) = match name { + "count" => ( + ITER_OVEREAGER_CLONED, + format!( + "called `cloned().{}()` on an `Iterator`. It may be more efficient to call\ + `.{}()` instead", + name, name + ), + ), + "next" | "last" => ( + ITER_OVEREAGER_CLONED, + format!( + "called `cloned().{}()` on an `Iterator`. It may be more efficient to call\ + `.{}().cloned()` instead", + name, name + ), + ), + _ => ( + ITER_OVEREAGER_CLONED, + format!( + "called `cloned().{}(...)` on an `Iterator`. It may be more efficient to call\ + `.{}(...).cloned()` instead", + name, name + ), + ), + }; + let iter_snippet = snippet(cx, recv.span, ".."); span_lint_and_sugg( cx, - CLONED_LAST, + lint, expr.span, - msg, + &msg, "try this", match name { "count" => { diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 12cd966d0cd0..f476a7b67ef0 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -9,7 +9,6 @@ mod chars_next_cmp_with_unwrap; mod clone_on_copy; mod clone_on_ref_ptr; mod cloned_instead_of_copied; -mod cloned_last; mod expect_fun_call; mod expect_used; mod extend_with_drain; @@ -31,6 +30,7 @@ mod iter_count; mod iter_next_slice; mod iter_nth; mod iter_nth_zero; +mod iter_overeager_cloned; mod iter_skip_next; mod iterator_step_by_zero; mod manual_saturating_arithmetic; @@ -110,25 +110,33 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for usage of `_.cloned().last()`. + /// Checks for usage of `_.cloned().()` where call to `.cloned()` can be postponed, + /// or often removed entirely. /// /// ### Why is this bad? - /// Performance, clone only need to be executed once. + /// It's often inefficient to clone all elements of an iterator, when eventually, only some + /// of them will be consumed. /// - /// ### Example + /// ### Examples /// ```rust /// # let vec = vec!["string".to_string()]; + /// + /// // Bad + /// vec.iter().cloned().take(10); + /// + /// // Good + /// vec.iter().take(10).cloned(); + /// + /// // Bad /// vec.iter().cloned().last(); - /// ``` - /// Could be written as - /// ```rust - /// # let vec = vec!["string".to_string()]; + /// + /// // Good /// vec.iter().last().cloned(); /// ``` #[clippy::version = "1.59.0"] - pub CLONED_LAST, - complexity, - "using `cloned().last()`, which is less efficient than `.last().cloned()`" + pub ITER_OVEREAGER_CLONED, + perf, + "using `cloned()` early with `Iterator::iter()` can lead to some performance inefficiencies" } declare_clippy_lint! { @@ -1970,7 +1978,7 @@ impl_lint_pass!(Methods => [ CLONE_ON_COPY, CLONE_ON_REF_PTR, CLONE_DOUBLE_REF, - CLONED_LAST, + ITER_OVEREAGER_CLONED, CLONED_INSTEAD_OF_COPIED, FLAT_MAP_OPTION, INEFFICIENT_TO_STRING, @@ -2275,7 +2283,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio _ => {}, }, (name @ "count", args @ []) => match method_call!(recv) { - Some(("cloned", [recv2], _)) => cloned_last::check(cx, expr, recv2, name, args), + Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args), Some((name2 @ ("into_iter" | "iter" | "iter_mut"), [recv2], _)) => { iter_count::check(cx, expr, recv2, name2); }, @@ -2316,7 +2324,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("last", args @ []) => { if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call!(recv) { if let ("cloned", []) = (name2, args2) { - cloned_last::check(cx, expr, recv2, name, args); + iter_overeager_cloned::check(cx, expr, recv2, name, args); } } }, @@ -2338,7 +2346,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio (name @ "next", args @ []) => { if let Some((name2, [recv2, args2 @ ..], _)) = method_call!(recv) { match (name2, args2) { - ("cloned", []) => cloned_last::check(cx, expr, recv2, name, args), + ("cloned", []) => iter_overeager_cloned::check(cx, expr, recv2, name, args), ("filter", [arg]) => filter_next::check(cx, expr, recv2, arg), ("filter_map", [arg]) => filter_map_next::check(cx, expr, recv2, arg, msrv), ("iter", []) => iter_next_slice::check(cx, expr, recv2), @@ -2350,7 +2358,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio }, ("nth", args @ [n_arg]) => match method_call!(recv) { Some(("bytes", [recv2], _)) => bytes_nth::check(cx, expr, recv2, n_arg), - Some(("cloned", [recv2], _)) => cloned_last::check(cx, expr, recv2, name, args), + Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args), Some(("iter", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false), Some(("iter_mut", [recv2], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true), _ => iter_nth_zero::check(cx, expr, recv, n_arg), @@ -2364,7 +2372,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("skip", args) => { if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call!(recv) { if let ("cloned", []) = (name2, args2) { - cloned_last::check(cx, expr, recv2, name, args); + iter_overeager_cloned::check(cx, expr, recv2, name, args); } } }, @@ -2388,7 +2396,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("take", args) => { if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call!(recv) { if let ("cloned", []) = (name2, args2) { - cloned_last::check(cx, expr, recv2, name, args); + iter_overeager_cloned::check(cx, expr, recv2, name, args); } } }, diff --git a/tests/ui/cloned_last.fixed b/tests/ui/cloned_last.fixed deleted file mode 100644 index c516e047a80f..000000000000 --- a/tests/ui/cloned_last.fixed +++ /dev/null @@ -1,26 +0,0 @@ -// run-rustfix -#![warn(clippy::all)] - -fn main() { - let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()]; - - let _: Option = vec - .iter().last().cloned(); - - let _: Option = vec - .iter().next().cloned(); - - let _: usize = vec - .iter().count(); - - let _: Vec<_> = vec - .iter().take(2).cloned() - .collect(); - - let _: Vec<_> = vec - .iter().skip(2).cloned() - .collect(); - - let _ = vec - .iter().nth(2).cloned(); -} diff --git a/tests/ui/cloned_last.rs b/tests/ui/cloned_last.rs deleted file mode 100644 index 33d56d66c774..000000000000 --- a/tests/ui/cloned_last.rs +++ /dev/null @@ -1,18 +0,0 @@ -// run-rustfix -#![warn(clippy::all)] - -fn main() { - let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()]; - - let _: Option = vec.iter().cloned().last(); - - let _: Option = vec.iter().cloned().next(); - - let _: usize = vec.iter().cloned().count(); - - let _: Vec<_> = vec.iter().cloned().take(2).collect(); - - let _: Vec<_> = vec.iter().cloned().skip(2).collect(); - - let _ = vec.iter().cloned().nth(2); -} diff --git a/tests/ui/cloned_last.stderr b/tests/ui/cloned_last.stderr deleted file mode 100644 index 4651607bc682..000000000000 --- a/tests/ui/cloned_last.stderr +++ /dev/null @@ -1,105 +0,0 @@ -error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().cloned()` instead - --> $DIR/cloned_last.rs:7:29 - | -LL | let _: Option = vec - | _____________________________^ -LL | | .iter() -LL | | .cloned() -LL | | .last(); - | |_______________^ - | - = note: `-D clippy::cloned-last` implied by `-D warnings` -help: try this - | -LL ~ let _: Option = vec -LL ~ .iter().last().cloned(); - | - -error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().cloned()` instead - --> $DIR/cloned_last.rs:12:29 - | -LL | let _: Option = vec - | _____________________________^ -LL | | .iter() -LL | | .cloned() -LL | | .next(); - | |_______________^ - | -help: try this - | -LL ~ let _: Option = vec -LL ~ .iter().next().cloned(); - | - -error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().cloned()` instead - --> $DIR/cloned_last.rs:17:20 - | -LL | let _: usize = vec - | ____________________^ -LL | | .iter() -LL | | .cloned() -LL | | .count(); - | |________________^ - | -help: try this - | -LL ~ let _: usize = vec -LL ~ .iter().count(); - | - -error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().cloned()` instead - --> $DIR/cloned_last.rs:22:21 - | -LL | let _: Vec<_> = vec - | _____________________^ -LL | | .iter() -LL | | .cloned() -LL | | .take(2) - | |________________^ - | -help: try this - | -LL ~ let _: Vec<_> = vec -LL + .iter().take(2).cloned() - | - -error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().cloned()` instead - --> $DIR/cloned_last.rs:28:21 - | -LL | let _: Vec<_> = vec - | _____________________^ -LL | | .iter() -LL | | .cloned() -LL | | .skip(2) - | |________________^ - | -help: try this - | -LL ~ let _: Vec<_> = vec -LL + .iter().skip(2).cloned() - | - -error: called `cloned().last()` on an `Iterator`. It may be more efficient to call - `.last().cloned()` instead - --> $DIR/cloned_last.rs:34:13 - | -LL | let _ = vec - | _____________^ -LL | | .iter() -LL | | .cloned() -LL | | .nth(2); - | |_______________^ - | -help: try this - | -LL ~ let _ = vec -LL ~ .iter().nth(2).cloned(); - | - -error: aborting due to 6 previous errors - diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed new file mode 100644 index 000000000000..59dc582f8e9e --- /dev/null +++ b/tests/ui/iter_overeager_cloned.fixed @@ -0,0 +1,33 @@ +// run-rustfix +#![warn(clippy::iter_overeager_cloned)] + +fn main() { + let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()]; + + let _: Option = vec.iter().last().cloned(); + + let _: Option = vec.iter().next().cloned(); + + let _: usize = vec.iter().count(); + + let _: Vec<_> = vec.iter().take(2).cloned().collect(); + + let _: Vec<_> = vec.iter().skip(2).cloned().collect(); + + let _ = vec.iter().nth(2).cloned(); + + // Not implemented yet + let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); + + // Not implemented yet + let _ = vec.iter().cloned().map(|x| x.len()); + + // This would fail if changed. + let _ = vec.iter().cloned().map(|x| x + "2"); + + // Not implemented yet + let _ = vec.iter().cloned().find(|x| x == "2"); + + // Not implemented yet + let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); +} diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs new file mode 100644 index 000000000000..9e9de8e0afb7 --- /dev/null +++ b/tests/ui/iter_overeager_cloned.rs @@ -0,0 +1,33 @@ +// run-rustfix +#![warn(clippy::iter_overeager_cloned)] + +fn main() { + let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()]; + + let _: Option = vec.iter().cloned().last(); + + let _: Option = vec.iter().cloned().next(); + + let _: usize = vec.iter().cloned().count(); + + let _: Vec<_> = vec.iter().cloned().take(2).collect(); + + let _: Vec<_> = vec.iter().cloned().skip(2).collect(); + + let _ = vec.iter().cloned().nth(2); + + // Not implemented yet + let _ = vec.iter().cloned().filter(|x| x.starts_with("2")); + + // Not implemented yet + let _ = vec.iter().cloned().map(|x| x.len()); + + // This would fail if changed. + let _ = vec.iter().cloned().map(|x| x + "2"); + + // Not implemented yet + let _ = vec.iter().cloned().find(|x| x == "2"); + + // Not implemented yet + let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); +} diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr new file mode 100644 index 000000000000..5ac372a0726a --- /dev/null +++ b/tests/ui/iter_overeager_cloned.stderr @@ -0,0 +1,48 @@ +error: called `cloned().last()` on an `Iterator`. It may be more efficient to call`.last().cloned()` instead + --> $DIR/iter_overeager_cloned.rs:7:29 + | +LL | let _: Option = vec.iter().cloned().last(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().last().cloned()` + | + = note: `-D clippy::iter-overeager-cloned` implied by `-D warnings` + +error: called `cloned().next()` on an `Iterator`. It may be more efficient to call`.next().cloned()` instead + --> $DIR/iter_overeager_cloned.rs:9:29 + | +LL | let _: Option = vec.iter().cloned().next(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().next().cloned()` + +error: called `cloned().count()` on an `Iterator`. It may be more efficient to call`.count()` instead + --> $DIR/iter_overeager_cloned.rs:11:20 + | +LL | let _: usize = vec.iter().cloned().count(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().count()` + +error: called `cloned().take(...)` on an `Iterator`. It may be more efficient to call`.take(...).cloned()` instead + --> $DIR/iter_overeager_cloned.rs:13:21 + | +LL | let _: Vec<_> = vec.iter().cloned().take(2).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().take(2).cloned()` + +error: called `cloned().skip(...)` on an `Iterator`. It may be more efficient to call`.skip(...).cloned()` instead + --> $DIR/iter_overeager_cloned.rs:15:21 + | +LL | let _: Vec<_> = vec.iter().cloned().skip(2).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().skip(2).cloned()` + +error: called `cloned().nth(...)` on an `Iterator`. It may be more efficient to call`.nth(...).cloned()` instead + --> $DIR/iter_overeager_cloned.rs:17:13 + | +LL | let _ = vec.iter().cloned().nth(2); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().nth(2).cloned()` + +error: single-character string constant used as pattern + --> $DIR/iter_overeager_cloned.rs:20:58 + | +LL | let _ = vec.iter().cloned().filter(|x| x.starts_with("2")); + | ^^^ help: try using a `char` instead: `'2'` + | + = note: `-D clippy::single-char-pattern` implied by `-D warnings` + +error: aborting due to 7 previous errors + From 9a6827654bf6ac3a017b09e5cb6699d8c1efab9e Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 07:19:59 -0800 Subject: [PATCH 28/47] Add `Known Problems` section --- clippy_lints/src/methods/mod.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f476a7b67ef0..25ea88dc303f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -132,8 +132,13 @@ declare_clippy_lint! { /// /// // Good /// vec.iter().last().cloned(); + /// /// ``` - #[clippy::version = "1.59.0"] + /// ### Known Problems + /// This `lint` removes the side of effect of cloning items in the iterator. + /// A code that relies on that side-effect could fail. + /// + /// #[clippy::version = "1.59.0"] pub ITER_OVEREAGER_CLONED, perf, "using `cloned()` early with `Iterator::iter()` can lead to some performance inefficiencies" From 1fd8c92ed5981b33a0a94595561436223ed253a0 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 07:24:50 -0800 Subject: [PATCH 29/47] return redundant clone in cause of `cloned.count` --- clippy_lints/src/methods/iter_overeager_cloned.rs | 3 ++- tests/ui/iter_overeager_cloned.fixed | 2 +- tests/ui/iter_overeager_cloned.rs | 2 +- tests/ui/iter_overeager_cloned.stderr | 2 ++ 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 0133dbc74abf..838c035d089a 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -7,6 +7,7 @@ use rustc_lint::LateContext; use rustc_span::sym; use super::ITER_OVEREAGER_CLONED; +use crate::redundant_clone::REDUNDANT_CLONE; /// lint use of `cloned().last()` for `Iterators` pub(super) fn check<'tcx>( @@ -23,7 +24,7 @@ pub(super) fn check<'tcx>( if recv_impls_iterator { let (lint, msg) = match name { "count" => ( - ITER_OVEREAGER_CLONED, + REDUNDANT_CLONE, format!( "called `cloned().{}()` on an `Iterator`. It may be more efficient to call\ `.{}()` instead", diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index 59dc582f8e9e..39ce623a3b4e 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -1,5 +1,5 @@ // run-rustfix -#![warn(clippy::iter_overeager_cloned)] +#![warn(clippy::iter_overeager_cloned, clippy::redundant_clone)] fn main() { let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()]; diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index 9e9de8e0afb7..5cc54450b973 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -1,5 +1,5 @@ // run-rustfix -#![warn(clippy::iter_overeager_cloned)] +#![warn(clippy::iter_overeager_cloned, clippy::redundant_clone)] fn main() { let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()]; diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index 5ac372a0726a..0c976d760d6c 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -17,6 +17,8 @@ error: called `cloned().count()` on an `Iterator`. It may be more efficient to c | LL | let _: usize = vec.iter().cloned().count(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().count()` + | + = note: `-D clippy::redundant-clone` implied by `-D warnings` error: called `cloned().take(...)` on an `Iterator`. It may be more efficient to call`.take(...).cloned()` instead --> $DIR/iter_overeager_cloned.rs:13:21 From 775052ba92383a668f73bae50e8825826bb345ae Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 07:28:29 -0800 Subject: [PATCH 30/47] Update lints --- CHANGELOG.md | 2 +- clippy_lints/src/lib.register_all.rs | 2 +- clippy_lints/src/lib.register_complexity.rs | 1 - clippy_lints/src/lib.register_lints.rs | 2 +- clippy_lints/src/lib.register_perf.rs | 1 + 5 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10e6fa74a79a..cf6140c1ab89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3048,7 +3048,7 @@ Released 2018-09-13 [`iter_not_returning_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_not_returning_iterator [`iter_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth [`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero -[`iter_overeager_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_overager_cloned +[`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 [`iterator_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iterator_step_by_zero [`just_underscores_and_digits`]: https://rust-lang.github.io/rust-clippy/master/index.html#just_underscores_and_digits diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index af2bcf3d8c01..e3d765a2c8dd 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -138,7 +138,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(methods::BYTES_NTH), LintId::of(methods::CHARS_LAST_CMP), LintId::of(methods::CHARS_NEXT_CMP), - LintId::of(methods::ITER_OVEREAGER_CLONED), LintId::of(methods::CLONE_DOUBLE_REF), LintId::of(methods::CLONE_ON_COPY), LintId::of(methods::EXPECT_FUN_CALL), @@ -154,6 +153,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(methods::ITER_NEXT_SLICE), LintId::of(methods::ITER_NTH), LintId::of(methods::ITER_NTH_ZERO), + LintId::of(methods::ITER_OVEREAGER_CLONED), LintId::of(methods::ITER_SKIP_NEXT), LintId::of(methods::MANUAL_FILTER_MAP), LintId::of(methods::MANUAL_FIND_MAP), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index e4afa5cc0c35..a21ddf73a115 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -32,7 +32,6 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(matches::MATCH_SINGLE_BINDING), LintId::of(matches::WILDCARD_IN_OR_PATTERNS), LintId::of(methods::BIND_INSTEAD_OF_MAP), - LintId::of(methods::ITER_OVEREAGER_CLONED), LintId::of(methods::CLONE_ON_COPY), LintId::of(methods::FILTER_MAP_IDENTITY), LintId::of(methods::FILTER_NEXT), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 9a2300b073a8..df041cb2c742 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -262,7 +262,6 @@ store.register_lints(&[ methods::CHARS_LAST_CMP, methods::CHARS_NEXT_CMP, methods::CLONED_INSTEAD_OF_COPIED, - methods::ITER_OVEREAGER_CLONED, methods::CLONE_DOUBLE_REF, methods::CLONE_ON_COPY, methods::CLONE_ON_REF_PTR, @@ -287,6 +286,7 @@ store.register_lints(&[ methods::ITER_NEXT_SLICE, methods::ITER_NTH, methods::ITER_NTH_ZERO, + methods::ITER_OVEREAGER_CLONED, methods::ITER_SKIP_NEXT, methods::MANUAL_FILTER_MAP, methods::MANUAL_FIND_MAP, diff --git a/clippy_lints/src/lib.register_perf.rs b/clippy_lints/src/lib.register_perf.rs index 2ea0b696f1fe..53c4bfd8bb38 100644 --- a/clippy_lints/src/lib.register_perf.rs +++ b/clippy_lints/src/lib.register_perf.rs @@ -14,6 +14,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![ LintId::of(methods::EXPECT_FUN_CALL), LintId::of(methods::EXTEND_WITH_DRAIN), LintId::of(methods::ITER_NTH), + LintId::of(methods::ITER_OVEREAGER_CLONED), LintId::of(methods::MANUAL_STR_REPEAT), LintId::of(methods::OR_FUN_CALL), LintId::of(methods::SINGLE_CHAR_PATTERN), From a2f5f6715a0c60f946e479749476b91fe949f044 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 07:50:42 -0800 Subject: [PATCH 31/47] Fix CI --- tests/ui/iter_overeager_cloned.fixed | 6 +++--- tests/ui/iter_overeager_cloned.rs | 8 ++++---- tests/ui/iter_overeager_cloned.stderr | 22 +++++++--------------- 3 files changed, 14 insertions(+), 22 deletions(-) diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index 39ce623a3b4e..73303d0b4a9a 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -6,15 +6,15 @@ fn main() { let _: Option = vec.iter().last().cloned(); - let _: Option = vec.iter().next().cloned(); + let _: Option = vec.iter().filter(|x| x == &"2").next().cloned(); - let _: usize = vec.iter().count(); + let _: usize = vec.iter().filter(|x| x == &"2").count(); let _: Vec<_> = vec.iter().take(2).cloned().collect(); let _: Vec<_> = vec.iter().skip(2).cloned().collect(); - let _ = vec.iter().nth(2).cloned(); + let _ = vec.iter().filter(|x| x == &"2").nth(2).cloned(); // Not implemented yet let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index 5cc54450b973..31c8aec17f0b 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -6,18 +6,18 @@ fn main() { let _: Option = vec.iter().cloned().last(); - let _: Option = vec.iter().cloned().next(); + let _: Option = vec.iter().filter(|x| x == &"2").cloned().next(); - let _: usize = vec.iter().cloned().count(); + let _: usize = vec.iter().filter(|x| x == &"2").cloned().count(); let _: Vec<_> = vec.iter().cloned().take(2).collect(); let _: Vec<_> = vec.iter().cloned().skip(2).collect(); - let _ = vec.iter().cloned().nth(2); + let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2); // Not implemented yet - let _ = vec.iter().cloned().filter(|x| x.starts_with("2")); + let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); // Not implemented yet let _ = vec.iter().cloned().map(|x| x.len()); diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index 0c976d760d6c..bf7b9ba1640c 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -9,14 +9,14 @@ LL | let _: Option = vec.iter().cloned().last(); error: called `cloned().next()` on an `Iterator`. It may be more efficient to call`.next().cloned()` instead --> $DIR/iter_overeager_cloned.rs:9:29 | -LL | let _: Option = vec.iter().cloned().next(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().next().cloned()` +LL | let _: Option = vec.iter().filter(|x| x == &"2").cloned().next(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").next().cloned()` error: called `cloned().count()` on an `Iterator`. It may be more efficient to call`.count()` instead --> $DIR/iter_overeager_cloned.rs:11:20 | -LL | let _: usize = vec.iter().cloned().count(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().count()` +LL | let _: usize = vec.iter().filter(|x| x == &"2").cloned().count(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").count()` | = note: `-D clippy::redundant-clone` implied by `-D warnings` @@ -35,16 +35,8 @@ LL | let _: Vec<_> = vec.iter().cloned().skip(2).collect(); error: called `cloned().nth(...)` on an `Iterator`. It may be more efficient to call`.nth(...).cloned()` instead --> $DIR/iter_overeager_cloned.rs:17:13 | -LL | let _ = vec.iter().cloned().nth(2); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().nth(2).cloned()` +LL | let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").nth(2).cloned()` -error: single-character string constant used as pattern - --> $DIR/iter_overeager_cloned.rs:20:58 - | -LL | let _ = vec.iter().cloned().filter(|x| x.starts_with("2")); - | ^^^ help: try using a `char` instead: `'2'` - | - = note: `-D clippy::single-char-pattern` implied by `-D warnings` - -error: aborting due to 7 previous errors +error: aborting due to 6 previous errors From 732df9b84aa15a7f22c89d8a5434e27fba6b871e Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 07:56:19 -0800 Subject: [PATCH 32/47] Fix CI --- tests/ui/iter_overeager_cloned.fixed | 2 +- tests/ui/iter_overeager_cloned.rs | 2 +- tests/ui/iter_overeager_cloned.stderr | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index 73303d0b4a9a..f30cc81e359d 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -6,7 +6,7 @@ fn main() { let _: Option = vec.iter().last().cloned(); - let _: Option = vec.iter().filter(|x| x == &"2").next().cloned(); + let _: Option = vec.iter().filter(|x| x.len() == 1).next().cloned(); let _: usize = vec.iter().filter(|x| x == &"2").count(); diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index 31c8aec17f0b..cf27dd9f91c7 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -6,7 +6,7 @@ fn main() { let _: Option = vec.iter().cloned().last(); - let _: Option = vec.iter().filter(|x| x == &"2").cloned().next(); + let _: Option = vec.iter().filter(|x| x.len() == 1).cloned().next(); let _: usize = vec.iter().filter(|x| x == &"2").cloned().count(); diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index bf7b9ba1640c..f71964f75228 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -9,8 +9,8 @@ LL | let _: Option = vec.iter().cloned().last(); error: called `cloned().next()` on an `Iterator`. It may be more efficient to call`.next().cloned()` instead --> $DIR/iter_overeager_cloned.rs:9:29 | -LL | let _: Option = vec.iter().filter(|x| x == &"2").cloned().next(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").next().cloned()` +LL | let _: Option = vec.iter().filter(|x| x.len() == 1).cloned().next(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x.len() == 1).next().cloned()` error: called `cloned().count()` on an `Iterator`. It may be more efficient to call`.count()` instead --> $DIR/iter_overeager_cloned.rs:11:20 From 1b672e86bdd987ba80bf4f9906000d9181aae1d6 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 08:02:02 -0800 Subject: [PATCH 33/47] Fix CI --- tests/ui/iter_overeager_cloned.fixed | 4 ++-- tests/ui/iter_overeager_cloned.rs | 4 ++-- tests/ui/iter_overeager_cloned.stderr | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index f30cc81e359d..c6a61bfb0aa9 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -1,12 +1,12 @@ // run-rustfix -#![warn(clippy::iter_overeager_cloned, clippy::redundant_clone)] +#![warn(clippy::iter_overeager_cloned, clippy::redundant_clone, clippy::filter_next)] fn main() { let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()]; let _: Option = vec.iter().last().cloned(); - let _: Option = vec.iter().filter(|x| x.len() == 1).next().cloned(); + let _: Option = vec.iter().chain(vec.iter()).next().cloned(); let _: usize = vec.iter().filter(|x| x == &"2").count(); diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index cf27dd9f91c7..471fe3d36ea4 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -1,12 +1,12 @@ // run-rustfix -#![warn(clippy::iter_overeager_cloned, clippy::redundant_clone)] +#![warn(clippy::iter_overeager_cloned, clippy::redundant_clone, clippy::filter_next)] fn main() { let vec = vec!["1".to_string(), "2".to_string(), "3".to_string()]; let _: Option = vec.iter().cloned().last(); - let _: Option = vec.iter().filter(|x| x.len() == 1).cloned().next(); + let _: Option = vec.iter().chain(vec.iter()).cloned().next(); let _: usize = vec.iter().filter(|x| x == &"2").cloned().count(); diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index f71964f75228..32f65c2732cd 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -9,8 +9,8 @@ LL | let _: Option = vec.iter().cloned().last(); error: called `cloned().next()` on an `Iterator`. It may be more efficient to call`.next().cloned()` instead --> $DIR/iter_overeager_cloned.rs:9:29 | -LL | let _: Option = vec.iter().filter(|x| x.len() == 1).cloned().next(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x.len() == 1).next().cloned()` +LL | let _: Option = vec.iter().chain(vec.iter()).cloned().next(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().chain(vec.iter()).next().cloned()` error: called `cloned().count()` on an `Iterator`. It may be more efficient to call`.count()` instead --> $DIR/iter_overeager_cloned.rs:11:20 From 077fa16029814c91ed3f9f0a745a64a515e56af9 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 08:31:39 -0800 Subject: [PATCH 34/47] Fix missing clippy version issue --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 25ea88dc303f..fcdb0805e908 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -138,7 +138,7 @@ declare_clippy_lint! { /// This `lint` removes the side of effect of cloning items in the iterator. /// A code that relies on that side-effect could fail. /// - /// #[clippy::version = "1.59.0"] + #[clippy::version = "1.59.0"] pub ITER_OVEREAGER_CLONED, perf, "using `cloned()` early with `Iterator::iter()` can lead to some performance inefficiencies" From 0229d0859187d76a27d84d418eb7ca617188c1b1 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 09:03:47 -0800 Subject: [PATCH 35/47] Add more TODOs --- tests/ui/iter_overeager_cloned.fixed | 6 ++++++ tests/ui/iter_overeager_cloned.rs | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index c6a61bfb0aa9..38b5c12d3c14 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -30,4 +30,10 @@ fn main() { // Not implemented yet let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); + + // Not implemented yet + let _ = vec.iter().cloned().all(|x| x.len() == 1); + + // Not implemented yet + let _ = vec.iter().cloned().any(|x| x.len() == 1); } diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index 471fe3d36ea4..e938caa4aca9 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -30,4 +30,10 @@ fn main() { // Not implemented yet let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty())); + + // Not implemented yet + let _ = vec.iter().cloned().all(|x| x.len() == 1); + + // Not implemented yet + let _ = vec.iter().cloned().any(|x| x.len() == 1); } From 78e19512b3158a3f6026128c57ce99ffb8ce6c39 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 11:43:11 -0800 Subject: [PATCH 36/47] Add flatten to the list --- .../src/methods/iter_overeager_cloned.rs | 4 ++-- clippy_lints/src/methods/mod.rs | 8 ++++---- tests/ui/iter_overeager_cloned.fixed | 3 +++ tests/ui/iter_overeager_cloned.rs | 5 +++++ tests/ui/iter_overeager_cloned.stderr | 18 +++++++++++++++++- 5 files changed, 31 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 838c035d089a..06a69990aadc 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -31,7 +31,7 @@ pub(super) fn check<'tcx>( name, name ), ), - "next" | "last" => ( + "next" | "last" | "flatten" => ( ITER_OVEREAGER_CLONED, format!( "called `cloned().{}()` on an `Iterator`. It may be more efficient to call\ @@ -60,7 +60,7 @@ pub(super) fn check<'tcx>( "count" => { format!("{}.{}()", iter_snippet, name) }, - "next" | "last" => { + "next" | "last" | "flatten" => { format!("{}.{}().cloned()", iter_snippet, name) }, _ => { diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index fcdb0805e908..c753880ea5d6 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2311,10 +2311,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio flat_map_identity::check(cx, expr, arg, span); flat_map_option::check(cx, expr, arg, span); }, - ("flatten", []) => { - if let Some(("map", [recv, map_arg], _)) = method_call!(recv) { - map_flatten::check(cx, expr, recv, map_arg); - } + (name @ "flatten", args @ []) => match method_call!(recv) { + Some(("map", [recv, map_arg], _)) => map_flatten::check(cx, expr, recv, map_arg), + Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args), + _ => {}, }, ("fold", [init, acc]) => unnecessary_fold::check(cx, expr, init, acc, span), ("for_each", [_]) => { diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index 38b5c12d3c14..01b64ff7ac45 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -16,6 +16,9 @@ fn main() { let _ = vec.iter().filter(|x| x == &"2").nth(2).cloned(); + let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] + .iter().flatten().cloned(); + // Not implemented yet let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index e938caa4aca9..fede4c733216 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -16,6 +16,11 @@ fn main() { let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2); + let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] + .iter() + .cloned() + .flatten(); + // Not implemented yet let _ = vec.iter().cloned().filter(|x| x.starts_with('2')); diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index 32f65c2732cd..4a58687f4212 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -38,5 +38,21 @@ error: called `cloned().nth(...)` on an `Iterator`. It may be more efficient to LL | let _ = vec.iter().filter(|x| x == &"2").cloned().nth(2); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `vec.iter().filter(|x| x == &"2").nth(2).cloned()` -error: aborting due to 6 previous errors +error: called `cloned().flatten()` on an `Iterator`. It may be more efficient to call`.flatten().cloned()` instead + --> $DIR/iter_overeager_cloned.rs:19:13 + | +LL | let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] + | _____________^ +LL | | .iter() +LL | | .cloned() +LL | | .flatten(); + | |__________________^ + | +help: try this + | +LL ~ let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] +LL ~ .iter().flatten().cloned(); + | + +error: aborting due to 7 previous errors From d1200a6dd925ef01baa665a6c6e22e524a421d9c Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Fri, 31 Dec 2021 11:47:49 -0800 Subject: [PATCH 37/47] Add failing test --- tests/ui/iter_overeager_cloned.fixed | 3 +++ tests/ui/iter_overeager_cloned.rs | 3 +++ tests/ui/iter_overeager_cloned.stderr | 8 +++++++- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index 01b64ff7ac45..7295ad871279 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -39,4 +39,7 @@ fn main() { // Not implemented yet let _ = vec.iter().cloned().any(|x| x.len() == 1); + + // Should probably stay as it is. + let _ = [0, 1, 2, 3, 4].iter().take(10).cloned(); } diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index fede4c733216..dd04e33a4b3a 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -41,4 +41,7 @@ fn main() { // Not implemented yet let _ = vec.iter().cloned().any(|x| x.len() == 1); + + // Should probably stay as it is. + let _ = [0, 1, 2, 3, 4].iter().cloned().take(10); } diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index 4a58687f4212..343614e75333 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -54,5 +54,11 @@ LL ~ let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] LL ~ .iter().flatten().cloned(); | -error: aborting due to 7 previous errors +error: called `cloned().take(...)` on an `Iterator`. It may be more efficient to call`.take(...).cloned()` instead + --> $DIR/iter_overeager_cloned.rs:46:13 + | +LL | let _ = [0, 1, 2, 3, 4].iter().cloned().take(10); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `[0, 1, 2, 3, 4].iter().take(10).cloned()` + +error: aborting due to 8 previous errors From 6c152137bb9b6749f735314e78385fdcc9a3b0a5 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Sat, 1 Jan 2022 06:37:55 -0800 Subject: [PATCH 38/47] Udpate comments to apply feedback --- clippy_lints/src/methods/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c753880ea5d6..fe5e2943f2d9 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -110,8 +110,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for usage of `_.cloned().()` where call to `.cloned()` can be postponed, - /// or often removed entirely. + /// Checks for usage of `_.cloned().()` where call to `.cloned()` can be postponed. /// /// ### Why is this bad? /// It's often inefficient to clone all elements of an iterator, when eventually, only some From 7c2d8d4de3c652960a6b97729b49dad9233db406 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Sat, 1 Jan 2022 07:13:33 -0800 Subject: [PATCH 39/47] Check number of with `.cloned().take(..)` --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index fe5e2943f2d9..b57c1a72ab6a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2397,7 +2397,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio } }, ("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg), - ("take", args) => { + ("take", args @ [_arg]) => { if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call!(recv) { if let ("cloned", []) = (name2, args2) { iter_overeager_cloned::check(cx, expr, recv2, name, args); From 9d9e5fad31dfb2d4c8ace771310b1ffac4ae634d Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Sat, 1 Jan 2022 12:46:37 -0800 Subject: [PATCH 40/47] Apply feedback --- clippy_lints/src/methods/iter_overeager_cloned.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 06a69990aadc..2e197a20667e 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -9,7 +9,7 @@ use rustc_span::sym; use super::ITER_OVEREAGER_CLONED; use crate::redundant_clone::REDUNDANT_CLONE; -/// lint use of `cloned().last()` for `Iterators` +/// lint use of `cloned().()` for `Iterators` pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, @@ -17,7 +17,7 @@ pub(super) fn check<'tcx>( name: &str, map_arg: &[hir::Expr<'_>], ) { - // lint if caller of `.cloned().last()` is an Iterator + // lint if caller of `.cloned().()` is an Iterator let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) }); @@ -31,7 +31,7 @@ pub(super) fn check<'tcx>( name, name ), ), - "next" | "last" | "flatten" => ( + _ if map_arg.is_empty() => ( ITER_OVEREAGER_CLONED, format!( "called `cloned().{}()` on an `Iterator`. It may be more efficient to call\ @@ -60,7 +60,7 @@ pub(super) fn check<'tcx>( "count" => { format!("{}.{}()", iter_snippet, name) }, - "next" | "last" | "flatten" => { + _ if map_arg.is_empty() => { format!("{}.{}().cloned()", iter_snippet, name) }, _ => { From 9bcef5d390045e4a41c91aecce818826c7e5b280 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Sat, 1 Jan 2022 12:52:53 -0800 Subject: [PATCH 41/47] Update comments based on feedback --- clippy_lints/src/methods/iter_overeager_cloned.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 2e197a20667e..f06c72e691e3 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -9,7 +9,7 @@ use rustc_span::sym; use super::ITER_OVEREAGER_CLONED; use crate::redundant_clone::REDUNDANT_CLONE; -/// lint use of `cloned().()` for `Iterators` +/// lint overeager use of `cloned()` for `Iterator`s pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, @@ -17,7 +17,7 @@ pub(super) fn check<'tcx>( name: &str, map_arg: &[hir::Expr<'_>], ) { - // lint if caller of `.cloned().()` is an Iterator + // lint if callee is an Iterator let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) }); From 4665853e5010c4498ef42470b4ae855ea334da27 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Sat, 1 Jan 2022 13:21:18 -0800 Subject: [PATCH 42/47] Do lint only if the values is cloned, but not copied --- clippy_lints/src/methods/iter_overeager_cloned.rs | 14 +++++++++++++- tests/ui/iter_overeager_cloned.fixed | 2 +- tests/ui/iter_overeager_cloned.stderr | 8 +------- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index f06c72e691e3..8eac95421811 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -1,9 +1,10 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet; -use clippy_utils::ty::implements_trait; +use clippy_utils::ty::{get_iterator_item_ty, implements_trait, is_copy}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; +use rustc_middle::ty; use rustc_span::sym; use super::ITER_OVEREAGER_CLONED; @@ -17,6 +18,17 @@ pub(super) fn check<'tcx>( name: &str, map_arg: &[hir::Expr<'_>], ) { + let recv_ty = cx.typeck_results().expr_ty_adjusted(recv); + let inner_ty = match get_iterator_item_ty(cx, recv_ty) { + Some(ty) => ty, + _ => return, + }; + + match inner_ty.kind() { + ty::Ref(_, ty, _) if is_copy(cx, ty) => return, + _ => {}, + }; + // lint if callee is an Iterator let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index 7295ad871279..a9041671101b 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -41,5 +41,5 @@ fn main() { let _ = vec.iter().cloned().any(|x| x.len() == 1); // Should probably stay as it is. - let _ = [0, 1, 2, 3, 4].iter().take(10).cloned(); + let _ = [0, 1, 2, 3, 4].iter().cloned().take(10); } diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index 343614e75333..4a58687f4212 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/tests/ui/iter_overeager_cloned.stderr @@ -54,11 +54,5 @@ LL ~ let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))] LL ~ .iter().flatten().cloned(); | -error: called `cloned().take(...)` on an `Iterator`. It may be more efficient to call`.take(...).cloned()` instead - --> $DIR/iter_overeager_cloned.rs:46:13 - | -LL | let _ = [0, 1, 2, 3, 4].iter().cloned().take(10); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `[0, 1, 2, 3, 4].iter().take(10).cloned()` - -error: aborting due to 8 previous errors +error: aborting due to 7 previous errors From af4914be2279d4180830011d25edfe511875caec Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Sat, 1 Jan 2022 23:27:25 -0800 Subject: [PATCH 43/47] Simplify code for this lint --- .../src/methods/iter_overeager_cloned.rs | 97 +++++++------------ clippy_lints/src/methods/mod.rs | 4 + 2 files changed, 40 insertions(+), 61 deletions(-) diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 8eac95421811..61bea7e1f083 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -1,11 +1,12 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet; -use clippy_utils::ty::{get_iterator_item_ty, implements_trait, is_copy}; +use clippy_utils::ty::{get_iterator_item_ty, is_copy}; +use itertools::Itertools; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; use rustc_middle::ty; -use rustc_span::sym; +use std::ops::Not; use super::ITER_OVEREAGER_CLONED; use crate::redundant_clone::REDUNDANT_CLONE; @@ -19,72 +20,46 @@ pub(super) fn check<'tcx>( map_arg: &[hir::Expr<'_>], ) { let recv_ty = cx.typeck_results().expr_ty_adjusted(recv); + // Check if it's iterator and get type associated with `Item`. let inner_ty = match get_iterator_item_ty(cx, recv_ty) { Some(ty) => ty, _ => return, }; match inner_ty.kind() { - ty::Ref(_, ty, _) if is_copy(cx, ty) => return, - _ => {}, + ty::Ref(_, ty, _) if !is_copy(cx, ty) => {}, + _ => return, }; - // lint if callee is an Iterator - let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { - implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) - }); - if recv_impls_iterator { - let (lint, msg) = match name { - "count" => ( - REDUNDANT_CLONE, - format!( - "called `cloned().{}()` on an `Iterator`. It may be more efficient to call\ - `.{}()` instead", - name, name - ), - ), - _ if map_arg.is_empty() => ( - ITER_OVEREAGER_CLONED, - format!( - "called `cloned().{}()` on an `Iterator`. It may be more efficient to call\ - `.{}().cloned()` instead", - name, name - ), - ), - _ => ( - ITER_OVEREAGER_CLONED, - format!( - "called `cloned().{}(...)` on an `Iterator`. It may be more efficient to call\ - `.{}(...).cloned()` instead", - name, name - ), - ), - }; + let (lint, preserve_clopned) = match name { + "count" => (REDUNDANT_CLONE, false), + _ => (ITER_OVEREAGER_CLONED, true), + }; + let wildcard_params = map_arg.is_empty().not().then(|| "...").unwrap_or_default(); + let msg = format!( + "called `cloned().{}({})` on an `Iterator`. It may be more efficient to call\ + `.{}({}){}` instead", + name, + wildcard_params, + name, + wildcard_params, + preserve_clopned.then(|| ".cloned()").unwrap_or_default(), + ); - let iter_snippet = snippet(cx, recv.span, ".."); - span_lint_and_sugg( - cx, - lint, - expr.span, - &msg, - "try this", - match name { - "count" => { - format!("{}.{}()", iter_snippet, name) - }, - _ if map_arg.is_empty() => { - format!("{}.{}().cloned()", iter_snippet, name) - }, - _ => { - format!( - "{}.{}({}).cloned()", - iter_snippet, - name, - snippet(cx, map_arg[0].span, "..") - ) - }, - }, - Applicability::MachineApplicable, - ); - } + let iter_snippet = snippet(cx, recv.span, ".."); + span_lint_and_sugg( + cx, + lint, + expr.span, + &msg, + "try this", + format!( + "{}.{}({}){}", + iter_snippet, + name, + map_arg.iter().map(|a| snippet(cx, a.span, "..")).join(", "), + preserve_clopned.then(|| ".cloned()").unwrap_or_default(), + ), + Applicability::MachineApplicable, + ); } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index b57c1a72ab6a..a3382133f493 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2302,6 +2302,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio string_extend_chars::check(cx, expr, recv, arg); extend_with_drain::check(cx, expr, recv, arg); }, + (name @ "filter", args @ []) => match method_call!(recv) { + Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args), + _ => {}, + }, ("filter_map", [arg]) => { unnecessary_filter_map::check(cx, expr, arg); filter_map_identity::check(cx, expr, arg, span); From e6b4646483e9c8d7a80d4dd49be4ed2c21b95937 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Sun, 2 Jan 2022 00:01:48 -0800 Subject: [PATCH 44/47] Undo changes --- clippy_lints/src/methods/mod.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index a3382133f493..b57c1a72ab6a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2302,10 +2302,6 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio string_extend_chars::check(cx, expr, recv, arg); extend_with_drain::check(cx, expr, recv, arg); }, - (name @ "filter", args @ []) => match method_call!(recv) { - Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args), - _ => {}, - }, ("filter_map", [arg]) => { unnecessary_filter_map::check(cx, expr, arg); filter_map_identity::check(cx, expr, arg, span); From dc76dcbeabe507583b459894f835b6d7e5038b44 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Sun, 2 Jan 2022 00:22:51 -0800 Subject: [PATCH 45/47] Undo changes --- clippy_lints/src/methods/mod.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index b57c1a72ab6a..2cc119cd2767 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2269,9 +2269,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("as_mut", []) => useless_asref::check(cx, expr, "as_mut", recv), ("as_ref", []) => useless_asref::check(cx, expr, "as_ref", recv), ("assume_init", []) => uninit_assumed_init::check(cx, expr, recv), - ("cloned", []) => { - cloned_instead_of_copied::check(cx, expr, recv, span, msrv); - }, + ("cloned", []) => cloned_instead_of_copied::check(cx, expr, recv, span, msrv), ("collect", []) => match method_call!(recv) { Some((name @ ("cloned" | "copied"), [recv2], _)) => { iter_cloned_collect::check(cx, name, expr, recv2); From e1edea29cc392c2b823e4e2172403872f0d3c465 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Mon, 3 Jan 2022 06:04:38 -0800 Subject: [PATCH 46/47] Cleanup --- clippy_lints/src/methods/iter_overeager_cloned.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 61bea7e1f083..19bc05305764 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -19,9 +19,8 @@ pub(super) fn check<'tcx>( name: &str, map_arg: &[hir::Expr<'_>], ) { - let recv_ty = cx.typeck_results().expr_ty_adjusted(recv); // Check if it's iterator and get type associated with `Item`. - let inner_ty = match get_iterator_item_ty(cx, recv_ty) { + let inner_ty = match get_iterator_item_ty(cx, cx.typeck_results().expr_ty_adjusted(recv)) { Some(ty) => ty, _ => return, }; @@ -31,7 +30,7 @@ pub(super) fn check<'tcx>( _ => return, }; - let (lint, preserve_clopned) = match name { + let (lint, preserve_cloned) = match name { "count" => (REDUNDANT_CLONE, false), _ => (ITER_OVEREAGER_CLONED, true), }; @@ -43,10 +42,9 @@ pub(super) fn check<'tcx>( wildcard_params, name, wildcard_params, - preserve_clopned.then(|| ".cloned()").unwrap_or_default(), + preserve_cloned.then(|| ".cloned()").unwrap_or_default(), ); - let iter_snippet = snippet(cx, recv.span, ".."); span_lint_and_sugg( cx, lint, @@ -55,10 +53,10 @@ pub(super) fn check<'tcx>( "try this", format!( "{}.{}({}){}", - iter_snippet, + snippet(cx, recv.span, ".."), name, map_arg.iter().map(|a| snippet(cx, a.span, "..")).join(", "), - preserve_clopned.then(|| ".cloned()").unwrap_or_default(), + preserve_cloned.then(|| ".cloned()").unwrap_or_default(), ), Applicability::MachineApplicable, ); From cf95b862e7e057724ee634e8ce26cd60e7f28489 Mon Sep 17 00:00:00 2001 From: Piotr Mikulski Date: Mon, 3 Jan 2022 06:06:17 -0800 Subject: [PATCH 47/47] Cleanup --- clippy_lints/src/methods/iter_overeager_cloned.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index 19bc05305764..9134e20ec0c2 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/clippy_lints/src/methods/iter_overeager_cloned.rs @@ -37,7 +37,7 @@ pub(super) fn check<'tcx>( let wildcard_params = map_arg.is_empty().not().then(|| "...").unwrap_or_default(); let msg = format!( "called `cloned().{}({})` on an `Iterator`. It may be more efficient to call\ - `.{}({}){}` instead", + `{}({}){}` instead", name, wildcard_params, name,