diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cda8003b05c..b5172267fcaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6653,6 +6653,7 @@ Released 2018-09-13 [`manual_string_new`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_string_new [`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip [`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap +[`manual_take`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_take [`manual_try_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_try_fold [`manual_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_unwrap_or [`manual_unwrap_or_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_unwrap_or_default diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index a1c079898594..f81dd421f59b 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -905,6 +905,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio * [`manual_split_once`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_split_once) * [`manual_str_repeat`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_str_repeat) * [`manual_strip`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip) +* [`manual_take`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_take) * [`manual_try_fold`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_try_fold) * [`map_clone`](https://rust-lang.github.io/rust-clippy/master/index.html#map_clone) * [`map_unwrap_or`](https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index e1d7c1d88eb9..849d9a613d80 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -780,6 +780,7 @@ fn span_from_toml_range(file: &SourceFile, span: Range) -> Span { manual_split_once, manual_str_repeat, manual_strip, + manual_take, manual_try_fold, map_clone, map_unwrap_or, diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index ef9da84c4407..549fc64d50e2 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -316,6 +316,7 @@ crate::manual_slice_size_calculation::MANUAL_SLICE_SIZE_CALCULATION_INFO, crate::manual_string_new::MANUAL_STRING_NEW_INFO, crate::manual_strip::MANUAL_STRIP_INFO, + crate::manual_take::MANUAL_TAKE_INFO, crate::map_unit_fn::OPTION_MAP_UNIT_FN_INFO, crate::map_unit_fn::RESULT_MAP_UNIT_FN_INFO, crate::match_result_ok::MATCH_RESULT_OK_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ef2461f8b097..eccff4789a20 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -219,6 +219,7 @@ mod manual_slice_size_calculation; mod manual_string_new; mod manual_strip; +mod manual_take; mod map_unit_fn; mod match_result_ok; mod matches; @@ -861,6 +862,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co Box::new(move |_| Box::new(manual_ilog2::ManualIlog2::new(conf))), Box::new(|_| Box::new(same_length_and_capacity::SameLengthAndCapacity)), Box::new(move |tcx| Box::new(duration_suboptimal_units::DurationSuboptimalUnits::new(tcx, conf))), + Box::new(move |_| Box::new(manual_take::ManualTake::new(conf))), // add late passes here, used by `cargo dev new_lint` ]; store.late_passes.extend(late_lints); diff --git a/clippy_lints/src/manual_take.rs b/clippy_lints/src/manual_take.rs new file mode 100644 index 000000000000..8e74d04c4556 --- /dev/null +++ b/clippy_lints/src/manual_take.rs @@ -0,0 +1,114 @@ +use clippy_config::Conf; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::msrvs::{MEM_TAKE, Msrv}; +use clippy_utils::source::snippet_with_context; +use rustc_ast::LitKind; +use rustc_errors::Applicability; +use rustc_hir::{Block, Expr, ExprKind, StmtKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_session::impl_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Detects manual re-implementations of `std::mem::take`. + /// + /// ### Why is this bad? + /// Because the function call is shorter and easier to read. + /// + /// ### Known issues + /// Currently the lint only detects cases involving `bool`s. + /// + /// ### Example + /// ```no_run + /// let mut x = true; + /// let _ = if x { + /// x = false; + /// true + /// } else { + /// false + /// }; + /// ``` + /// Use instead: + /// ```no_run + /// let mut x = true; + /// let _ = std::mem::take(&mut x); + /// ``` + #[clippy::version = "1.94.0"] + pub MANUAL_TAKE, + complexity, + "manual `mem::take` implementation" +} +pub struct ManualTake { + msrv: Msrv, +} + +impl ManualTake { + pub fn new(conf: &'static Conf) -> Self { + Self { msrv: conf.msrv } + } +} + +impl_lint_pass!(ManualTake => [MANUAL_TAKE]); + +impl LateLintPass<'_> for ManualTake { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + if let ExprKind::If(cond, then, Some(otherwise)) = expr.kind + && let ExprKind::Path(_) = cond.kind + && let ExprKind::Block( + Block { + stmts: [stmt], + expr: Some(then_expr), + .. + }, + .., + ) = then.kind + && let ExprKind::Block( + Block { + stmts: [], + expr: Some(else_expr), + .. + }, + .., + ) = otherwise.kind + && let StmtKind::Semi(assignment) = stmt.kind + && let ExprKind::Assign(mut_c, possible_false, _) = assignment.kind + && let ExprKind::Path(_) = mut_c.kind + && !expr.span.in_external_macro(cx.sess().source_map()) + && let Some(std_or_core) = clippy_utils::std_or_core(cx) + && self.msrv.meets(cx, MEM_TAKE) + && clippy_utils::SpanlessEq::new(cx).eq_expr(cond, mut_c) + && Some(false) == as_const_bool(possible_false) + && let Some(then_bool) = as_const_bool(then_expr) + && let Some(else_bool) = as_const_bool(else_expr) + && then_bool != else_bool + { + span_lint_and_then( + cx, + MANUAL_TAKE, + expr.span, + "manual implementation of `mem::take`", + |diag| { + let mut app = Applicability::MachineApplicable; + let negate = if then_bool { "" } else { "!" }; + let taken = snippet_with_context(cx, cond.span, expr.span.ctxt(), "_", &mut app).0; + diag.span_suggestion_verbose( + expr.span, + "use", + format!("{negate}{std_or_core}::mem::take(&mut {taken})"), + app, + ); + }, + ); + } + } +} + +fn as_const_bool(e: &Expr<'_>) -> Option { + if let ExprKind::Lit(lit) = e.kind + && let LitKind::Bool(b) = lit.node + { + Some(b) + } else { + None + } +} diff --git a/tests/ui/manual_take.fixed b/tests/ui/manual_take.fixed new file mode 100644 index 000000000000..5891bd49c81c --- /dev/null +++ b/tests/ui/manual_take.fixed @@ -0,0 +1,57 @@ +#![warn(clippy::manual_take)] + +fn main() { + msrv_1_39(); + msrv_1_40(); + let mut x = true; + let mut y = false; + + let _lint_negated = !std::mem::take(&mut x); + + let _ = if x { + y = false; + true + } else { + false + }; + + let _ = if x { + x = true; + true + } else { + false + }; + + let _ = if x { + x = false; + y = true; + false + } else { + true + }; + + let _ = if x { + x = false; + false + } else { + y = true; + true + }; +} + +#[clippy::msrv = "1.39.0"] +fn msrv_1_39() -> bool { + let mut x = true; + if x { + x = false; + true + } else { + false + } +} + +#[clippy::msrv = "1.40.0"] +fn msrv_1_40() -> bool { + let mut x = true; + std::mem::take(&mut x) +} diff --git a/tests/ui/manual_take.rs b/tests/ui/manual_take.rs new file mode 100644 index 000000000000..89903fcb2416 --- /dev/null +++ b/tests/ui/manual_take.rs @@ -0,0 +1,69 @@ +#![warn(clippy::manual_take)] + +fn main() { + msrv_1_39(); + msrv_1_40(); + let mut x = true; + let mut y = false; + + let _lint_negated = if x { + //~^ manual_take + x = false; + false + } else { + true + }; + + let _ = if x { + y = false; + true + } else { + false + }; + + let _ = if x { + x = true; + true + } else { + false + }; + + let _ = if x { + x = false; + y = true; + false + } else { + true + }; + + let _ = if x { + x = false; + false + } else { + y = true; + true + }; +} + +#[clippy::msrv = "1.39.0"] +fn msrv_1_39() -> bool { + let mut x = true; + if x { + x = false; + true + } else { + false + } +} + +#[clippy::msrv = "1.40.0"] +fn msrv_1_40() -> bool { + let mut x = true; + if x { + //~^ manual_take + x = false; + true + } else { + false + } +} diff --git a/tests/ui/manual_take.stderr b/tests/ui/manual_take.stderr new file mode 100644 index 000000000000..69ba7778a275 --- /dev/null +++ b/tests/ui/manual_take.stderr @@ -0,0 +1,53 @@ +error: manual implementation of `mem::take` + --> tests/ui/manual_take.rs:9:25 + | +LL | let _lint_negated = if x { + | _________________________^ +LL | | +LL | | x = false; +LL | | false +LL | | } else { +LL | | true +LL | | }; + | |_____^ + | + = note: `-D clippy::manual-take` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_take)]` +help: use + | +LL - let _lint_negated = if x { +LL - +LL - x = false; +LL - false +LL - } else { +LL - true +LL - }; +LL + let _lint_negated = !std::mem::take(&mut x); + | + +error: manual implementation of `mem::take` + --> tests/ui/manual_take.rs:62:5 + | +LL | / if x { +LL | | +LL | | x = false; +LL | | true +LL | | } else { +LL | | false +LL | | } + | |_____^ + | +help: use + | +LL - if x { +LL - +LL - x = false; +LL - true +LL - } else { +LL - false +LL - } +LL + std::mem::take(&mut x) + | + +error: aborting due to 2 previous errors + diff --git a/tests/ui/manual_take_nocore.rs b/tests/ui/manual_take_nocore.rs new file mode 100644 index 000000000000..ef4f23395028 --- /dev/null +++ b/tests/ui/manual_take_nocore.rs @@ -0,0 +1,37 @@ +//@ check-pass +#![feature(no_core, lang_items)] +#![no_core] +#![allow(clippy::missing_safety_doc)] +#![warn(clippy::manual_take)] + +#[link(name = "c")] +unsafe extern "C" {} + +#[lang = "pointee_sized"] +pub trait PointeeSized {} + +#[lang = "meta_sized"] +pub trait MetaSized: PointeeSized {} + +#[lang = "sized"] +pub trait Sized: MetaSized {} +#[lang = "copy"] +pub trait Copy {} +#[lang = "freeze"] +pub unsafe trait Freeze {} + +#[lang = "start"] +fn start(_main: fn() -> T, _argc: isize, _argv: *const *const u8, _sigpipe: u8) -> isize { + 0 +} + +fn main() { + let mut x = true; + // this should not lint because we don't have std nor core + let _manual_take = if x { + x = false; + true + } else { + false + }; +} diff --git a/tests/ui/manual_take_nocore.stderr b/tests/ui/manual_take_nocore.stderr new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/ui/manual_take_nostd.fixed b/tests/ui/manual_take_nostd.fixed new file mode 100644 index 000000000000..123c5a91998c --- /dev/null +++ b/tests/ui/manual_take_nostd.fixed @@ -0,0 +1,10 @@ +#![no_std] +#![warn(clippy::manual_take)] + +pub fn manual_mem_take_should_reference_core() { + let mut x = true; + + let _lint_negated = !core::mem::take(&mut x); + + let _lint = core::mem::take(&mut x); +} diff --git a/tests/ui/manual_take_nostd.rs b/tests/ui/manual_take_nostd.rs new file mode 100644 index 000000000000..0df352477b74 --- /dev/null +++ b/tests/ui/manual_take_nostd.rs @@ -0,0 +1,22 @@ +#![no_std] +#![warn(clippy::manual_take)] + +pub fn manual_mem_take_should_reference_core() { + let mut x = true; + + let _lint_negated = if x { + //~^ manual_take + x = false; + false + } else { + true + }; + + let _lint = if x { + //~^ manual_take + x = false; + true + } else { + false + }; +} diff --git a/tests/ui/manual_take_nostd.stderr b/tests/ui/manual_take_nostd.stderr new file mode 100644 index 000000000000..71a5066e4db3 --- /dev/null +++ b/tests/ui/manual_take_nostd.stderr @@ -0,0 +1,54 @@ +error: manual implementation of `mem::take` + --> tests/ui/manual_take_nostd.rs:7:25 + | +LL | let _lint_negated = if x { + | _________________________^ +LL | | +LL | | x = false; +LL | | false +LL | | } else { +LL | | true +LL | | }; + | |_____^ + | + = note: `-D clippy::manual-take` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_take)]` +help: use + | +LL - let _lint_negated = if x { +LL - +LL - x = false; +LL - false +LL - } else { +LL - true +LL - }; +LL + let _lint_negated = !core::mem::take(&mut x); + | + +error: manual implementation of `mem::take` + --> tests/ui/manual_take_nostd.rs:15:17 + | +LL | let _lint = if x { + | _________________^ +LL | | +LL | | x = false; +LL | | true +LL | | } else { +LL | | false +LL | | }; + | |_____^ + | +help: use + | +LL - let _lint = if x { +LL - +LL - x = false; +LL - true +LL - } else { +LL - false +LL - }; +LL + let _lint = core::mem::take(&mut x); + | + +error: aborting due to 2 previous errors +