result_map_unwrap_or_else suggestion masks error from result.

[darobs]

cargo clippy -V
clippy 0.0.212 (2e26fdc 2018-11-22)

In regards to result_map_unwrap_or_else

I have an (extremely) contrived example below where I care about the error result coming from my_function, but the clippy suggestion (ok().map_or_else()) would hide the error result, and the suggestion in the clippy output doesn't compile! (See the bottom of this post.)

The real example is transforming multiple kinds of errors, such as missing fields, malformed data, or error results from other crates, and I'm interested in why this function fails. In my case, the readability is much less important than getting the details from the error.

If it makes a difference, the real example is transforming a Result into a FutureResult, and the success path makes additional calls which returns Futures. My function returns a Future with Either::A() as the success result and Either::B() as the failure result, so I want to preserve the error information.

I think my particular problem would be better served with result.map_or_else(), but that's not available on the stable channel. I also think that result.map_or_else(), if and when it becomes stable, would be a better suggestion for clippy to make in this case.

I disagree with the sentiment behind the result_map_unwrap_or_else lint. As a consequence, I'm allowing this lint for my current crate. If this is the preferred way of handling this, then please just close the issue.

EXAMPLE:

#![cfg_attr(feature = "cargo-clippy", deny(clippy::all, clippy::pedantic))]

#[derive(Clone, Copy)]
pub enum Error {
    Thing1(u8),
    Thing2(i32),
}

pub struct Error2 {
    pub result_string: String,
}

impl Error2 {
    pub fn new(result_string: String) -> Self {
        Self { result_string }
    }
    pub fn from(e: Error) -> Self {
        Self::new(match e {
            Error::Thing1(t1) => format!("THING 1 bad: {}", t1),
            Error::Thing2(t2) => format!("THING 2 bad: {}", t2),
        })
    }
}

fn my_function(thing1: u8, thing2: i32) -> Result<String, Error> {
    if thing1 == 0 {
        return Err(Error::Thing1(thing1));
    }
    if thing2 == 0 {
        return Err(Error::Thing2(thing2));
    }
    Ok(format!("{}:{}", thing1, thing2))
}

fn main() {
// clippy doesn't like:
    println!(
        "{}",
        my_function(1, 0)
            .map_err(Error2::from)
            .map(|success| format!("SUCCESS: {}", success))
            .unwrap_or_else(|err| format!("FAILURE: {}", err.result_string))
    );
// clippy approved:
    println!(
        "{}",
        my_function(1, 0).ok().map_or_else(
            || "FAILURE: <no information from error>".to_string(),
            |success| format!("SUCCESS: {}", success)
        )
    );
}

Output from cargo run:

FAILURE: THING 2 bad: 0
FAILURE: <no information from error>

Output from clippy:

error: called `map(f).unwrap_or_else(g)` on a Result value. This can be done more directly by calling `ok().map_or_else(g, f)` instead                            
  --> src/main.rs:38:9                                                                                                                                            
   |                                                                                                                                                              
38 | /         my_function(1, 0)                                                                                                                                  
39 | |             .map_err(Error2::from)                                                                                                                         
40 | |             .map(|success| format!("SUCCESS: {}", success))                                                                                                
41 | |             .unwrap_or_else(|err| format!("FAILURE: {}", err.result_string))                                                                               
   | |____________________________________________________________________________^                                                                               
   |                                                                                                                                                              
note: lint level defined here                                                                                                                                     
  --> src/main.rs:1:57                                                                                                                                            
   |                                                                                                                                                              
1  | #![cfg_attr(feature = "cargo-clippy", deny(clippy::all, clippy::pedantic))]                                                                                  
   |                                                         ^^^^^^^^^^^^^^^^                                                                                     
   = note: #[deny(clippy::result_map_unwrap_or_else)] implied by #[deny(clippy::pedantic)]                                                                        
   = note: replace `map(|success| format!("SUCCESS: {}", success)).unwrap_or_else(|err| format!("FAILURE: {}", err.result_string))` with `ok().map_or_else(|err| format!("FAILURE: {}", err.result_string), |success| format!("SUCCESS: {}", success))`
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/master/index.html#result_map_unwrap_or_else                              

clippy's suggestion actually doesn't compile:

error[E0593]: closure is expected to take 0 arguments, but it takes 1 argument                                                                                    
  --> src/main.rs:45:54                                                                                                                                           
   |                                                                                                                                                              
45 |         my_function(1, 0).map_err(Error2::from).ok().map_or_else(                                                                                            
   |                                                      ^^^^^^^^^^^ expected closure that takes 0 arguments                                                     
46 |             |err| format!("FAILURE: {}", err.result_string),                                                                                                 
   |             ----- takes 1 argument                                                                                                                           
                                                                                                                                                                  
error: aborting due to previous error                                                                                                                             

[ghost]

This lint is bad because it's suggestion isn't equivalent to the code it wants to replace. I think it should be moved to nursery until result.map_or_else stabilizes.

[Arnavion]

This was discussed in parallel in the original lint's issue, and was fixed by #4848 changing the lint suggestion in line with Result::map_or_else being stabilized.