Skip to content
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.

Sign up for GitHub

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

fix suggestion in option_map_or_none #7971

Merged
merged 11 commits into from
Nov 17, 2021
Merged

fix suggestion in option_map_or_none #7971

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2ed4a8a
fix suggestion in option_map_or_none
togami2864 Nov 13, 2021
4f71ff3
add test case for option_map_or_none
togami2864 Nov 15, 2021
bbffe82
add reduce_unit_expression
togami2864 Nov 15, 2021
02e0726
fix a bug that the closure arguments are not displayed
togami2864 Nov 15, 2021
cd57816
add comment
togami2864 Nov 15, 2021
300282c
fix fmt
togami2864 Nov 15, 2021
0a30fdc
move the let statement out of the macro
togami2864 Nov 15, 2021
7605bac
resolve CI
togami2864 Nov 16, 2021
e34927e
add multi-line test case
togami2864 Nov 17, 2021
006c442
check whether stmts is empty or not in block
togami2864 Nov 17, 2021
8e317f5
fix suggestion message
togami2864 Nov 17, 2021
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
6 changes: 3 additions & 3 deletions clippy_lints/src/methods/option_map_or_none.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ pub(super) fn check<'tcx>(
let self_snippet = snippet(cx, recv.span, "..");
let func_snippet = snippet(cx, map_arg.span, "..");
let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
`and_then(..)` instead";
`map(..)` instead";
(
OPTION_MAP_OR_NONE,
msg,
"try using `and_then` instead",
format!("{0}.and_then({1})", self_snippet, func_snippet),
"try using `map` instead",
format!("{0}.map({1})", self_snippet, func_snippet),
)
} else if f_arg_is_some {
let msg = "called `map_or(None, Some)` on a `Result` value. This can be done more directly by calling \
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/option_map_or_none.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ fn main() {

// Check `OPTION_MAP_OR_NONE`.
// Single line case.
let _ = opt.and_then(|x| Some(x + 1));
let _ = opt.map(|x| Some(x + 1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, "..");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@togami2864 togami2864 Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Multi-line case.
#[rustfmt::skip]
let _ = opt.and_then(|x| {
let _ = opt.map(|x| {
Some(x + 1)
});
}
10 changes: 5 additions & 5 deletions tests/ui/option_map_or_none.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead
--> $DIR/option_map_or_none.rs:10:13
|
LL | let _ = opt.map_or(None, |x| Some(x + 1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(|x| Some(x + 1))`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `map` instead: `opt.map(|x| Some(x + 1))`
|
= note: `-D clippy::option-map-or-none` implied by `-D warnings`

error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead
--> $DIR/option_map_or_none.rs:13:13
|
LL | let _ = opt.map_or(None, |x| {
Expand All @@ -15,9 +15,9 @@ LL | | Some(x + 1)
LL | | });
| |_________________________^
|
help: try using `and_then` instead
help: try using `map` instead
|
LL ~ let _ = opt.and_then(|x| {
LL ~ let _ = opt.map(|x| {
LL + Some(x + 1)
LL ~ });
|
Expand Down