`non_local_definitions` lint fires for `impl Trait for NonLocalType<SomeLocalType>`, probably shouldn't

[jstarks]

#120363 introduces some warnings that seem like bad advice. Nightly now warns if a function has an internal implementation of a trait on a non-internal (to that function) type. That seems like a reasonable warning in the general case, but it doesn't seem so reasonable for types like Box<Inner>, where Inner is defined within the function and cannot be named outside it.

It rather seems like something like the orphan rule should apply. You should be able to impl traits on Foo<T> as long as T is defined at the same level as the impl.

Playground link

I tried this code on nightly:

#![allow(dead_code)]
trait Tr {}
fn foo() {
    struct Bar;
    impl Tr for Box<Bar> {}
}

I expected to see this happen: no errors/warnings

Instead, this happened:

warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
 --> src/lib.rs:5:5
  |
5 |     impl Tr for Box<Bar> {}
  |     ^^^^^--^^^^^---^^^^^
  |          |      |
  |          |      `Box` is not local
  |          `Tr` 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 the `impl` block outside of this function `foo`
 --> src/lib.rs:3:1
  |
3 | fn foo() {
  | ^^^^^^^^
4 |     struct Bar;
  |     ---------- 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

Meta

rustc --version --verbose (well, from the playground):

Build using the Nightly version: 1.81.0-nightly
(2024-06-19 d8a38b00024cd7156dea)

Backtrace

<backtrace>

[Urgau]

where Inner is defined within the function and cannot be named outside it.

It's not because it cannot be named that it cannot be inferred.

This can be demonstrate with your example, here Bar can be inferred outside the foo function, showing that the impl definition is not local:

trait Tr {}

fn foo() {
    struct Bar;
    impl Tr for Box<Bar> {} // with this impl definition
}

fn do_stuff<U: Tr>() {}

fn main() {
    do_stuff::<Box<_>>(); // this code will compile, as `_` will be resolved to `Bar`
}

@rustbot labels -C-bug +C-discussion +L-non_local_definitions -needs-triage

[traviscross]

@rustbot labels +I-lang-nominated

This is kind of a surprising interaction of things. Nominating as we should discuss for visibility and to confirm whether this is what we meant to do.

(In general, we had wanted to support people making local impls for local types on outer traits. While it's clear the impls in this issue do have a non-local effect, the only workaround is to promote the local type to a broader scope outside of the function. That seems a bit strange and unfortunate, and probably isn't what the user wanted to do. There may or may not be a better option; we should just discuss to confirm.)

[Urgau]

@traviscross I agree that this interaction is surprising and I think that because it is surprising (and non-intuitive) we should warn about it.

@workingjubilee brought up in #125068 (comment) an example where someone might write a private type that for someone might rely on privacy to not be constructible outside of the function:

Also, just to remove all doubt as to whether this is a Real Problem, I present a new example of a completed escape:

pub trait Trait {}
fn test() {
    // Private field to make it more annoying to construct!
    #[derive(Default)] // oh, but what's this...?
    struct Local(i32);
    impl Trait for Option<Local> {}
}

fn do_stuff<U: Trait + Default>() -> U {
    Default::default()
}

fn main() {
    let x = do_stuff::<Option<_>>().unwrap_or_default();
    println!("an instance of {} escaped", std::any::type_name_of_val(&x));
}

I think that code would be something someone would probably write, especially if we're talking about a fairly big private function, not just the 4 lines in this example but more like 400 lines. They happen. But if someone was relying on that type not being constructible outside that fn, they are now dead wrong!

Note that it doesn't work with just U: Default though because that won't unambiguously resolve and it will demand type annotations. So this is relying on the fact rustc secretly knows there is another type, but only one, that can apply.

It's still not clear to me whether we should trigger the lint on the initial example here, as this requires two steps... a way to make the type constructible... but the answer to "is there something unexpected", i.e. is this making the code (and moreover, the code's implications) harder to understand... is probably Very Yes.

As for "bad" or "good", well, I don't like the thought of calling this code "bad" because it doesn't seem inherently objectionable. You may simply not care that much. But I don't think it's a great shining example of code either. It's just... there.

[traviscross]

@rustbot labels -I-lang-nominated -C-discussion +C-bug

We discussed this in the lang call today. We had everyone there, and our unanimous consensus was that we do not want the RFC 3373 lint to fire in these cases. E.g., we do not want to lint against:

trait Tr {}
fn foo() {
    struct S {};
    impl Tr for &S {}
}

This has non-local effect due to the "only one thing could go here" inference rule. But still, we consider this beyond the intention of the lint, and if we were to later try to tighten things up (e.g. for the benefit of tooling), we may consider instead trying, in a new edition, to do away with or limit the inference rule (e.g. by adding a check to error if the type being inferred can't be named). For that, we discussed how there may be some precedent for it in the RFC 2145 work from @petrochenkov.

That is, we'd expect people to be able to write (lint-free) the code above, and so we're OK with accepting the leakage here, as we tentatively plan to deal with that in a different way.

Thanks to @jstarks for raising this point and filing this issue.