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.

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

Extend option-map-unwrap-or-else to Result #1590

Closed
oli-obk opened this issue Mar 3, 2017 · 4 comments · Fixed by #4848
Closed

Extend option-map-unwrap-or-else to Result #1590

oli-obk opened this issue Mar 3, 2017 · 4 comments · Fixed by #4848
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Mar 3, 2017

opt.map(f).unwrap_or_else(g) is linted to suggest opt.map_or_else(g, f), but res.map(f).unwrap_or_else(g) is not linted even though it could be res.ok().map_or_else(|_| g(), f)

Somewhat ugly suggestion, so should be allow-by-default, maybe we should poke rustc to add Result::map_or_else

@oli-obk oli-obk added good-first-issue These issues are a good way to get started with Clippy C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages T-middle Type: Probably requires verifiying types labels Mar 3, 2017
gendx added a commit to gendx/rust-clippy that referenced this issue Oct 30, 2017
oli-obk added a commit that referenced this issue Nov 2, 2017
@Arnavion
Copy link
Contributor

Arnavion commented Feb 1, 2019

This was implemented in #2189, so it could be closed, though I'll note that both the OP suggestion and the implementation of the lint are wrong in the general case. res.map(f).unwrap_or_else(g) cannot be replaced with res.ok().map_or_else(g, f) if g uses the Err value. The lint should be tightened to only fire if g is |_|

@mathstuf
Copy link
Contributor

mathstuf commented Oct 25, 2019

Why is it wrong? f doesn't get called if res is Err, so it goes right to g. If it is Ok, g isn't getting called at all. Maybe you confused it with and_then where g could get res or f's Err depending on the behavior of f?

map followed by unwrap_or_else:

let intermediate = match res {
  Ok(v) => Ok(f(v)),
  Err(err) => Err(err),
};
match intermediate {
  Ok(v) => v,
  Err(err) => g(err),
}

map_or_else:

match res {
  Ok(v) => f(v),
  Err(err) => g(err),
}

These look isomorphic to me.

@oli-obk Good to close?

@Arnavion
Copy link
Contributor

@mathstuf If you were responding to my comment, you should read more closely what I wrote and the lint is suggesting you to use. Notice that it's not suggesting Result::map_or_else

@mathstuf
Copy link
Contributor

mathstuf commented Oct 25, 2019

Ah, missed the .ok() that's in there. If we just get rid of that in the suggestion, this is resolved then, right?

mathstuf added a commit to mathstuf/rust-clippy that referenced this issue Oct 25, 2019
Adding the `.ok()` changes the semantics since that assumes that `g` was
ignoring its argument. Instead, allow `g` to keep taking its argument
and preserve the isomorphism to the original code in the suggestion.

Fixes rust-lang#1590
mathstuf added a commit to mathstuf/rust-clippy that referenced this issue Oct 25, 2019
Adding the `.ok()` changes the semantics since that assumes that `g` was
ignoring its argument. Instead, allow `g` to keep taking its argument
and preserve the isomorphism to the original code in the suggestion.

Fixes rust-lang#1590
@bors bors closed this as completed in 7f745da Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types
Projects
None yet
4 participants