Skip to content

Commit e9ebc00

Browse files
matthiaskrgrUriopass
authored andcommitted
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 reprise: rebase, update and address all concerns
1 parent c493090 commit e9ebc00

File tree

6 files changed

+263
-0
lines changed

6 files changed

+263
-0
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1673,6 +1673,7 @@ Released 2018-09-13
16731673
[`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
16741674
[`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
16751675
[`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
1676+
[`unintentional_unit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#unintentional_unit_return
16761677
[`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
16771678
[`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp
16781679
[`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
@@ -298,6 +298,7 @@ mod trivially_copy_pass_by_ref;
298298
mod try_err;
299299
mod types;
300300
mod unicode;
301+
mod unintentional_unit_return;
301302
mod unnamed_address;
302303
mod unnecessary_sort_by;
303304
mod unnested_or_patterns;
@@ -820,6 +821,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
820821
&unicode::NON_ASCII_LITERAL,
821822
&unicode::UNICODE_NOT_NFC,
822823
&unicode::ZERO_WIDTH_SPACE,
824+
&unintentional_unit_return::UNINTENTIONAL_UNIT_RETURN,
823825
&unnamed_address::FN_ADDRESS_COMPARISONS,
824826
&unnamed_address::VTABLE_ADDRESS_COMPARISONS,
825827
&unnecessary_sort_by::UNNECESSARY_SORT_BY,
@@ -885,6 +887,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
885887
store.register_late_pass(|| box attrs::Attributes);
886888
store.register_late_pass(|| box blocks_in_if_conditions::BlocksInIfConditions);
887889
store.register_late_pass(|| box unicode::Unicode);
890+
store.register_late_pass(|| box unintentional_unit_return::UnintentionalUnitReturn);
888891
store.register_late_pass(|| box strings::StringAdd);
889892
store.register_late_pass(|| box implicit_return::ImplicitReturn);
890893
store.register_late_pass(|| box implicit_saturating_sub::ImplicitSaturatingSub);
@@ -1727,6 +1730,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
17271730
LintId::of(&path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE),
17281731
LintId::of(&redundant_pub_crate::REDUNDANT_PUB_CRATE),
17291732
LintId::of(&transmute::USELESS_TRANSMUTE),
1733+
LintId::of(&unintentional_unit_return::UNINTENTIONAL_UNIT_RETURN),
17301734
LintId::of(&use_self::USE_SELF),
17311735
]);
17321736
}
Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
use crate::utils::{get_trait_def_id, paths, span_lint, span_lint_and_help};
2+
use if_chain::if_chain;
3+
use rustc_hir::def_id::DefId;
4+
use rustc_hir::{Expr, ExprKind, StmtKind};
5+
use rustc_lint::{LateContext, LateLintPass};
6+
use rustc_middle::ty;
7+
use rustc_middle::ty::{GenericPredicates, PredicateKind, ProjectionPredicate, TraitPredicate};
8+
use rustc_session::{declare_lint_pass, declare_tool_lint};
9+
use rustc_span::{BytePos, Span};
10+
11+
declare_clippy_lint! {
12+
/// **What it does:** Checks for functions that expect closures of type
13+
/// Fn(...) -> Ord where the implemented closure has a semi-colon
14+
/// at the end of the last statement.
15+
///
16+
/// **Why is this bad?** Likely the semi-colon is unintentional which
17+
/// returns () instead of the result of last statement. Since () implements Ord
18+
/// it doesn't cause a compilation error
19+
///
20+
/// **Known problems:** If returning unit is intentional, then there is no
21+
/// way of specifying this without triggering needless_return lint
22+
///
23+
/// **Example:**
24+
///
25+
/// ```rust
26+
/// let mut twins = vec!((1,1), (2,2));
27+
/// twins.sort_by_key(|x| { x.1; });
28+
/// ```
29+
pub UNINTENTIONAL_UNIT_RETURN,
30+
nursery,
31+
"fn arguments of type Fn(...) -> Once having last statements with a semi-colon, suggesting to remove the semi-colon if it is unintentional."
32+
}
33+
34+
declare_lint_pass!(UnintentionalUnitReturn => [UNINTENTIONAL_UNIT_RETURN]);
35+
36+
fn get_trait_predicates_for_trait_id<'tcx>(
37+
cx: &LateContext<'tcx>,
38+
generics: GenericPredicates<'tcx>,
39+
trait_id: Option<DefId>,
40+
) -> Vec<TraitPredicate<'tcx>> {
41+
let mut preds = Vec::new();
42+
for (pred, _) in generics.predicates {
43+
if_chain! {
44+
if let PredicateKind::Trait(poly_trait_pred, _) = pred.kind();
45+
let trait_pred = cx.tcx.erase_late_bound_regions(&poly_trait_pred);
46+
if let Some(trait_def_id) = trait_id;
47+
if trait_def_id == trait_pred.trait_ref.def_id;
48+
then {
49+
preds.push(trait_pred);
50+
}
51+
}
52+
}
53+
preds
54+
}
55+
56+
fn get_projection_pred<'tcx>(
57+
cx: &LateContext<'tcx>,
58+
generics: GenericPredicates<'tcx>,
59+
pred: TraitPredicate<'tcx>,
60+
) -> Option<ProjectionPredicate<'tcx>> {
61+
generics.predicates.iter().find_map(|(proj_pred, _)| {
62+
if let PredicateKind::Projection(proj_pred) = proj_pred.kind() {
63+
let projection_pred = cx.tcx.erase_late_bound_regions(proj_pred);
64+
if projection_pred.projection_ty.substs == pred.trait_ref.substs {
65+
return Some(projection_pred);
66+
}
67+
}
68+
None
69+
})
70+
}
71+
72+
fn get_args_to_check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Vec<(usize, String)> {
73+
let mut args_to_check = Vec::new();
74+
if let Some(def_id) = cx.tables().type_dependent_def_id(expr.hir_id) {
75+
let fn_sig = cx.tcx.fn_sig(def_id);
76+
let generics = cx.tcx.predicates_of(def_id);
77+
let fn_mut_preds = get_trait_predicates_for_trait_id(cx, generics, cx.tcx.lang_items().fn_mut_trait());
78+
let ord_preds = get_trait_predicates_for_trait_id(cx, generics, get_trait_def_id(cx, &paths::ORD));
79+
let partial_ord_preds =
80+
get_trait_predicates_for_trait_id(cx, generics, cx.tcx.lang_items().partial_ord_trait());
81+
// Trying to call erase_late_bound_regions on fn_sig.inputs() gives the following error
82+
// The trait `rustc::ty::TypeFoldable<'_>` is not implemented for `&[&rustc::ty::TyS<'_>]`
83+
let inputs_output = cx.tcx.erase_late_bound_regions(&fn_sig.inputs_and_output());
84+
inputs_output
85+
.iter()
86+
.rev()
87+
.skip(1)
88+
.rev()
89+
.enumerate()
90+
.for_each(|(i, inp)| {
91+
for trait_pred in &fn_mut_preds {
92+
if_chain! {
93+
if trait_pred.self_ty() == inp;
94+
if let Some(return_ty_pred) = get_projection_pred(cx, generics, *trait_pred);
95+
then {
96+
if ord_preds.iter().any(|ord| ord.self_ty() == return_ty_pred.ty) {
97+
args_to_check.push((i, "Ord".to_string()));
98+
} else if partial_ord_preds.iter().any(|pord| pord.self_ty() == return_ty_pred.ty) {
99+
args_to_check.push((i, "PartialOrd".to_string()));
100+
}
101+
}
102+
}
103+
}
104+
});
105+
}
106+
args_to_check
107+
}
108+
109+
fn check_arg<'tcx>(cx: &LateContext<'tcx>, arg: &'tcx Expr<'tcx>) -> Option<(Span, Option<Span>)> {
110+
if_chain! {
111+
if let ExprKind::Closure(_, _fn_decl, body_id, span, _) = arg.kind;
112+
if let ty::Closure(_def_id, substs) = &cx.tables().node_type(arg.hir_id).kind;
113+
let ret_ty = substs.as_closure().sig().output();
114+
let ty = cx.tcx.erase_late_bound_regions(&ret_ty);
115+
if ty.is_unit();
116+
then {
117+
if_chain! {
118+
let body = cx.tcx.hir().body(body_id);
119+
if let ExprKind::Block(block, _) = body.value.kind;
120+
if block.expr.is_none();
121+
if let Some(stmt) = block.stmts.last();
122+
if let StmtKind::Semi(_) = stmt.kind;
123+
then {
124+
let data = stmt.span.data();
125+
// Make a span out of the semicolon for the help message
126+
Some((span, Some(Span::new(data.hi-BytePos(1), data.hi, data.ctxt))))
127+
} else {
128+
Some((span, None))
129+
}
130+
}
131+
} else {
132+
None
133+
}
134+
}
135+
}
136+
137+
impl<'tcx> LateLintPass<'tcx> for UnintentionalUnitReturn {
138+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
139+
if let ExprKind::MethodCall(_, _, ref args, _) = expr.kind {
140+
let arg_indices = get_args_to_check(cx, expr);
141+
for (i, trait_name) in arg_indices {
142+
if i < args.len() {
143+
match check_arg(cx, &args[i]) {
144+
Some((span, None)) => {
145+
span_lint(
146+
cx,
147+
UNINTENTIONAL_UNIT_RETURN,
148+
span,
149+
&format!(
150+
"this closure returns \
151+
the unit type which also implements {}",
152+
trait_name
153+
),
154+
);
155+
},
156+
Some((span, Some(last_semi))) => {
157+
span_lint_and_help(
158+
cx,
159+
UNINTENTIONAL_UNIT_RETURN,
160+
span,
161+
&format!(
162+
"this closure returns \
163+
the unit type which also implements {}",
164+
trait_name
165+
),
166+
Some(last_semi),
167+
&"probably caused by this trailing semicolon".to_string(),
168+
);
169+
},
170+
None => {},
171+
}
172+
}
173+
}
174+
}
175+
}
176+
}

src/lintlist/mod.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2250,6 +2250,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
22502250
deprecation: None,
22512251
module: "methods",
22522252
},
2253+
Lint {
2254+
name: "unintentional_unit_return",
2255+
group: "nursery",
2256+
desc: "fn arguments of type Fn(...) -> Once having last statements with a semi-colon, suggesting to remove the semi-colon if it is unintentional.",
2257+
deprecation: None,
2258+
module: "unintentional_unit_return",
2259+
},
22532260
Lint {
22542261
name: "unit_arg",
22552262
group: "complexity",

tests/ui/unintentional_unit_return.rs

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
#![warn(clippy::unintentional_unit_return)]
2+
#![allow(clippy::needless_return)]
3+
#![allow(clippy::unused_unit)]
4+
#![feature(is_sorted)]
5+
6+
struct Struct {
7+
field: isize,
8+
}
9+
10+
fn double(i: isize) -> isize {
11+
i * 2
12+
}
13+
14+
fn unit(_i: isize) {}
15+
16+
fn main() {
17+
let mut structs = vec![Struct { field: 2 }, Struct { field: 1 }];
18+
structs.sort_by_key(|s| {
19+
double(s.field);
20+
});
21+
structs.sort_by_key(|s| double(s.field));
22+
structs.is_sorted_by_key(|s| {
23+
double(s.field);
24+
});
25+
structs.is_sorted_by_key(|s| {
26+
if s.field > 0 {
27+
()
28+
} else {
29+
return ();
30+
}
31+
});
32+
structs.sort_by_key(|s| {
33+
return double(s.field);
34+
});
35+
structs.sort_by_key(|s| unit(s.field));
36+
}
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
error: this closure returns the unit type which also implements Ord
2+
--> $DIR/unintentional_unit_return.rs:18:25
3+
|
4+
LL | structs.sort_by_key(|s| {
5+
| ^^^
6+
|
7+
= note: `-D clippy::unintentional-unit-return` implied by `-D warnings`
8+
help: probably caused by this trailing semicolon
9+
--> $DIR/unintentional_unit_return.rs:19:24
10+
|
11+
LL | double(s.field);
12+
| ^
13+
14+
error: this closure returns the unit type which also implements PartialOrd
15+
--> $DIR/unintentional_unit_return.rs:22:30
16+
|
17+
LL | structs.is_sorted_by_key(|s| {
18+
| ^^^
19+
|
20+
help: probably caused by this trailing semicolon
21+
--> $DIR/unintentional_unit_return.rs:23:24
22+
|
23+
LL | double(s.field);
24+
| ^
25+
26+
error: this closure returns the unit type which also implements PartialOrd
27+
--> $DIR/unintentional_unit_return.rs:25:30
28+
|
29+
LL | structs.is_sorted_by_key(|s| {
30+
| ^^^
31+
32+
error: this closure returns the unit type which also implements Ord
33+
--> $DIR/unintentional_unit_return.rs:35:25
34+
|
35+
LL | structs.sort_by_key(|s| unit(s.field));
36+
| ^^^
37+
38+
error: aborting due to 4 previous errors
39+

0 commit comments

Comments
 (0)