`non_local_definitions` common issues: impl for `&Local`, `From<Local> for Global`, ...

[dtolnay]

I wonder whether the following code should be triggering non_local_definitions. It currently does and I think it's a bug:

pub trait Trait {}

fn main() {
    struct Thing;
    impl Trait for &Thing {}
}

warning: non-local `impl` definition, they should be avoided as they go against expectation
 --> src/main.rs:5:5
  |
5 |     impl Trait for &Thing {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: move this `impl` block outside the of the current function `main`
  = note: an `impl` definition is non-local if it is nested inside an item and neither the type nor the trait are at the same nesting level as the `impl` block
  = note: one exception to the rule are anon-const (`const _: () = { ... }`) at top-level module and anon-const at the same nesting as the trait or type
  = 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

A real-world occurrence can be seen in https://github.com/serde-rs/json/blob/e1b3a6d8a161ff5ec4865b487d148c17d0188e3e/tests/test.rs#L2334-L2346.

In general #[fundamental] exists to allow what would otherwise be a coherence violation, and it would make sense for the same exception to be applied here for the non_local_definitions lint.

---

• Tracking issue: Tracking Issue for RFC 3373: Avoid non-local definitions in functions #120363

[mumbleskates]

It's not clear to me what this has to do with #[fundamental] specifically. In your example, the type &Thing is only defined inside the scope of the main function. The lint warning is complaining that the impl for that type is not hoisted out of the function to the top level of the module, which is patently impossible. It would be similarly impossible for any other trait or type that names Thing.

[mumbleskates]

Take for example:

trait Trait<T> {}
struct Foo;

pub fn foo() {
    struct Bar;
    impl Trait<Bar> for Foo {}
}

with rustc 1.78.0-nightly (0ecbd0605 2024-02-25):

warning: non-local `impl` definition, they should be avoided as they go against expectation
 --> src/lib.rs:6:5
  |
6 |     impl Trait<Bar> for Foo {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: move this `impl` block outside the of the current function `foo`
  = note: an `impl` definition is non-local if it is nested inside an item and neither the type nor the trait are at the same nesting level as the `impl` block
  = note: one exception to the rule are anon-const (`const _: () = { ... }`) at top-level module and anon-const at the same nesting as the trait or type
  = 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

That impl cannot be moved outside of its current function either. The lint is asking for the impossible.

[dtolnay]

In my understanding of rust-lang/rfcs#3373, the "patently impossible" aspect of relocating an impl does not come into play.

The motivation for the RFC is: "This change helps humans limit the scope of their search and avoid looking for definitions inside other functions or items, without missing any relevant definitions."

To understand why #[fundamental] is the relevant consideration for this false positive, consider this variant of your example:

trait Trait<T> {
    fn method(&self) {}
}

struct Foo;

fn _foo() {
    struct Bar;
    impl Trait<Bar> for Foo {}
}

fn main() {
    Foo.method();
}

This code compiles, but correctly triggers non_local_definitions.

In your framing of the warning, non_local_definitions should not be triggered because relocating impl Trait<Bar> for Foo out of fn _foo without relocating struct Bar does not work.

But actually, the warning does need to apply here in order to achieve its motivation, because if a reader were to limit the scope of their search to avoid looking for definitions inside other functions, they would miss the relevant definition that Foo.method() is resolving to. What the warning wants is for you to move both struct Bar and the impl out.

#[fundamental] is different because such an impl cannot have effect outside the function it's located in, if the fundamental type's type parameter is local.

[WaffleLapkin]

I believe the lint is correct here. Note that you can actually invoke Trait's method:

pub trait Trait: Sized {
    fn method(self) {}
}

fn _local() {
    struct Thing;
    impl Trait for &Thing {}
}

fn main() {
    Trait::method(&{ todo!() });
    // this is a bit silly, but you could imagine `Thing: Default`, 
    // or some other way to actually construct `Thing`
}

(play)

[WaffleLapkin]

In general #[fundamental] exists to allow what would otherwise be a coherence violation, and it would make sense for the same exception to be applied here for the non_local_definitions lint.

Yeah, I originally also thought that this lint is basically a coherence check, but it actually turns out that it needs to be much stricter, because of the examples like the above.