Merge pull request #3233 from rust-lang-nursery/unused-unit

new lint: unused_unit

Author: oli-obk

CHANGELOG.md

@@ -878,6 +878,7 @@ All notable changes to this project will be documented in this file.
 [`unused_collect`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_collect
 [`unused_io_amount`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_io_amount
 [`unused_label`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_label
+[`unused_unit`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_unit
 [`use_debug`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#use_debug
 [`use_self`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#use_self
 [`used_underscore_binding`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#used_underscore_binding

README.md

@@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 279 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)
+[There are 280 lints included in this crate!](https://rust-lang-nursery.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

@@ -687,6 +687,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         regex::TRIVIAL_REGEX,
         returns::LET_AND_RETURN,
         returns::NEEDLESS_RETURN,
+        returns::UNUSED_UNIT,
         serde_api::SERDE_API_MISUSE,
         strings::STRING_LIT_AS_BYTES,
         suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL,
@@ -803,6 +804,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         regex::TRIVIAL_REGEX,
         returns::LET_AND_RETURN,
         returns::NEEDLESS_RETURN,
+        returns::UNUSED_UNIT,
         strings::STRING_LIT_AS_BYTES,
         types::FN_TO_NUMERIC_CAST,
         types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION,

clippy_lints/src/returns.rs

@@ -14,8 +14,8 @@ use if_chain::if_chain;
 use crate::syntax::ast;
 use crate::syntax::source_map::Span;
 use crate::syntax::visit::FnKind;
+use crate::syntax_pos::BytePos;
 use crate::rustc_errors::Applicability;
-
 use crate::utils::{in_macro, match_path_ast, snippet_opt, span_lint_and_then, span_note_and_lint};
 
 /// **What it does:** Checks for return statements at the end of a block.
@@ -68,6 +68,25 @@ declare_clippy_lint! {
      the end of a block"
 }
 
+/// **What it does:** Checks for unit (`()`) expressions that can be removed.
+///
+/// **Why is this bad?** Such expressions add no value, but can make the code
+/// less readable. Depending on formatting they can make a `break` or `return`
+/// statement look like a function call.
+///
+/// **Known problems:** The lint currently misses unit return types in types,
+/// e.g. the `F` in `fn generic_unit<F: Fn() -> ()>(f: F) { .. }`.
+///
+/// **Example:**
+/// ```rust
+/// fn return_unit() -> () { () }
+/// ```
+declare_clippy_lint! {
+    pub UNUSED_UNIT,
+    style,
+    "needless unit expression"
+}
+
 #[derive(Copy, Clone)]
 pub struct ReturnPass;
 
@@ -162,23 +181,98 @@ impl ReturnPass {
 
 impl LintPass for ReturnPass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(NEEDLESS_RETURN, LET_AND_RETURN)
+        lint_array!(NEEDLESS_RETURN, LET_AND_RETURN, UNUSED_UNIT)
     }
 }
 
 impl EarlyLintPass for ReturnPass {
-    fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, _: &ast::FnDecl, _: Span, _: ast::NodeId) {
+    fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, decl: &ast::FnDecl, span: Span, _: ast::NodeId) {
         match kind {
             FnKind::ItemFn(.., block) | FnKind::Method(.., block) => self.check_block_return(cx, block),
             FnKind::Closure(body) => self.check_final_expr(cx, body, Some(body.span)),
         }
+        if_chain! {
+            if let ast::FunctionRetTy::Ty(ref ty) = decl.output;
+            if let ast::TyKind::Tup(ref vals) = ty.node;
+            if vals.is_empty() && !in_macro(ty.span) && get_def(span) == get_def(ty.span);
+            then {
+                let (rspan, appl) = if let Ok(fn_source) =
+                        cx.sess().source_map()
+                                 .span_to_snippet(span.with_hi(ty.span.hi())) {
+                    if let Some(rpos) = fn_source.rfind("->") {
+                        (ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)),
+                            Applicability::MachineApplicable)
+                    } else {
+                        (ty.span, Applicability::MaybeIncorrect)
+                    }
+                } else {
+                    (ty.span, Applicability::MaybeIncorrect)
+                };
+                span_lint_and_then(cx, UNUSED_UNIT, rspan, "unneeded unit return type", |db| {
+                    db.span_suggestion_with_applicability(
+                        rspan,
+                        "remove the `-> ()`",
+                        String::new(),
+                        appl,
+                    );
+                });
+            }
+        }
     }
 
     fn check_block(&mut self, cx: &EarlyContext<'_>, block: &ast::Block) {
         self.check_let_return(cx, block);
+        if_chain! {
+            if let Some(ref stmt) = block.stmts.last();
+            if let ast::StmtKind::Expr(ref expr) = stmt.node;
+            if is_unit_expr(expr) && !in_macro(expr.span);
+            then {
+                let sp = expr.span;
+                span_lint_and_then(cx, UNUSED_UNIT, sp, "unneeded unit expression", |db| {
+                    db.span_suggestion_with_applicability(
+                        sp,
+                        "remove the final `()`",
+                        String::new(),
+                        Applicability::MachineApplicable,
+                    );
+                });
+            }
+        }
+    }
+
+    fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) {
+        match e.node {
+            ast::ExprKind::Ret(Some(ref expr)) | ast::ExprKind::Break(_, Some(ref expr)) => {
+                if is_unit_expr(expr) && !in_macro(expr.span) {
+                    span_lint_and_then(cx, UNUSED_UNIT, expr.span, "unneeded `()`", |db| {
+                        db.span_suggestion_with_applicability(
+                            expr.span,
+                            "remove the `()`",
+                            String::new(),
+                            Applicability::MachineApplicable,
+                        );
+                    });
+                }
+            }
+            _ => ()
+        }
     }
 }
 
 fn attr_is_cfg(attr: &ast::Attribute) -> bool {
     attr.meta_item_list().is_some() && attr.name() == "cfg"
 }
+
+// get the def site
+fn get_def(span: Span) -> Option<Span> {
+    span.ctxt().outer().expn_info().and_then(|info| info.def_site)
+}
+
+// is this expr a `()` unit?
+fn is_unit_expr(expr: &ast::Expr) -> bool {
+    if let ast::ExprKind::Tup(ref vals) = expr.node {
+        vals.is_empty()
+    } else {
+        false
+    }
+}

tests/ui/copies.rs

@@ -7,12 +7,11 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+#![allow(clippy::blacklisted_name, clippy::collapsible_if, clippy::cyclomatic_complexity, clippy::eq_op, clippy::needless_continue,
+         clippy::needless_return, clippy::never_loop, clippy::no_effect, clippy::zero_divided_by_zero, clippy::unused_unit)]
 
 
 
-#![allow(clippy::blacklisted_name, clippy::collapsible_if, clippy::cyclomatic_complexity, clippy::eq_op, clippy::needless_continue,
-         clippy::needless_return, clippy::never_loop, clippy::no_effect, clippy::zero_divided_by_zero)]
-
 fn bar<T>(_: T) {}
 fn foo() -> bool { unimplemented!() }
 
@@ -28,6 +27,7 @@ pub enum Abc {
 
 #[warn(clippy::if_same_then_else)]
 #[warn(clippy::match_same_arms)]
+#[allow(clippy::unused_unit)]
 fn if_same_then_else() -> Result<&'static str, ()> {
     if true {
         Foo { bar: 42 };

tests/ui/my_lint.rs

@@ -0,0 +1,7 @@
+#[clippy::author]
+#[cfg(any(target_arch = "x86"))]
+pub struct Foo {
+    x: u32,
+}
+
+fn main() {}

tests/ui/unused_unit.rs

@@ -0,0 +1,52 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// run-rustfix
+// compile-pass
+
+// The output for humans should just highlight the whole span without showing
+// the suggested replacement, but we also want to test that suggested
+// replacement only removes one set of parentheses, rather than naïvely
+// stripping away any starting or ending parenthesis characters—hence this
+// test of the JSON error format.
+
+
+#![deny(clippy::unused_unit)]
+#![allow(clippy::needless_return)]
+
+struct Unitter;
+
+impl Unitter {
+    // try to disorient the lint with multiple unit returns and newlines
+    pub fn get_unit<F: Fn() -> (), G>(&self, f: F, _g: G) ->
+        ()
+    where G: Fn() -> () {
+        let _y: &Fn() -> () = &f;
+        (); // this should not lint, as it's not in return type position
+    }
+}
+
+impl Into<()> for Unitter {
+    fn into(self) -> () {
+        ()
+    }
+}
+
+fn return_unit() -> () { () }
+
+fn main() {
+    let u = Unitter;
+    assert_eq!(u.get_unit(|| {}, return_unit), u.into());
+    return_unit();
+    loop {
+        break();
+    }
+    return();
+}

tests/ui/unused_unit.stderr

@@ -0,0 +1,52 @@
+error: unneeded unit return type
+  --> $DIR/unused_unit.rs:28:59
+   |
+28 |       pub fn get_unit<F: Fn() -> (), G>(&self, f: F, _g: G) ->
+   |  ___________________________________________________________^
+29 | |         ()
+   | |__________^ help: remove the `-> ()`
+   |
+note: lint level defined here
+  --> $DIR/unused_unit.rs:21:9
+   |
+21 | #![deny(clippy::unused_unit)]
+   |         ^^^^^^^^^^^^^^^^^^^
+
+error: unneeded unit return type
+  --> $DIR/unused_unit.rs:37:19
+   |
+37 |     fn into(self) -> () {
+   |                   ^^^^^ help: remove the `-> ()`
+
+error: unneeded unit expression
+  --> $DIR/unused_unit.rs:38:9
+   |
+38 |         ()
+   |         ^^ help: remove the final `()`
+
+error: unneeded unit return type
+  --> $DIR/unused_unit.rs:42:18
+   |
+42 | fn return_unit() -> () { () }
+   |                  ^^^^^ help: remove the `-> ()`
+
+error: unneeded unit expression
+  --> $DIR/unused_unit.rs:42:26
+   |
+42 | fn return_unit() -> () { () }
+   |                          ^^ help: remove the final `()`
+
+error: unneeded `()`
+  --> $DIR/unused_unit.rs:49:14
+   |
+49 |         break();
+   |              ^^ help: remove the `()`
+
+error: unneeded `()`
+  --> $DIR/unused_unit.rs:51:11
+   |
+51 |     return();
+   |           ^^ help: remove the `()`
+
+error: aborting due to 7 previous errors
+