Rework non_local_definitions lint to only be a syntactic heuristic

Author: Urgau

compiler/rustc_lint/messages.ftl

@@ -592,7 +592,6 @@ lint_non_local_definitions_cargo_update = the {$macro_kind} `{$macro_name}` may
 lint_non_local_definitions_deprecation = 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>
 
 lint_non_local_definitions_impl = non-local `impl` definition, `impl` blocks should be written at the same level as their item
-    .remove_help = remove `{$may_remove_part}` to make the `impl` local
     .without_trait = methods and associated constants are still usable outside the current expression, only `impl Local` and `impl dyn Local` can ever be private, and only if the type is nested in the same item as the `impl`
     .with_trait = an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
     .bounds = `impl` may be usable in bounds, etc. from outside the expression, which might e.g. make something constructible that previously wasn't, because it's still on a publicly-visible type
@@ -617,8 +616,6 @@ lint_non_local_definitions_macro_rules = non-local `macro_rules!` definition, `#
         remove the `#[macro_export]` or make this doc-test a standalone test with its own `fn main() {"{"} ... {"}"}`
     .non_local = a `macro_rules!` definition is non-local if it is nested inside an item and has a `#[macro_export]` attribute
 
-lint_non_local_definitions_may_move = may need to be moved as well
-
 lint_non_local_definitions_of_trait_not_local = `{$of_trait_str}` is not local
 
 lint_non_local_definitions_self_ty_not_local = `{$self_ty_str}` is not local

compiler/rustc_lint/src/lints.rs

@@ -1375,9 +1375,7 @@ pub(crate) enum NonLocalDefinitionsDiag {
         body_name: String,
         cargo_update: Option<NonLocalDefinitionsCargoUpdateNote>,
         const_anon: Option<Option<Span>>,
-        move_to: Option<(Span, Vec<Span>)>,
         doctest: bool,
-        may_remove: Option<(Span, String)>,
         has_trait: bool,
         self_ty_str: String,
         of_trait_str: Option<String>,
@@ -1401,9 +1399,7 @@ impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag {
                 body_name,
                 cargo_update,
                 const_anon,
-                move_to,
                 doctest,
-                may_remove,
                 has_trait,
                 self_ty_str,
                 of_trait_str,
@@ -1434,27 +1430,10 @@ impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag {
                     diag.note(fluent::lint_without_trait);
                 }
 
-                if let Some((move_help, may_move)) = move_to {
-                    let mut ms = MultiSpan::from_span(move_help);
-                    for sp in may_move {
-                        ms.push_span_label(sp, fluent::lint_non_local_definitions_may_move);
-                    }
-                    diag.span_help(ms, fluent::lint_non_local_definitions_impl_move_help);
-                }
                 if doctest {
                     diag.help(fluent::lint_doctest);
                 }
 
-                if let Some((span, part)) = may_remove {
-                    diag.arg("may_remove_part", part);
-                    diag.span_suggestion(
-                        span,
-                        fluent::lint_remove_help,
-                        "",
-                        Applicability::MaybeIncorrect,
-                    );
-                }
-
                 if let Some(const_anon) = const_anon {
                     diag.note(fluent::lint_exception);
                     if let Some(const_anon) = const_anon {

compiler/rustc_lint/src/non_local_def.rs

@@ -2,19 +2,11 @@ use rustc_errors::MultiSpan;
 use rustc_hir::def::DefKind;
 use rustc_hir::intravisit::{self, Visitor};
 use rustc_hir::{Body, HirId, Item, ItemKind, Node, Path, QPath, TyKind};
-use rustc_infer::infer::InferCtxt;
-use rustc_infer::traits::{Obligation, ObligationCause};
-use rustc_middle::ty::{
-    self, Binder, EarlyBinder, TraitRef, Ty, TyCtxt, TypeFoldable, TypeFolder, TypeSuperFoldable,
-};
+use rustc_middle::ty::TyCtxt;
 use rustc_session::{declare_lint, impl_lint_pass};
 use rustc_span::def_id::{DefId, LOCAL_CRATE};
 use rustc_span::symbol::kw;
-use rustc_span::{ExpnKind, MacroKind, Span, Symbol, sym};
-use rustc_trait_selection::error_reporting::traits::ambiguity::{
-    CandidateSource, compute_applicable_impls_for_diagnostics,
-};
-use rustc_trait_selection::infer::TyCtxtInferExt;
+use rustc_span::{ExpnKind, MacroKind, Span, Symbol, sym, sym};
 
 use crate::lints::{NonLocalDefinitionsCargoUpdateNote, NonLocalDefinitionsDiag};
 use crate::{LateContext, LateLintPass, LintContext, fluent_generated as fluent};
@@ -142,42 +134,22 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
                     None
                 };
 
-                // Part 1: Is the Self type local?
-                let self_ty_has_local_parent =
-                    ty_has_local_parent(&impl_.self_ty.kind, cx, parent, parent_parent);
-
-                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 of_trait_has_local_parent {
-                    return;
+                // 1. We collect all the `hir::Path` from the `Self` type and `Trait` ref
+                // of the `impl` definition
+                let mut collector = PathCollector { paths: Vec::new() };
+                collector.visit_ty(&impl_.self_ty);
+                if let Some(of_trait) = &impl_.of_trait {
+                    collector.visit_trait_ref(of_trait);
                 }
 
-                // 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 {
+                // 2. We check if any of path reference a "local" parent and if that the case
+                // we bail out as asked by T-lang, even though this isn't correct from a
+                // type-system point of view, as inference exists and could still leak the impl.
+                if collector
+                    .paths
+                    .iter()
+                    .any(|path| path_has_local_parent(path, cx, parent, parent_parent))
+                {
                     return;
                 }
 
@@ -199,18 +171,6 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
                 let const_anon = matches!(parent_def_kind, DefKind::Const | DefKind::Static { .. })
                     .then_some(span_for_const_anon_suggestion);
 
-                let may_remove = match &impl_.self_ty.kind {
-                    TyKind::Ptr(mut_ty) | TyKind::Ref(_, mut_ty)
-                        if ty_has_local_parent(&mut_ty.ty.kind, cx, parent, parent_parent) =>
-                    {
-                        let type_ =
-                            if matches!(impl_.self_ty.kind, TyKind::Ptr(_)) { "*" } else { "&" };
-                        let part = format!("{}{}", type_, mut_ty.mutbl.prefix_str());
-                        Some((impl_.self_ty.span.shrink_to_lo().until(mut_ty.ty.span), part))
-                    }
-                    _ => None,
-                };
-
                 let impl_span = item.span.shrink_to_lo().to(impl_.self_ty.span);
                 let mut ms = MultiSpan::from_span(impl_span);
 
@@ -221,6 +181,7 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
                     self_ty_span,
                     fluent::lint_non_local_definitions_self_ty_not_local,
                 );
+
                 let of_trait_str = if let Some(of_trait) = &impl_.of_trait {
                     ms.push_span_label(
                         path_span_without_args(&of_trait.path),
@@ -231,44 +192,14 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
                     None
                 };
 
-                let (doctest, move_to) = if is_at_toplevel_doctest() {
-                    (true, None)
-                } else {
-                    let mut collector = PathCollector { paths: Vec::new() };
-                    collector.visit_ty(&impl_.self_ty);
-                    if let Some(of_trait) = &impl_.of_trait {
-                        collector.visit_trait_ref(of_trait);
-                    }
-                    collector.visit_generics(&impl_.generics);
-
-                    let mut may_move: Vec<Span> = collector
-                        .paths
-                        .into_iter()
-                        .filter_map(|path| {
-                            if let Some(did) = path.res.opt_def_id()
-                                && did_has_local_parent(did, cx.tcx, parent, parent_parent)
-                            {
-                                Some(cx.tcx.def_span(did))
-                            } else {
-                                None
-                            }
-                        })
-                        .collect();
-                    may_move.sort();
-                    may_move.dedup();
-
-                    let move_to = if may_move.is_empty() {
-                        ms.push_span_label(
-                            cx.tcx.def_span(parent),
-                            fluent::lint_non_local_definitions_impl_move_help,
-                        );
-                        None
-                    } else {
-                        Some((cx.tcx.def_span(parent), may_move))
-                    };
+                let doctest = is_at_toplevel_doctest();
 
-                    (false, move_to)
-                };
+                if !doctest {
+                    ms.push_span_label(
+                        cx.tcx.def_span(parent),
+                        fluent::lint_non_local_definitions_impl_move_help,
+                    );
+                }
 
                 let macro_to_change =
                     if let ExpnKind::Macro(kind, name) = item.span.ctxt().outer_expn_data().kind {
@@ -287,9 +218,7 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
                     const_anon,
                     self_ty_str,
                     of_trait_str,
-                    move_to,
                     doctest,
-                    may_remove,
                     has_trait: impl_.of_trait.is_some(),
                     macro_to_change,
                 })
@@ -316,90 +245,6 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
     }
 }
 
-// Detecting if the impl definition is leaking outside of its 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<'tcx, TraitRef<'tcx>>,
-    mut did_has_local_parent: impl FnMut(DefId) -> bool,
-) -> bool {
-    let infcx = tcx
-        .infer_ctxt()
-        // We use the new trait solver since the obligation we are trying to
-        // prove here may overflow and those are fatal in the old trait solver.
-        // Which is unacceptable for a lint.
-        //
-        // Thanksfully the part we use here are very similar to the
-        // new-trait-solver-as-coherence, which is in stabilization.
-        //
-        // https://github.com/rust-lang/rust/issues/123573
-        .with_next_trait_solver(true)
-        .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 cx(&self) -> TyCtxt<'tcx> {
-        self.infcx.tcx
-    }
-
-    fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> {
-        if let Some(def) = t.ty_adt_def()
-            && (self.did_has_local_parent)(def.did())
-        {
-            self.infcx.next_ty_var(self.infer_span)
-        } else {
-            t.super_fold_with(self)
-        }
-    }
-}
-
 /// Simple hir::Path collector
 struct PathCollector<'tcx> {
     paths: Vec<Path<'tcx>>,
@@ -412,42 +257,6 @@ impl<'tcx> Visitor<'tcx> for PathCollector<'tcx> {
     }
 }
 
-/// Given a `Ty` we check if the (outermost) type is local.
-fn ty_has_local_parent(
-    ty_kind: &TyKind<'_>,
-    cx: &LateContext<'_>,
-    impl_parent: DefId,
-    impl_parent_parent: Option<DefId>,
-) -> bool {
-    match ty_kind {
-        TyKind::Path(QPath::Resolved(_, ty_path)) => {
-            path_has_local_parent(ty_path, cx, impl_parent, impl_parent_parent)
-        }
-        TyKind::TraitObject([principle_poly_trait_ref, ..], _, _) => path_has_local_parent(
-            principle_poly_trait_ref.0.trait_ref.path,
-            cx,
-            impl_parent,
-            impl_parent_parent,
-        ),
-        TyKind::TraitObject([], _, _)
-        | TyKind::InferDelegation(_, _)
-        | TyKind::Slice(_)
-        | TyKind::Array(_, _)
-        | TyKind::Ptr(_)
-        | TyKind::Ref(_, _)
-        | TyKind::BareFn(_)
-        | TyKind::Never
-        | TyKind::Tup(_)
-        | TyKind::Path(_)
-        | TyKind::Pat(..)
-        | TyKind::AnonAdt(_)
-        | TyKind::OpaqueDef(_, _, _)
-        | TyKind::Typeof(_)
-        | TyKind::Infer
-        | TyKind::Err(_) => false,
-    }
-}
-
 /// 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.
 ///

tests/ui/lint/non-local-defs/exhaustive.rs

@@ -55,18 +55,13 @@ fn main() {
 
     struct InsideMain;
 
+    impl Trait for &InsideMain {}
     impl Trait for *mut InsideMain {}
-    //~^ WARN non-local `impl` definition
     impl Trait for *mut [InsideMain] {}
-    //~^ WARN non-local `impl` definition
     impl Trait for [InsideMain; 8] {}
-    //~^ WARN non-local `impl` definition
     impl Trait for (InsideMain,) {}
-    //~^ WARN non-local `impl` definition
     impl Trait for fn(InsideMain) -> () {}
-    //~^ WARN non-local `impl` definition
     impl Trait for fn() -> InsideMain {}
-    //~^ WARN non-local `impl` definition
 
     fn inside_inside() {
         impl Display for InsideMain {