Improve diagnostic output the non_local_definitions lint

[Urgau]

This PR improves (or at least tries to improve) the diagnostic output the non_local_definitions lint, by simplifying the wording, by adding a "sort of" explanation of bounds interaction that leak the impl...

This PR is best reviewed commit by commit and is voluntarily made a bit vague as to have a starting point to improve on.

Related to https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/non_local_defs.20wording.20improvements

Fixes #125068
Fixes #124396
cc @workingjubilee
r? @estebank

[workingjubilee]

feel free to "nah actually" I am just trying to throw a couple wording ideas at the wall.

[Urgau]

I pushed several more commits that tries to improve the clarity the message were are trying to pass, mainly by moving the "move help message" to a separate span with some labels.

warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
  --> $DIR/consts.rs:43:5
   |
LL |     impl Test {
   |     ^^^^^^^^^
   |
   = note: methods and assoc const are still usable outside the current expression, only `impl Local` and `impl dyn Local` are local and only if the `Local` type is at the same nesting as the `impl` block
help: move this `impl` block and outside the of the current function `main`
  --> $DIR/consts.rs:43:5
   |
LL |       impl Test {
   |       ^    ---- may need to be moved as well
   |  _____|
   | |
LL | |
LL | |         fn foo() {}
LL | |     }
   | |_____^
   = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue <https://github.com/rust-lang/rust/issues/120363>

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[Urgau]

I continued to improved the diagnostic output by adding labels that points to Self type and Trait to better put emphasis on the fact that they are non-local for the lint.

With the example in #125068 (comment), it now gives:

warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
 --> a.rs:6:5
  |
6 |     impl Trait for Option<Local> {}
  |     ^^^^^-----^^^^^------^^^^^^^
  |          |         |
  |          |         `Option` is not local
  |          `Trait` is not local
  |
  = note: `impl` may be usable in bounds, etc. from outside the expression, which might e.g. make something constructible that previously wasn't, because it's still on a publicly-visible type
  = note: an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
help: move this `impl` block outside of the current function `main`
 --> a.rs:6:5
  |
6 |     impl Trait for Option<Local> {}
  |     ^^^^^^^^^^^^^^^^^^^^^^-----^^^^
  |                           |
  |                           may need to be moved as well
  = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue <https://github.com/rust-lang/rust/issues/120363>
  = note: `#[warn(non_local_definitions)]` on by default

with colors

[estebank]

There's quite a bit of code that I need to continue reviewing, but I have some nitpicks based on the output.

[estebank]

I see that you're hedging your bets that the type itself might also be in a weird scope that would otherwise not be accessible to the impl in the new location. Ideally we would check that that is the case before saying anything.

[Urgau]

I don't understand your comment, it's not betting, we are checking in the code if the type is at the same nesting, and if it is we emit the note saying that it might (as in may not be something you might want to do) need to be moved, there is no guess here.

[estebank]

Ah! I see. In that case I would make it a message explaining alternative ways of sealing traits and types?

[Urgau]

It doesn't make sense to recommend sealing the trait since if we warn that means it already not local so moving the impl shouldn't change it's availability/visibility.

Regarding sealing the type, idk if that's a thing, but it also wouldn't change anything, since if it is not local it is already leaked.

In general it is very difficult to say how users should change their code to make a impl local (which from the type-system point-of-view doesn't exist anyway), so in general the lint "fallback" to proposing to make the impl def true to what it is, a global impl, and as such should be in a "global context" (module) and not inside a function, consts, array count, ...

[Urgau]

All the review comments have been addressed or have been replied to.

@rustbot ready

[bors]

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