fix suggestion in option_map_or_none

[togami2864]

fix: #7960
changelog: change suggestion in the lint rule option_map_or_none

[rust-highfive]

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

[llogiq]

I think this introduces a change of behavior. Please add a test case with the type of the operation given, e.g. change let _ = .. to let _: Option<i32> or some such.

[llogiq]

I think you should also remove the Some( ), otherwise you'll change the type from Option<_> to Option<Option<_>>.

[togami2864]

Is it correct that I should somehow remove Some ( ) in this line?

rust-clippy/clippy_lints/src/methods/option_map_or_none.rs

Line 54 in f51fb34

let func_snippet = snippet(cx, map_arg.span, "..");

[llogiq]

Exactly. The problem here is that while this should usually be Some(_) which is an ExprCall(..) with a single item in args, it could also be any other expression that evaluates to an Option, e.g. { let foo = Some(bar); foo } or function_returning_option().

The right way to do this is to check if this is a ExprKind::Call to Some(_), then remove the call and use the zeroeth argument with the map, or using the original expression with and_then otherwise.

[togami2864]

Now, I don't understand how to identify whether Some is in the code or not.

What I did

case1:

I tried to identify it by match with expr.kind and write like this.

if_chain! {
                if let hir::ExprKind::MethodCall(path, _, _,_ ) = &expr.kind;
                if let hir::ExprKind::Path(ref qpath) = path[0].kind;
                 .....
            }

But I couldn't get the path of Some( ) in Closure which is expected as the oneth arg in map_or.

case2:

I tried to identify it by match with map_arg.

rust-clippy/clippy_lints/src/methods/option_map_or_none.rs

Line 20 in f82bf47

map_arg: &'tcx hir::Expr<'_>,

However, the type of map_arg differs depending on the code you lint. (in this case ExprKind::Closure or ExprKind::Path.)

In both cases, I couldn't specify Some() in code.
Would you have any advice? Due to my limited understanding, it is possible that my approach is fundamentally wrong.

[llogiq]

First, this is not an ExprKind::MethodCall, but an ExprKind::Call. The rest has been done before, e.g. in clippy_lints/src/methods/chars_cmp.rs#L22-26.

[togami2864]

Thanks.
I tried to output expr.kind, but it seems to be judged as ExprKind::MethodCall🤔

rust-clippy/clippy_lints/src/methods/option_map_or_none.rs

Line 17 in f82bf47

expr: &'tcx hir::Expr<'_>,

println!({:?}, expr.kind)

Result

MethodCall(PathSegment { ident: map_or#0, hir_id: Some(HirId { owner: DefId(0:3 ~ option_map_or_none[aa57]::main), local_id: 26 }), res: Some(Err), args: None, infer_args: true }, tests/ui/option_map_or_none.rs:13:30: 13:36 (#0), [Expr { hir_id: HirId { owner: DefId(0:3 ~ option_map_or_none[aa57]::main), local_id: 28 }, kind: Path(Resolved(None, Path { span: tests/ui/option_map_or_none.rs:13:26: 13:29 (#0), res: Local(HirId { owner: DefId(0:3 ~ option_map_or_none[aa57]::main), local_id: 7 }), segments: [PathSegment { ident: opt#0, hir_id: Some(HirId { owner: DefId(0:3 ~ option_map_or_none[aa57]::main), local_id: 27 }), res: Some(Local(HirId { owner: DefId(0:3 ~ option_map_or_none[aa57]::main), local_id: 7 })), args: None, infer_args: true }] })), span: tests/ui/option_map_or_none.rs:13:26: 13:29 (#0) }, Expr { hir_id: HirId { owner: DefId(0:3 ~ option_map_or_none[aa57]::main), local_id: 30 }, kind: Path(Resolved(None, Path { span: tests/ui/option_map_or_none.rs:13:37: 13:41 (#0), res: Def(Ctor(Variant, Const), DefId(2:36962 ~ core[489a]::option::Option::None::{constructor#0})), segments: [PathSegment { ident: None#0, hir_id: Some(HirId { owner: DefId(0:3 ~ option_map_or_none[aa57]::main), local_id: 29 }), res: Some(Err), args: None, infer_args: true }] })), span: tests/ui/option_map_or_none.rs:13:37: 13:41 (#0) }, Expr { hir_id: HirId { owner: DefId(0:3 ~ option_map_or_none[aa57]::main), local_id: 41 }, kind: Closure(Ref, FnDecl { inputs: [Ty { hir_id: HirId { owner: DefId(0:3 ~ option_map_or_none[aa57]::main), local_id: 40 }, kind: Infer, span: tests/ui/option_map_or_none.rs:13:44: 13:45 (#0) }], output: DefaultReturn(tests/ui/option_map_or_none.rs:13:47: 13:47 (#0)), c_variadic: false, implicit_self: None }, BodyId { hir_id: HirId { owner: DefId(0:3 ~ option_map_or_none[aa57]::main), local_id: 39 } }, tests/ui/option_map_or_none.rs:13:43: 13:46 (#0), None), span: tests/ui/option_map_or_none.rs:13:43: 13:58 (#0) }], tests/ui/option_map_or_none.rs:13:30: 13:59 (#0))

[llogiq]

I wasn't talking about the .map_or(..) call expr, but the 2nd argument, which is a closure of which you need to get the Body first. Sorry I forgot about that.

[llogiq]

This is already looking quite good. I only found one final problem with blocks in closures, then it should be ready to merge.

[llogiq]

Great! I think this is ready for merging now. Thank you for your patience in seeing this through.

@bors r+

[bors]

📌 Commit 8e317f5 has been approved by llogiq

[bors]

⌛ Testing commit 8e317f5 with merge 6ac42fe...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 6ac42fe to master...