Rework use_self impl based on ty::Ty comparison #3410 | Take 2

[flip1995]

This builds on top of #5531

I already reviewed and approved the commits by @montrivo. So only the review of my commits should be necessary.

I would also appreciate your review @montrivo, since you are familiar with the challenges here.

Fixes #3410 and Fixes #4143 (same problem)
Fixes #2843
Fixes #3859
Fixes #4734 and fixes #6221
Fixes #4305
Fixes #5078 (even at expression level now 🎉)
Fixes #3881 and Fixes #4887 (same problem)
Fixes #3909

Not yet: #4140 (test added)

All the credit for the fixes goes to @montrivo. I only refactored and copy and pasted his code.

changelog: rewrite [use_self] lint and fix multiple (8) FPs. One to go.

[rust-highfive]

r? @ebroto

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

[ebroto]

Sorry I still have not reviewed this. It's on my radar but it's quite long and TBH it was sitting there for some time already :)

I'm giving priority to some PRs by new contributors and smaller ones, I expect I'll be able to get to this soonish.

[flip1995]

Don't worry, I'm also low on time and also prioritize reviewing over implementing things, if I got a bit of time.

[bors]

☔ The latest upstream changes (presumably #6338) 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

[ebroto]

I tried to rebase this but I'm not familiar enough with the changes.

[flip1995]

Oh this is still open 😄 I try to rebase it tomorrow.

[flip1995]

@ebroto I rebased it successfully. It was indeed quite a bit of work. The only remaining issue is #4140. I added it to the known problems section.

I think we should merge it like this, since I don't think I will get to it in the near future and it improves the situation of this lint immensely. Not sure if we should move the lint out of nursery now. WDYT?

[ebroto]

I just found some small stuff

---

The work done here is extraordinary, but besides #4140, #3881 #3909 #4887 are still relevant. Since we know the lint has still FPs, I would not move it from the nursery for now.

That said, the issues besides #4140 would be fixed by avoiding macros altogether and IMO we should go that way, as there's little benefit in linting local macros in this case. After that, just one known FP would remain.

Let me known what you think.

[bors]

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

[flip1995]

I like that bors is reminding me about this every now and then.

[ebroto]

r? @phansch (I'm leaving the team, so I'm reassigning my PRs to other active members)

[phansch]

I can only agree with ebroto here: 'The work done here is extraordinary'. I just have some documentation nits. Let me know if you need any help with macro testing 🙂

[flip1995]

@bors try

[bors]

⌛ Trying commit 52f98d8 with merge 3ad10ff...

[bors]

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 3ad10ff (3ad10ffec26f76e2cd7a786b0ed0c5b4fda52618)

[flip1995]

This passes now! Thanks @camsteffen! A second pair of eyes on this really helped. Did you manage to produce a minimal reproducer for the ICE?

[camsteffen]

Glad I could help! No, sadly I could not reproduce the ICE without testing against log and chalk. I don't fully understand the cause.

[phansch]

LGTM! I think it would be nice to have a reproducer for the ICE, but then again, this fixes a lot of other issues with the lint. Let's get this in for now =)

@bors r+

[bors]

📌 Commit 52f98d8 has been approved by phansch

[bors]

⌛ Testing commit 52f98d8 with merge 605e9ba...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch
Pushing 605e9ba to master...

[flip1995]

I kind of expect that this still causes ICEs in some weird cases. We'll see with the next sync in 2 weeks...

[tnielens]

Many thanks for the great work here! ❤️

[Y-Nak]

I found minimal reproducer of the ICE on Fix qpath_res call: 7e1c1c1.
I think the ICE was caused by here.

Minimal reproducer:

#![warn(clippy::use_self)]
#![allow(dead_code)]

fn main() {}

struct Foo {}

impl Foo {
    fn foo() -> Self {
        impl Foo {
            fn bar() {
            }
        }

        let x: _ = 1;

        Self {}
    }
}

[camsteffen]

I kind of expect that this still causes ICEs in some weird cases.

@flip1995 Yeah I'm not quite 100% confident either. In particular, the logic that prevents hir_ty_to_ty from being invoked in bodies is very indirect and fragile. It would be better to see logic like if !self.in_body ... This would probably require using check_body and pushing onto a stack. Additionally this could probably replace some logic from LintTyCollecter.

I found minimal reproducer of the ICE on Fix qpath_res call: 7e1c1c1.

@Y-Nak Nice! Want to open a PR with that?

I think the ICE was caused by here.

Yep that is also what I decided.

[Y-Nak]

@camsteffen Yeah! I'll open a PR tomorrow.

[flip1995]

In particular, the logic that prevents hir_ty_to_ty from being invoked in bodies is very indirect and fragile. It would be better to see logic like if !self.in_body ...

Agreed. But I'm pretty sure I tried to just gate if we're in a body and it failed. I think the reason was, that arguments of closures/functions are in a body from the AST perspective, but not really in a body from the typeck perspective.

[camsteffen]

Hmm. How about something like this?

• check_item => self.in_body.push(false)
• check_fn => self.in_body.push(false) (works for closures)
• check_body => self.in_body.push(true)
• *_post => self.in_body.pop()

[camsteffen]

...or invert it and call it in_header. I think that is what we're actually interested in?

Edit: No I take that back. Consider a type annotation on a static.

[flip1995]

I mean, we could try. I just know, that I didn't find a reliable way to differentiate between the part of the body we're currently in. Well, the LintTyCollector was my solution for this.