Skip to content

Commit e7d54ce

Browse files
committed
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
1 parent 0e5e2c4 commit e7d54ce

File tree

8 files changed

+178
-2
lines changed

8 files changed

+178
-2
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1503,6 +1503,7 @@ Released 2018-09-13
15031503
[`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
15041504
[`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
15051505
[`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
1506+
[`unintentional_unit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#unintentional_unit_return
15061507
[`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
15071508
[`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp
15081509
[`unknown_clippy_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#unknown_clippy_lints

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
77

8-
[There are 361 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
8+
[There are 362 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
99

1010
We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
1111

clippy_lints/src/lib.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,7 @@ pub mod trivially_copy_pass_by_ref;
307307
pub mod try_err;
308308
pub mod types;
309309
pub mod unicode;
310+
pub mod unintentional_unit_return;
310311
pub mod unsafe_removed_from_name;
311312
pub mod unused_io_amount;
312313
pub mod unused_self;
@@ -812,6 +813,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
812813
&unicode::NON_ASCII_LITERAL,
813814
&unicode::UNICODE_NOT_NFC,
814815
&unicode::ZERO_WIDTH_SPACE,
816+
&unintentional_unit_return::UNINTENTIONAL_UNIT_RETURN,
815817
&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,
816818
&unused_io_amount::UNUSED_IO_AMOUNT,
817819
&unused_self::UNUSED_SELF,
@@ -1021,6 +1023,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
10211023
store.register_late_pass(|| box wildcard_imports::WildcardImports);
10221024
store.register_early_pass(|| box macro_use::MacroUseImports);
10231025
store.register_late_pass(|| box verbose_file_reads::VerboseFileReads);
1026+
store.register_late_pass(|| box unintentional_unit_return::UnintentionalUnitReturn);
10241027

10251028
store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
10261029
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1670,6 +1673,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
16701673
LintId::of(&mutex_atomic::MUTEX_INTEGER),
16711674
LintId::of(&needless_borrow::NEEDLESS_BORROW),
16721675
LintId::of(&path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE),
1676+
LintId::of(&unintentional_unit_return::UNINTENTIONAL_UNIT_RETURN),
16731677
LintId::of(&use_self::USE_SELF),
16741678
]);
16751679
}
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
use crate::utils::{get_trait_def_id, paths, span_lint};
2+
use if_chain::if_chain;
3+
use rustc::ty::{GenericPredicates, Predicate, ProjectionPredicate, TraitPredicate};
4+
use rustc_hir::{Expr, ExprKind, StmtKind};
5+
use rustc_lint::{LateContext, LateLintPass};
6+
use rustc_session::{declare_lint_pass, declare_tool_lint};
7+
8+
declare_clippy_lint! {
9+
/// **What it does:** Checks for functions that expect closures of type
10+
/// Fn(...) -> Ord where the implemented closure has a semi-colon
11+
/// at the end of the last statement.
12+
///
13+
/// **Why is this bad?** Likely the semi-colon is unintentional which
14+
/// returns () instead of the result of last statement. Since () implements Ord
15+
/// it doesn't cause a compilation error
16+
///
17+
/// **Known problems:** If returning unit is intentional, then there is no
18+
/// way of specifying this without triggering needless_return lint
19+
///
20+
/// **Example:**
21+
///
22+
/// ```rust
23+
/// let mut twins = vec!((1,1), (2,2));
24+
/// twins.sort_by_key(|x| { x.1; });
25+
/// ```
26+
pub UNINTENTIONAL_UNIT_RETURN,
27+
nursery,
28+
"fn arguments of type Fn(...) -> Once having last statements with a semi-colon, suggesting to remove the semi-colon if it is unintentional."
29+
}
30+
31+
declare_lint_pass!(UnintentionalUnitReturn => [UNINTENTIONAL_UNIT_RETURN]);
32+
33+
fn unwrap_trait_pred<'tcx>(cx: &LateContext<'_, 'tcx>, pred: &Predicate<'tcx>) -> Option<TraitPredicate<'tcx>> {
34+
if let Predicate::Trait(poly_trait_pred, _) = pred {
35+
let trait_pred = cx.tcx.erase_late_bound_regions(&poly_trait_pred);
36+
Some(trait_pred)
37+
} else {
38+
None
39+
}
40+
}
41+
42+
fn get_predicates_for_trait_path<'tcx>(
43+
cx: &LateContext<'_, 'tcx>,
44+
generics: GenericPredicates<'tcx>,
45+
trait_path: &[&str],
46+
) -> Vec<&'tcx Predicate<'tcx>> {
47+
let mut preds = Vec::new();
48+
generics.predicates.iter().for_each(|(pred, _)| {
49+
if let Some(trait_pred) = unwrap_trait_pred(cx, pred) {
50+
if let Some(trait_def_id) = get_trait_def_id(cx, trait_path) {
51+
if trait_def_id == trait_pred.trait_ref.def_id {
52+
preds.push(pred);
53+
}
54+
}
55+
}
56+
});
57+
preds
58+
}
59+
60+
fn get_projection_pred<'tcx>(
61+
cx: &LateContext<'_, 'tcx>,
62+
generics: GenericPredicates<'tcx>,
63+
pred: TraitPredicate<'tcx>,
64+
) -> Option<ProjectionPredicate<'tcx>> {
65+
generics.predicates.iter().find_map(|(proj_pred, _)| {
66+
if let Predicate::Projection(proj_pred) = proj_pred {
67+
let projection_pred = cx.tcx.erase_late_bound_regions(proj_pred);
68+
if projection_pred.projection_ty.substs == pred.trait_ref.substs {
69+
return Some(projection_pred);
70+
}
71+
}
72+
None
73+
})
74+
}
75+
76+
fn get_args_to_check<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &'tcx Expr<'tcx>) -> Vec<usize> {
77+
let mut args_to_check = Vec::new();
78+
if let Some(def_id) = cx.tables.type_dependent_def_id(expr.hir_id) {
79+
let fn_sig = cx.tcx.fn_sig(def_id);
80+
let generics = cx.tcx.predicates_of(def_id);
81+
let fn_mut_preds = get_predicates_for_trait_path(cx, generics, &paths::FN_MUT);
82+
let ord_preds = get_predicates_for_trait_path(cx, generics, &paths::ORD);
83+
// Trying to call erase_late_bound_regions on fn_sig.inputs() gives the following error
84+
// The trait `rustc::ty::TypeFoldable<'_>` is not implemented for `&[&rustc::ty::TyS<'_>]`
85+
let inputs_output = cx.tcx.erase_late_bound_regions(&fn_sig.inputs_and_output());
86+
inputs_output.iter().enumerate().for_each(|(i, inp)| {
87+
// Ignore output param
88+
if i == inputs_output.len() - 1 {
89+
return;
90+
}
91+
fn_mut_preds.iter().for_each(|pred| {
92+
let trait_pred = unwrap_trait_pred(cx, pred).unwrap();
93+
if trait_pred.self_ty() == *inp {
94+
if let Some(projection_pred) = get_projection_pred(cx, generics, trait_pred) {
95+
let ret_is_ord = ord_preds
96+
.iter()
97+
.any(|ord_pred| unwrap_trait_pred(cx, ord_pred).unwrap().self_ty() == projection_pred.ty);
98+
if ret_is_ord {
99+
args_to_check.push(i);
100+
}
101+
}
102+
}
103+
});
104+
});
105+
}
106+
args_to_check
107+
}
108+
109+
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnintentionalUnitReturn {
110+
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'tcx>) {
111+
let arg_indices = get_args_to_check(cx, expr);
112+
if_chain! {
113+
if let ExprKind::MethodCall(_, _, ref args) = expr.kind;
114+
then {
115+
for i in arg_indices {
116+
if_chain! {
117+
if i < args.len();
118+
if let ExprKind::Closure(_, _fn_decl, body_id, _span, _) = args[i].kind;
119+
let body = cx.tcx.hir().body(body_id);
120+
if let ExprKind::Block(block, _) = body.value.kind;
121+
if let Some(stmt) = block.stmts.last();
122+
if let StmtKind::Semi(_) = stmt.kind;
123+
then {
124+
//TODO : Maybe only filter the closures where the last statement return type also is an unit
125+
span_lint(cx,
126+
UNINTENTIONAL_UNIT_RETURN,
127+
stmt.span,
128+
"Semi-colon on the last line of this closure returns \
129+
the unit type which also implements Ord.");
130+
}
131+
}
132+
}
133+
}
134+
}
135+
}
136+
}

clippy_lints/src/utils/paths.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ pub const FILE_TYPE: [&str; 3] = ["std", "fs", "FileType"];
3636
pub const FMT_ARGUMENTS_NEW_V1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"];
3737
pub const FMT_ARGUMENTS_NEW_V1_FORMATTED: [&str; 4] = ["core", "fmt", "Arguments", "new_v1_formatted"];
3838
pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"];
39+
pub const FN_MUT: [&str; 3] = ["std", "ops", "FnMut"];
3940
pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"];
4041
pub const FROM_TRAIT: [&str; 3] = ["core", "convert", "From"];
4142
pub const HASH: [&str; 2] = ["hash", "Hash"];

src/lintlist/mod.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ pub use lint::Lint;
66
pub use lint::LINT_LEVELS;
77

88
// begin lint list, do not remove this comment, it’s used in `update_lints`
9-
pub const ALL_LINTS: [Lint; 361] = [
9+
pub const ALL_LINTS: [Lint; 362] = [
1010
Lint {
1111
name: "absurd_extreme_comparisons",
1212
group: "correctness",
@@ -2191,6 +2191,13 @@ pub const ALL_LINTS: [Lint; 361] = [
21912191
deprecation: None,
21922192
module: "methods",
21932193
},
2194+
Lint {
2195+
name: "unintentional_unit_return",
2196+
group: "nursery",
2197+
desc: "fn arguments of type Fn(...) -> Once having last statements with a semi-colon, suggesting to remove the semi-colon if it is unintentional.",
2198+
deprecation: None,
2199+
module: "unintentional_unit_return",
2200+
},
21942201
Lint {
21952202
name: "unit_arg",
21962203
group: "complexity",

tests/ui/unintentional_unit_return.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
#![warn(clippy::unintentional_unit_return)]
2+
3+
struct Struct {
4+
field: isize,
5+
}
6+
7+
fn double(i: isize) -> isize {
8+
i * 2
9+
}
10+
11+
fn main() {
12+
let mut structs = vec![Struct { field: 2 }, Struct { field: 1 }];
13+
structs.sort_by_key(|s| {
14+
double(s.field);
15+
});
16+
structs.sort_by_key(|s| double(s.field));
17+
}
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
error: Semi-colon on the last line of this closure returns the unit type which also implements Ord.
2+
--> $DIR/unintentional_unit_return.rs:14:9
3+
|
4+
LL | double(s.field);
5+
| ^^^^^^^^^^^^^^^^
6+
|
7+
= note: `-D clippy::unintentional-unit-return` implied by `-D warnings`
8+
9+
error: aborting due to previous error
10+

0 commit comments

Comments
 (0)