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

[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.

[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.

@samueltardieu With regard to your last suggestion, I'm not sure if I can implement it as I'm already trying to handle cases where DropTemps is present/absent.

[samueltardieu]

@samueltardieu With regard to your last suggestion, I'm not sure if I can implement it as I'm already trying to handle cases where DropTemps is present/absent.

Is there any case where DropTemps is absent? As far as I can see in the compiler source code, it is always inserted in the case of a regular if (no if let). And anyway, even if this was possible, that would be the job of higher::If to deal with this.

[Arc8ne]

@samueltardieu With regard to your last suggestion, I'm not sure if I can implement it as I'm already trying to handle cases where DropTemps is present/absent.

Is there any case where DropTemps is absent? As far as I can see in the compiler source code, it is always inserted in the case of a regular if (no if let). And anyway, even if this was possible, that would be the job of higher::If to deal with this.

@samueltardieu I see, thanks for letting me know about that. Based on my current understanding, DropTemps seems to be absent in the case of an if let statement.

Now that I think about it, I should be able to implement the last suggestion.