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

Lint nested and independent if let clauses #5218

Open
Luro02 opened this issue Feb 23, 2020 · 4 comments
Open

Lint nested and independent if let clauses #5218

Luro02 opened this issue Feb 23, 2020 · 4 comments
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-complexity Lint: Belongs in the complexity lint group S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@Luro02
Copy link

Luro02 commented Feb 23, 2020

This code

if let Some(start) = (1_usize).checked_add(5) {
    if let Some(end) = (2_usize).checked_add(5) {
        // ...
    }
}

could be written like this

if let (Some(start), Some(end)) = ((1_usize).checked_add(5), (2_usize).checked_add(5)) {
    // ...
}

which would remove a layer of nesting.


The lint should only lint nested if let clauses, that are independent of each other.

For example this must be ignored (or handled like in #2521):

if let Some(start) = (1_usize).checked_add(5) {
    if let Some(end) = call_fn(start) {
        // ...
    }
}

but it should not matter, if there is code in between the if let clauses, as long as it does not influence the other if let clause(s):

if let Some(start) = (1_usize).checked_add(5) {
    let x = 13_usize;
    if let Some(end) = (2_usize).checked_add(5) {
        // ...
    }
    let y = 15_usize;
}

I think that there should not be a limit on how many nested if let clauses should be linted, because it would improve the readability either way.

Another thing to consider would be else clauses. I think this lint should only trigger, if the nested else clauses have the same body:

if let Some(start) = (1_usize).checked_add(5) {
    if let Some(end) = (2_usize).checked_add(5) {
        // ...
    } else {
        call_x();
    }
} else {
    call_x();
}

would become

if let (Some(start), Some(end)) = ((1_usize).checked_add(5), (2_usize).checked_add(5)) {
    // ...
} else {
    call_x();
}

It might be too difficult (for clippy), but it would be possible to refactor nested if let-clauses with different else-clauses too

if let Some(start) = (1_usize).checked_add(5) {
    if let Some(end) = (2_usize).checked_add(5) {
        // ...
    } else {
        call_z(start);
    }
} else {
    call_y();
}

->

match ((1_usize).checked_add(5), (2_usize).checked_add(5)) {
    (Some(start), Some(end)) => {
        // ...
    }

    (Some(start), None) => {
        call_z(start);
    }

    (None, None) => {
        call_y();
    }

    _ => {
        // this was not reachable in the original code (end would only be matched if Some(start))
    }
}

Should I move this issue in to several smaller ones?

@flip1995
Copy link
Member

which would remove a layer of nesting.

While this is true, this could increase the cognitive complexity, since the if-condition is longer. So we have to be careful here.

For example this must be ignored (or handled like in #2521):

This has to be ignored, except for the special in #2521, where the x in Some(x) is directly used and not in a parent expression.

as long as it does not influence the other if let clause(s):

I don't think we should lint this, since detecting side-effects is really hard. In the collapsible_if lint, the lint isn't even triggered when there is a comment in between. We should also use this heuristic here.

I think that there should not be a limit on how many nested if let clauses should be linted, because it would improve the readability either way.

As stated above, I'm not sure about that, we may need some other opinions about this.

It might be too difficult (for clippy), but it would be possible to refactor nested if let-clauses with different else-clauses too

This seems hard for more complex enums, than Option.

Should I move this issue in to several smaller ones?

We can track this in this issue.

@flip1995 flip1995 added L-complexity Lint: Belongs in the complexity lint group S-needs-discussion Status: Needs further discussion before merging or work can be started E-medium Call for participation: Medium difficulty level problem and requires some initial experience. A-lint Area: New lints labels Feb 23, 2020
@Luro02
Copy link
Author

Luro02 commented Mar 1, 2020

which would remove a layer of nesting.

While this is true, this could increase the cognitive complexity, since the if-condition is longer. So we have to be careful here.

I think that there should not be a limit on how many nested if let clauses should be linted, because it would improve the readability either way.

As stated above, I'm not sure about that, we may need some other opinions about this.

let variable_01 = Some(1_usize);
let variable_02 = Some(2_usize);
let variable_03 = Some(3_usize);
let variable_04 = Some(4_usize);
let variable_05 = Some(5_usize);
let variable_06 = Some(6_usize);
let variable_07 = Some(7_usize);
let variable_08 = Some(8_usize);

if let Some(variable_01) = variable_01 {
    if let Some(variable_02) = variable_02 {
        if let Some(variable_03) = variable_03 {
            if let Some(variable_04) = variable_04 {
                if let Some(variable_05) = variable_05 {
                    if let Some(variable_06) = variable_06 {
                        if let Some(variable_07) = variable_07 {
                            if let Some(variable_08) = variable_08 {
                                println!("{}", variable_01);
                                println!("{}", variable_02);
                                println!("{}", variable_03);
                                println!("{}", variable_04);
                                println!("{}", variable_05);
                                println!("{}", variable_06);
                                println!("{}", variable_07);
                                println!("{}", variable_08);
                            }
                        }
                    }
                }
            }
        }
    }
}

if let (
    Some(variable_01),
    Some(variable_02),
    Some(variable_03),
    Some(variable_04),
    Some(variable_05),
    Some(variable_06),
    Some(variable_07),
    Some(variable_08),
) = (
    variable_01,
    variable_02,
    variable_03,
    variable_04,
    variable_05,
    variable_06,
    variable_07,
    variable_08,
) {
    println!("{}", variable_01);
    println!("{}", variable_02);
    println!("{}", variable_03);
    println!("{}", variable_04);
    println!("{}", variable_05);
    println!("{}", variable_06);
    println!("{}", variable_07);
    println!("{}", variable_08);
}

or with a more complex enum

enum Example {
    Variant01 {
        string: String,
        number: usize,
        character: char,
    },
    Variant02 {
        string: String,
        number: usize,
        character: char,
    },
    Variant03 {
        string: String,
        number: usize,
        character: char,
    },
    Variant04 {
        string: String,
        number: usize,
        character: char,
    },
    Variant05 {
        string: String,
        number: usize,
        character: char,
    },
    Variant06 {
        string: String,
        number: usize,
        character: char,
    },
    Variant07 {
        string: String,
        number: usize,
        character: char,
    },
    Variant08 {
        string: String,
        number: usize,
        character: char,
    },
}

let value_01 = Example::Variant01 {
    "example".into(),
    2_usize,
    'b',
};

let value_02 = Example::Variant02 {
    "example".into(),
    2_usize,
    'b',
};

let value_03 = Example::Variant03 {
    "example".into(),
    2_usize,
    'b',
};

let value_04 = Example::Variant04 {
    "example".into(),
    2_usize,
    'b',
};

let value_05 = Example::Variant05 {
    "example".into(),
    2_usize,
    'b',
};

let value_06 = Example::Variant06 {
    "example".into(),
    2_usize,
    'b',
};

let value_07 = Example::Variant07 {
    "example".into(),
    2_usize,
    'b',
};

let value_08 = Example::Variant08 {
    "example".into(),
    2_usize,
    'b',
};


if let Example::Variant01 { string_01, number_01, character_01 } = value_01 {
    println!("{}: {},{}", string_01, number_01, character_01);

    if let Example::Variant02 { string_02, number_02, character_02 } = value_02 {
        println!("{}: {},{}", string_02, number_02, character_02);

        if let Example::Variant03 { string_03, number_03, character_03 } = value_03 {
            println!("{}: {},{}", string_03, number_03, character_03);

            if let Example::Variant04 { string_04, number_04, character_04 } = value_04 {
                println!("{}: {},{}", string_04, number_04, character_04);

                if let Example::Variant05 { string_05, number_05, character_05 } = value_05 {
                    println!("{}: {},{}", string_05, number_05, character_05);

                    if let Example::Variant06 { string_06, number_06, character_06 } = value_06 {
                        println!("{}: {},{}", string_06, number_06, character_06);

                        if let Example::Variant07 { string_07, number_07, character_07 } = value_07 {
                            println!("{}: {},{}", string_07, number_07, character_07);

                            if let Example::Variant08 { string_08, number_08, character_08 } = value_08 {
                                println!("{}: {},{}", string_08, number_08, character_08);

                            }
                        }
                    }
                }
            }
        }
    }
}

if let (
    Example::Variant01 { string_01, number_01, character_01 },
    Example::Variant02 { string_02, number_02, character_02 },
    Example::Variant03 { string_03, number_03, character_03 },
    Example::Variant04 { string_04, number_04, character_04 },
    Example::Variant05 { string_05, number_05, character_05 },
    Example::Variant06 { string_06, number_06, character_06 },
    Example::Variant07 { string_07, number_07, character_07 },
    Example::Variant08 { string_08, number_08, character_08 },
) = (
    value_01,
    value_02,
    value_03,
    value_04,
    value_05,
    value_06,
    value_07,
    value_08,
) {
    println!("{}: {},{}", string_01, number_01, character_01);
    println!("{}: {},{}", string_02, number_02, character_02);
    println!("{}: {},{}", string_03, number_03, character_03);
    println!("{}: {},{}", string_04, number_04, character_04);
    println!("{}: {},{}", string_05, number_05, character_05);
    println!("{}: {},{}", string_06, number_06, character_06);
    println!("{}: {},{}", string_07, number_07, character_07);
    println!("{}: {},{}", string_08, number_08, character_08);
}

both of the above examples look cleaner with the tuple version.


We would have to look out for name shadowing: (maybe suggest to change the variable names to something unique?)

if let Example::Variant01 { string, number, character } = value_01 {
    println!("{}: {},{}", string, number, character);

    if let Example::Variant02 { string, number, character } = value_02 {
        println!("{}: {},{}", string, number, character);
    }
}

if let (
    Example::Variant01 { string, number, character },
    Example::Variant02 { string, number, character },
) = (value_01, value_02) {
    println!("{}: {},{}", string, number, character);
    println!("{}: {},{}", string, number, character);
}

@Luro02
Copy link
Author

Luro02 commented Mar 7, 2020

This lint might be changed with
rust-lang/rfcs#2497

@camsteffen
Copy link
Contributor

Wouldn't this remove short-circuiting?

if let (Some(a), Some(b)) = (expensive(), expensive()) { .. }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-complexity Lint: Belongs in the complexity lint group S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

No branches or pull requests

3 participants