Auto merge of #120393 - Urgau:rfc3373-non-local-defs, r=WaffleLapkin

Implement RFC 3373: Avoid non-local definitions in functions

This PR implements [RFC 3373: Avoid non-local definitions in functions](#120363).

Author: bors

Cargo.lock

@@ -4101,6 +4101,7 @@ dependencies = [
  "rustc_target",
  "rustc_trait_selection",
  "rustc_type_ir",
+ "smallvec",
  "tracing",
  "unicode-security",
 ]

compiler/rustc_hir_analysis/src/coherence/inherent_impls_overlap.rs

@@ -24,6 +24,11 @@ struct InherentOverlapChecker<'tcx> {
     tcx: TyCtxt<'tcx>,
 }
 
+rustc_index::newtype_index! {
+    #[orderable]
+    pub struct RegionId {}
+}
+
 impl<'tcx> InherentOverlapChecker<'tcx> {
     /// Checks whether any associated items in impls 1 and 2 share the same identifier and
     /// namespace.
@@ -205,11 +210,6 @@ impl<'tcx> InherentOverlapChecker<'tcx> {
             // This is advantageous to running the algorithm over the
             // entire graph when there are many connected regions.
 
-            rustc_index::newtype_index! {
-                #[orderable]
-                pub struct RegionId {}
-            }
-
             struct ConnectedRegion {
                 idents: SmallVec<[Symbol; 8]>,
                 impl_blocks: FxHashSet<usize>,

compiler/rustc_lint/Cargo.toml

@@ -23,6 +23,7 @@ rustc_span = { path = "../rustc_span" }
 rustc_target = { path = "../rustc_target" }
 rustc_trait_selection = { path = "../rustc_trait_selection" }
 rustc_type_ir = { path = "../rustc_type_ir" }
+smallvec = { version = "1.8.1", features = ["union", "may_dangle"] }
 tracing = "0.1"
 unicode-security = "0.1.0"
 # tidy-alphabetical-end

compiler/rustc_lint/messages.ftl

@@ -414,6 +414,29 @@ lint_non_fmt_panic_unused =
     }
     .add_fmt_suggestion = or add a "{"{"}{"}"}" format string to use the message literally
 
+lint_non_local_definitions_cargo_update = the {$macro_kind} `{$macro_name}` may come from an old version of the `{$crate_name}` crate, try updating your dependency with `cargo update -p {$crate_name}`
+
+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, they should be avoided as they go against expectation
+    .help =
+        move this `impl` block outside the of the current {$body_kind_descr} {$depth ->
+            [one] `{$body_name}`
+           *[other] `{$body_name}` and up {$depth} bodies
+        }
+    .non_local = 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
+    .exception = 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
+    .const_anon = use a const-anon item to suppress this lint
+
+lint_non_local_definitions_macro_rules = non-local `macro_rules!` definition, they should be avoided as they go against expectation
+    .help =
+        remove the `#[macro_export]` or move this `macro_rules!` outside the of the current {$body_kind_descr} {$depth ->
+            [one] `{$body_name}`
+           *[other] `{$body_name}` and up {$depth} bodies
+        }
+    .non_local = a `macro_rules!` definition is non-local if it is nested inside an item and has a `#[macro_export]` attribute
+    .exception = one exception to the rule are anon-const (`const _: () = {"{"} ... {"}"}`) at top-level module
+
 lint_non_snake_case = {$sort} `{$name}` should have a snake case name
     .rename_or_convert_suggestion = rename the identifier or convert it to a snake case raw identifier
     .cannot_convert_note = `{$sc}` cannot be used as a raw identifier

compiler/rustc_lint/src/lib.rs

@@ -70,6 +70,7 @@ mod methods;
 mod multiple_supertrait_upcastable;
 mod non_ascii_idents;
 mod non_fmt_panic;
+mod non_local_def;
 mod nonstandard_style;
 mod noop_method_call;
 mod opaque_hidden_inferred_bound;
@@ -105,6 +106,7 @@ use methods::*;
 use multiple_supertrait_upcastable::*;
 use non_ascii_idents::*;
 use non_fmt_panic::NonPanicFmt;
+use non_local_def::*;
 use nonstandard_style::*;
 use noop_method_call::*;
 use opaque_hidden_inferred_bound::*;
@@ -229,6 +231,7 @@ late_lint_methods!(
             MissingDebugImplementations: MissingDebugImplementations,
             MissingDoc: MissingDoc,
             AsyncFnInTrait: AsyncFnInTrait,
+            NonLocalDefinitions: NonLocalDefinitions::default(),
         ]
     ]
 );

compiler/rustc_lint/src/lints.rs

@@ -1334,6 +1334,45 @@ pub struct SuspiciousDoubleRefCloneDiag<'a> {
     pub ty: Ty<'a>,
 }
 
+// non_local_defs.rs
+#[derive(LintDiagnostic)]
+pub enum NonLocalDefinitionsDiag {
+    #[diag(lint_non_local_definitions_impl)]
+    #[help]
+    #[note(lint_non_local)]
+    #[note(lint_exception)]
+    #[note(lint_non_local_definitions_deprecation)]
+    Impl {
+        depth: u32,
+        body_kind_descr: &'static str,
+        body_name: String,
+        #[subdiagnostic]
+        cargo_update: Option<NonLocalDefinitionsCargoUpdateNote>,
+        #[suggestion(lint_const_anon, code = "_", applicability = "machine-applicable")]
+        const_anon: Option<Span>,
+    },
+    #[diag(lint_non_local_definitions_macro_rules)]
+    #[help]
+    #[note(lint_non_local)]
+    #[note(lint_exception)]
+    #[note(lint_non_local_definitions_deprecation)]
+    MacroRules {
+        depth: u32,
+        body_kind_descr: &'static str,
+        body_name: String,
+        #[subdiagnostic]
+        cargo_update: Option<NonLocalDefinitionsCargoUpdateNote>,
+    },
+}
+
+#[derive(Subdiagnostic)]
+#[note(lint_non_local_definitions_cargo_update)]
+pub struct NonLocalDefinitionsCargoUpdateNote {
+    pub macro_kind: &'static str,
+    pub macro_name: Symbol,
+    pub crate_name: Symbol,
+}
+
 // pass_by_value.rs
 #[derive(LintDiagnostic)]
 #[diag(lint_pass_by_value)]

compiler/rustc_lint/src/non_local_def.rs

@@ -0,0 +1,222 @@
+use rustc_hir::{def::DefKind, Body, Item, ItemKind, Node, Path, QPath, TyKind};
+use rustc_span::def_id::{DefId, LOCAL_CRATE};
+use rustc_span::{sym, symbol::kw, ExpnKind, MacroKind};
+
+use smallvec::{smallvec, SmallVec};
+
+use crate::lints::{NonLocalDefinitionsCargoUpdateNote, NonLocalDefinitionsDiag};
+use crate::{LateContext, LateLintPass, LintContext};
+
+declare_lint! {
+    /// The `non_local_definitions` lint checks for `impl` blocks and `#[macro_export]`
+    /// macro inside bodies (functions, enum discriminant, ...).
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// trait MyTrait {}
+    /// struct MyStruct;
+    ///
+    /// fn foo() {
+    ///     impl MyTrait for MyStruct {}
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Creating non-local definitions go against expectation and can create discrepancies
+    /// in tooling. It should be avoided. It may become deny-by-default in edition 2024
+    /// and higher, see see the tracking issue <https://github.com/rust-lang/rust/issues/120363>.
+    ///
+    /// 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.
+    ///
+    /// 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,
+    Warn,
+    "checks for non-local definitions",
+    report_in_external_macro
+}
+
+#[derive(Default)]
+pub struct NonLocalDefinitions {
+    body_depth: u32,
+}
+
+impl_lint_pass!(NonLocalDefinitions => [NON_LOCAL_DEFINITIONS]);
+
+// FIXME(Urgau): Figure out how to handle modules nested in bodies.
+// It's currently not handled by the current logic because modules are not bodies.
+// They don't even follow the correct order (check_body -> check_mod -> check_body_post)
+// instead check_mod is called after every body has been handled.
+
+impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
+    fn check_body(&mut self, _cx: &LateContext<'tcx>, _body: &'tcx Body<'tcx>) {
+        self.body_depth += 1;
+    }
+
+    fn check_body_post(&mut self, _cx: &LateContext<'tcx>, _body: &'tcx Body<'tcx>) {
+        self.body_depth -= 1;
+    }
+
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
+        if self.body_depth == 0 {
+            return;
+        }
+
+        let parent = cx.tcx.parent(item.owner_id.def_id.into());
+        let parent_def_kind = cx.tcx.def_kind(parent);
+        let parent_opt_item_name = cx.tcx.opt_item_name(parent);
+
+        // Per RFC we (currently) ignore anon-const (`const _: Ty = ...`) in top-level module.
+        if self.body_depth == 1
+            && parent_def_kind == DefKind::Const
+            && parent_opt_item_name == Some(kw::Underscore)
+        {
+            return;
+        }
+
+        let cargo_update = || {
+            let oexpn = item.span.ctxt().outer_expn_data();
+            if let Some(def_id) = oexpn.macro_def_id
+                && let ExpnKind::Macro(macro_kind, macro_name) = oexpn.kind
+                && def_id.krate != LOCAL_CRATE
+                && std::env::var_os("CARGO").is_some()
+            {
+                Some(NonLocalDefinitionsCargoUpdateNote {
+                    macro_kind: macro_kind.descr(),
+                    macro_name,
+                    crate_name: cx.tcx.crate_name(def_id.krate),
+                })
+            } else {
+                None
+            }
+        };
+
+        match item.kind {
+            ItemKind::Impl(impl_) => {
+                // The RFC states:
+                //
+                // > An item nested inside an expression-containing item (through any
+                // > level of nesting) may not define an impl Trait for Type unless
+                // > either the **Trait** or the **Type** is also nested inside the
+                // > same expression-containing item.
+                //
+                // To achieve this we get try to get the paths of the _Trait_ and
+                // _Type_, and we look inside thoses paths to try a find in one
+                // of them a type whose parent is the same as the impl definition.
+                //
+                // If that's the case this means that this impl block declaration
+                // is using local items and so we don't lint on it.
+
+                // We also ignore anon-const in item by including the anon-const
+                // parent as well; and since it's quite uncommon, we use smallvec
+                // to avoid unnecessary heap allocations.
+                let local_parents: SmallVec<[DefId; 1]> = if parent_def_kind == DefKind::Const
+                    && parent_opt_item_name == Some(kw::Underscore)
+                {
+                    smallvec![parent, cx.tcx.parent(parent)]
+                } else {
+                    smallvec![parent]
+                };
+
+                let self_ty_has_local_parent = match impl_.self_ty.kind {
+                    TyKind::Path(QPath::Resolved(_, ty_path)) => {
+                        path_has_local_parent(ty_path, cx, &*local_parents)
+                    }
+                    TyKind::TraitObject([principle_poly_trait_ref, ..], _, _) => {
+                        path_has_local_parent(
+                            principle_poly_trait_ref.trait_ref.path,
+                            cx,
+                            &*local_parents,
+                        )
+                    }
+                    TyKind::TraitObject([], _, _)
+                    | TyKind::InferDelegation(_, _)
+                    | TyKind::Slice(_)
+                    | TyKind::Array(_, _)
+                    | TyKind::Ptr(_)
+                    | TyKind::Ref(_, _)
+                    | TyKind::BareFn(_)
+                    | TyKind::Never
+                    | TyKind::Tup(_)
+                    | TyKind::Path(_)
+                    | TyKind::AnonAdt(_)
+                    | TyKind::OpaqueDef(_, _, _)
+                    | TyKind::Typeof(_)
+                    | TyKind::Infer
+                    | TyKind::Err(_) => false,
+                };
+
+                let of_trait_has_local_parent = impl_
+                    .of_trait
+                    .map(|of_trait| path_has_local_parent(of_trait.path, cx, &*local_parents))
+                    .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) {
+                    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,
+                        },
+                    )
+                }
+            }
+            ItemKind::Macro(_macro, MacroKind::Bang)
+                if cx.tcx.has_attr(item.owner_id.def_id, sym::macro_export) =>
+            {
+                cx.emit_span_lint(
+                    NON_LOCAL_DEFINITIONS,
+                    item.span,
+                    NonLocalDefinitionsDiag::MacroRules {
+                        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(),
+                    },
+                )
+            }
+            _ => {}
+        }
+    }
+}
+
+/// 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.
+///
+/// Given this path, we will look at the path (and ignore any generic args):
+///
+/// ```text
+///    std::convert::PartialEq<Foo<Bar>>
+///    ^^^^^^^^^^^^^^^^^^^^^^^
+/// ```
+fn path_has_local_parent(path: &Path<'_>, cx: &LateContext<'_>, local_parents: &[DefId]) -> bool {
+    path.res.opt_def_id().is_some_and(|did| local_parents.contains(&cx.tcx.parent(did)))
+}

compiler/rustc_macros/src/diagnostics/mod.rs

@@ -55,7 +55,8 @@ use synstructure::Structure;
 ///
 /// See rustc dev guide for more examples on using the `#[derive(Diagnostic)]`:
 /// <https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-structs.html>
-pub fn session_diagnostic_derive(s: Structure<'_>) -> TokenStream {
+pub fn session_diagnostic_derive(mut s: Structure<'_>) -> TokenStream {
+    s.underscore_const(true);
     DiagnosticDerive::new(s).into_tokens()
 }
 
@@ -101,7 +102,8 @@ pub fn session_diagnostic_derive(s: Structure<'_>) -> TokenStream {
 ///
 /// See rustc dev guide for more examples on using the `#[derive(LintDiagnostic)]`:
 /// <https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-structs.html#reference>
-pub fn lint_diagnostic_derive(s: Structure<'_>) -> TokenStream {
+pub fn lint_diagnostic_derive(mut s: Structure<'_>) -> TokenStream {
+    s.underscore_const(true);
     LintDiagnosticDerive::new(s).into_tokens()
 }
 
@@ -151,6 +153,7 @@ pub fn lint_diagnostic_derive(s: Structure<'_>) -> TokenStream {
 ///
 /// diag.subdiagnostic(RawIdentifierSuggestion { span, applicability, ident });
 /// ```
-pub fn session_subdiagnostic_derive(s: Structure<'_>) -> TokenStream {
+pub fn session_subdiagnostic_derive(mut s: Structure<'_>) -> TokenStream {
+    s.underscore_const(true);
     SubdiagnosticDeriveBuilder::new().into_tokens(s)
 }