Rework non_local_definitions lint to only use a syntactic heuristic

[Urgau]

This PR reworks the non_local_definitions lint to only use a syntactic heuristic, i.e. not use a type-system logic for whenever an impl is local or not.

Instead the new logic wanted by T-lang in #126768 (comment), which is to consider every paths in Self and Trait and to no longer use the type-system inference trick.

@rustbot labels +L-non_local_definitions
Fixes #126768

[rustbot]

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
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

[BoxyUwU]

r? @WaffleLapkin since you reviewed the original PR

[traviscross]

I looked over the tests; looks right to me.

[workingjubilee]

just typo nits

[bors]

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

[WaffleLapkin]

I don't know how to review this. T-lang's proposal looks to me like "let's make the lint incorrect". And I don't see why the lint is useful, if it's incorrect...

[workingjubilee]

r? lang

[workingjubilee]

cc @rust-lang/lang also posted a followup comment in #126768 (comment)

[traviscross]

Replied with further context and background over in #126768 (comment).

r? @workingjubilee

[workingjubilee]

💭 ...if I asked for lang to review this, and then TC nominated me, does that mean I'm on lang? Is this my punishment for ribbing T-lang one too many times? Am I now the target of my own japes? ...oh no, I'm barely even awake at 9, how am I going to make a meeting at 8?

[traviscross]

Maybe all of that? /s.... but no, seriously that was me just handing back off after answering the lang questions. Looking again, though, I mean to hand it back to compiler, so...

r? compiler

[michaelwoerister]

r? diagnostics

[oli-obk]

I've tried to review this every day of the past 3 days and each time finished with a bit more knowledge but mostly feeling unhappy with my understanding of it and the exact things we want to lint. I don't think I can reasonably review this without just removing it entirely, and writing it from scratch and hoping I come up with a close enough final result.

That said, the diff makes sense and looks like it does what T-lang wants, I'm just not confident in the full behaviour of this lint now, the cases it's missing and the cases it's not missing.

I'm not sure my feelings should be blocking it, but I also don't want to pass on the hot potato for long enough until someone feels like it's not important to understand it fully.

[Urgau]

and the exact things we want to lint

My understanding is that the lint should fire for what TC calls "sneaky inner impl".

What I understand from this is that the lint should warn on impl defs where syntactically none of "part" (idk the right word) of the Self and Trait types refer to something at the same nesting as the impl def it-self.

In particular it should not take into account the perfect type-system definition since T-lang doesn't "feel" like impls that would lint because of the 1-impl rule are "sneaky".

I think one could say that an impl def is sneaky if is hidden (that is nested inside other items) and that none of the syntactic part¹ of the Self and Trait types refer to something at the same nesting as the impl definition it-self.

I think it's also clear from unanimous decision from T-lang that they were not looking at a perfect non local impl defs detection, which the above rule is nowhere near.

@traviscross could you confirm or deny what I just said.

Footnotes

1. in the compiler those are {ast,hir}::Path, idk if we have a word or acronym at the syntax level ↩

[BoxyUwU]

I can review this if you want, I only reassigned because I figured waffle might want to review this since it was on the original PR 🤷‍♀️

[workingjubilee]

Let's go!

r? @BoxyUwU

[traviscross]

@Urgau: That sounds right to me, but I'd probably point more normatively to my earlier comments if there were any questions here. And I think @BoxyUwU has a handle on this in any case.