non-local-definitions lint fires for impl using private types

[ijackson]

To reproduce

pub trait Trait {}

pub fn test() {
    struct Local;

    impl Trait for Option<Local> {}
}

https://play.rust-lang.org/?version=beta&mode=debug&edition=2021&gist=6c4a42e00403c3a76de54d5010d7df08

Compile with Beta ("Build using the Beta version: 1.79.0-beta.4 (2024-05-10 a269819)")

Expected results

I expected this to compile without warning.

Actual results

warning: non-local `impl` definition, they should be avoided as they go against expectation
 --> src/lib.rs:6:5
  |
6 |     impl Trait for Option<Local> {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: move this `impl` block outside the of the current function `test`
  = 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

Analysis

This is the new lint from RFC3373 (tracking issue).

In this code, the trait implementation cannot be moved out of the function, because it relies on types that are local to the function. The impl is coherence-legal only because the trait is within the same crate (so, this function couldn't be written outside Trait's crate), since by coherence rules it's an impl on Option. But the impl is reasonable where it is because its effects are invisible outside the function - no-one outside this function can see Local, so they can't see Option<Local> either - so they can't be affected by the impl.

In principle, the types could be moved too. But this will usually be undesirable. The compiler definitely ought not to be suggesting the programmer needlessly expose private items, just to satisfy this lint. (Also moving the types might involve renaming them.)

ISTM that this demonstrates that the rules for this lint need to be considerably more complicated than they are.

@rustbot label +regression-from-stable-to-beta

[ijackson]

I think at the very least this lint should be downgraded from future-compat before this reaches stable, since we don't want people contorting their code to satisfy it.

[ijackson]

References to real-world code that triggered this: crate tor-persist , pattern introduced in 0.10.1 (2024-03-04). impl InstancePurgeHandler for Option<&'_ Expect>, arti.git MR to allow it for now.

[Urgau]

This is not a false-positive, the impl (definition) is non-local, it has an effect outside of the function. See this sample code for a direct proof.

pub trait Trait {}

pub fn test() {
    struct Local;

    impl Trait for Option<Local> {}  // with this impl this code compiles
}

fn do_stuff<U: Trait>() {}

fn main() {
    do_stuff::<Option<_>>();  // but without the impl, it would error here
}

Note that nowhere in main I mentioned Local.

Also note that the non_local_definitions lint and rules have nothing to do with coherence, as can be seen above.

@rustbot labels -regression-from-stable-to-beta -I-prioritize -C-bug

[ijackson]

This is not a false-positive, the impl (definition) is non-local, it has an effect outside of the function. See this sample code for a direct proof.

How exciting. I stand corrected.

However, I nevertheless disagree with the categorisation as "not a bug", and with the categorisation as "not a regression". The compiler prints a new lint, but:

1. The suggested resolution, moving the impl, does not work. So the compiler output is wrong.

2. Extending the suggested resolution (moving the type too) is undesirable and disruptive. Therefore there is no good resolution to the lint in this case, other than to allow it.

3. It is not appropriate to have a future compat lint for a reasonable use case, when there is no good way to avoid the lint. After all, that implies a plan to make this reasonable use case impossible, without a good alternative.