Skip to content

Commit

Permalink
Consider only the outermost const-anon in non_local_def lint
Browse files Browse the repository at this point in the history
  • Loading branch information
Urgau committed Oct 10, 2024
1 parent 0b16baa commit 5560d46
Show file tree
Hide file tree
Showing 5 changed files with 164 additions and 45 deletions.
109 changes: 67 additions & 42 deletions compiler/rustc_lint/src/non_local_def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() };
Expand All @@ -148,14 +138,31 @@ 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.
if collector
.paths
.iter()
.any(|path| path_has_local_parent(path, cx, parent, parent_parent))
{
// 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 impl_parent = peal_parent_while(cx.tcx, parent, |tcx, did| {
tcx.def_kind(did) == DefKind::Const
&& tcx.opt_item_name(did) == Some(kw::Underscore)
})
.unwrap_or(def_id);

// 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, impl_parent)) {
return;
}

Expand Down Expand Up @@ -263,36 +270,54 @@ impl<'tcx> Visitor<'tcx> for PathCollector<'tcx> {
/// ^^^^^^^^^^^^^^^^^^^^^^^
/// ```
#[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_has_local_parent(did, cx.tcx, impl_parent, impl_parent_parent))
fn path_has_local_parent(path: &Path<'_>, cx: &LateContext<'_>, impl_parent: DefId) -> bool {
path.res.opt_def_id().is_some_and(|did| did_has_local_parent(did, cx.tcx, 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 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.
#[inline]
fn did_has_local_parent(
did: DefId,
fn did_has_local_parent(did: DefId, tcx: TyCtxt<'_>, impl_parent: DefId) -> bool {
if !did.is_local() {
return false;
}

let Some(parent_did) = tcx.opt_parent(did) else {
return false;
};

peal_parent_while(tcx, parent_did, |tcx, did| tcx.def_kind(did) == DefKind::Mod)
.map(|parent_did| parent_did == 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 peal_parent_while(
tcx: TyCtxt<'_>,
impl_parent: DefId,
impl_parent_parent: Option<DefId>,
) -> 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)
did: DefId,
mut f: impl FnMut(TyCtxt<'_>, DefId) -> bool,
) -> Option<DefId> {
#[inline]
fn inner(
tcx: TyCtxt<'_>,
did: DefId,
f: &mut impl FnMut(TyCtxt<'_>, DefId) -> bool,
) -> Option<DefId> {
if !did.is_crate_root() && f(tcx, did) {
tcx.opt_parent(did)
.filter(|parent_did| parent_did.is_local())
.map(|parent_did| inner(tcx, parent_did, f))
.flatten()
} else {
false
Some(did)
}
}

inner(tcx, did, &mut f)
}

/// Return for a given `Path` the span until the last args
Expand Down
21 changes: 21 additions & 0 deletions tests/ui/lint/non-local-defs/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,27 @@ fn main() {

1
};

const _: () = {
const _: () = {
impl Test {}
//~^ WARN non-local `impl` definition
};
};

const _: () = {
const _: () = {
mod tmp {
pub(super) struct InnerTest;
}

impl tmp::InnerTest {}
//~^ WARN non-local `impl` definition
// It's unclear if the above should lint or not, it's
// an edge case of an edge case, we can fix it latter
// if there are users.
};
};
}

trait Uto9 {}
Expand Down
35 changes: 32 additions & 3 deletions tests/ui/lint/non-local-defs/consts.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,36 @@ LL | impl Test {
= note: 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>

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 <https://github.com/rust-lang/rust/issues/120363>

warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
--> $DIR/consts.rs:80:13
|
LL | const _: () = {
| ----------- move the `impl` block outside of this constant `_` and up 3 bodies
...
LL | impl tmp::InnerTest {}
| ^^^^^--------------
| |
| `InnerTest` 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 <https://github.com/rust-lang/rust/issues/120363>

warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
--> $DIR/consts.rs:93:9
|
LL | let _a = || {
| -- move the `impl` block outside of this closure `<unnameable>` and up 2 bodies
Expand All @@ -109,7 +138,7 @@ LL | impl Uto9 for Test {}
= note: 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>

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:100:9
|
LL | type A = [u32; {
| ____________________-
Expand All @@ -126,5 +155,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 <https://github.com/rust-lang/rust/issues/120363>

warning: 8 warnings emitted
warning: 10 warnings emitted

24 changes: 24 additions & 0 deletions tests/ui/lint/non-local-defs/convoluted-locals-131474-with-mods.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// 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 {}
};
};
};
};

fn main() {}
20 changes: 20 additions & 0 deletions tests/ui/lint/non-local-defs/convoluted-locals-131474.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// 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 Default for Test {
fn default() -> Test {
Test
}
}
};
};

fn main() {}

0 comments on commit 5560d46

Please sign in to comment.