Eliminate false-positives in the non-local lint with the type-system

compiler/rustc_lint/src/non_local_def.rs

@@ -1,6 +1,18 @@
-use rustc_hir::{def::DefKind, Body, Item, ItemKind, Node, Path, QPath, TyKind};
+use rustc_hir::{def::DefKind, Body, Item, ItemKind, Node, TyKind};
+use rustc_hir::{Path, QPath};
+use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
+use rustc_infer::infer::InferCtxt;
+use rustc_infer::traits::{Obligation, ObligationCause};
+use rustc_middle::query::Key;
+use rustc_middle::ty::{self, Binder, Ty, TyCtxt, TypeFoldable, TypeFolder};
+use rustc_middle::ty::{EarlyBinder, TraitRef, TypeSuperFoldable};
 use rustc_span::def_id::{DefId, LOCAL_CRATE};
+use rustc_span::Span;
 use rustc_span::{sym, symbol::kw, ExpnKind, MacroKind};
+use rustc_trait_selection::infer::TyCtxtInferExt;
+use rustc_trait_selection::traits::error_reporting::ambiguity::{
+    compute_applicable_impls_for_diagnostics, CandidateSource,
+};
 
 use crate::lints::{NonLocalDefinitionsCargoUpdateNote, NonLocalDefinitionsDiag};
 use crate::{LateContext, LateLintPass, LintContext};
@@ -66,7 +78,8 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
             return;
         }
 
-        let parent = cx.tcx.parent(item.owner_id.def_id.into());
+        let def_id = item.owner_id.def_id.into();
+        let parent = cx.tcx.parent(def_id);
         let parent_def_kind = cx.tcx.def_kind(parent);
         let parent_opt_item_name = cx.tcx.opt_item_name(parent);
 
@@ -121,6 +134,7 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
                     None
                 };
 
+                // Part 1: Is the Self type local?
                 let self_ty_has_local_parent = match impl_.self_ty.kind {
                     TyKind::Path(QPath::Resolved(_, ty_path)) => {
                         path_has_local_parent(ty_path, cx, parent, parent_parent)
@@ -150,41 +164,70 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
                     | TyKind::Err(_) => false,
                 };
 
+                if self_ty_has_local_parent {
+                    return;
+                }
+
+                // Part 2: Is the Trait local?
                 let of_trait_has_local_parent = impl_
                     .of_trait
                     .map(|of_trait| path_has_local_parent(of_trait.path, cx, parent, parent_parent))
                     .unwrap_or(false);
 
-                // If none of them have a local parent (LOGICAL NOR) this means that
+                if of_trait_has_local_parent {
-                // this impl definition is a non-local definition and so we lint on it.
+                    return;
-                if !(self_ty_has_local_parent || of_trait_has_local_parent) {
-                    let const_anon = if self.body_depth == 1
-                        && parent_def_kind == DefKind::Const
-                        && parent_opt_item_name != Some(kw::Underscore)
-                        && let Some(parent) = parent.as_local()
-                        && let Node::Item(item) = cx.tcx.hir_node_by_def_id(parent)
-                        && let ItemKind::Const(ty, _, _) = item.kind
-                        && let TyKind::Tup(&[]) = ty.kind
-                    {
-                        Some(item.ident.span)
-                    } else {
-                        None
-                    };
-
-                    cx.emit_span_lint(
-                        NON_LOCAL_DEFINITIONS,
-                        item.span,
-                        NonLocalDefinitionsDiag::Impl {
-                            depth: self.body_depth,
-                            body_kind_descr: cx.tcx.def_kind_descr(parent_def_kind, parent),
-                            body_name: parent_opt_item_name
-                                .map(|s| s.to_ident_string())
-                                .unwrap_or_else(|| "<unnameable>".to_string()),
-                            cargo_update: cargo_update(),
-                            const_anon,
-                        },
-                    )
                 }
+
+                // Part 3: Is the impl definition leaking outside it's defining scope?
+                //
+                // We always consider inherent impls to be leaking.
+                let impl_has_enough_non_local_candidates = cx
+                    .tcx
+                    .impl_trait_ref(def_id)
+                    .map(|binder| {
+                        impl_trait_ref_has_enough_non_local_candidates(
+                            cx.tcx,
+                            item.span,
+                            def_id,
+                            binder,
+                            |did| did_has_local_parent(did, cx.tcx, parent, parent_parent),
+                        )
+                    })
+                    .unwrap_or(false);
+
+                if impl_has_enough_non_local_candidates {
+                    return;
+                }
+
+                // Get the span of the parent const item ident (if it's a not a const anon).
+                //
+                // Used to suggest changing the const item to a const anon.
+                let span_for_const_anon_suggestion = if self.body_depth == 1
+                    && parent_def_kind == DefKind::Const
+                    && parent_opt_item_name != Some(kw::Underscore)
+                    && let Some(parent) = parent.as_local()
+                    && let Node::Item(item) = cx.tcx.hir_node_by_def_id(parent)
+                    && let ItemKind::Const(ty, _, _) = item.kind
+                    && let TyKind::Tup(&[]) = ty.kind
+                {
+                    Some(item.ident.span)
+                } else {
+                    None
+                };
+
+                cx.emit_span_lint(
+                    NON_LOCAL_DEFINITIONS,
+                    item.span,
+                    NonLocalDefinitionsDiag::Impl {
+                        depth: self.body_depth,
+                        body_kind_descr: cx.tcx.def_kind_descr(parent_def_kind, parent),
+                        body_name: parent_opt_item_name
+                            .map(|s| s.to_ident_string())
+                            .unwrap_or_else(|| "<unnameable>".to_string()),
+                        cargo_update: cargo_update(),
+                        const_anon: span_for_const_anon_suggestion,
+                    },
+                )
             }
             ItemKind::Macro(_macro, MacroKind::Bang)
                 if cx.tcx.has_attr(item.owner_id.def_id, sym::macro_export) =>
@@ -207,6 +250,81 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
     }
 }
 
+// Detecting if the impl definition is leaking outside of it's defining scope.
+//
+// Rule: 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.
+//
+// https://github.com/rust-lang/rust/issues/121621#issuecomment-1976826895
+fn impl_trait_ref_has_enough_non_local_candidates<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    infer_span: Span,
+    trait_def_id: DefId,
+    binder: EarlyBinder<TraitRef<'tcx>>,
+    mut did_has_local_parent: impl FnMut(DefId) -> bool,
+) -> bool {
+    let infcx = tcx.infer_ctxt().build();
+    let trait_ref = binder.instantiate(tcx, infcx.fresh_args_for_item(infer_span, trait_def_id));
+
+    let trait_ref = trait_ref.fold_with(&mut ReplaceLocalTypesWithInfer {
+        infcx: &infcx,
+        infer_span,
+        did_has_local_parent: &mut did_has_local_parent,
+    });
+
+    let poly_trait_obligation = Obligation::new(
+        tcx,
+        ObligationCause::dummy(),
+        ty::ParamEnv::empty(),
+        Binder::dummy(trait_ref),
+    );
+
+    let ambiguities = compute_applicable_impls_for_diagnostics(&infcx, &poly_trait_obligation);
+
+    let mut it = ambiguities.iter().filter(|ambi| match ambi {
+        CandidateSource::DefId(did) => !did_has_local_parent(*did),
+        CandidateSource::ParamEnv(_) => unreachable!(),
+    });
+
+    let _ = it.next();
+    it.next().is_some()
+}
+
+/// Replace every local type by inference variable.
+///
+/// ```text
+/// <Global<Local> as std::cmp::PartialEq<Global<Local>>>
+/// to
+/// <Global<_> as std::cmp::PartialEq<Global<_>>>
+/// ```
+struct ReplaceLocalTypesWithInfer<'a, 'tcx, F: FnMut(DefId) -> bool> {
+    infcx: &'a InferCtxt<'tcx>,
+    did_has_local_parent: F,
+    infer_span: Span,
+}
+
+impl<'a, 'tcx, F: FnMut(DefId) -> bool> TypeFolder<TyCtxt<'tcx>>
+    for ReplaceLocalTypesWithInfer<'a, 'tcx, F>
+{
+    fn interner(&self) -> TyCtxt<'tcx> {
+        self.infcx.tcx
+    }
+
+    fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> {
+        if let Some(ty_did) = t.ty_def_id()
+            && (self.did_has_local_parent)(ty_did)
+        {
+            self.infcx.next_ty_var(TypeVariableOrigin {
+                kind: TypeVariableOriginKind::TypeInference,
+                span: self.infer_span,
+            })
+        } else {
+            t.super_fold_with(self)
+        }
+    }
+}
+
 /// Given a path and a parent impl def id, this checks if the if parent resolution
 /// def id correspond to the def id of the parent impl definition.
 ///
@@ -216,16 +334,29 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
 ///    std::convert::PartialEq<Foo<Bar>>
 ///    ^^^^^^^^^^^^^^^^^^^^^^^
 /// ```
+#[inline]
 fn path_has_local_parent(
     path: &Path<'_>,
     cx: &LateContext<'_>,
     impl_parent: DefId,
     impl_parent_parent: Option<DefId>,
 ) -> bool {
-    path.res.opt_def_id().is_some_and(|did| {
+    path.res
-        did.is_local() && {
+        .opt_def_id()
-            let res_parent = cx.tcx.parent(did);
+        .is_some_and(|did| did_has_local_parent(did, cx.tcx, impl_parent, impl_parent_parent))
-            res_parent == impl_parent || Some(res_parent) == impl_parent_parent
+}
-        }
+
-    })
+/// Given a def id and a parent impl def id, this checks if the parent
+/// def id correspond to the def id of the parent impl definition.
+#[inline]
+fn did_has_local_parent(
+    did: DefId,
+    tcx: TyCtxt<'_>,
+    impl_parent: DefId,
+    impl_parent_parent: Option<DefId>,
+) -> bool {
+    did.is_local() && {
+        let res_parent = tcx.parent(did);
+        res_parent == impl_parent || Some(res_parent) == impl_parent_parent
+    }
 }

tests/ui/lint/non_local_definitions.rs

@@ -282,7 +282,6 @@ struct Cat;
 
 fn meow() {
     impl From<Cat> for () {
-    //~^ WARN non-local `impl` definition
         fn from(_: Cat) -> () {
             todo!()
         }
@@ -374,6 +373,72 @@ fn rawr() {
             todo!()
         }
     }
+
+    #[derive(Debug)]
+    struct Elephant;
+
+    impl From<Wrap<Wrap<Elephant>>> for () {
+    //~^ WARN non-local `impl` definition
+        fn from(_: Wrap<Wrap<Elephant>>) -> Self {
+            todo!()
+        }
+    }
+}
+
+pub trait StillNonLocal {}
+
+impl StillNonLocal for &str {}
+
+fn only_global() {
+    struct Foo;
+    impl StillNonLocal for &Foo {}
+    //~^ WARN non-local `impl` definition
+}
+
+struct GlobalSameFunction;
+
+fn same_function() {
+    struct Local1(GlobalSameFunction);
+    impl From<Local1> for GlobalSameFunction {
+    //~^ WARN non-local `impl` definition
+        fn from(x: Local1) -> GlobalSameFunction {
+            x.0
+        }
+    }
+
+    struct Local2(GlobalSameFunction);
+    impl From<Local2> for GlobalSameFunction {
+    //~^ WARN non-local `impl` definition
+        fn from(x: Local2) -> GlobalSameFunction {
+            x.0
+        }
+    }
+}
+
+struct GlobalDifferentFunction;
+
+fn diff_foo() {
+    struct Local(GlobalDifferentFunction);
+
+    impl From<Local> for GlobalDifferentFunction {
+    // FIXME(Urgau): Should warn but doesn't since we currently consider
+    // the other impl to be "global", but that's not the case for the type-system
+        fn from(x: Local) -> GlobalDifferentFunction {
+            x.0
+        }
+    }
+}
+
+fn diff_bar() {
+    struct Local(GlobalDifferentFunction);
+
+    impl From<Local> for GlobalDifferentFunction {
+    // FIXME(Urgau): Should warn but doesn't since we currently consider
+    // the other impl to be "global", but that's not the case for the type-system
+        fn from(x: Local) -> GlobalDifferentFunction {
+            x.0
+        }
+    }
 }
 
 macro_rules! m {
@@ -404,3 +469,24 @@ fn bitflags() {
         impl Flags {}
     };
 }
+
+// https://github.com/rust-lang/rust/issues/121621#issuecomment-1976826895
+fn commonly_reported() {
+    struct Local(u8);
+    impl From<Local> for u8 {
+        fn from(x: Local) -> u8 {
+            x.0
+        }
+    }
+}
+
+// https://github.com/rust-lang/rust/issues/121621#issue-2153187542
+pub trait Serde {}
+
+impl Serde for &[u8] {}
+impl Serde for &str {}
+
+fn serde() {
+    struct Thing;
+    impl Serde for &Thing {}
+}

tests/ui/lint/non_local_definitions.stderr

@@ -480,23 +480,7 @@ LL | |     }
    = 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>
 
 warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:284:5
+  --> $DIR/non_local_definitions.rs:292:5
-   |
-LL | /     impl From<Cat> for () {
-LL | |
-LL | |         fn from(_: Cat) -> () {
-LL | |             todo!()
-LL | |         }
-LL | |     }
-   | |_____^
-   |
-   = help: move this `impl` block outside the of the current function `meow`
-   = 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>
-
-warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:293:5
    |
 LL | /     impl AsRef<Cat> for () {
 LL | |
@@ -510,7 +494,7 @@ LL | |     }
    = 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>
 
 warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:304:5
+  --> $DIR/non_local_definitions.rs:303:5
    |
 LL | /     impl PartialEq<B> for G {
 LL | |
@@ -526,7 +510,7 @@ LL | |     }
    = 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>
 
 warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:321:5
+  --> $DIR/non_local_definitions.rs:320:5
    |
 LL | /     impl PartialEq<Dog> for &Dog {
 LL | |
@@ -542,7 +526,7 @@ LL | |     }
    = 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>
 
 warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:328:5
+  --> $DIR/non_local_definitions.rs:327:5
    |
 LL | /     impl PartialEq<()> for Dog {
 LL | |
@@ -558,7 +542,7 @@ LL | |     }
    = 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>
 
 warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:335:5
+  --> $DIR/non_local_definitions.rs:334:5
    |
 LL | /     impl PartialEq<()> for &Dog {
 LL | |
@@ -574,7 +558,7 @@ LL | |     }
    = 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>
 
 warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:342:5
+  --> $DIR/non_local_definitions.rs:341:5
    |
 LL | /     impl PartialEq<Dog> for () {
 LL | |
@@ -590,7 +574,7 @@ LL | |     }
    = 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>
 
 warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:364:5
+  --> $DIR/non_local_definitions.rs:363:5
    |
 LL | /     impl From<Wrap<Wrap<Lion>>> for () {
 LL | |
@@ -606,7 +590,7 @@ LL | |     }
    = 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>
 
 warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:371:5
+  --> $DIR/non_local_definitions.rs:370:5
    |
 LL | /     impl From<()> for Wrap<Lion> {
 LL | |
@@ -622,7 +606,66 @@ LL | |     }
    = 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>
 
 warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:384:13
+  --> $DIR/non_local_definitions.rs:380:5
+   |
+LL | /     impl From<Wrap<Wrap<Elephant>>> for () {
+LL | |
+LL | |         fn from(_: Wrap<Wrap<Elephant>>) -> Self {
+LL | |             todo!()
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+   = help: move this `impl` block outside the of the current function `rawr`
+   = 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>
+
+warning: non-local `impl` definition, they should be avoided as they go against expectation
+  --> $DIR/non_local_definitions.rs:394:5
+   |
+LL |     impl StillNonLocal for &Foo {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: move this `impl` block outside the of the current function `only_global`
+   = 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>
+
+warning: non-local `impl` definition, they should be avoided as they go against expectation
+  --> $DIR/non_local_definitions.rs:402:5
+   |
+LL | /     impl From<Local1> for GlobalSameFunction {
+LL | |
+LL | |         fn from(x: Local1) -> GlobalSameFunction {
+LL | |             x.0
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+   = help: move this `impl` block outside the of the current function `same_function`
+   = 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>
+
+warning: non-local `impl` definition, they should be avoided as they go against expectation
+  --> $DIR/non_local_definitions.rs:410:5
+   |
+LL | /     impl From<Local2> for GlobalSameFunction {
+LL | |
+LL | |         fn from(x: Local2) -> GlobalSameFunction {
+LL | |             x.0
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+   = help: move this `impl` block outside the of the current function `same_function`
+   = 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>
+
+warning: non-local `impl` definition, they should be avoided as they go against expectation
+  --> $DIR/non_local_definitions.rs:449:13
    |
 LL |             impl MacroTrait for OutsideStruct {}
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -637,7 +680,7 @@ LL | m!();
    = note: this warning originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 warning: non-local `impl` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:394:1
+  --> $DIR/non_local_definitions.rs:459:1
    |
 LL | non_local_macro::non_local_impl!(CargoUpdate);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -650,7 +693,7 @@ LL | non_local_macro::non_local_impl!(CargoUpdate);
    = note: this warning originates in the macro `non_local_macro::non_local_impl` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 warning: non-local `macro_rules!` definition, they should be avoided as they go against expectation
-  --> $DIR/non_local_definitions.rs:397:1
+  --> $DIR/non_local_definitions.rs:462:1
    |
 LL | non_local_macro::non_local_macro_rules!(my_macro);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -662,5 +705,5 @@ LL | non_local_macro::non_local_macro_rules!(my_macro);
    = note: the macro `non_local_macro::non_local_macro_rules` may come from an old version of the `non_local_macro` crate, try updating your dependency with `cargo update -p non_local_macro`
    = note: this warning originates in the macro `non_local_macro::non_local_macro_rules` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-warning: 52 warnings emitted
+warning: 55 warnings emitted