new lint: Unintentional return of unit from closures expecting Ord

CHANGELOG.md

@@ -1505,6 +1505,7 @@ Released 2018-09-13
 [`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
 [`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
 [`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
+[`unintentional_unit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#unintentional_unit_return
 [`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
 [`unknown_clippy_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#unknown_clippy_lints

clippy_lints/src/lib.rs

@@ -310,6 +310,7 @@ pub mod try_err;
 pub mod types;
 pub mod unicode;
 pub mod unnamed_address;
+pub mod unintentional_unit_return;
 pub mod unsafe_removed_from_name;
 pub mod unused_io_amount;
 pub mod unused_self;
@@ -821,6 +822,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &unicode::ZERO_WIDTH_SPACE,
         &unnamed_address::FN_ADDRESS_COMPARISONS,
         &unnamed_address::VTABLE_ADDRESS_COMPARISONS,
+        &unintentional_unit_return::UNINTENTIONAL_UNIT_RETURN,
         &unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,
         &unused_io_amount::UNUSED_IO_AMOUNT,
         &unused_self::UNUSED_SELF,
@@ -1031,6 +1033,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box verbose_file_reads::VerboseFileReads);
     store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default());
     store.register_late_pass(|| box unnamed_address::UnnamedAddress);
+    store.register_late_pass(|| box unintentional_unit_return::UnintentionalUnitReturn);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1682,6 +1685,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE),
         LintId::of(&redundant_pub_crate::REDUNDANT_PUB_CRATE),
         LintId::of(&transmute::USELESS_TRANSMUTE),
+        LintId::of(&unintentional_unit_return::UNINTENTIONAL_UNIT_RETURN),
         LintId::of(&use_self::USE_SELF),
     ]);
 }

clippy_lints/src/unintentional_unit_return.rs

@@ -0,0 +1,155 @@
+use crate::utils::{get_trait_def_id, paths, span_lint};
+use if_chain::if_chain;
+use rustc_middle::ty::{GenericPredicates, Predicate, ProjectionPredicate, TraitPredicate};
+use rustc_hir::{Expr, ExprKind, Stmt, StmtKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for functions that expect closures of type
+    /// Fn(...) -> Ord where the implemented closure has a semi-colon
+    /// at the end of the last statement.
+    ///
+    /// **Why is this bad?** Likely the semi-colon is unintentional which
+    /// returns () instead of the result of last statement. Since () implements Ord
+    /// it doesn't cause a compilation error
+    ///
+    /// **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 UNINTENTIONAL_UNIT_RETURN,
+    nursery,
+    "fn arguments of type Fn(...) -> Once having last statements with a semi-colon, suggesting to remove the semi-colon if it is unintentional."
+}
+
+declare_lint_pass!(UnintentionalUnitReturn => [UNINTENTIONAL_UNIT_RETURN]);
+
+fn unwrap_trait_pred<'tcx>(cx: &LateContext<'_, 'tcx>, pred: &Predicate<'tcx>) -> Option<TraitPredicate<'tcx>> {
+    if let Predicate::Trait(poly_trait_pred, _) = pred {
+        let trait_pred = cx.tcx.erase_late_bound_regions(&poly_trait_pred);
+        Some(trait_pred)
+    } else {
+        None
+    }
+}
+
+fn get_predicates_for_trait_path<'tcx>(
+    cx: &LateContext<'_, 'tcx>,
+    generics: GenericPredicates<'tcx>,
+    trait_path: &[&str],
+) -> Vec<&'tcx Predicate<'tcx>> {
+    let mut preds = Vec::new();
+    generics.predicates.iter().for_each(|(pred, _)| {
+        if_chain! {
+            if let Some(trait_pred) = unwrap_trait_pred(cx, pred);
+            if let Some(trait_def_id) = get_trait_def_id(cx, trait_path);
+            if trait_def_id == trait_pred.trait_ref.def_id;
+            then {
+                    preds.push(pred);
+                }
+        }
+    });
+    preds
+}
+
+fn get_projection_pred<'tcx>(
+    cx: &LateContext<'_, 'tcx>,
+    generics: GenericPredicates<'tcx>,
+    pred: TraitPredicate<'tcx>,
+) -> Option<ProjectionPredicate<'tcx>> {
+    generics.predicates.iter().find_map(|(proj_pred, _)| {
+        if let Predicate::Projection(proj_pred) = proj_pred {
+            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_predicates_for_trait_path(cx, generics, &paths::FN_MUT);
+        let ord_preds = get_predicates_for_trait_path(cx, generics, &paths::ORD);
+        let partial_ord_preds = get_predicates_for_trait_path(cx, generics, &paths::PARTIAL_ORD);
+        // 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().enumerate().for_each(|(i, inp)| {
+            // Ignore output param
+            if i == inputs_output.len() - 1 {
+                return;
+            }
+            fn_mut_preds.iter().for_each(|pred| {
+                if_chain! {
+                    if let Some(trait_pred) = unwrap_trait_pred(cx, pred);
+                    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|  {
+                            unwrap_trait_pred(cx, ord).unwrap().self_ty()  == return_ty_pred.ty
+                        }) {
+                            args_to_check.push((i, "Ord".to_string()));
+                        }
+                        else if partial_ord_preds.iter().any(|pord| {
+                            unwrap_trait_pred(cx, pord).unwrap().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<&'tcx Stmt<'tcx>> {
+    if let ExprKind::Closure(_, _fn_decl, body_id, _span, _) = arg.kind {
+        if_chain! {
+            let body = cx.tcx.hir().body(body_id);
+            if let ExprKind::Block(block, _) = body.value.kind;
+            if let Some(stmt) = block.stmts.last();
+            if let StmtKind::Semi(_) = stmt.kind;
+            then { return Some(stmt) }
+        }
+    }
+    return None;
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnintentionalUnitReturn {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'tcx>) {
+        let arg_indices = get_args_to_check(cx, expr);
+        if_chain! {
+            if let ExprKind::MethodCall(_, _, ref args) = expr.kind;
+            then {
+                for (i, trait_name) in arg_indices {
+                    if_chain! {
+                        if i < args.len();
+                        if let Some(stmt) = check_arg(cx, &args[i]);
+                        then {
+                            //TODO : Maybe only filter the closures where the last statement return type also is an unit
+                            span_lint(cx,
+                                      UNINTENTIONAL_UNIT_RETURN,
+                                      stmt.span,
+                                      &format!(
+                                      "Semi-colon on the last line of this closure returns \
+                                       the unit type which also implements {}.",
+                                      trait_name));
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

clippy_lints/src/utils/paths.rs

@@ -37,6 +37,7 @@ pub const FILE_TYPE: [&str; 3] = ["std", "fs", "FileType"];
 pub const FMT_ARGUMENTS_NEW_V1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"];
 pub const FMT_ARGUMENTS_NEW_V1_FORMATTED: [&str; 4] = ["core", "fmt", "Arguments", "new_v1_formatted"];
 pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"];
+pub const FN_MUT: [&str; 3] = ["std", "ops", "FnMut"];
 pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"];
 pub const FROM_TRAIT: [&str; 3] = ["core", "convert", "From"];
 pub const HASH: [&str; 2] = ["hash", "Hash"];
@@ -71,6 +72,7 @@ pub const ORD: [&str; 3] = ["core", "cmp", "Ord"];
 pub const OS_STRING: [&str; 4] = ["std", "ffi", "os_str", "OsString"];
 pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString", "as_os_str"];
 pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"];
+pub const PARTIAL_ORD: [&str; 3] = ["std", "cmp", "PartialOrd"];
 pub const PATH: [&str; 3] = ["std", "path", "Path"];
 pub const PATH_BUF: [&str; 3] = ["std", "path", "PathBuf"];
 pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"];

src/lintlist/mod.rs

@@ -2201,6 +2201,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "methods",
     },
+    Lint {
+        name: "unintentional_unit_return",
+        group: "nursery",
+        desc: "fn arguments of type Fn(...) -> Once having last statements with a semi-colon, suggesting to remove the semi-colon if it is unintentional.",
+        deprecation: None,
+        module: "unintentional_unit_return",
+    },
     Lint {
         name: "unit_arg",
         group: "complexity",

tests/ui/unintentional_unit_return.rs

@@ -0,0 +1,29 @@
+#![warn(clippy::unintentional_unit_return)]
+#![feature(is_sorted)]
+
+struct Struct {
+    field: isize,
+}
+
+fn double(i: isize) -> isize {
+    i * 2
+}
+
+/*
+fn fn_with_closure<T, F, K>(mut v: Vec<T>, f: F) where
+    F: FnMut(&T) -> K,
+    K: Ord {
+    v.sort_by_key(f)
+}
+*/
+
+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);
+    });
+}

tests/ui/unintentional_unit_return.stderr

@@ -0,0 +1,16 @@
+error: Semi-colon on the last line of this closure returns the unit type which also implements Ord.
+  --> $DIR/unintentional_unit_return.rs:23:9
+   |
+LL |         double(s.field);
+   |         ^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::unintentional-unit-return` implied by `-D warnings`
+
+error: Semi-colon on the last line of this closure returns the unit type which also implements PartialOrd.
+  --> $DIR/unintentional_unit_return.rs:27:9
+   |
+LL |         double(s.field);
+   |         ^^^^^^^^^^^^^^^^
+
+error: aborting due to 2 previous errors
+