`From<Local> for Global` with `non_local_definitions` lint

[Urgau]

There's another false positive:

struct A;

fn foo() {
    struct B;
    
    impl From<B> for A {
        fn from(b: B) -> A {
            todo!()
        }
    }
}

The error shown is:

warning: non-local `impl` definition, they should be avoided as they go against expectation
  --> src/lib.rs:6:5
   |
6  | /     impl From<B> for A {
7  | |         fn from(b: B) -> A {
8  | |             todo!()
9  | |         }
10 | |     }
   | |_____^
   |
   = 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 may impact type checking outside of that item. This can be the case if neither the trait or the self type are at the same nesting level as the `impl`
   = 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

Originally posted by @glandium in #124396 (comment)

[Urgau]

I don't think this is a false-positive.

The presence or absence of the impl definition has an impact outside of the function, making it non-local. See this slightly modified example:

#[derive(Default)]
struct A;

fn _foo() {
    struct B;
    
    impl From<B> for A {  // without this impl, this code compiles fine
        fn from(b: B) -> A {
            todo!()
        }
    }
}

fn main() {
    let _ = A::from(Default::default());  // but with it, this line errors with an ambiguity error
}

error[E0283]: type annotations needed
  --> src/main.rs:15:13
   |
15 |     let _ = A::from(Default::default());
   |             ^ cannot infer type for type parameter `T` declared on the trait `From`
   |
note: multiple `impl`s satisfying `A: From<_>` found
  --> src/main.rs:7:5
   |
7  |     impl From<B> for A {
   |     ^^^^^^^^^^^^^^^^^^
   = note: and more `impl`s found in the following crates: `core`:
           - impl<T> From<!> for T;
           - impl<T> From<T> for T;

Obviously this example is a bit silly, it could be made more complex cases with generic functions and what not. But it already demonstrate the effect of the impl From<B> for A definition outside the _foo function, and through is IMO correctly categorised as non-local.

@rustbot labels -needs-triage +C-discussion +T-compiler

[glandium]

I would argue that your modified example shouldn't be an error in the first place. From a human perspective, there is no ambiguity in A::from's use in main because B not being in scope, it can't be From'ed from.

[Urgau]

I agree with you, having local (for human) definitions have a global effect is unexpected and goes against expectation. That is what RFC 3373: Avoid non-local definitions in functions is about, warning slowly but surely against those cases, so we can at some point turn them into errors.

[Urgau]

Here is an un-modified example demonstrating the global effect of your impl From<B> for A.

struct A;

fn foo() {
    struct B;
    
    impl From<B> for A { // with this impl, this code below doesn't compile anymore
        fn from(b: B) -> A {
            todo!()
        }
    }
}

fn bar<T: From<U>, U>() {}

fn main() {
    bar::<A, _>(); // without the impl, this would compile fine, but with it it's a ambiguity error
                   // therefore the impl is not local and has a global effect
}

[glandium]

What I'm saying is that it's an ambiguity error because rustc currently makes it one, but it doesn't have to be one.

[bjorn3]

How is the ambiguity error avoidable? Ignoring the local impl entirely would be incorrect as there are cases where the local impl is the only applicable impl. For those cases ignoring the local impl would be a breaking change. For example:

trait MyFrom<T> {}

struct A;

fn foo() {
    struct B;
    
    impl MyFrom<B> for A {}
}

fn bar<T: From<U>, U>() {}

fn main() {
    bar::<A, _>();
}

which is basically #124557 (comment) except without the impl<T> From<T> for T that cases an ambiguity for in that test.

[Urgau]

As was said in the thread, this isn't a false positive. Closing.