diff --git a/CHANGELOG.md b/CHANGELOG.md index f3b1073988b0..f35b7aabf316 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5d5b5d9a6da5..dc5e25cc37e7 100644 --- a/clippy_lints/src/lib.rs +++ b/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), ]); } diff --git a/clippy_lints/src/unintentional_unit_return.rs b/clippy_lints/src/unintentional_unit_return.rs new file mode 100644 index 000000000000..aa750b7ec2f1 --- /dev/null +++ b/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> { + 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> { + 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)); + } + } + } + } + } + } +} diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 4a4ee5baf002..ea128ab187ba 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/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"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 3b89f5d19477..2afe6b173cf5 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -2201,6 +2201,13 @@ pub static ref ALL_LINTS: Vec = 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", diff --git a/tests/ui/unintentional_unit_return.rs b/tests/ui/unintentional_unit_return.rs new file mode 100644 index 000000000000..aa2ae8fa6f75 --- /dev/null +++ b/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(mut v: Vec, 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); + }); +} diff --git a/tests/ui/unintentional_unit_return.stderr b/tests/ui/unintentional_unit_return.stderr new file mode 100644 index 000000000000..262fefaa8cc5 --- /dev/null +++ b/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 +