`non_local_definitions` lint fires for `impl Trait for NonLocalType<SomeLocalType>`, probably shouldn't about rust HOT 4 OPEN

@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.

from rust.

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

from rust.

@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.)

from rust.

@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.

from rust.