From 7e05da8d42a730c15983bbd56dfbf744b32032d4 Mon Sep 17 00:00:00 2001 From: Urgau Date: Thu, 10 Oct 2024 14:22:20 +0200 Subject: [PATCH] Consider outermost const-anon in non_local_def lint --- compiler/rustc_lint/src/non_local_def.rs | 90 ++++++++++++------- tests/ui/lint/non-local-defs/consts.rs | 7 ++ tests/ui/lint/non-local-defs/consts.stderr | 20 ++++- .../convoluted-locals-131474-with-mods.rs | 34 +++++++ .../convoluted-locals-131474.rs | 24 +++++ tests/ui/lint/non-local-defs/local.rs | 7 ++ 6 files changed, 148 insertions(+), 34 deletions(-) create mode 100644 tests/ui/lint/non-local-defs/convoluted-locals-131474-with-mods.rs create mode 100644 tests/ui/lint/non-local-defs/convoluted-locals-131474.rs diff --git a/compiler/rustc_lint/src/non_local_def.rs b/compiler/rustc_lint/src/non_local_def.rs index 56f930ea7f62e..6fecddb331947 100644 --- a/compiler/rustc_lint/src/non_local_def.rs +++ b/compiler/rustc_lint/src/non_local_def.rs @@ -124,16 +124,6 @@ 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. - let parent_parent = if parent_def_kind == DefKind::Const - && parent_opt_item_name == Some(kw::Underscore) - { - Some(cx.tcx.parent(parent)) - } else { - None - }; - // 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() }; @@ -148,13 +138,33 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { |p| matches!(p.res, Res::Def(def_kind, _) if def_kind != DefKind::TyParam), ); - // 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. + // 1.9. We retrieve the parent def id of the impl item, ... + // + // ... modulo const-anons items, for enhanced compatibility with the ecosystem + // as that pattern is common with `serde`, `bevy`, ... + // + // For this example we want the `DefId` parent of the outermost const-anon items. + // ``` + // const _: () = { // the parent of this const-anon + // const _: () = { + // impl Foo {} + // }; + // }; + // ``` + let outermost_impl_parent = peel_parent_while(cx.tcx, parent, |tcx, did| { + tcx.def_kind(did) == DefKind::Const + && tcx.opt_item_name(did) == Some(kw::Underscore) + }); + + // 2. We check if any of the paths reference a the `impl`-parent. + // + // If that the case we bail out, as was asked by T-lang, even though this isn't + // correct from a type-system point of view, as inference exists and one-impl-rule + // make its so that we could still leak the impl. if collector .paths .iter() - .any(|path| path_has_local_parent(path, cx, parent, parent_parent)) + .any(|path| path_has_local_parent(path, cx, parent, outermost_impl_parent)) { return; } @@ -253,8 +263,8 @@ impl<'tcx> Visitor<'tcx> for PathCollector<'tcx> { } } -/// 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 a path, this checks if the if the parent resolution def id corresponds to +/// the def id of the parent impl definition (the direct one and the outermost one). /// /// Given this path, we will look at the path (and ignore any generic args): /// @@ -267,32 +277,50 @@ fn path_has_local_parent( path: &Path<'_>, cx: &LateContext<'_>, impl_parent: DefId, - impl_parent_parent: Option, + outermost_impl_parent: Option, ) -> bool { path.res .opt_def_id() - .is_some_and(|did| did_has_local_parent(did, cx.tcx, impl_parent, impl_parent_parent)) + .is_some_and(|did| did_has_local_parent(did, cx.tcx, impl_parent, outermost_impl_parent)) } -/// Given a def id and a parent impl def id, this checks if the parent -/// def id (modulo modules) correspond to the def id of the parent impl definition. +/// Given a def id this checks if the parent def id (modulo modules) correspond to +/// the def id of the parent impl definition (the direct one and the outermost one). #[inline] fn did_has_local_parent( did: DefId, tcx: TyCtxt<'_>, impl_parent: DefId, - impl_parent_parent: Option, + outermost_impl_parent: Option, ) -> bool { - did.is_local() - && if let Some(did_parent) = tcx.opt_parent(did) { - did_parent == impl_parent - || Some(did_parent) == impl_parent_parent - || !did_parent.is_crate_root() - && tcx.def_kind(did_parent) == DefKind::Mod - && did_has_local_parent(did_parent, tcx, impl_parent, impl_parent_parent) - } else { - false - } + if !did.is_local() { + return false; + } + + let Some(parent_did) = tcx.opt_parent(did) else { + return false; + }; + + peel_parent_while(tcx, parent_did, |tcx, did| tcx.def_kind(did) == DefKind::Mod) + .map(|parent_did| parent_did == impl_parent || Some(parent_did) == outermost_impl_parent) + .unwrap_or(false) +} + +/// Given a `DefId` checks if it satisfies `f` if it does check with it's parent and continue +/// until it doesn't satisfies `f` and return the last `DefId` checked. +/// +/// In other word this method return the first `DefId` that doesn't satisfies `f`. +#[inline] +fn peel_parent_while( + tcx: TyCtxt<'_>, + mut did: DefId, + mut f: impl FnMut(TyCtxt<'_>, DefId) -> bool, +) -> Option { + while !did.is_crate_root() && f(tcx, did) { + did = tcx.opt_parent(did).filter(|parent_did| parent_did.is_local())?; + } + + Some(did) } /// Return for a given `Path` the span until the last args diff --git a/tests/ui/lint/non-local-defs/consts.rs b/tests/ui/lint/non-local-defs/consts.rs index d8a497e43e502..5b9dab3cfd369 100644 --- a/tests/ui/lint/non-local-defs/consts.rs +++ b/tests/ui/lint/non-local-defs/consts.rs @@ -63,6 +63,13 @@ fn main() { 1 }; + + const _: () = { + const _: () = { + impl Test {} + //~^ WARN non-local `impl` definition + }; + }; } trait Uto9 {} diff --git a/tests/ui/lint/non-local-defs/consts.stderr b/tests/ui/lint/non-local-defs/consts.stderr index 7f76056c0210c..4bf50bae5eac1 100644 --- a/tests/ui/lint/non-local-defs/consts.stderr +++ b/tests/ui/lint/non-local-defs/consts.stderr @@ -95,7 +95,21 @@ LL | impl Test { = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item - --> $DIR/consts.rs:72:9 + --> $DIR/consts.rs:69:13 + | +LL | const _: () = { + | ----------- move the `impl` block outside of this constant `_` and up 3 bodies +LL | impl Test {} + | ^^^^^---- + | | + | `Test` is not local + | + = note: 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` + = note: items in an anonymous const item (`const _: () = { ... }`) are treated as in the same scope as the anonymous const's declaration for the purpose of this lint + = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue + +warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item + --> $DIR/consts.rs:79:9 | LL | let _a = || { | -- move the `impl` block outside of this closure `` and up 2 bodies @@ -109,7 +123,7 @@ LL | impl Uto9 for Test {} = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item - --> $DIR/consts.rs:79:9 + --> $DIR/consts.rs:86:9 | LL | type A = [u32; { | ____________________- @@ -126,5 +140,5 @@ LL | | }]; = note: 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` = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue -warning: 8 warnings emitted +warning: 9 warnings emitted diff --git a/tests/ui/lint/non-local-defs/convoluted-locals-131474-with-mods.rs b/tests/ui/lint/non-local-defs/convoluted-locals-131474-with-mods.rs new file mode 100644 index 0000000000000..cef0f0205e809 --- /dev/null +++ b/tests/ui/lint/non-local-defs/convoluted-locals-131474-with-mods.rs @@ -0,0 +1,34 @@ +// This test check that no matter the nesting of const-anons and modules +// we consider them as transparent. +// +// Similar to https://github.com/rust-lang/rust/issues/131474 + +//@ check-pass + +pub mod tmp { + pub mod tmp { + pub struct Test; + } +} + +const _: () = { + const _: () = { + const _: () = { + const _: () = { + impl tmp::tmp::Test {} + }; + }; + }; +}; + +const _: () = { + const _: () = { + mod tmp { + pub(super) struct InnerTest; + } + + impl tmp::InnerTest {} + }; +}; + +fn main() {} diff --git a/tests/ui/lint/non-local-defs/convoluted-locals-131474.rs b/tests/ui/lint/non-local-defs/convoluted-locals-131474.rs new file mode 100644 index 0000000000000..4881723f13bce --- /dev/null +++ b/tests/ui/lint/non-local-defs/convoluted-locals-131474.rs @@ -0,0 +1,24 @@ +// This test check that no matter the nesting of const-anons we consider +// them as transparent. +// +// https://github.com/rust-lang/rust/issues/131474 + +//@ check-pass + +pub struct Test; + +const _: () = { + const _: () = { + impl Test {} + }; +}; + +const _: () = { + const _: () = { + struct InnerTest; + + impl InnerTest {} + }; +}; + +fn main() {} diff --git a/tests/ui/lint/non-local-defs/local.rs b/tests/ui/lint/non-local-defs/local.rs index 166ee88c0210c..f7ecd68f59e90 100644 --- a/tests/ui/lint/non-local-defs/local.rs +++ b/tests/ui/lint/non-local-defs/local.rs @@ -51,3 +51,10 @@ fn bitflags() { impl Flags {} }; } + +fn bitflags_internal() { + const _: () = { + struct InternalFlags; + impl InternalFlags {} + }; +}