False positive and highly misleading suggestions for the non-local-definitions lint

[weiznich]

I tried this code: https://github.com/diesel-rs/diesel/blob/974814c6792c534736498a0bd7abf95e21f8bf7b/diesel_derives/tests/associations.rs#L37

I expected to see this happen: I get an error message that explains that this macro expands to a trait impl and therefore it triggers the lint as I (the user) placed the macro in a function scope.

Instead, this happened: I get a rather generic error that points to the macro (ok so far) and claims that this might be an issue with the dependency version (diesel in this case) I'm using.

Example error message:

error: non-local `impl` definition, they should be avoided as they go against expectation
  --> diesel_derives/tests/associations.rs:37:5
   |
37 |     joinable!(posts -> users(user_id));
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: move this `impl` block outside the of the current constant `_` and up 2 bodies
   = 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: the macro `$crate::joinable_inner` may come from an old version of the `diesel` crate, try updating your dependency with `cargo update -p diesel`
   = note: this error originates in the macro `$crate::joinable_inner` which comes from the expansion of the macro `joinable` (in Nightly builds, run with -Z macro-backtrace for more info)

Especially the suggestion = note: the macro allow_tables_to_appear_in_same_query may come from an old version of the diesel crate, try updating your dependency with cargo update -p diesel` is highly problematic in this context as that never could resolve the error given above. The wording might result in cases where the user will believe that this is an upstream diesel issue, rather than an issue with their local code.

Instead I propose that the lint detects weather the impl macro is placed in an local scope or not and adjusts the error message based on that.

The other interesting observation about that specific code example is that the lint is only triggered by the macro rules macro there and not by any of the proc macros above/below the linked line. In fact the macro also only generates impls that refer only to local types defined by the table! macro above, so arguably the lint is even wrong at all in this case as it does not add a non local impl.

(code for joinable!)

Relevant for #120363

Meta

rustc --version:

rustc 1.79.0-nightly (3a36386dc 2024-04-25)

[Urgau]

I think I have reduced the issue to this MCVE:

#![allow(dead_code, non_camel_case_types)]

trait JoinTo {}

fn simple_belongs_to() {
    mod posts {
        pub struct table {}
    }
    
    impl JoinTo for posts::table {}
}

playground

[ChayimFriedman2]

A gap in the implementation of #122747? The check probably doesn't account for this case:

rust/compiler/rustc_lint/src/non_local_def.rs

Lines 167 to 169 in 6acb9e7

	if self_ty_has_local_parent {
	return;
	}

[Urgau]

The RFC says that either the Trait or the Type should at the same nesting as the impl block, in the implementation we look at the parent item to see if they are equal.

In the MCVE:

• the impl block parent is fn simple_belongs_to
• JoinTo trait parent is the crate root
• and posts::table parent is mod posts

None of those are equal, so we emit the lint.

However, this is were things get interesting, rust-analyzer actually also consider "all ancestor of the current block"; which means that the impl block would be correctly taken into consideration, because in r-a terms block are not modules, just expressions; so making modules transparent would fix the issue in a way that is compatible with r-a.

edit: to be ignored

Except that it would fall apart quickly for r-a as not all parent blocks are not taken into consideration. EDIT: I was wrong, I was missing a import.

trait JoinTo {
    fn gg(&self) {}
}

fn simple_belongs_to() {
    mod posts {
        pub mod posts {
            pub struct table;
        }
        
        fn hh() {
            use crate::JoinTo; // EDIT: actually `rustc` doesn't compile without this import
                               // and adding it makes r-a also resolve this 🤔 
            let _a = posts::table.gg(); // --this is NOT resolved by r-a--
        }
    }
    
    impl JoinTo for posts::posts::table {}

    let _a = posts::posts::table.gg(); // but here it is resolved by r-a
}

Therefore strictly speaking I don't think is a false positive.

[Urgau]

Thanks to @Veykril, I now better understand r-a internals. I have confirmed with him that making modules transparent when checking if the impl def is at the same nesting as the Type or Trait would be fine for r-a¹.

Implementation wise making modules transparent would be very similar to the const _: () = { ... } exception that we already have. In the sense that we would consider all parent modules of the Type/Trait to be transparent, no neighbours.

I tried finding examples where type inference would make the impl non-local but couldn't find any, that doesn't mean there aren't, just that if there are, I was not able to find any.

Whenever we should do this is beyond me. cc @joshtriplett

Footnotes

1. Internally, when doing type/method resolutions, r-a considers, among other things, all ancestors of the current block, in which modules are basically transparent. ↩

[joshtriplett]

I don't see any obvious ways to get non-local impls out of this either. Assuming nobody finds any, this exception (treating a local module's contents as local) seems reasonable and consistent with what we're trying to do here.

I also don't see any obvious ways in which this closes off future avenues for adding ways to reach inside/outside of a module-inside-function. We've talked about adding a way for a nested module to reference the containing fn's scope, but that wouldn't create non-local behavior.

I'm nominating this for lang. If anyone in lang can see a way to get a non-local impl out of a module-inside-fn like this, please speak up. Otherwise, I think it's reasonable to fix this as proposed.

[glandium]

There's another false positive:

struct A;

fn foo() {
    struct B;
    
    impl From<B> for A {
        fn from(b: B) -> A {
            todo!()
        }
    }
}

The error shown is:

warning: non-local `impl` definition, they should be avoided as they go against expectation
  --> src/lib.rs:6:5
   |
6  | /     impl From<B> for A {
7  | |         fn from(b: B) -> A {
8  | |             todo!()
9  | |         }
10 | |     }
   | |_____^
   |
   = 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 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

Should I file a new issue?

[weiznich]

Thanks you all for your swift response. It would be great to see that resolved before it hits stable. The lint seems now to be enabled on the beta branch as well.

Also I want to highlight the other part of the issue (misleading message) again:

Especially the suggestion = note: the macro allow_tables_to_appear_in_same_query may come from an old version of the diesel crate, try updating your dependency with cargo update -p diesel is highly problematic in this context as that never could resolve the error given above. The wording might result in cases where the user will believe that this is an upstream diesel issue, rather than an issue with their local code.

I feel that this should detect whether an update could even resolve this issue. This is not the case if the user put the macro call inside a function on it's own, which I feel is something that should be easy to detect while emitting the lint. (If someone points to the right location I even can have a look at that myself.)

[Urgau]

Updating the labels so we track this issue as a regression.

@rustbot labels -needs-triage +regression-from-stable-to-beta

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this in the lang meeting today. Our consensus was in favor of adopting the modification to the RFC 3373 rules discussed here, to make "modules transparent when checking if the impl def is at the same nesting as the" type or trait.

I.e., this code is not a "sneaky inner impl" and should not lint:

trait Trait {}
fn foo() {
    mod m {
        pub struct Ty {}
    }
    impl Trait for m::Ty {}
}

This is good to go on the lang side, and we leave it as usual to the compiler team to decide on the appropriate follow-up actions.

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

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

Since this looks like a diag regression so maybe confusing but not critical.

Fixed by #124539 (beta-nominated)

@rustbot label -I-prioritize +P-medium

[wesleywiser]

I think since this lint is new in 1.79 this is a higher priority than a normal diagnostic regression. Retagging as P-high.

@rustbot label -P-medium +P-high

[Urgau]

The fixed has landed in 1.80 (#124539) and now in 1.79 (#124947) as well.