Fixing a false positive for the match_single_binding lint #5552

[xFrednet]

This is a fix for a false positive in the match_single_binding lint when using #[cfg()] on a branch. It is sadly a bit hacky but maybe the best solution as rust removes the other branch from the AST before we can even validate it. This fix looks at the code snippet itself and returns if it includes another thick arrow => besides the one matching arm we found. This can again cause false negatives if someone has the following code:

match x {
    // => <-- Causes a false negative
    _ => 1,
}

I thought about making the code more complex and maybe validating against other things like the #[cfg()] macro but I believe that this is the best solution. This has basically switched the issue from a false positive to a false negative in a very specific case.

I'm happy to make some changes if you have any suggestions 🙃.

---

Fixes #5552

changelog: Fixed a false positive in the match_single_binding lint with #[cfg()] macro

[rust-highfive]

r? @ebroto

(rust-highfive has picked a reviewer for you, use r? to override)

[camsteffen]

I think a proper fix here would involve register_pre_expansion_pass to inspect arms before cfg expansion. But I think it would be the first time a lint has both pre-expansion and post-expansion passes so that would require some high-level changes.

[xFrednet]

That sounds like a nice way of doing that. Should I try to solve it like you suggested @camsteffen or should this first be discusses further? 🤔

[camsteffen]

I think a maintainer needs to weigh in. I'm just some random guy.

[xFrednet]

It is still very helpful, thank you 🙃. I thought about implementing it by first keeping track of all the arms and your comment goves a good starting point with register_pre_expansion_pass. I'll wait on a maintainer comment until I restructure the code.

[xFrednet]

@camsteffen I've read through your comment again and I'm not 100% sure that I understand you correctly 😅.

I think it would be the first time a lint has both pre-expansion and post-expansion

Do you mean that we register a pre-expansion and post-expansion lint pass for the same lint or for the same struct?

The lint it self can be checked in the EarlyLintPass all together, I believe. It is also ignorant of the other lint checks so it should probably be pretty strait forward to create a new struct that impls EarlyLintPass for this lint. (Current lint check call)

[ebroto]

Thanks for your contribution @xFrednet, and @camsteffen for your review.

Unfortunately, since #5518 we're actively avoiding adding new pre-expansion passes for the reasons stated there. TL;DR it's not clear they will be supported in the future in rustc. Pre-expansion passes are also quite problematic since you miss a lot of code. In this particular case, we're explicitly avoiding macros, so it would work, but otherwise this same construct would not be detected if it was generated during expansion.

I think this is a problem that could affect almost any lint. The inability to use pre-expansion passes is also a recurring problem. IMO the best approach here would be to find a general solution, probably implemented directly in rustc. Right now I'm not sure what would be the best approach, so I'm going to take note of this and think about opening a more general issue.

For the moment I'm inclined to accept this patch, but understanding that this should not set a precedent.

[ebroto]

We could use an if_chain here to reduce the right-ward drift.

[ebroto]

I think we should state more clearly in the comment that this is a HACK: and that we can't do better currently.

[xFrednet]

I've implemented your suggestions and rebased on master.

I agree that there should be a general non hacky way to do this. It would be awesome if you would share the issue if you decide to create one. Thank you for the work and the review @ebroto! 🙃

[ebroto]

LGTM, thanks!

Could you squash before we merge? There are some intermediary commits.

---

It would be awesome if you would share the issue if you decide to create one

I hope this gets into the roadmap for 2021 because it's been a recurrent problem for the past months. I will be glad to share the issue if you would like to help!

[xFrednet]

Jup, they are squashed.

---

I would like to help! However, I'm a bit unsure if I have the sufficient knowledge to be a good support.

[ebroto]

@bors r+

---

I would like to help! However, I'm a bit unsure if I have the sufficient knowledge to be a good support.

Any kind of help is welcome! I'll make sure to ping you after this has been discussed and we have a plan. Thanks for your willingness to help!

[bors]

📌 Commit a37af06 has been approved by ebroto

[bors]

⌛ Testing commit a37af06 with merge d924164...

[bors]

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