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.

[Urgau]

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

It is implied by the suggestion that not only the impl but also the "rest" should be moved out of the function, what about "move the impl and all the necessary types ..." ?

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.

It may be undesirable but it is nevertheless a solution.

There is also another solution, moving the Trait within the function, I suspect this also wouldn't work in your case, but it is also a solution.

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.

Technical note here, the lint is not a future incompat lint, it's a regular lint, that may (although quite unlikely at this point) be moved to deny-by-default in Rust 2024.

The goal of the lint, it to make it predictable to humans and machine where they need to search for impl defs, and with the lint (and next steps) both humans and tools can limit the scope of their search and avoid looking for definitions inside other functions.

So any impl def that would affect type-check outside it's function is off the table as it completely goes against to goal of the lint.

[workingjubilee]

I thought we were specifically excepting the cases where the items were only referenced inside the function.

[workingjubilee]

Well, I suppose an impl specifically for a Global<Local> def is a bit of a headscratcher on which way we should cut.

[workingjubilee]

This is an ambiguous case which the RFC does not address directly. We can still resolve this bug by specifying a disposition to these cases, but we cannot simply refer to the RFC's text to do so. It is empty of any such command.

[workingjubilee]

Indeed, one might naively assume it to be specifically permitted by this clause:

• An item nested inside an expression-containing item (through any level of
  nesting) may not define an impl Trait for Type unless either the Trait or
  the Type is also nested inside the same expression-containing item.

cc @joshtriplett Did you have an intention in mind regarding cases like this one?

[Urgau]

This is an ambiguous case which the RFC does not address directly.

I don't think so, the impl is clearly visible outside the function. The RFC text is IMO pretty clear on this:

Add a warn-by-default lint for items inside functions or expressions that implement methods or traits that are visible outside the function or expression.

It then explain in the "Explanation" section how it should be determined if the impl is non-local which is the strictly the case here. (note that we already refined the lint a bit to reduce some false-positives, but this isn't the case, the impl is non-local).

I therefore disagree that this is (for now) a rule bug, T-lang may want to further refine the lint, but at least for now the lint (as in where it fires) is working as the RFC describes it.

[workingjubilee]

It is not, in my opinion, clear at all. It is thick as mud. Therefore, I disagree. It is a reasonable expectation that something might actually explain itself regarding such a case of composited (non-)locality before deciding to lint on it, but it does not. If the lint cannot explain itself to @ijackson why it applies, it is a bug.

[Urgau]

If the lint cannot explain itself to @ijackson why it applies, it is a bug.

I agree, the lint should better explain it's reasons. T-compiler touch this in the latest T-compiler meeting.
I started (a few days ago) a T-compiler topic on this: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/non_local_defs.20wording.20improvements

[ijackson]

If the lint cannot explain itself to @ijackson why it applies, it is a bug.

I think it needs to explain not just why it applies, but also suggest a reasonable alternative to the code it is impugning. (Assuming the code it is impugning is itself reasonable, which it is in this cae.)

I think this is a fundamental point. It's all very well to say "the lint is working as expected". But a lint ought to trigger only for code which is bad in some way.

In this case, the existing code isn't bad[1], and there isn't a good alternative. That makes linting it inappropriate.

([1] at least, no-one seems to be arguing here that the code is bad. At worst (reading the motivation for the RFC) it may be slightly awkward for certain IDEs. It's fine for the human reader. An argument that the code is bad ought to be accompanied by a suggestion for better code. It seems clear to me that "hoist the private types out of the fn" is worse.)

[workingjubilee]

As Urgau mentioned, the weird effect that was mentioned is a gotcha that might affect people's code negatively if they were relying on visibility to e.g. control a type's constructibility while still offering a public API based on a given trait:

pub trait Trait {}
pub fn test() {
    struct Local;
    impl Trait for Option<Local> {}
}

fn do_stuff<U: Trait>() {}

fn main() {
    do_stuff::<Option<_>>();
}

In that case, we should:

• say their impl may now 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.
• consider suggesting in this specific case they stop using Option<Local> and use a handrolled Enum<Local> if they were relying on visibility control there, or just implement whatever for just Local and not Option<Local>, if possible

[workingjubilee]

We may also want to link them to an issue for collecting more feedback from people, just so everyone shows up and goes "huhwhatwherewhy?" in the same spot.

[jieyouxu]

Edited the tracking issue to link to this.

[workingjubilee]

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 {}
pub 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.

[Urgau]

@ijackson We are trying to improve the diagnostic output in #125089. I've posted in #125089 (comment) the potential new diagnostic output for your case. If your have time, we would appreciate your opinion on it.