False positive and highly misleading suggestions for the `non-local-definitions` lint about rust HOT 12 CLOSED

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

from rust.

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

• rust-lang/rfcs#3373
• #120363

from rust.

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

from rust.

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.

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
}

from rust.

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

from rust.

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.

from rust.

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?

from rust.

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

from rust.

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

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

from rust.

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

from rust.

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

from rust.

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

from rust.