Skip to content

fix: support map_or for or_fun_call lint #9689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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