Add Redundant else lint

[camsteffen]

changelog: Add redundant_else lint

It seemed appropriate for "pedantic".

Closes #112 *blows off dust*

[rust-highfive]

r? @ebroto

(rust_highfive has picked a reviewer for you, use r? to override)

[ebroto]

First of all sorry for the delay in reviewing, I didn't have much free time lately

---

I did not dive into the implementation itself, but I think I've found some problematic cases (suggestion would cause compile error) when the result of the linted if expression is used in another expression. Some examples:

    // Binary
    42 + if false {
        return;
    } else {
        21
    };

    // Unary
    -if false {
        return;
    } else {
        42
    };

    // Parens + Cast
    (if false {
        return;
    } else {
        42
    }) as i64;

    // Parameter to function call
    Box::new(if false {
        return;
    } else {
        42
    });

    // Array expression
    [if false {
        return;
    } else {
        42
    }];

    // Parens + Method call
    (if false {
        return;
    } else {
        42
    })
    .clone();

There are probably more cases like this. I'm not sure about the way to go to consistently avoid these... maybe we could simplify by linting only if the "parent" expression of the linted if is a block?

[camsteffen]

maybe we could simplify by linting only if the "parent" expression of the linted if is a block?

Good catch. Yeah that makes a lot of sense as I think about it.

[bors]

☔ The latest upstream changes (presumably #6389) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[camsteffen]

It is much simpler now!

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[ebroto]

LGTM mostly, but I would like to clarify something I don't understand before merging

[ebroto]

The type annotations do not seem to be needed (here and below)

[camsteffen]

The type annotation coerces &P<Expr> to &Expr. I think this is preferable to doing &**expr inside the match?

[ebroto]

Hmm, just removing the annotations compiles fine. I guess it could be related with match ergonomics being able to auto-deref if the pattern matches?

[camsteffen]

Yes I think so.

1. type annotations - auto-deref at binding - clear and smart-pointer-agnostic
2. &**expr - manual deref before binding - less clear and smart-pointer-dependant
3. neither - auto-deref at each usage - simplest code but inefficient

[ebroto]

I think it won't make a big difference, so I think it's fine to merge it as is. Thanks!

---

To explain my reasoning:

1. I believe type annotations are generally less idiomatic if they are not needed to disambiguate (that's why I raised a concern).
2. Correct me if I'm wrong, but I would say in this case using the annotation would just change the place where the dereference happens. From the let binding to the match/function call later, but I would say the number of derefs would be the same?

[ebroto]

But yeah, I see your point, there would be cases where you could need more derefs than if you do it once when binding 👍

[ebroto]

@bors r+

Thanks!

[bors]

📌 Commit 70f6a2c has been approved by ebroto

[bors]

⌛ Testing commit 70f6a2c with merge 50bca8a...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing 50bca8a to master...