handle another type of collapsible if-statement

[Arc8ne]

fixes #812

---

changelog: [collapsible_if]: handle another type of collapsible if-statement

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

I'll take this one as I have already #14906 on my plate.
r? samueltardieu

[rustbot]

⚠️ Warning ⚠️

• There are issue links (such as #123) in the commit messages of the following commits.
  Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.

  • 4713ffa

• The following commits have merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

  • b0fb3c7

  You can start a rebase with the following commands:

  $ # rebase
  $ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
  $ git push --force-with-lease
  

[samueltardieu]

The current state of commits (merges, followed by revert of merges) makes it harder to review. Can you please cleanup the commits and have only one commit on top of master?

Some notes about the current code:

• You should run cargo dev dogfood and fix the issues before requesting a review, otherwise the CI will be red. In particular, if statements can be collapsed, which is kinda ironical given that the changes are in "collapsible_if.rs".
• You can inline the spanless_eq variable at the place of its only use.
• Use Sugg::hir_with_applicability() in order to get a proper applicability to use in suggestion. Here, you use MachineApplicable in any case, even if Sugg::hir() used the placeholders.
• What does it mean for cond_sugg to contain the empty string as a suggestion? You should not need to make it mutable, write it as let cond_sugg = if …. Mutability complicates the code, as you have to check every code path to make sure the variable isn't modified.
• The check_collapsible_if_nested_if_else() function should be pushed down later in the file, it is surprising to see it right after the lint declaration.
• How will the code behave in the presence of comments between the two if? It should probably not lint if comments are present and would be removed by the rewrite. Maybe the lint_commented_code config option could be used if we can find a sensible way to preserve comments.
• Instead of dealing with DropTemps, you should look at higher::If and see if it could be used instead to deconstruct the if statements.

[Arc8ne]

The current state of commits (merges, followed by revert of merges) makes it harder to review. Can you please cleanup the commits and have only one commit on top of master?

Some notes about the current code:

• You should run cargo dev dogfood and fix the issues before requesting a review, otherwise the CI will be red. In particular, if statements can be collapsed, which is kinda ironical given that the changes are in "collapsible_if.rs".

• You can inline the spanless_eq variable at the place of its only use.

• Use Sugg::hir_with_applicability() in order to get a proper applicability to use in suggestion. Here, you use MachineApplicable in any case, even if Sugg::hir() used the placeholders.

• What does it mean for cond_sugg to contain the empty string as a suggestion? You should not need to make it mutable, write it as let cond_sugg = if …. Mutability complicates the code, as you have to check every code path to make sure the variable isn't modified.

• The check_collapsible_if_nested_if_else() function should be pushed down later in the file, it is surprising to see it right after the lint declaration.

• How will the code behave in the presence of comments between the two if? It should probably not lint if comments are present and would be removed by the rewrite. Maybe the lint_commented_code config option could be used if we can find a sensible way to preserve comments.

• Instead of dealing with DropTemps, you should look at higher::If and see if it could be used instead to deconstruct the if statements.

Sorry for the inconvenience and thank you for the advice, I'll work on implementing your suggestions.