Skip to content

Commit

Permalink
Catch function calls in argument lists, add tests that tuples don't g…
Browse files Browse the repository at this point in the history 
…et linted
  • Loading branch information
@bugadani
bugadani committed Jul 26, 2020
1 parent 2d8c05f commit 6d08a3b
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 37 deletions.
25 changes: 14 additions & 11 deletions clippy_lints/src/methods/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2684,12 +2684,8 @@ fn lint_lazy_eval<'a, 'tcx>(
paths.iter().any(|candidate| match_qpath(path, candidate))
}

if let hir::ExprKind::Closure(_, _, eid, _, _) = args[1].kind {
let body = cx.tcx.hir().body(eid);
let ex = &body.value;
let params = &body.params;

let simplify = match ex.kind {
fn can_simplify(expr: &hir::Expr<'_>, params: &[hir::Param<'_>], variant_calls: bool) -> bool {
match expr.kind {
// Closures returning literals can be unconditionally simplified
hir::ExprKind::Lit(_) => true,

Expand All @@ -2707,15 +2703,16 @@ fn lint_lazy_eval<'a, 'tcx>(
hir::ExprKind::Field(ref object, _) => !expr_uses_argument(object, params),

// Paths can be simplified if the root is not the argument, this also covers None
hir::ExprKind::Path(_) => !expr_uses_argument(ex, params),
hir::ExprKind::Path(_) => !expr_uses_argument(expr, params),

// Calls to Some, Ok, Err can be considered literals if they don't derive an argument
hir::ExprKind::Call(ref func, ref args) => if_chain! {
if allow_variant_calls; // Disable lint when rules conflict with bind_instead_of_map
if variant_calls; // Disable lint when rules conflict with bind_instead_of_map
if let hir::ExprKind::Path(ref path) = func.kind;
if match_any_qpath(path, &[&["Some"], &["Ok"], &["Err"]]);
then {
!args.iter().any(|arg| expr_uses_argument(arg, params))
// Recursively check all arguments
args.iter().all(|arg| can_simplify(arg, params, variant_calls))
} else {
false
}
Expand All @@ -2724,9 +2721,15 @@ fn lint_lazy_eval<'a, 'tcx>(
// For anything more complex than the above, a closure is probably the right solution,
// or the case is handled by an other lint
_ => false,
};
}
}

if let hir::ExprKind::Closure(_, _, eid, _, _) = args[1].kind {
let body = cx.tcx.hir().body(eid);
let ex = &body.value;
let params = &body.params;

if simplify {
if can_simplify(ex, params, allow_variant_calls) {
let msg = if is_option {
"unnecessary closure used to substitute value for `Option::None`"
} else {
Expand Down
8 changes: 7 additions & 1 deletion tests/ui/unnecessary_lazy_eval.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@ fn main() {
let ext_arr: [usize; 1] = [2];
let ext_str = SomeStruct { some_field: 10 };

// Should lint - Option
let mut opt = Some(42);
let ext_opt = Some(42);
let nested_opt = Some(Some(42));
let nested_tuple_opt = Some(Some((42, 43)));

// Should lint - Option
let _ = opt.unwrap_or(2);
let _ = opt.unwrap_or(astronomers_pi);
let _ = opt.unwrap_or(ext_str.some_field);
Expand Down Expand Up @@ -56,6 +59,9 @@ fn main() {

// Should not lint - Option
let _ = opt.unwrap_or_else(|| ext_str.return_some_field());
let _ = nested_opt.unwrap_or_else(|| Some(some_call()));
let _ = nested_tuple_opt.unwrap_or_else(|| Some((1, 2)));
let _ = nested_tuple_opt.unwrap_or_else(|| Some((some_call(), some_call())));
let _ = opt.or_else(some_call);
let _ = opt.or_else(|| some_call());
let _: Result<usize, usize> = opt.ok_or_else(|| some_call());
Expand Down
8 changes: 7 additions & 1 deletion tests/ui/unnecessary_lazy_eval.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@ fn main() {
let ext_arr: [usize; 1] = [2];
let ext_str = SomeStruct { some_field: 10 };

// Should lint - Option
let mut opt = Some(42);
let ext_opt = Some(42);
let nested_opt = Some(Some(42));
let nested_tuple_opt = Some(Some((42, 43)));

// Should lint - Option
let _ = opt.unwrap_or_else(|| 2);
let _ = opt.unwrap_or_else(|| astronomers_pi);
let _ = opt.unwrap_or_else(|| ext_str.some_field);
Expand Down Expand Up @@ -56,6 +59,9 @@ fn main() {

// Should not lint - Option
let _ = opt.unwrap_or_else(|| ext_str.return_some_field());
let _ = nested_opt.unwrap_or_else(|| Some(some_call()));
let _ = nested_tuple_opt.unwrap_or_else(|| Some((1, 2)));
let _ = nested_tuple_opt.unwrap_or_else(|| Some((some_call(), some_call())));
let _ = opt.or_else(some_call);
let _ = opt.or_else(|| some_call());
let _: Result<usize, usize> = opt.ok_or_else(|| some_call());
Expand Down
48 changes: 24 additions & 24 deletions tests/ui/unnecessary_lazy_eval.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,145 +1,145 @@
error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:31:13
--> $DIR/unnecessary_lazy_eval.rs:34:13
|
LL | let _ = opt.unwrap_or_else(|| 2);
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `unwrap_or` instead: `opt.unwrap_or(2)`
|
= note: `-D clippy::unnecessary-lazy-evaluation` implied by `-D warnings`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:32:13
--> $DIR/unnecessary_lazy_eval.rs:35:13
|
LL | let _ = opt.unwrap_or_else(|| astronomers_pi);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `unwrap_or` instead: `opt.unwrap_or(astronomers_pi)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:33:13
--> $DIR/unnecessary_lazy_eval.rs:36:13
|
LL | let _ = opt.unwrap_or_else(|| ext_str.some_field);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `unwrap_or` instead: `opt.unwrap_or(ext_str.some_field)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:34:13
--> $DIR/unnecessary_lazy_eval.rs:37:13
|
LL | let _ = opt.unwrap_or_else(|| ext_arr[0]);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `unwrap_or` instead: `opt.unwrap_or(ext_arr[0])`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:35:13
--> $DIR/unnecessary_lazy_eval.rs:38:13
|
LL | let _ = opt.and_then(|_| ext_opt);
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `and` instead: `opt.and(ext_opt)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:36:13
--> $DIR/unnecessary_lazy_eval.rs:39:13
|
LL | let _ = opt.or_else(|| ext_opt);
| ^^^^^^^^^^^^^^^^^^^^^^^ help: Use `or` instead: `opt.or(ext_opt)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:37:13
--> $DIR/unnecessary_lazy_eval.rs:40:13
|
LL | let _ = opt.or_else(|| None);
| ^^^^^^^^^^^^^^^^^^^^ help: Use `or` instead: `opt.or(None)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:38:13
--> $DIR/unnecessary_lazy_eval.rs:41:13
|
LL | let _ = opt.get_or_insert_with(|| 2);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `get_or_insert` instead: `opt.get_or_insert(2)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:39:13
--> $DIR/unnecessary_lazy_eval.rs:42:13
|
LL | let _ = opt.ok_or_else(|| 2);
| ^^^^^^^^^^^^^^^^^^^^ help: Use `ok_or` instead: `opt.ok_or(2)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:40:13
--> $DIR/unnecessary_lazy_eval.rs:43:13
|
LL | let _ = opt.ok_or_else(|| ext_arr[0]);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `ok_or` instead: `opt.ok_or(ext_arr[0])`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:43:13
--> $DIR/unnecessary_lazy_eval.rs:46:13
|
LL | let _ = Some(10).unwrap_or_else(|| 2);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `unwrap_or` instead: `Some(10).unwrap_or(2)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:44:13
--> $DIR/unnecessary_lazy_eval.rs:47:13
|
LL | let _ = Some(10).and_then(|_| ext_opt);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `and` instead: `Some(10).and(ext_opt)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:45:28
--> $DIR/unnecessary_lazy_eval.rs:48:28
|
LL | let _: Option<usize> = None.or_else(|| ext_opt);
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `or` instead: `None.or(ext_opt)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:46:13
--> $DIR/unnecessary_lazy_eval.rs:49:13
|
LL | let _ = None.get_or_insert_with(|| 2);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `get_or_insert` instead: `None.get_or_insert(2)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:47:35
--> $DIR/unnecessary_lazy_eval.rs:50:35
|
LL | let _: Result<usize, usize> = None.ok_or_else(|| 2);
| ^^^^^^^^^^^^^^^^^^^^^ help: Use `ok_or` instead: `None.ok_or(2)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:48:28
--> $DIR/unnecessary_lazy_eval.rs:51:28
|
LL | let _: Option<usize> = None.or_else(|| None);
| ^^^^^^^^^^^^^^^^^^^^^ help: Use `or` instead: `None.or(None)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:51:13
--> $DIR/unnecessary_lazy_eval.rs:54:13
|
LL | let _ = deep.0.unwrap_or_else(|| 2);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `unwrap_or` instead: `deep.0.unwrap_or(2)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:52:13
--> $DIR/unnecessary_lazy_eval.rs:55:13
|
LL | let _ = deep.0.and_then(|_| ext_opt);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `and` instead: `deep.0.and(ext_opt)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:53:13
--> $DIR/unnecessary_lazy_eval.rs:56:13
|
LL | let _ = deep.0.or_else(|| None);
| ^^^^^^^^^^^^^^^^^^^^^^^ help: Use `or` instead: `deep.0.or(None)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:54:13
--> $DIR/unnecessary_lazy_eval.rs:57:13
|
LL | let _ = deep.0.get_or_insert_with(|| 2);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `get_or_insert` instead: `deep.0.get_or_insert(2)`

error: unnecessary closure used to substitute value for `Option::None`
--> $DIR/unnecessary_lazy_eval.rs:55:13
--> $DIR/unnecessary_lazy_eval.rs:58:13
|
LL | let _ = deep.0.ok_or_else(|| 2);
| ^^^^^^^^^^^^^^^^^^^^^^^ help: Use `ok_or` instead: `deep.0.ok_or(2)`

error: unnecessary closure used to substitute value for `Result::Err`
--> $DIR/unnecessary_lazy_eval.rs:78:13
--> $DIR/unnecessary_lazy_eval.rs:84:13
|
LL | let _ = res2.unwrap_or_else(|_| 2);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `unwrap_or` instead: `res2.unwrap_or(2)`

error: unnecessary closure used to substitute value for `Result::Err`
--> $DIR/unnecessary_lazy_eval.rs:79:13
--> $DIR/unnecessary_lazy_eval.rs:85:13
|
LL | let _ = res2.unwrap_or_else(|_| astronomers_pi);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `unwrap_or` instead: `res2.unwrap_or(astronomers_pi)`

error: unnecessary closure used to substitute value for `Result::Err`
--> $DIR/unnecessary_lazy_eval.rs:80:13
--> $DIR/unnecessary_lazy_eval.rs:86:13
|
LL | let _ = res2.unwrap_or_else(|_| ext_str.some_field);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Use `unwrap_or` instead: `res2.unwrap_or(ext_str.some_field)`
Expand Down

0 comments on commit 6d08a3b

Please sign in to comment.