new lint: Unintentional return of 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

Author: rabisg0

CHANGELOG.md

@@ -1503,6 +1503,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

README.md

@@ -5,7 +5,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 361 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 362 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/lib.rs

@@ -307,6 +307,7 @@ pub mod trivially_copy_pass_by_ref;
 pub mod try_err;
 pub mod types;
 pub mod unicode;
+pub mod unintentional_unit_return;
 pub mod unsafe_removed_from_name;
 pub mod unused_io_amount;
 pub mod unused_self;
@@ -812,6 +813,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,
+        &unintentional_unit_return::UNINTENTIONAL_UNIT_RETURN,
         &unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,
         &unused_io_amount::UNUSED_IO_AMOUNT,
         &unused_self::UNUSED_SELF,
@@ -1021,6 +1023,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box wildcard_imports::WildcardImports);
     store.register_early_pass(|| box macro_use::MacroUseImports);
     store.register_late_pass(|| box verbose_file_reads::VerboseFileReads);
+    store.register_late_pass(|| box unintentional_unit_return::UnintentionalUnitReturn);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1670,6 +1673,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&mutex_atomic::MUTEX_INTEGER),
         LintId::of(&needless_borrow::NEEDLESS_BORROW),
         LintId::of(&path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE),
+        LintId::of(&unintentional_unit_return::UNINTENTIONAL_UNIT_RETURN),
         LintId::of(&use_self::USE_SELF),
     ]);
 }

clippy_lints/src/unintentional_unit_return.rs

@@ -0,0 +1,65 @@
+use crate::utils::span_lint;
+use if_chain::if_chain;
+use rustc_data_structures::fx::FxHashMap;
+use rustc_hir::{Expr, ExprKind, 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]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnintentionalUnitReturn {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
+        // Whitelist of function_name -> [argument_index] which are checked
+        let mut func_args_to_lint = FxHashMap::default();
+        func_args_to_lint.insert("sort_by_key".to_string(), vec![1]);
+
+        if_chain! {
+            if let ExprKind::MethodCall(ref path, _, ref args) = expr.kind;
+            if let Some(arg_indices) = func_args_to_lint.get(&path.ident.to_string());
+
+            then {
+                for i in arg_indices {
+                    if_chain! {
+                        if *i < args.len();
+                        if let ExprKind::Closure(_, _fn_decl, body_id, _span, _) = args[*i].kind;
+                        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 {
+                            //TODO : Maybe only filter the closures where the last statement return type also is an unit
+                            span_lint(cx,
+                                      UNINTENTIONAL_UNIT_RETURN,
+                                      stmt.span,
+                                      "Semi-colon on the last line of this closure returns \
+                                       the unit type which also implements Ord.");
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 361] = [
+pub const ALL_LINTS: [Lint; 362] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -2191,6 +2191,13 @@ pub const ALL_LINTS: [Lint; 361] = [
         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,17 @@
+#![warn(clippy::unintentional_unit_return)]
+
+struct Struct {
+    field: isize,
+}
+
+fn double(i: isize) -> isize {
+    i * 2
+}
+
+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));
+}

tests/ui/unintentional_unit_return.stderr

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