non_local_definitions common issues: impl for &Local, From<Local> for Global, ...

[dtolnay]

I wonder whether the following code should be triggering non_local_definitions. It currently does and I think it's a bug:

pub trait Trait {}

fn main() {
    struct Thing;
    impl Trait for &Thing {}
}

warning: non-local `impl` definition, they should be avoided as they go against expectation
 --> src/main.rs:5:5
  |
5 |     impl Trait for &Thing {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: move this `impl` block outside the of the current function `main`
  = note: an `impl` definition is non-local if it is nested inside an item and neither the type nor the trait are at the same nesting level as the `impl` block
  = 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

A real-world occurrence can be seen in https://github.com/serde-rs/json/blob/e1b3a6d8a161ff5ec4865b487d148c17d0188e3e/tests/test.rs#L2334-L2346.

In general #[fundamental] exists to allow what would otherwise be a coherence violation, and it would make sense for the same exception to be applied here for the non_local_definitions lint.

---

• Tracking issue: Tracking Issue for RFC 3373: Avoid non-local definitions in functions #120363

[mumbleskates]

It's not clear to me what this has to do with #[fundamental] specifically. In your example, the type &Thing is only defined inside the scope of the main function. The lint warning is complaining that the impl for that type is not hoisted out of the function to the top level of the module, which is patently impossible. It would be similarly impossible for any other trait or type that names Thing.

[mumbleskates]

Take for example:

trait Trait<T> {}
struct Foo;

pub fn foo() {
    struct Bar;
    impl Trait<Bar> for Foo {}
}

with rustc 1.78.0-nightly (0ecbd0605 2024-02-25):

warning: non-local `impl` definition, they should be avoided as they go against expectation
 --> src/lib.rs:6:5
  |
6 |     impl Trait<Bar> for Foo {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = 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 neither the type nor the trait are at the same nesting level as the `impl` block
  = 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

That impl cannot be moved outside of its current function either. The lint is asking for the impossible.

[dtolnay]

In my understanding of rust-lang/rfcs#3373, the "patently impossible" aspect of relocating an impl does not come into play.

The motivation for the RFC is: "This change helps humans limit the scope of their search and avoid looking for definitions inside other functions or items, without missing any relevant definitions."

To understand why #[fundamental] is the relevant consideration for this false positive, consider this variant of your example:

trait Trait<T> {
    fn method(&self) {}
}

struct Foo;

fn _foo() {
    struct Bar;
    impl Trait<Bar> for Foo {}
}

fn main() {
    Foo.method();
}

This code compiles, but correctly triggers non_local_definitions.

In your framing of the warning, non_local_definitions should not be triggered because relocating impl Trait<Bar> for Foo out of fn _foo without relocating struct Bar does not work.

But actually, the warning does need to apply here in order to achieve its motivation, because if a reader were to limit the scope of their search to avoid looking for definitions inside other functions, they would miss the relevant definition that Foo.method() is resolving to. What the warning wants is for you to move both struct Bar and the impl out.

#[fundamental] is different because such an impl cannot have effect outside the function it's located in, if the fundamental type's type parameter is local.

[WaffleLapkin]

I believe the lint is correct here. Note that you can actually invoke Trait's method:

pub trait Trait: Sized {
    fn method(self) {}
}

fn _local() {
    struct Thing;
    impl Trait for &Thing {}
}

fn main() {
    Trait::method(&{ todo!() });
    // this is a bit silly, but you could imagine `Thing: Default`, 
    // or some other way to actually construct `Thing`
}

(play)

[WaffleLapkin]

In general #[fundamental] exists to allow what would otherwise be a coherence violation, and it would make sense for the same exception to be applied here for the non_local_definitions lint.

Yeah, I originally also thought that this lint is basically a coherence check, but it actually turns out that it needs to be much stricter, because of the examples like the above.

[daxpedda]

Is this also supposed to trigger the lint:

trait Trait<T> {
    fn method(&self) {}
}

struct Foo;

fn _foo() {
    struct Bar;
    const _: () = {
        impl Trait<Bar> for Foo {}
    };
}

fn main() {
    Foo.method();
}

(playground)

Otherwise I'm not really getting the "anon-const exception" from the warning.

[WaffleLapkin]

@daxpedda yes. the idea of the lint is that "you don't need to look into functions". to check if there is an anon-const in the function you need to look into the function, so this would not make a very useful lint :)

An example where const _ suppresses the lint is something like this:

trait Trait<T> {
    fn method(&self) {}
}

struct Foo;

const _: () = {
    struct Bar;
    impl Trait<Bar> for Foo {}
};
    
fn main() {
    Foo.method();
}

(play)

[davidhewitt]

For what it's worth we hit this lint a lot in PyO3's CI this morning. We for example have a create_exception! macro, which can be used to create a new Python exception type. Because PyO3 uses references a lot (although we are reworking that at the moment), the macro expands to a lot of trait implementations for &Local.

e.g.

#[test]
fn custom_exception() {
    pyo3::create_exception!(mymodule, CustomError, PyException);
    
    // ...   
}

expands to

#[test]
fn custom_exception() {
    pub struct CustomError(::pyo3::PyAny);
    
    // this implementation triggers the lint, for example
    impl ::std::convert::From<&CustomError> for ::pyo3::PyErr {  
        #[inline]
        fn from(err: &CustomError) -> ::pyo3::PyErr {
            #[allow(deprecated)]
            ::pyo3::PyErr::from_value(err)
        }
    }
    
    // this block does not trigger the lint
    impl CustomError {
        // ...
    }
    
    // lots more stuff
    
    // ...
}

---

PyO3 can rework as needed but it seemed worth flagging here.

For what it's worth, PyO3 is also hitting the case @daxpedda shows above where we define impls for a type inside an anon-const next to the type. This is convenient for us because we can use whatever we need inside the anon-const block. Again we can refactor if the consensus is we need to move.

[mumbleskates]

Damn. Is there any possible way to preclude that action-at-a-distance? I'm not finding any and this might mean I have to swap the Foo and Bar for some of my traits

For that matter, it appears to be not permissible to implement From<LocalType> for usize or other foreign types, and it's not clear if there's any good alternative to that.

[dtolnay]

Thank you @WaffleLapkin. #121621 (comment) is convincing about why the &Local case is intended behavior, not a false positive. I would close the issue but since there have been 4 commenters in the past 15 minutes, maybe I'll let the discussion play out a bit before closing. Thank you again.

I think this has been valuable signal for this unresolved question from the RFC: "We need a crater run to look at how widespread this pattern is in existing code." It is extremely widespread. The case of someone putting a non-local impl for a non-local type into a function body for shits and giggles isn't necessarily widespread. However, impls that are considered non-local by the lint, but are somehow local enough in practice like the PyO3 case, appear to be widespread.

[WaffleLapkin]

@davidhewitt note that const _ next to a type is fine (#121621 (comment)), the issue only arises if the anon const is inside a function.

[Urgau]

@dtolnay Regarding how widespread it is, we did a crater run on implementation PR, the analysis can be found here, we concluded that yes it is somewhat widespread but can also be fixed in 90% of the cases by a dependency update. As for the cases described here, we are hitting the "harder" cases where a manual intervention is needed.

[WaffleLapkin]

For what it's worth we could carve out exceptions, given that there is significant fallout. @davidhewitt's example in #121621 (comment) is, I'm pretty sure, fine. The problem is that "can you mention any items from an implementation in a function" — is a complicated question.

The example is only fine because there is another impl of From (namely, the identity T: From<T>), so the "only single impl" rule doesn't apply and we can't infer the local type outside in any way. (I'm pretty sure? might be missing something)

This is tricky though so I'm unsure if we can actually implement such carve-outs.

[daxpedda]

However, impls that are considered non-local by the lint, but are somehow local enough in practice like the PyO3 case, appear to be widespread.

This is also very frequently used with #[wasm_bindgen], where it is sometimes undesirable to make bindings accessible at the top-level:

fn do_some_work() {
    #[wasm_bindgen]
    extern "C" {
        type Global;

        #[wasm_bindgen(method, getter, js_name = Window)]
        fn window(this: &Global) -> JsValue;
    }

    let global: Global = js_sys::global().uncheck_into();

    if !global.window().is_undefined() {
        // do work on `global`.
    }
}

Where the idea is to define some bindings only where they are needed and not make them accessible outside their scope.

Specifically wasm-bindgen runs into two problems:

• Implementing a trait for &Global, maybe this can be removed on wasm-bindgens side in favor of a blanket implementation.
• Implementing From<Global> for JsValue (JsValue being a type defined outside the local scope).

---

I guess the problem could be addressed if Rust gains the ability to prevent local trait implementations being used outside the scope they are defined in. The only thing I remember in this regard is the restrictions RFC under "Future Possibilities":

• Trait implementations could be restricted to being used within a certain scope.

But given the context of the restrictions RFC I presume this is meant to handle module-level scopes only, not function-level scopes.

[dtolnay]

@Urgau, #120363 mentions evaluating a hard error for 2027. From your crater report analysis, do you get the impression that's still in the cards?

As the one publishing the dependency updates that other people pull in via cargo update to fix this warning, sadly this is what they looked like: dtolnay/typetag@71bf063. For that macro, I don't see anything else it could do. It would just become unusable in functions if allow would no longer work, which seems needless. Hopefully we are able to do some clever carve-outs.

[davidhewitt]

@davidhewitt note that const _ next to a type is fine (#121621 (comment)), the issue only arises if the anon const is inside a function.

Thanks @WaffleLapkin, while true, PyO3 is not under control (and has no way of knowing) of whether the user chooses to place our macro inside a function. For example:

// lint does not fire here because the anon-const is allowed
create_exception!(mymodule, CustomError, PyException);

#[test]
fn test_custom_exception() {
    // lint DOES fire here because the same structure is anon-const inside a function, which
    // is not allowed currently
    create_exception!(mymodule, CustomError, PyException);
}

So even though the anon-const exception is allowed at top level, PyO3 cannot rely on it because users may innocently move their types to be more local and have complex complex lints thrown at them as a result.

---

EDIT: I've slightly mixed examples here, it is our #[pyclass] and #[pyfunction] macros that are using anon-const and I believe can be reworked to not use it.

But I think my point still stands that proc macros cannot know where their expansion is, so they would not be able to rely on the exception, and the lint error which would be seen by users if a macro did trigger this is probably confusing for them.

Probably in 99% of cases the right hint for users would just be "move this type and its macro annotation outside of fn foo".

[Urgau]

#120363 mentions evaluating a hard error for 2027. From your crater report analysis, do you get the impression that's still in the cards?

I don't know, we would first have to make the lint deny-by-default in the 2024 edition (since it currently isn't, the note is only indicative that we may do it) and then in 3 years and only then we would a do a another crater run and decide.

[Urgau]

EDIT: I've slightly mixed examples here, it is our #[pyclass] and #[pyfunction] macros that are using anon-const and I believe can be reworked to not use it.

@davidhewitt I don't think it is necessary the crater run we did did not found significant issues with #[pyclass] and #[pyfunction]. I tried to signal-it before-hand to the most impacted crates, but couldn't do it for every crates.

Note the anon-const is an exception because of how widespread the pattern of using const _: () = { impl ... for TypeJustAbove } is (looking at you serde_derive ;-).

But I think my point still stands that proc macros cannot know where their expansion is, so they would not be able to rely on the exception, and the lint error which would be seen by users if a macro did trigger this is probably confusing for them.

This is indeed an issue, for which we don't have a solution for; we cannot lint in the macro definition, we are forced to lint at the definition, even if that means in downstream users.

Probably in 99% of cases the right hint for users would just be "move this type and its macro annotation outside of fn foo".

I also expect this. I tried making the help messages as helpful as I could. Let me know how we can improve it (maybe with a best-effort structured suggestion).

---

It's a change of paradigm, we are becoming more strict, instead of less strict (allowing more, like we usually do; but it's for the better), it is therefore perfectly understandable that it will take time for the ecosystem to adapt to the change.

[mumbleskates]

In the case where the type is local and the impl cannot be hoisted without it, the warning may make sense (even if it is very unfortunate)... except in cases where that local type could never be inferred ex nihilo. Which is, as far as I can tell, actually most cases in practice. I think that the minimum bar for this is "there is literally any* other impl" but I could be wrong.

(*confusable: having a different type in the same generic argument)

Would this mean that we could possibly give a pass when the impl is at the same nesting as the local type, and the trait is known to have another confusable impl either in its scope of definition or in the outer scopes of the local?

example of a confusable and a non-confusable impl

use std::fmt::Debug;

trait Trait<T: Debug, U: Debug> {
    fn method(&self, t: T, u: U) {
        println!("{t:?} {u:?}")
    }
}

struct Foo;

#[allow(dead_code)]
#[derive(Debug, Default)]
struct NotThis {
    not_chosen: String,
}
impl Trait<NotThis, usize> for Foo {}

fn _foo() {
    // This is correctly caught by the lint
    #[derive(Debug, Default)]
    struct LocalFoo;
    impl Trait<LocalFoo, u8> for Foo {}
}

fn _bar() {
    // This is caught by the lint, but could perhaps be allowed
    #[derive(Debug, Default)]
    struct LocalBar;
    impl Trait<LocalBar, usize> for Foo {}
}

fn main() {
    Foo.method(Default::default(), 1u8); // Ok: prints "LocalFoo 1"

    // Foo.method(Default::default(), 1usize); // Does not compile
    //     ^^^^^^ ------------------ type must be known at this point
}

[Urgau]

what kinds of nested impls are actually okay to implement

We have, either the Trait or Type (of impl Trait for Type) must be at the same nesting level as the impl (modulo anon-const).

the following patterns would be forbidden

This generates a local impl definition, impl Clone for Nested, the type (Nested) is at the same nesting as the impl.

Along the lines of what has already been discussed, even if we somehow concede to carving out an exception for builtin traits, the following examples still compile without warning and exhibit hidden coherence: [..]

1. The first example seems fine to me, putting impls in modules is expected, your example is no different than with a simple mod inner; (technically speaking a module is not a body)
2. Modules inside a function should be warned against, this is a known deficiency of the lint as currently implemented

[davidhewitt]

Modules inside a function should be warned against, this is a known deficiency of the lint as currently implemented

Using mod itself is local, no? I would have thought it would just be items within that nested module that need to be checked for violations of this lint.

[Veykril]

Modules inside a function should be warned against, this is a known deficiency of the lint as currently implemented

Using mod itself is local, no? I would have thought it would just be items within that nested module that need to be checked for violations of this lint.

Mods (specifically mod foo;, not mod foo { ... }) inside functions mean you need to parse all function bodies to be able to know the complete crate graph, which basically gives at least IDEs the same problems that these kinds of impls the lint forbids give.

[Urgau]

T-types members. @WaffleLapkin and I were wondering if you had any inputs on this issue.

RFC 3373: avoid-nonlocal-definitions-in-fns: basically defines two rules for linting on "non-local impl definitions":

• An item nested inside an expression-containing item (through any level of nesting) may not define an impl Type block unless the Type is also nested inside the same expression-containing item.
• 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.

Note that they exclude any generics or references, as they can leak. But doing From<Local> for Global inside functions is widespread enough.

We were wondering if you had any idea make a more accurate rule? (if that's even possible)

@rustbot label +I-types-nominated

[mumbleskates]

putting impls in modules is expected

So the lint, and the proposed constraint, is specifically about having nested impls inside nested scopes that aren't modules. That's a caveat that didn't seem especially vocal. Note that this can mean the impl is in another file, arbitrarily nested in the crate. Previously my understanding was that this is a problem solely by for the crate author, and not some kind of coherence hazard we need to globally forbid.

This generates a local impl definition, impl Clone for Nested, the type (Nested) is at the same nesting as the impl.

Okay, I've fiddled with this more and I think I'm convinced that the Self type of a trait implementor can't ever be trivially deduced the way it can on methods and functions of other types.

This still leaves us some annoying exceptions where the pattern of implementation is to attach the impl to the other type, like From, which I personally believe can still be valid for user traits.

[dhardy]

Copied from above:

trait Trait<T> {
    fn method(&self) {
        println!("method for {}", std::any::type_name::<T>());
    }
}

struct Foo;

fn _nested_mod_trait() {
    mod func_inner {
        pub(super) struct Nested;
        impl super::Trait<Nested> for super::Foo {}
    }
}

fn main() {
    Foo.method()
}

Surely the real issue here is that type inference should not be able to resolve a not locally accessible type (this is distinct from something which is nameable but not in scope, and also from something which is "accessible" but not nameable like the type of a local closure). IIUC that should be enough to allow Nested to be considered private to _nested_mod_trait, and thus make this call to Foo.method() illegal.

While a breaking change, I guess that would have a much lower breakage rate than the current lint.

[mumbleskates]

That or something very close to that, yes. The fact that deduced types can end up being types that have no path to be addressed from the calling code seems bad. If the type is reachable but is private this already fails to compile: if pub(super) is removed from the above example Nested is no less visible than before, yet it now fails to compile.

The fact that items defined inside a function don't act as if they are members of a private and unnameable module is strange. The fact that they can be marked public as well is even stranger (this doesn't seem to serve a purpose in normal usage, but when the method call is in another module it affects whether deducing the type is allowed).

The fact that this kind of type deduction can happen, not the fact that the impl can exist, is by far the more surprising behavior to me. I don't believe I'm alone in this.

To note, I'm not interested in arguing aobut the legalism of the purposes and mechanics of pub/private as compared to those of an item's accessibility which cause these things to happen. I think it's a sensible comparison of ideas, and am offering a contrast between systems that do seem to work in a sensible way today with ones that do not, and I think we should strive for a world where both of them behave equally intuitively.

[WaffleLapkin]

@dhardy this is part of the issue, but not all. The "single applicable impl" rule¹ makes it so adding an impl when there is exactly one other impl is still a problem:

trait Trait<T> {
    fn method(&self) {
        println!("method for {}", std::any::type_name::<T>());
    }
}

mod private {
    // struct Private;
    // impl super::Trait<Private> for () {}
}

struct Public;
impl self::Trait<Public> for () {}

fn main() {
    ().method()
}

(uncommenting the private impl makes the code not compile; play)

Footnotes

1. I am not sure if this is proper terminology; not a trait solving expert ↩

[dhardy]

@WaffleLapkin yes, the "single applicable impl" rule is indeed the problem. If impl ForeignTrait<LocalType> for ForeignType is allowed where that rule does not apply (and where it is not the only thing preventing that rule from not being applicable), then I think that solves most/all of the problems here?

I wonder if we should add std::marker::PhantomType such that one can write impl MyTrait for PhantomType { .. }¹ purely to prevent that rule from being applicable.

Footnotes

1. Obviously this needs an RFC and possibly new syntax just to avoid having to write useless impls for each method. ↩

[mumbleskates]

I wonder if we should add std::marker::PhantomType such that one can write impl MyTrait for PhantomType { .. }1 purely to prevent that rule from being applicable.

Would this be superceded in usability by impl MyTrait for never { .. } if and when that feature is stabilized?

[lcnr]

t-types discussed this on zulip: https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/.23121621.3A.20.60non_local_definitions.60.20common.20issues.3A.20impl.20for.20.60.26.E2.80.A6 and then during the sync meeting https://rust-lang.zulipchat.com/#narrow/stream/326132-t-types.2Fmeetings/topic/2024-03-04.20planning.20meeting/near/424676409

firstly, the impl in the following example cannot leak from the local scope, so this is a false positive of the lint:

fn foo() {
    struct Local(u8);
    impl From<Local> for u8 {
        fn from(x: Local) -> u8 {
            x.0
        }
    }
}

fn main() {
    foo();
}

we discussed a possible rule to check whether an impl can "leak out of the local scope" in https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/.23121621.3A.20.60non_local_definitions.60.20common.20issues.3A.20impl.20for.20.60.26.E2.80.A6/near/423642405

pretty much:

for each impl, instantiate all local types with inference vars and then assemble candidates for that goal, if there are more than 1 (non-private impls), it does not leak

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this in the meeting today. Our feeling was that we should probably reevaluate after what @lcnr proposed above is implemented. If there are still questions that remain after this is done, please renominate for us.

[vlad20012]

@lcnr Is it true that with these new rules this code does not trigger the lint:

struct Global;

fn foo() {
    struct Local(Global);
    impl From<Local> for Global {
        fn from(x: Local) -> Global {
            x.0
        }
    }
}

fn bar() {
    struct Local(Global);
    impl From<Local> for Global {
        fn from(x: Local) -> Global {
            x.0
        }
    }
}

fn main() {
    foo();
    bar();
}

While the lint will be triggered in the function foo if I comment out the function bar?

[lcnr]

The lint should trigger here, as the rule is:

for each impl, instantiate all local types with inference vars and then assemble candidates for that goal, if there are more than 1 (non-private impls), it does not leak

so when looking at candidates, we ignore all candidates which are "hidden", so Global: From<?infer> has only global impl which would apply Global: From<Global>