From f2a9356d3a7d62d7de6f6cee15422e857347c66a Mon Sep 17 00:00:00 2001 From: Urgau Date: Thu, 29 Feb 2024 19:14:59 +0100 Subject: [PATCH 1/2] Add description method to OwnerNode --- compiler/rustc_hir/src/hir.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index 78e7c636a3e74..9634540e2db3c 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/compiler/rustc_hir/src/hir.rs @@ -3347,6 +3347,20 @@ pub enum OwnerNode<'hir> { } impl<'hir> OwnerNode<'hir> { + pub fn descr(&self) -> &'static str { + match self { + OwnerNode::Item(item) => item.kind.descr(), + OwnerNode::ForeignItem(foreign_item) => match foreign_item.kind { + ForeignItemKind::Fn(_, _, _) => "function", + ForeignItemKind::Static(_, _) => "static", + ForeignItemKind::Type => "extern type", + }, + OwnerNode::TraitItem(_) => "trait item", + OwnerNode::ImplItem(_) => "impl item", + OwnerNode::Crate(_) => "crate", + } + } + pub fn ident(&self) -> Option { match self { OwnerNode::Item(Item { ident, .. }) From 5e73be3efdb20897f6098a3249497243ae7f36fe Mon Sep 17 00:00:00 2001 From: Urgau Date: Thu, 29 Feb 2024 19:16:14 +0100 Subject: [PATCH 2/2] Only use HIR in the non_local_def lint --- Cargo.lock | 1 - compiler/rustc_lint/Cargo.toml | 1 - compiler/rustc_lint/src/non_local_def.rs | 118 +++++++++++++-------- tests/ui/lint/non_local_definitions.stderr | 32 +++--- 4 files changed, 91 insertions(+), 61 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4e00c9034bd9b..8518bb2a91f1d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4101,7 +4101,6 @@ dependencies = [ "rustc_target", "rustc_trait_selection", "rustc_type_ir", - "smallvec", "tracing", "unicode-security", ] diff --git a/compiler/rustc_lint/Cargo.toml b/compiler/rustc_lint/Cargo.toml index 2271321b8bf22..fa1133e7780ff 100644 --- a/compiler/rustc_lint/Cargo.toml +++ b/compiler/rustc_lint/Cargo.toml @@ -23,7 +23,6 @@ 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 diff --git a/compiler/rustc_lint/src/non_local_def.rs b/compiler/rustc_lint/src/non_local_def.rs index 6cb6fd1cbd550..2d2d4badfff87 100644 --- a/compiler/rustc_lint/src/non_local_def.rs +++ b/compiler/rustc_lint/src/non_local_def.rs @@ -1,9 +1,9 @@ -use rustc_hir::{def::DefKind, Body, Item, ItemKind, Node, Path, QPath, TyKind}; -use rustc_span::def_id::{DefId, LOCAL_CRATE}; +use rustc_hir::def_id::LocalDefId; +use rustc_hir::{Body, Item, ItemKind, OwnerId, OwnerNode, Path, QPath, TyKind}; +use rustc_span::def_id::LOCAL_CRATE; +use rustc_span::symbol::Ident; use rustc_span::{sym, symbol::kw, ExpnKind, MacroKind}; -use smallvec::{smallvec, SmallVec}; - use crate::lints::{NonLocalDefinitionsCargoUpdateNote, NonLocalDefinitionsDiag}; use crate::{LateContext, LateLintPass, LintContext}; @@ -67,24 +67,19 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { 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); + let mut parent_iter = cx.tcx.hir().parent_owner_iter(item.hir_id()); - // 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; - } + // Unwrap SAFETY: `ParentOwnerIterator` documentation garenties that + // it only panic when reaching the crate root but we made sure above + // that we are not at crate root. So we are fine here. + let (parent_owner_id, parent_owner_node) = parent_iter.next().unwrap(); 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() + && rustc_session::utils::was_invoked_from_cargo() { Some(NonLocalDefinitionsCargoUpdateNote { macro_kind: macro_kind.descr(), @@ -112,26 +107,35 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { // 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 parent_owner_is_anon_const = matches!( + parent_owner_node, + OwnerNode::Item(Item { + ident: Ident { name: kw::Underscore, .. }, + kind: ItemKind::Const(..), + .. + }) + ); + + // Per RFC we (currently) ignore `impl` def in anon-const (`const _: Ty = ...`) + // at the top-level module. + if self.body_depth == 1 && parent_owner_is_anon_const { + return; + } + + let parent_parent_def_id = parent_owner_is_anon_const + .then(|| parent_iter.next().map(|(owner_id, _)| owner_id.def_id)) + .flatten(); 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) + path_has_local_parent(ty_path, cx, parent_owner_id, parent_parent_def_id) } TyKind::TraitObject([principle_poly_trait_ref, ..], _, _) => { path_has_local_parent( principle_poly_trait_ref.trait_ref.path, cx, - &*local_parents, + parent_owner_id, + parent_parent_def_id, ) } TyKind::TraitObject([], _, _) @@ -151,19 +155,25 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { | 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); + let of_trait_has_local_parent = self_ty_has_local_parent + || impl_ + .of_trait + .map(|of_trait| { + path_has_local_parent( + of_trait.path, + cx, + parent_owner_id, + parent_parent_def_id, + ) + }) + .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 OwnerNode::Item(item) = parent_owner_node + && item.ident.name != kw::Underscore && let ItemKind::Const(ty, _, _) = item.kind && let TyKind::Tup(&[]) = ty.kind { @@ -177,9 +187,10 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { 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()) + body_kind_descr: parent_owner_node.descr(), + body_name: parent_owner_node + .ident() + .map(|s| s.name.to_ident_string()) .unwrap_or_else(|| "".to_string()), cargo_update: cargo_update(), const_anon, @@ -195,9 +206,10 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { 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()) + body_kind_descr: parent_owner_node.descr(), + body_name: parent_owner_node + .ident() + .map(|s| s.name.to_ident_string()) .unwrap_or_else(|| "".to_string()), cargo_update: cargo_update(), }, @@ -217,6 +229,26 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { /// std::convert::PartialEq> /// ^^^^^^^^^^^^^^^^^^^^^^^ /// ``` -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))) +fn path_has_local_parent<'tcx>( + path: &Path<'_>, + cx: &LateContext<'tcx>, + local_parent: OwnerId, + extra_local_parent: Option, +) -> bool { + let Some(res_did) = path.res.opt_def_id() else { + return true; + }; + let Some(did) = res_did.as_local() else { + return false; + }; + + let res_parent = { + let Some(hir_id) = cx.tcx.opt_local_def_id_to_hir_id(did) else { + return true; + }; + let owner_id = cx.tcx.hir().get_parent_item(hir_id); + owner_id.def_id + }; + + res_parent == local_parent.def_id || Some(res_parent) == extra_local_parent } diff --git a/tests/ui/lint/non_local_definitions.stderr b/tests/ui/lint/non_local_definitions.stderr index f9f29ec63a805..e0c2d9dd49fb2 100644 --- a/tests/ui/lint/non_local_definitions.stderr +++ b/tests/ui/lint/non_local_definitions.stderr @@ -7,7 +7,7 @@ LL | const Z: () = { LL | impl Uto for &Test {} | ^^^^^^^^^^^^^^^^^^^^^ | - = help: move this `impl` block outside the of the current constant `Z` + = help: move this `impl` block outside the of the current constant item `Z` = 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 @@ -19,7 +19,7 @@ warning: non-local `impl` definition, they should be avoided as they go against LL | impl Uto for *mut Test {} | ^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: move this `impl` block outside the of the current constant expression `` + = help: move this `impl` block outside the of the current type alias `A` = 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 @@ -30,7 +30,7 @@ warning: non-local `impl` definition, they should be avoided as they go against LL | impl Uto for Test {} | ^^^^^^^^^^^^^^^^^^^^ | - = help: move this `impl` block outside the of the current constant expression `` + = help: move this `impl` block outside the of the current enum `Enum` = 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 @@ -41,7 +41,7 @@ warning: non-local `impl` definition, they should be avoided as they go against LL | impl Uto2 for Test {} | ^^^^^^^^^^^^^^^^^^^^^ | - = help: move this `impl` block outside the of the current static `A` + = help: move this `impl` block outside the of the current static item `A` = 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 @@ -52,7 +52,7 @@ warning: non-local `impl` definition, they should be avoided as they go against LL | impl Uto3 for Test {} | ^^^^^^^^^^^^^^^^^^^^^ | - = help: move this `impl` block outside the of the current constant `B` + = help: move this `impl` block outside the of the current constant item `B` = 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 @@ -63,7 +63,7 @@ warning: non-local `macro_rules!` definition, they should be avoided as they go LL | macro_rules! m0 { () => { } }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: remove the `#[macro_export]` or move this `macro_rules!` outside the of the current constant `B` + = help: remove the `#[macro_export]` or move this `macro_rules!` outside the of the current constant item `B` = note: a `macro_rules!` definition is non-local if it is nested inside an item and has a `#[macro_export]` attribute = note: one exception to the rule are anon-const (`const _: () = { ... }`) at top-level module = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue @@ -102,7 +102,7 @@ LL | | fn bar() {} LL | | } | |_________^ | - = help: move this `impl` block outside the of the current constant expression `` and up 2 bodies + = help: move this `impl` block outside the of the current function `main` and up 2 bodies = 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 @@ -116,7 +116,7 @@ LL | | fn hoo() {} LL | | } | |_________^ | - = help: move this `impl` block outside the of the current inline constant `` and up 2 bodies + = help: move this `impl` block outside the of the current function `main` and up 2 bodies = 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 @@ -130,7 +130,7 @@ LL | | fn foo2() {} LL | | } | |_________^ | - = help: move this `impl` block outside the of the current constant `_` and up 2 bodies + = help: move this `impl` block outside the of the current constant item `_` and up 2 bodies = 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 @@ -256,7 +256,7 @@ warning: non-local `impl` definition, they should be avoided as they go against LL | impl Uto5 for Test {} | ^^^^^^^^^^^^^^^^^^^^^ | - = help: move this `impl` block outside the of the current closure `` and up 2 bodies + = help: move this `impl` block outside the of the current function `main` and up 2 bodies = 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 @@ -267,7 +267,7 @@ warning: non-local `impl` definition, they should be avoided as they go against LL | impl Uto5 for &Test {} | ^^^^^^^^^^^^^^^^^^^^^^ | - = help: move this `impl` block outside the of the current constant expression `` and up 2 bodies + = help: move this `impl` block outside the of the current type alias `A` and up 2 bodies = 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 @@ -278,7 +278,7 @@ warning: non-local `impl` definition, they should be avoided as they go against LL | impl Uto5 for &(Test,) {} | ^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: move this `impl` block outside the of the current constant expression `` and up 2 bodies + = help: move this `impl` block outside the of the current function `a` and up 2 bodies = 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 @@ -289,7 +289,7 @@ warning: non-local `impl` definition, they should be avoided as they go against LL | impl Uto5 for &(Test,Test) {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: move this `impl` block outside the of the current constant expression `` and up 2 bodies + = help: move this `impl` block outside the of the current function `b` and up 2 bodies = 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 @@ -399,7 +399,7 @@ warning: non-local `macro_rules!` definition, they should be avoided as they go LL | macro_rules! m2 { () => { } }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: remove the `#[macro_export]` or move this `macro_rules!` outside the of the current associated function `bar` and up 3 bodies + = help: remove the `#[macro_export]` or move this `macro_rules!` outside the of the current impl item `bar` and up 3 bodies = note: a `macro_rules!` definition is non-local if it is nested inside an item and has a `#[macro_export]` attribute = note: one exception to the rule are anon-const (`const _: () = { ... }`) at top-level module = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue @@ -616,7 +616,7 @@ warning: non-local `impl` definition, they should be avoided as they go against LL | non_local_macro::non_local_impl!(CargoUpdate); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: move this `impl` block outside the of the current constant `_IMPL_DEBUG` + = help: move this `impl` block outside the of the current constant item `_IMPL_DEBUG` = 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 @@ -629,7 +629,7 @@ warning: non-local `macro_rules!` definition, they should be avoided as they go LL | non_local_macro::non_local_macro_rules!(my_macro); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: remove the `#[macro_export]` or move this `macro_rules!` outside the of the current constant `_MACRO_EXPORT` + = help: remove the `#[macro_export]` or move this `macro_rules!` outside the of the current constant item `_MACRO_EXPORT` = note: a `macro_rules!` definition is non-local if it is nested inside an item and has a `#[macro_export]` attribute = note: one exception to the rule are anon-const (`const _: () = { ... }`) at top-level module = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue