Skip to content

Commit

Permalink
Auto merge of #9689 - koka831:fix/or_fun_call_map_or, r=flip1995
Browse files Browse the repository at this point in the history
fix: support `map_or` for `or_fun_call` lint

fixes #8993

The methods defined in `KNOW_TYPES`, except for `map_or`, accepts only one argument, so the matching `if let [arg] = args` works only for these methods.
This PR adds `rest_arg` argument to `check_general_case` method and handling of cases with two arguments to support `map_or`.

changelog: `or_fun_call` support `map_or`

* add `rest_arg` to pass second argument from `map_or(U, F)`
* extract some procedures into closure
  • Loading branch information
bors committed Oct 23, 2022
2 parents 3f4287c + a41cb7a commit b97d29a
Show file tree
Hide file tree
Showing 7 changed files with 92 additions and 21 deletions.
57 changes: 41 additions & 16 deletions clippy_lints/src/methods/or_fun_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ pub(super) fn check<'tcx>(
method_span: Span,
self_expr: &hir::Expr<'_>,
arg: &'tcx hir::Expr<'_>,
// `Some` if fn has second argument
second_arg: Option<&hir::Expr<'_>>,
span: Span,
// None if lambda is required
fun_span: Option<Span>,
Expand All @@ -109,30 +111,40 @@ pub(super) fn check<'tcx>(
if poss.contains(&name);

then {
let macro_expanded_snipped;
let sugg: Cow<'_, str> = {
let sugg = {
let (snippet_span, use_lambda) = match (fn_has_arguments, fun_span) {
(false, Some(fun_span)) => (fun_span, false),
_ => (arg.span, true),
};
let snippet = {
let not_macro_argument_snippet = snippet_with_macro_callsite(cx, snippet_span, "..");
if not_macro_argument_snippet == "vec![]" {
macro_expanded_snipped = snippet(cx, snippet_span, "..");

let format_span = |span: Span| {
let not_macro_argument_snippet = snippet_with_macro_callsite(cx, span, "..");
let snip = if not_macro_argument_snippet == "vec![]" {
let macro_expanded_snipped = snippet(cx, snippet_span, "..");
match macro_expanded_snipped.strip_prefix("$crate::vec::") {
Some(stripped) => Cow::from(stripped),
Some(stripped) => Cow::Owned(stripped.to_owned()),
None => macro_expanded_snipped,
}
} else {
not_macro_argument_snippet
}
};

snip.to_string()
};

if use_lambda {
let snip = format_span(snippet_span);
let snip = if use_lambda {
let l_arg = if fn_has_arguments { "_" } else { "" };
format!("|{l_arg}| {snippet}").into()
format!("|{l_arg}| {snip}")
} else {
snippet
snip
};

if let Some(f) = second_arg {
let f = format_span(f.span);
format!("{snip}, {f}")
} else {
snip
}
};
let span_replace_word = method_span.with_hi(span.hi());
Expand All @@ -149,8 +161,8 @@ pub(super) fn check<'tcx>(
}
}

if let [arg] = args {
let inner_arg = if let hir::ExprKind::Block(
let extract_inner_arg = |arg: &'tcx hir::Expr<'_>| {
if let hir::ExprKind::Block(
hir::Block {
stmts: [],
expr: Some(expr),
Expand All @@ -162,19 +174,32 @@ pub(super) fn check<'tcx>(
expr
} else {
arg
};
}
};

if let [arg] = args {
let inner_arg = extract_inner_arg(arg);
match inner_arg.kind {
hir::ExprKind::Call(fun, or_args) => {
let or_has_args = !or_args.is_empty();
if !check_unwrap_or_default(cx, name, fun, arg, or_has_args, expr.span, method_span) {
let fun_span = if or_has_args { None } else { Some(fun.span) };
check_general_case(cx, name, method_span, receiver, arg, expr.span, fun_span);
check_general_case(cx, name, method_span, receiver, arg, None, expr.span, fun_span);
}
},
hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => {
check_general_case(cx, name, method_span, receiver, arg, expr.span, None);
check_general_case(cx, name, method_span, receiver, arg, None, expr.span, None);
},
_ => (),
}
}

// `map_or` takes two arguments
if let [arg, lambda] = args {
let inner_arg = extract_inner_arg(arg);
if let hir::ExprKind::Call(fun, or_args) = inner_arg.kind {
let fun_span = if or_args.is_empty() { Some(fun.span) } else { None };
check_general_case(cx, name, method_span, receiver, arg, Some(lambda), expr.span, fun_span);
}
}
}
1 change: 1 addition & 0 deletions tests/ui/manual_ok_or.fixed
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// run-rustfix
#![warn(clippy::manual_ok_or)]
#![allow(clippy::or_fun_call)]
#![allow(clippy::disallowed_names)]
#![allow(clippy::redundant_closure)]
#![allow(dead_code)]
Expand Down
1 change: 1 addition & 0 deletions tests/ui/manual_ok_or.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// run-rustfix
#![warn(clippy::manual_ok_or)]
#![allow(clippy::or_fun_call)]
#![allow(clippy::disallowed_names)]
#![allow(clippy::redundant_closure)]
#![allow(dead_code)]
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/manual_ok_or.stderr
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
error: this pattern reimplements `Option::ok_or`
--> $DIR/manual_ok_or.rs:11:5
--> $DIR/manual_ok_or.rs:12:5
|
LL | foo.map_or(Err("error"), |v| Ok(v));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `foo.ok_or("error")`
|
= note: `-D clippy::manual-ok-or` implied by `-D warnings`

error: this pattern reimplements `Option::ok_or`
--> $DIR/manual_ok_or.rs:14:5
--> $DIR/manual_ok_or.rs:15:5
|
LL | foo.map_or(Err("error"), Ok);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `foo.ok_or("error")`

error: this pattern reimplements `Option::ok_or`
--> $DIR/manual_ok_or.rs:17:5
--> $DIR/manual_ok_or.rs:18:5
|
LL | None::<i32>.map_or(Err("error"), |v| Ok(v));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `None::<i32>.ok_or("error")`

error: this pattern reimplements `Option::ok_or`
--> $DIR/manual_ok_or.rs:21:5
--> $DIR/manual_ok_or.rs:22:5
|
LL | / foo.map_or(Err::<i32, &str>(
LL | | &format!(
Expand Down
16 changes: 16 additions & 0 deletions tests/ui/or_fun_call.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -236,4 +236,20 @@ mod issue9608 {
}
}

mod issue8993 {
fn g() -> i32 {
3
}

fn f(n: i32) -> i32 {
n
}

fn test_map_or() {
let _ = Some(4).map_or_else(g, |v| v);
let _ = Some(4).map_or_else(g, f);
let _ = Some(4).map_or(0, f);
}
}

fn main() {}
16 changes: 16 additions & 0 deletions tests/ui/or_fun_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,4 +236,20 @@ mod issue9608 {
}
}

mod issue8993 {
fn g() -> i32 {
3
}

fn f(n: i32) -> i32 {
n
}

fn test_map_or() {
let _ = Some(4).map_or(g(), |v| v);
let _ = Some(4).map_or(g(), f);
let _ = Some(4).map_or(0, f);
}
}

fn main() {}
14 changes: 13 additions & 1 deletion tests/ui/or_fun_call.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -156,5 +156,17 @@ error: use of `unwrap_or` followed by a call to `new`
LL | .unwrap_or(String::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_default()`

error: aborting due to 26 previous errors
error: use of `map_or` followed by a function call
--> $DIR/or_fun_call.rs:249:25
|
LL | let _ = Some(4).map_or(g(), |v| v);
| ^^^^^^^^^^^^^^^^^^ help: try this: `map_or_else(g, |v| v)`

error: use of `map_or` followed by a function call
--> $DIR/or_fun_call.rs:250:25
|
LL | let _ = Some(4).map_or(g(), f);
| ^^^^^^^^^^^^^^ help: try this: `map_or_else(g, f)`

error: aborting due to 28 previous errors

0 comments on commit b97d29a

Please sign in to comment.