Auto merge of rust-lang#122747 - Urgau:non-local-defs_perfect_impl, r…

…=<try>

Implement T-types suggested logic for perfect non-local impl detection

This implement [T-types suggested logic](rust-lang#121621 (comment)) for perfect non-local impl detection:

> 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

This extension to the current logic is meant to address issues reported in rust-lang#121621.

This PR also re-enables the lint `non_local_definitions` to warn-by-default.

Implementation was discussed in this [zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/144729-t-types/topic/Implementing.20new.20non-local.20impl.20defs.20logic).

r? `@lcnr` *(feel free to re-roll)*

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::TypeSuperFoldable;
+use rustc_middle::ty::{self, Binder, Ty, TyCtxt, TypeFoldable, TypeFolder};
 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, Ambiguity,
+};
 
 use crate::lints::{NonLocalDefinitionsCargoUpdateNote, NonLocalDefinitionsDiag};
 use crate::{LateContext, LateLintPass, LintContext};
@@ -35,7 +47,7 @@ declare_lint! {
     /// All nested bodies (functions, enum discriminant, array length, consts) (expect for
     /// `const _: Ty = { ... }` in top-level module, which is still undecided) are checked.
     pub NON_LOCAL_DEFINITIONS,
-    Allow,
+    Warn,
     "checks for non-local definitions",
     report_in_external_macro
 }
@@ -66,7 +78,9 @@ 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);
 
@@ -155,9 +169,54 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
                     .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
-                // this impl definition is a non-local definition and so we lint on it.
-                if !(self_ty_has_local_parent || of_trait_has_local_parent) {
+                // 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
+                let impl_is_not_leaky = cx
+                    .tcx
+                    .impl_trait_ref(def_id)
+                    .map(|binder| {
+                        let infcx = cx.tcx.infer_ctxt().build();
+                        let trait_ref = binder
+                            .instantiate(cx.tcx, infcx.fresh_args_for_item(item.span, def_id));
+
+                        let trait_ref = trait_ref.fold_with(&mut LocalTypeInferenceFolder {
+                            infcx: &infcx,
+                            to_ignore: 1,
+                            impl_parent: parent,
+                            impl_parent_parent: parent_parent,
+                            span: item.span,
+                        });
+
+                        let poly_trait_obligation = Obligation::new(
+                            cx.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 {
+                            Ambiguity::DefId(did) => {
+                                !did_has_local_parent(*did, cx.tcx, parent, parent_parent)
+                            }
+                            Ambiguity::ParamEnv(_) => false,
+                        });
+
+                        let _ = it.next();
+                        it.next().is_some()
+                    })
+                    .unwrap_or(false);
+
+                if !(self_ty_has_local_parent || of_trait_has_local_parent || impl_is_not_leaky) {
                     let const_anon = if self.body_depth == 1
                         && parent_def_kind == DefKind::Const
                         && parent_opt_item_name != Some(kw::Underscore)
@@ -207,6 +266,47 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
     }
 }
 
+/// Replace every local type by inference variable.
+///
+/// ```text
+/// <Global<Local> as std::cmp::PartialEq<Global<Local>>>
+/// to
+/// <Global<_> as std::cmp::PartialEq<Global<_>>>
+/// ```
+struct LocalTypeInferenceFolder<'a, 'tcx> {
+    infcx: &'a InferCtxt<'tcx>,
+    to_ignore: u32,
+    impl_parent: DefId,
+    impl_parent_parent: Option<DefId>,
+    span: Span,
+}
+
+impl<'a, 'tcx> TypeFolder<TyCtxt<'tcx>> for LocalTypeInferenceFolder<'a, 'tcx> {
+    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()
+            && did_has_local_parent(
+                ty_did,
+                self.infcx.tcx,
+                self.impl_parent,
+                self.impl_parent_parent,
+            )
+            && self.to_ignore == 0
+        {
+            self.infcx.next_ty_var(TypeVariableOrigin {
+                kind: TypeVariableOriginKind::TypeInference,
+                span: self.span,
+            })
+        } else {
+            self.to_ignore = self.to_ignore.saturating_sub(1);
+            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 +316,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| {
-        did.is_local() && {
-            let res_parent = cx.tcx.parent(did);
-            res_parent == impl_parent || Some(res_parent) == impl_parent_parent
-        }
-    })
+    path.res
+        .opt_def_id()
+        .is_some_and(|did| did_has_local_parent(did, cx.tcx, impl_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
+    }
 }

compiler/rustc_trait_selection/src/traits/error_reporting/ambiguity.rs

@@ -7,18 +7,21 @@ use rustc_span::{Span, DUMMY_SP};
 
 use crate::traits::ObligationCtxt;
 
+#[derive(Debug)]
 pub enum Ambiguity {
     DefId(DefId),
     ParamEnv(Span),
 }
 
-pub fn recompute_applicable_impls<'tcx>(
+pub fn compute_applicable_impls_for_diagnostics<'tcx>(
     infcx: &InferCtxt<'tcx>,
     obligation: &PolyTraitObligation<'tcx>,
 ) -> Vec<Ambiguity> {
     let tcx = infcx.tcx;
     let param_env = obligation.param_env;
 
+    let predicate_polarity = obligation.predicate.skip_binder().polarity;
+
     let impl_may_apply = |impl_def_id| {
         let ocx = ObligationCtxt::new(infcx);
         infcx.enter_forall(obligation.predicate, |placeholder_obligation| {
@@ -40,6 +43,21 @@ pub fn recompute_applicable_impls<'tcx>(
                 return false;
             }
 
+            let impl_trait_header = tcx.impl_trait_header(impl_def_id).unwrap();
+
+            let impl_polarity = impl_trait_header.polarity;
+            match impl_polarity {
+                ty::ImplPolarity::Positive | ty::ImplPolarity::Negative => {
+                    match impl_polarity == predicate_polarity {
+                        true => {}
+                        false => return false,
+                    }
+                }
+                ty::ImplPolarity::Reservation => {
+                    return false;
+                }
+            }
+
             let impl_predicates = tcx.predicates_of(impl_def_id).instantiate(tcx, impl_args);
             ocx.register_obligations(impl_predicates.predicates.iter().map(|&predicate| {
                 Obligation::new(tcx, ObligationCause::dummy(), param_env, predicate)

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

@@ -1,6 +1,6 @@
 // ignore-tidy-filelength :(
 
-mod ambiguity;
+pub mod ambiguity;
 mod infer_ctxt_ext;
 pub mod on_unimplemented;
 pub mod suggestions;

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

@@ -2382,7 +2382,7 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
                     )
                 };
 
-                let mut ambiguities = ambiguity::recompute_applicable_impls(
+                let mut ambiguities = ambiguity::compute_applicable_impls_for_diagnostics(
                     self.infcx,
                     &obligation.with(self.tcx, trait_ref),
                 );

tests/ui/lint/non_local_definitions.rs

@@ -4,7 +4,7 @@
 //@ rustc-env:CARGO_CRATE_NAME=non_local_def
 
 #![feature(inline_const)]
-#![warn(non_local_definitions)]
+
 
 extern crate non_local_macro;
 
@@ -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 {}
+}