From 126790999a128a880ca276c49afd2927a66ffbbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Mon, 30 Mar 2020 11:02:14 +0200 Subject: [PATCH] new lint: Returning unit from closures expecting Ord This lint catches cases where the last statement of a closure expecting an instance of Ord has a trailing semi-colon. It compiles since the closure ends up return () which also implements Ord but causes unexpected results in cases such as sort_by_key. Fixes #5080 reprise: rebase, update and address all concerns --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 5 + clippy_lints/src/unit_return_expecting_ord.rs | 177 ++++++++++++++++++ src/lintlist/mod.rs | 7 + tests/ui/unit_return_expecting_ord.rs | 36 ++++ tests/ui/unit_return_expecting_ord.stderr | 39 ++++ 6 files changed, 265 insertions(+) create mode 100644 clippy_lints/src/unit_return_expecting_ord.rs create mode 100644 tests/ui/unit_return_expecting_ord.rs create mode 100644 tests/ui/unit_return_expecting_ord.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d08b44ba40..1c927b5f83a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1679,6 +1679,7 @@ Released 2018-09-13 [`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init [`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg [`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp +[`unit_return_expecting_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_return_expecting_ord [`unknown_clippy_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#unknown_clippy_lints [`unnecessary_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast [`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 32e79317f82..7a4ca3902b3 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -300,6 +300,7 @@ macro_rules! declare_clippy_lint { mod try_err; mod types; mod unicode; +mod unit_return_expecting_ord; mod unnamed_address; mod unnecessary_sort_by; mod unnested_or_patterns; @@ -826,6 +827,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &unicode::NON_ASCII_LITERAL, &unicode::UNICODE_NOT_NFC, &unicode::ZERO_WIDTH_SPACE, + &unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD, &unnamed_address::FN_ADDRESS_COMPARISONS, &unnamed_address::VTABLE_ADDRESS_COMPARISONS, &unnecessary_sort_by::UNNECESSARY_SORT_BY, @@ -891,6 +893,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box attrs::Attributes); store.register_late_pass(|| box blocks_in_if_conditions::BlocksInIfConditions); store.register_late_pass(|| box unicode::Unicode); + store.register_late_pass(|| box unit_return_expecting_ord::UnitReturnExpectingOrd); store.register_late_pass(|| box strings::StringAdd); store.register_late_pass(|| box implicit_return::ImplicitReturn); store.register_late_pass(|| box implicit_saturating_sub::ImplicitSaturatingSub); @@ -1436,6 +1439,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::UNNECESSARY_CAST), LintId::of(&types::VEC_BOX), LintId::of(&unicode::ZERO_WIDTH_SPACE), + LintId::of(&unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD), LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS), LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS), LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY), @@ -1692,6 +1696,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::CAST_REF_TO_MUT), LintId::of(&types::UNIT_CMP), LintId::of(&unicode::ZERO_WIDTH_SPACE), + LintId::of(&unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD), LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS), LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS), LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT), diff --git a/clippy_lints/src/unit_return_expecting_ord.rs b/clippy_lints/src/unit_return_expecting_ord.rs new file mode 100644 index 00000000000..fceb885516b --- /dev/null +++ b/clippy_lints/src/unit_return_expecting_ord.rs @@ -0,0 +1,177 @@ +use crate::utils::{get_trait_def_id, paths, span_lint, span_lint_and_help}; +use if_chain::if_chain; +use rustc_hir::def_id::DefId; +use rustc_hir::{Expr, ExprKind, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_middle::ty::{GenericPredicates, PredicateKind, ProjectionPredicate, TraitPredicate}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{BytePos, Span}; + +declare_clippy_lint! { + /// **What it does:** Checks for functions that expect closures of type + /// Fn(...) -> Ord where the implemented closure returns the unit type. + /// The lint also suggests to remove the semi-colon at the end of the statement if present. + /// + /// **Why is this bad?** Likely, returning the unit type is unintentional, and + /// could simply be caused by an extra semi-colon. Since () implements Ord + /// it doesn't cause a compilation error. + /// This is the same reasoning behind the unit_cmp lint. + /// + /// **Known problems:** If returning unit is intentional, then there is no + /// way of specifying this without triggering needless_return lint + /// + /// **Example:** + /// + /// ```rust + /// let mut twins = vec!((1,1), (2,2)); + /// twins.sort_by_key(|x| { x.1; }); + /// ``` + pub UNIT_RETURN_EXPECTING_ORD, + correctness, + "fn arguments of type Fn(...) -> Ord returning the unit type ()." +} + +declare_lint_pass!(UnitReturnExpectingOrd => [UNIT_RETURN_EXPECTING_ORD]); + +fn get_trait_predicates_for_trait_id<'tcx>( + cx: &LateContext<'tcx>, + generics: GenericPredicates<'tcx>, + trait_id: Option, +) -> Vec> { + let mut preds = Vec::new(); + for (pred, _) in generics.predicates { + if_chain! { + if let PredicateKind::Trait(poly_trait_pred, _) = pred.kind(); + let trait_pred = cx.tcx.erase_late_bound_regions(&poly_trait_pred); + if let Some(trait_def_id) = trait_id; + if trait_def_id == trait_pred.trait_ref.def_id; + then { + preds.push(trait_pred); + } + } + } + preds +} + +fn get_projection_pred<'tcx>( + cx: &LateContext<'tcx>, + generics: GenericPredicates<'tcx>, + pred: TraitPredicate<'tcx>, +) -> Option> { + generics.predicates.iter().find_map(|(proj_pred, _)| { + if let PredicateKind::Projection(proj_pred) = proj_pred.kind() { + let projection_pred = cx.tcx.erase_late_bound_regions(proj_pred); + if projection_pred.projection_ty.substs == pred.trait_ref.substs { + return Some(projection_pred); + } + } + None + }) +} + +fn get_args_to_check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Vec<(usize, String)> { + let mut args_to_check = Vec::new(); + if let Some(def_id) = cx.tables().type_dependent_def_id(expr.hir_id) { + let fn_sig = cx.tcx.fn_sig(def_id); + let generics = cx.tcx.predicates_of(def_id); + let fn_mut_preds = get_trait_predicates_for_trait_id(cx, generics, cx.tcx.lang_items().fn_mut_trait()); + let ord_preds = get_trait_predicates_for_trait_id(cx, generics, get_trait_def_id(cx, &paths::ORD)); + let partial_ord_preds = + get_trait_predicates_for_trait_id(cx, generics, cx.tcx.lang_items().partial_ord_trait()); + // Trying to call erase_late_bound_regions on fn_sig.inputs() gives the following error + // The trait `rustc::ty::TypeFoldable<'_>` is not implemented for `&[&rustc::ty::TyS<'_>]` + let inputs_output = cx.tcx.erase_late_bound_regions(&fn_sig.inputs_and_output()); + inputs_output + .iter() + .rev() + .skip(1) + .rev() + .enumerate() + .for_each(|(i, inp)| { + for trait_pred in &fn_mut_preds { + if_chain! { + if trait_pred.self_ty() == inp; + if let Some(return_ty_pred) = get_projection_pred(cx, generics, *trait_pred); + then { + if ord_preds.iter().any(|ord| ord.self_ty() == return_ty_pred.ty) { + args_to_check.push((i, "Ord".to_string())); + } else if partial_ord_preds.iter().any(|pord| pord.self_ty() == return_ty_pred.ty) { + args_to_check.push((i, "PartialOrd".to_string())); + } + } + } + } + }); + } + args_to_check +} + +fn check_arg<'tcx>(cx: &LateContext<'tcx>, arg: &'tcx Expr<'tcx>) -> Option<(Span, Option)> { + if_chain! { + if let ExprKind::Closure(_, _fn_decl, body_id, span, _) = arg.kind; + if let ty::Closure(_def_id, substs) = &cx.tables().node_type(arg.hir_id).kind; + let ret_ty = substs.as_closure().sig().output(); + let ty = cx.tcx.erase_late_bound_regions(&ret_ty); + if ty.is_unit(); + then { + if_chain! { + let body = cx.tcx.hir().body(body_id); + if let ExprKind::Block(block, _) = body.value.kind; + if block.expr.is_none(); + if let Some(stmt) = block.stmts.last(); + if let StmtKind::Semi(_) = stmt.kind; + then { + let data = stmt.span.data(); + // Make a span out of the semicolon for the help message + Some((span, Some(Span::new(data.hi-BytePos(1), data.hi, data.ctxt)))) + } else { + Some((span, None)) + } + } + } else { + None + } + } +} + +impl<'tcx> LateLintPass<'tcx> for UnitReturnExpectingOrd { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if let ExprKind::MethodCall(_, _, ref args, _) = expr.kind { + let arg_indices = get_args_to_check(cx, expr); + for (i, trait_name) in arg_indices { + if i < args.len() { + match check_arg(cx, &args[i]) { + Some((span, None)) => { + span_lint( + cx, + UNIT_RETURN_EXPECTING_ORD, + span, + &format!( + "this closure returns \ + the unit type which also implements {}", + trait_name + ), + ); + }, + Some((span, Some(last_semi))) => { + span_lint_and_help( + cx, + UNIT_RETURN_EXPECTING_ORD, + span, + &format!( + "this closure returns \ + the unit type which also implements {}", + trait_name + ), + Some(last_semi), + &"probably caused by this trailing semicolon".to_string(), + ); + }, + None => {}, + } + } + } + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index b89a8712862..96b004904aa 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -2292,6 +2292,13 @@ deprecation: None, module: "types", }, + Lint { + name: "unit_return_expecting_ord", + group: "correctness", + desc: "fn arguments of type Fn(...) -> Ord returning the unit type ().", + deprecation: None, + module: "unit_return_expecting_ord", + }, Lint { name: "unknown_clippy_lints", group: "style", diff --git a/tests/ui/unit_return_expecting_ord.rs b/tests/ui/unit_return_expecting_ord.rs new file mode 100644 index 00000000000..bdb4710cc69 --- /dev/null +++ b/tests/ui/unit_return_expecting_ord.rs @@ -0,0 +1,36 @@ +#![warn(clippy::unit_return_expecting_ord)] +#![allow(clippy::needless_return)] +#![allow(clippy::unused_unit)] +#![feature(is_sorted)] + +struct Struct { + field: isize, +} + +fn double(i: isize) -> isize { + i * 2 +} + +fn unit(_i: isize) {} + +fn main() { + let mut structs = vec![Struct { field: 2 }, Struct { field: 1 }]; + structs.sort_by_key(|s| { + double(s.field); + }); + structs.sort_by_key(|s| double(s.field)); + structs.is_sorted_by_key(|s| { + double(s.field); + }); + structs.is_sorted_by_key(|s| { + if s.field > 0 { + () + } else { + return (); + } + }); + structs.sort_by_key(|s| { + return double(s.field); + }); + structs.sort_by_key(|s| unit(s.field)); +} diff --git a/tests/ui/unit_return_expecting_ord.stderr b/tests/ui/unit_return_expecting_ord.stderr new file mode 100644 index 00000000000..e63d5874609 --- /dev/null +++ b/tests/ui/unit_return_expecting_ord.stderr @@ -0,0 +1,39 @@ +error: this closure returns the unit type which also implements Ord + --> $DIR/unit_return_expecting_ord.rs:18:25 + | +LL | structs.sort_by_key(|s| { + | ^^^ + | + = note: `-D clippy::unit-return-expecting-ord` implied by `-D warnings` +help: probably caused by this trailing semicolon + --> $DIR/unit_return_expecting_ord.rs:19:24 + | +LL | double(s.field); + | ^ + +error: this closure returns the unit type which also implements PartialOrd + --> $DIR/unit_return_expecting_ord.rs:22:30 + | +LL | structs.is_sorted_by_key(|s| { + | ^^^ + | +help: probably caused by this trailing semicolon + --> $DIR/unit_return_expecting_ord.rs:23:24 + | +LL | double(s.field); + | ^ + +error: this closure returns the unit type which also implements PartialOrd + --> $DIR/unit_return_expecting_ord.rs:25:30 + | +LL | structs.is_sorted_by_key(|s| { + | ^^^ + +error: this closure returns the unit type which also implements Ord + --> $DIR/unit_return_expecting_ord.rs:35:25 + | +LL | structs.sort_by_key(|s| unit(s.field)); + | ^^^ + +error: aborting due to 4 previous errors +