add Result to question_mark

[TheAlgorythm]

What it does

The lint should hint to flatten the error propagation from using match/if let to a more readable declarative propagation with the ? operator.

Categories

• Kind: clippy::style, maybe clippy::pedantic

The control structures distract in the flow of reading and therefore the ? operator was created. A comprehensive list of arguments can be found in the documentation of Rust

Drawbacks

As long as only the Err will always return an Err, there should be no false-positives.

Example

fn propagate_match_input(input: Result<String, u64>) -> Result<String, u64> {
    let inner = match input {
        Ok(inner) => inner,
        Err(code) => return Err(code),
    };

    Ok(inner)
}

fn match_input(input: Result<String, u64>) -> Result<String, bool> {
    let inner = match input {
        Ok(inner) => inner,
        Err(code) => return Err(code != 0),
    };

    Ok(inner)
}

fn if_let_input(input: Result<String, u64>) -> Result<(), bool> {
    if let Err(code) = input {
        return Err(code != 0);
    }

    Ok(())
}

Could be written as:

fn propagate_match_input(input: Result<String, u64>) -> Result<String, u64> {
    let inner = input?;

    Ok(inner)
}

fn match_input(input: Result<String, u64>) -> Result<String, bool> {
    let inner = input.map_err(|code| code != 0)?;

    Ok(inner)
}

fn if_let_input(input: Result<String, u64>) -> Result<(), bool> {
    input.map_err(|code| code != 0)?;

    Ok(())
}

[giraffate]

We already have the question_mark lint, which is currently for Option. So it would be better to improve the question_mark lint so that it can be used in Result as well.

[TheAlgorythm]

Yeah, the question_mark lint is exactly what I thought of. I will update the title.

[TheAlgorythm]

I found more linting cases which are currently unsupported by question_mark.

fn if_let_option(input: Option<String>) -> Option<usize> {
    if let Some(string) = input {
        return Some(string.len());
    }

    None
}

fn if_let_else_option(input: Option<String>) -> Option<usize> {
    if let Some(string) = input {
        Some(string.len())
    } else {
        None
    }
}

fn match_option(input: Option<String>) -> Option<usize> {
    match input {
        Some(string) => Some(string.len()),
        None => None,
    }
}

Could be rewritten as:

fn map_option(input: Option<String>) -> Option<usize> {
    input.map(|string| string.len())
}
// or
fn propagate_option(input: Option<String>) -> Option<usize> {
    Some(input?.len())
}

[camsteffen]

I think the map_err cases should be a new lint.

I found more linting cases which are currently unsupported by question_mark.

All those cases would be a better fit for Option::map IMO.

[TheAlgorythm]

@camsteffen

All those cases would be a better fit for Option::map IMO.

Absolutely, but as I designed them I wanted to construct cases, which are equivalent to the map_err cases.
On contrary, I don't know if Option::map is the best case on larger conversions.

I think the map_err cases should be a new lint.

I am not sure as most linting cases are undeniably different from question_mark but propagate_match_input and propagate_option match it pretty well. Maybe they should be splitted into separate lints

[camsteffen]

Let me put it differently.

1. match x { .. } -> x?
   This is question_mark. It should include Option and Result.
2. match x { .. } -> x.map_err(..)?
   This should be a new lint like question_mark_map_err because it is more picky.
3. match x { .. } -> Some(x?.y) or Ok(x?.y)
   I don't think we should suggest this since it is not better style IMO. But manual_map might lint instead.