Skip to content

Commit b3c2cd0

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 c211cea commit b3c2cd0

File tree

7 files changed

+214
-0
lines changed

7 files changed

+214
-0
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1505,6 +1505,7 @@ Released 2018-09-13
15051505
[`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
15061506
[`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
15071507
[`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
1508+
[`unintentional_unit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#unintentional_unit_return
15081509
[`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
15091510
[`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp
15101511
[`unknown_clippy_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#unknown_clippy_lints

clippy_lints/src/lib.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,7 @@ pub mod try_err;
310310
pub mod types;
311311
pub mod unicode;
312312
pub mod unnamed_address;
313+
pub mod unintentional_unit_return;
313314
pub mod unsafe_removed_from_name;
314315
pub mod unused_io_amount;
315316
pub mod unused_self;
@@ -821,6 +822,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
821822
&unicode::ZERO_WIDTH_SPACE,
822823
&unnamed_address::FN_ADDRESS_COMPARISONS,
823824
&unnamed_address::VTABLE_ADDRESS_COMPARISONS,
825+
&unintentional_unit_return::UNINTENTIONAL_UNIT_RETURN,
824826
&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,
825827
&unused_io_amount::UNUSED_IO_AMOUNT,
826828
&unused_self::UNUSED_SELF,
@@ -1031,6 +1033,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
10311033
store.register_late_pass(|| box verbose_file_reads::VerboseFileReads);
10321034
store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default());
10331035
store.register_late_pass(|| box unnamed_address::UnnamedAddress);
1036+
store.register_late_pass(|| box unintentional_unit_return::UnintentionalUnitReturn);
10341037

10351038
store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
10361039
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1682,6 +1685,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
16821685
LintId::of(&path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE),
16831686
LintId::of(&redundant_pub_crate::REDUNDANT_PUB_CRATE),
16841687
LintId::of(&transmute::USELESS_TRANSMUTE),
1688+
LintId::of(&unintentional_unit_return::UNINTENTIONAL_UNIT_RETURN),
16851689
LintId::of(&use_self::USE_SELF),
16861690
]);
16871691
}
Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
use crate::utils::{get_trait_def_id, paths, span_lint};
2+
use if_chain::if_chain;
3+
use rustc_middle::ty::{GenericPredicates, Predicate, ProjectionPredicate, TraitPredicate};
4+
use rustc_hir::{Expr, ExprKind, Stmt, 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_chain! {
50+
if let Some(trait_pred) = unwrap_trait_pred(cx, pred);
51+
if let Some(trait_def_id) = get_trait_def_id(cx, trait_path);
52+
if trait_def_id == trait_pred.trait_ref.def_id;
53+
then {
54+
preds.push(pred);
55+
}
56+
}
57+
});
58+
preds
59+
}
60+
61+
fn get_projection_pred<'tcx>(
62+
cx: &LateContext<'_, 'tcx>,
63+
generics: GenericPredicates<'tcx>,
64+
pred: TraitPredicate<'tcx>,
65+
) -> Option<ProjectionPredicate<'tcx>> {
66+
generics.predicates.iter().find_map(|(proj_pred, _)| {
67+
if let Predicate::Projection(proj_pred) = proj_pred {
68+
let projection_pred = cx.tcx.erase_late_bound_regions(proj_pred);
69+
if projection_pred.projection_ty.substs == pred.trait_ref.substs {
70+
return Some(projection_pred);
71+
}
72+
}
73+
None
74+
})
75+
}
76+
77+
fn get_args_to_check<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &'tcx Expr<'tcx>) -> Vec<(usize, String)> {
78+
let mut args_to_check = Vec::new();
79+
if let Some(def_id) = cx.tables.type_dependent_def_id(expr.hir_id) {
80+
let fn_sig = cx.tcx.fn_sig(def_id);
81+
let generics = cx.tcx.predicates_of(def_id);
82+
let fn_mut_preds = get_predicates_for_trait_path(cx, generics, &paths::FN_MUT);
83+
let ord_preds = get_predicates_for_trait_path(cx, generics, &paths::ORD);
84+
let partial_ord_preds = get_predicates_for_trait_path(cx, generics, &paths::PARTIAL_ORD);
85+
// Trying to call erase_late_bound_regions on fn_sig.inputs() gives the following error
86+
// The trait `rustc::ty::TypeFoldable<'_>` is not implemented for `&[&rustc::ty::TyS<'_>]`
87+
let inputs_output = cx.tcx.erase_late_bound_regions(&fn_sig.inputs_and_output());
88+
inputs_output.iter().enumerate().for_each(|(i, inp)| {
89+
// Ignore output param
90+
if i == inputs_output.len() - 1 {
91+
return;
92+
}
93+
fn_mut_preds.iter().for_each(|pred| {
94+
if_chain! {
95+
if let Some(trait_pred) = unwrap_trait_pred(cx, pred);
96+
if trait_pred.self_ty() == *inp;
97+
if let Some(return_ty_pred) = get_projection_pred(cx, generics, trait_pred);
98+
then {
99+
if ord_preds.iter().any(|ord| {
100+
unwrap_trait_pred(cx, ord).unwrap().self_ty() == return_ty_pred.ty
101+
}) {
102+
args_to_check.push((i, "Ord".to_string()));
103+
}
104+
else if partial_ord_preds.iter().any(|pord| {
105+
unwrap_trait_pred(cx, pord).unwrap().self_ty() == return_ty_pred.ty
106+
}) {
107+
args_to_check.push((i, "PartialOrd".to_string()));
108+
}
109+
}
110+
}
111+
});
112+
});
113+
}
114+
args_to_check
115+
}
116+
117+
fn check_arg<'tcx>(cx: &LateContext<'_, 'tcx>, arg: &'tcx Expr<'tcx>) -> Option<&'tcx Stmt<'tcx>> {
118+
if let ExprKind::Closure(_, _fn_decl, body_id, _span, _) = arg.kind {
119+
if_chain! {
120+
let body = cx.tcx.hir().body(body_id);
121+
if let ExprKind::Block(block, _) = body.value.kind;
122+
if let Some(stmt) = block.stmts.last();
123+
if let StmtKind::Semi(_) = stmt.kind;
124+
then { return Some(stmt) }
125+
}
126+
}
127+
return None;
128+
}
129+
130+
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnintentionalUnitReturn {
131+
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'tcx>) {
132+
let arg_indices = get_args_to_check(cx, expr);
133+
if_chain! {
134+
if let ExprKind::MethodCall(_, _, ref args) = expr.kind;
135+
then {
136+
for (i, trait_name) in arg_indices {
137+
if_chain! {
138+
if i < args.len();
139+
if let Some(stmt) = check_arg(cx, &args[i]);
140+
then {
141+
//TODO : Maybe only filter the closures where the last statement return type also is an unit
142+
span_lint(cx,
143+
UNINTENTIONAL_UNIT_RETURN,
144+
stmt.span,
145+
&format!(
146+
"Semi-colon on the last line of this closure returns \
147+
the unit type which also implements {}.",
148+
trait_name));
149+
}
150+
}
151+
}
152+
}
153+
}
154+
}
155+
}

clippy_lints/src/utils/paths.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ pub const FILE_TYPE: [&str; 3] = ["std", "fs", "FileType"];
3737
pub const FMT_ARGUMENTS_NEW_V1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"];
3838
pub const FMT_ARGUMENTS_NEW_V1_FORMATTED: [&str; 4] = ["core", "fmt", "Arguments", "new_v1_formatted"];
3939
pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"];
40+
pub const FN_MUT: [&str; 3] = ["std", "ops", "FnMut"];
4041
pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"];
4142
pub const FROM_TRAIT: [&str; 3] = ["core", "convert", "From"];
4243
pub const HASH: [&str; 2] = ["hash", "Hash"];
@@ -71,6 +72,7 @@ pub const ORD: [&str; 3] = ["core", "cmp", "Ord"];
7172
pub const OS_STRING: [&str; 4] = ["std", "ffi", "os_str", "OsString"];
7273
pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString", "as_os_str"];
7374
pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"];
75+
pub const PARTIAL_ORD: [&str; 3] = ["std", "cmp", "PartialOrd"];
7476
pub const PATH: [&str; 3] = ["std", "path", "Path"];
7577
pub const PATH_BUF: [&str; 3] = ["std", "path", "PathBuf"];
7678
pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"];

src/lintlist/mod.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2201,6 +2201,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
22012201
deprecation: None,
22022202
module: "methods",
22032203
},
2204+
Lint {
2205+
name: "unintentional_unit_return",
2206+
group: "nursery",
2207+
desc: "fn arguments of type Fn(...) -> Once having last statements with a semi-colon, suggesting to remove the semi-colon if it is unintentional.",
2208+
deprecation: None,
2209+
module: "unintentional_unit_return",
2210+
},
22042211
Lint {
22052212
name: "unit_arg",
22062213
group: "complexity",

tests/ui/unintentional_unit_return.rs

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
#![warn(clippy::unintentional_unit_return)]
2+
#![feature(is_sorted)]
3+
4+
struct Struct {
5+
field: isize,
6+
}
7+
8+
fn double(i: isize) -> isize {
9+
i * 2
10+
}
11+
12+
/*
13+
fn fn_with_closure<T, F, K>(mut v: Vec<T>, f: F) where
14+
F: FnMut(&T) -> K,
15+
K: Ord {
16+
v.sort_by_key(f)
17+
}
18+
*/
19+
20+
fn main() {
21+
let mut structs = vec![Struct { field: 2 }, Struct { field: 1 }];
22+
structs.sort_by_key(|s| {
23+
double(s.field);
24+
});
25+
structs.sort_by_key(|s| double(s.field));
26+
structs.is_sorted_by_key(|s| {
27+
double(s.field);
28+
});
29+
}
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
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:23:9
3+
|
4+
LL | double(s.field);
5+
| ^^^^^^^^^^^^^^^^
6+
|
7+
= note: `-D clippy::unintentional-unit-return` implied by `-D warnings`
8+
9+
error: Semi-colon on the last line of this closure returns the unit type which also implements PartialOrd.
10+
--> $DIR/unintentional_unit_return.rs:27:9
11+
|
12+
LL | double(s.field);
13+
| ^^^^^^^^^^^^^^^^
14+
15+
error: aborting due to 2 previous errors
16+

0 commit comments

Comments
 (0)