diff --git a/CHANGELOG.md b/CHANGELOG.md index c9bea1e8ef5..e6792c06894 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 14a5e56cdea..b07bac9b11a 100644 --- a/README.md +++ b/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: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c343cf364f6..3d8179e4782 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -685,6 +685,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, @@ -801,6 +802,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, diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index f4360802483..d083387e852 100644 --- a/clippy_lints/src/returns.rs +++ b/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: 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.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 + } +} diff --git a/tests/ui/copies.rs b/tests/ui/copies.rs index 8d0bc802daf..5c4bbecf822 100644 --- a/tests/ui/copies.rs +++ b/tests/ui/copies.rs @@ -7,11 +7,10 @@ // 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::needless_return, clippy::never_loop, clippy::no_effect, clippy::zero_divided_by_zero, clippy::unused_unit)] + + fn bar(_: 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 }; diff --git a/tests/ui/my_lint.rs b/tests/ui/my_lint.rs new file mode 100644 index 00000000000..c27fd5be134 --- /dev/null +++ b/tests/ui/my_lint.rs @@ -0,0 +1,7 @@ +#[clippy::author] +#[cfg(any(target_arch = "x86"))] +pub struct Foo { + x: u32, +} + +fn main() {} diff --git a/tests/ui/unused_unit.rs b/tests/ui/unused_unit.rs new file mode 100644 index 00000000000..a7f08c28939 --- /dev/null +++ b/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 or the MIT license +// , 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 (), 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(); +} diff --git a/tests/ui/unused_unit.stderr b/tests/ui/unused_unit.stderr new file mode 100644 index 00000000000..b5d5bdbcbee --- /dev/null +++ b/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 (), 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 +