Skip to content

Commit 2ecc2ac

Browse files
tnielenstnielens
committed
unit-arg - improve suggestion
1 parent 6220dff commit 2ecc2ac

4 files changed

Lines changed: 115 additions & 112 deletions

File tree

‎clippy_lints/src/types.rs‎

Lines changed: 50 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ use rustc_hir as hir;
1111
use rustc_hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisitorMap, Visitor};
1212
use rustc_hir::{
1313
BinOpKind, Block, Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem,
14-
ImplItemKind, Item, ItemKind, Lifetime, Local, MatchSource, MutTy, Mutability, QPath, Stmt, StmtKind, TraitFn,
15-
TraitItem, TraitItemKind, TyKind, UnOp,
14+
ImplItemKind, Item, ItemKind, Lifetime, Local, MatchSource, MutTy, Mutability, Node, QPath, Stmt, StmtKind,
15+
TraitFn, TraitItem, TraitItemKind, TyKind, UnOp,
1616
};
1717
use rustc_lint::{LateContext, LateLintPass, LintContext};
1818
use rustc_middle::hir::map::Map;
@@ -29,10 +29,10 @@ use rustc_typeck::hir_ty_to_ty;
2929
use crate::consts::{constant, Constant};
3030
use crate::utils::paths;
3131
use crate::utils::{
32-
clip, comparisons, differing_macro_contexts, higher, in_constant, indent_of, int_bits, is_type_diagnostic_item,
32+
clip, comparisons, differing_macro_contexts, higher, in_constant, int_bits, is_type_diagnostic_item,
3333
last_path_segment, match_def_path, match_path, method_chain_args, multispan_sugg, numeric_literal::NumericLiteral,
34-
qpath_res, sext, snippet, snippet_block_with_applicability, snippet_opt, snippet_with_applicability,
35-
snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, unsext,
34+
qpath_res, sext, snippet, snippet_opt, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
35+
span_lint_and_help, span_lint_and_sugg, span_lint_and_then, unsext,
3636
};
3737

3838
declare_clippy_lint! {
@@ -844,43 +844,54 @@ fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Exp
844844
Applicability::MaybeIncorrect,
845845
);
846846
or = "or ";
847+
applicability = Applicability::MaybeIncorrect;
847848
});
848-
let sugg = args_to_recover
849+
850+
let arg_snippets: Vec<String> = args_to_recover
851+
.iter()
852+
.filter_map(|arg| snippet_opt(cx, arg.span))
853+
.collect();
854+
let arg_snippets_without_empty_blocks: Vec<String> = args_to_recover
849855
.iter()
850856
.filter(|arg| !is_empty_block(arg))
851-
.enumerate()
852-
.map(|(i, arg)| {
853-
let indent = if i == 0 {
854-
0
855-
} else {
856-
indent_of(cx, expr.span).unwrap_or(0)
857-
};
858-
format!(
859-
"{}{};",
860-
" ".repeat(indent),
861-
snippet_block_with_applicability(cx, arg.span, "..", Some(expr.span), &mut applicability)
862-
)
863-
})
864-
.collect::<Vec<String>>();
865-
let mut and = "";
866-
if !sugg.is_empty() {
867-
let plural = if sugg.len() > 1 { "s" } else { "" };
868-
db.span_suggestion(
869-
expr.span.with_hi(expr.span.lo()),
870-
&format!("{}move the expression{} in front of the call...", or, plural),
871-
format!("{}\n", sugg.join("\n")),
872-
applicability,
873-
);
874-
and = "...and "
857+
.filter_map(|arg| snippet_opt(cx, arg.span))
858+
.collect();
859+
860+
if let Some(mut sugg) = snippet_opt(cx, expr.span) {
861+
arg_snippets.iter().for_each(|arg| {
862+
sugg = sugg.replacen(arg, "()", 1);
863+
});
864+
sugg = format!("{}{}{}", arg_snippets_without_empty_blocks.join("; "), "; ", sugg);
865+
let parent_node = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(expr.hir_id));
866+
if !matches!(parent_node, Some(Node::Block(_))) && !matches!(parent_node, Some(Node::Stmt(_))) {
867+
// expr is not in a block statement or result expression position, wrap in a block
868+
sugg = format!("{{ {} }}", sugg);
869+
}
870+
871+
if arg_snippets_without_empty_blocks.is_empty() {
872+
db.multipart_suggestion(
873+
&format!("use {}unit literal{} instead", singular, plural),
874+
args_to_recover
875+
.iter()
876+
.map(|arg| (arg.span, "()".to_string()))
877+
.collect::<Vec<_>>(),
878+
applicability,
879+
);
880+
} else {
881+
let plural = arg_snippets_without_empty_blocks.len() > 1;
882+
let empty_or_s = if plural { "s" } else { "" };
883+
let it_or_them = if plural { "them" } else { "it" };
884+
db.span_suggestion(
885+
expr.span,
886+
&format!(
887+
"{}move the expression{} in front of the call and replace {} with the unit literal `()`",
888+
or, empty_or_s, it_or_them
889+
),
890+
sugg,
891+
applicability,
892+
);
893+
}
875894
}
876-
db.multipart_suggestion(
877-
&format!("{}use {}unit literal{} instead", and, singular, plural),
878-
args_to_recover
879-
.iter()
880-
.map(|arg| (arg.span, "()".to_string()))
881-
.collect::<Vec<_>>(),
882-
applicability,
883-
);
884895
},
885896
);
886897
}
@@ -2055,6 +2066,7 @@ impl PartialOrd for FullInt {
20552066
})
20562067
}
20572068
}
2069+
20582070
impl Ord for FullInt {
20592071
#[must_use]
20602072
fn cmp(&self, other: &Self) -> Ordering {

‎tests/ui/unit_arg.rs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#![warn(clippy::unit_arg)]
2-
#![allow(clippy::no_effect, unused_must_use, unused_variables)]
2+
#![allow(clippy::no_effect, unused_must_use, unused_variables, clippy::unused_unit)]
33

44
use std::fmt::Debug;
55

@@ -47,6 +47,11 @@ fn bad() {
4747
foo(3);
4848
},
4949
);
50+
// here Some(foo(2)) isn't the top level statement expression, wrap the suggestion in a block
51+
None.or(Some(foo(2)));
52+
// in this case, the suggestion can be inlined, no need for a surrounding block
53+
// foo(()); foo(()) instead of { foo(()); foo(()) }
54+
foo(foo(()))
5055
}
5156

5257
fn ok() {

‎tests/ui/unit_arg.stderr‎

Lines changed: 53 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -11,31 +11,23 @@ help: remove the semicolon from the last statement in the block
1111
|
1212
LL | 1
1313
|
14-
help: or move the expression in front of the call...
14+
help: or move the expression in front of the call and replace it with the unit literal `()`
1515
|
1616
LL | {
1717
LL | 1;
18-
LL | };
18+
LL | }; foo(());
1919
|
20-
help: ...and use a unit literal instead
21-
|
22-
LL | foo(());
23-
| ^^
2420

2521
error: passing a unit value to a function
2622
--> $DIR/unit_arg.rs:26:5
2723
|
2824
LL | foo(foo(1));
2925
| ^^^^^^^^^^^
3026
|
31-
help: move the expression in front of the call...
32-
|
33-
LL | foo(1);
27+
help: move the expression in front of the call and replace it with the unit literal `()`
3428
|
35-
help: ...and use a unit literal instead
36-
|
37-
LL | foo(());
38-
| ^^
29+
LL | foo(1); foo(());
30+
| ^^^^^^^^^^^^^^^
3931

4032
error: passing a unit value to a function
4133
--> $DIR/unit_arg.rs:27:5
@@ -50,17 +42,13 @@ help: remove the semicolon from the last statement in the block
5042
|
5143
LL | foo(2)
5244
|
53-
help: or move the expression in front of the call...
45+
help: or move the expression in front of the call and replace it with the unit literal `()`
5446
|
5547
LL | {
5648
LL | foo(1);
5749
LL | foo(2);
58-
LL | };
59-
|
60-
help: ...and use a unit literal instead
50+
LL | }; foo(());
6151
|
62-
LL | foo(());
63-
| ^^
6452

6553
error: passing a unit value to a function
6654
--> $DIR/unit_arg.rs:32:5
@@ -74,32 +62,23 @@ help: remove the semicolon from the last statement in the block
7462
|
7563
LL | 1
7664
|
77-
help: or move the expression in front of the call...
65+
help: or move the expression in front of the call and replace it with the unit literal `()`
7866
|
7967
LL | {
8068
LL | 1;
81-
LL | };
69+
LL | }; b.bar(());
8270
|
83-
help: ...and use a unit literal instead
84-
|
85-
LL | b.bar(());
86-
| ^^
8771

8872
error: passing unit values to a function
8973
--> $DIR/unit_arg.rs:35:5
9074
|
9175
LL | taking_multiple_units(foo(0), foo(1));
9276
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
9377
|
94-
help: move the expressions in front of the call...
95-
|
96-
LL | foo(0);
97-
LL | foo(1);
98-
|
99-
help: ...and use unit literals instead
78+
help: move the expressions in front of the call and replace them with the unit literal `()`
10079
|
101-
LL | taking_multiple_units((), ());
102-
| ^^ ^^
80+
LL | foo(0); foo(1); taking_multiple_units((), ());
81+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10382

10483
error: passing unit values to a function
10584
--> $DIR/unit_arg.rs:36:5
@@ -114,18 +93,13 @@ help: remove the semicolon from the last statement in the block
11493
|
11594
LL | foo(2)
11695
|
117-
help: or move the expressions in front of the call...
96+
help: or move the expressions in front of the call and replace them with the unit literal `()`
11897
|
119-
LL | foo(0);
120-
LL | {
98+
LL | foo(0); {
12199
LL | foo(1);
122100
LL | foo(2);
123-
LL | };
124-
|
125-
help: ...and use unit literals instead
101+
LL | }; taking_multiple_units((), ());
126102
|
127-
LL | taking_multiple_units((), ());
128-
| ^^ ^^
129103

130104
error: passing unit values to a function
131105
--> $DIR/unit_arg.rs:40:5
@@ -147,35 +121,56 @@ help: remove the semicolon from the last statement in the block
147121
|
148122
LL | foo(3)
149123
|
150-
help: or move the expressions in front of the call...
124+
help: or move the expressions in front of the call and replace them with the unit literal `()`
151125
|
152126
LL | {
153-
LL | foo(0);
154-
LL | foo(1);
155-
LL | };
156-
LL | {
157-
LL | foo(2);
127+
LL | foo(0);
128+
LL | foo(1);
129+
LL | }; {
130+
LL | foo(2);
131+
LL | foo(3);
158132
...
159-
help: ...and use unit literals instead
133+
134+
error: use of `or` followed by a function call
135+
--> $DIR/unit_arg.rs:51:10
160136
|
161-
LL | (),
162-
LL | (),
137+
LL | None.or(Some(foo(2)));
138+
| ^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some(foo(2)))`
163139
|
140+
= note: `-D clippy::or-fun-call` implied by `-D warnings`
164141

165142
error: passing a unit value to a function
166-
--> $DIR/unit_arg.rs:82:5
143+
--> $DIR/unit_arg.rs:51:13
167144
|
168-
LL | Some(foo(1))
145+
LL | None.or(Some(foo(2)));
146+
| ^^^^^^^^^^^^
147+
|
148+
help: move the expression in front of the call and replace it with the unit literal `()`
149+
|
150+
LL | None.or({ foo(2); Some(()) });
151+
| ^^^^^^^^^^^^^^^^^^^^
152+
153+
error: passing a unit value to a function
154+
--> $DIR/unit_arg.rs:54:5
155+
|
156+
LL | foo(foo(()))
169157
| ^^^^^^^^^^^^
170158
|
171-
help: move the expression in front of the call...
159+
help: move the expression in front of the call and replace it with the unit literal `()`
160+
|
161+
LL | foo(()); foo(())
162+
|
163+
164+
error: passing a unit value to a function
165+
--> $DIR/unit_arg.rs:87:5
166+
|
167+
LL | Some(foo(1))
168+
| ^^^^^^^^^^^^
172169
|
173-
LL | foo(1);
170+
help: move the expression in front of the call and replace it with the unit literal `()`
174171
|
175-
help: ...and use a unit literal instead
172+
LL | foo(1); Some(())
176173
|
177-
LL | Some(())
178-
| ^^
179174

180-
error: aborting due to 8 previous errors
175+
error: aborting due to 11 previous errors
181176

‎tests/ui/unit_arg_empty_blocks.stderr‎

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,30 +22,21 @@ error: passing unit values to a function
2222
LL | taking_two_units({}, foo(0));
2323
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2424
|
25-
help: move the expression in front of the call...
25+
help: move the expression in front of the call and replace it with the unit literal `()`
2626
|
27-
LL | foo(0);
28-
|
29-
help: ...and use unit literals instead
30-
|
31-
LL | taking_two_units((), ());
32-
| ^^ ^^
27+
LL | foo(0); taking_two_units((), ());
28+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3329

3430
error: passing unit values to a function
3531
--> $DIR/unit_arg_empty_blocks.rs:18:5
3632
|
3733
LL | taking_three_units({}, foo(0), foo(1));
3834
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3935
|
40-
help: move the expressions in front of the call...
41-
|
42-
LL | foo(0);
43-
LL | foo(1);
44-
|
45-
help: ...and use unit literals instead
36+
help: move the expressions in front of the call and replace them with the unit literal `()`
4637
|
47-
LL | taking_three_units((), (), ());
48-
| ^^ ^^ ^^
38+
LL | foo(0); foo(1); taking_three_units((), (), ());
39+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
4940

5041
error: aborting due to 4 previous errors
5142

0 commit comments

Comments
 (0)