Warning about non-local impl-definition in derive-macro output

[avl]

Code

I tried this code:

const _: () = {
    const _: () = {
        impl Callable for Dummy {}
    };
    pub trait Callable {}
    struct Dummy;
};

I expected to see this happen: The code should compile without errors or warnings.

Instead, this happened: There is a warning: "warning: non-local impl definition, impl blocks should be written at the same level as their item".

This might seem very convoluted. It is a minimization of code generated by the savefile-derive crate when it is generating implementations of AbiExportable for traits returning boxed Fn-trait objects. It generates helper-types for these, and then effectively recurses, generating impls for these helpers types. That's why we get two levels of the const _: () = {...}-trick. This trick is used by crates like savefile and serde, but I suppose serde doesn't end up recursing and is probably not affected by this issue.

Version it worked on

It works in stable rust 1.81, and also in rustc 1.83.0-nightly (52fd99839 2024-10-10).

Version with regression

It does not work in rustc 1.83.0-nightly (1bc403daa 2024-10-11).

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged

[jieyouxu]

cc @Urgau as you know about non_local_definitions.

EDIT: I'm dumb and was looking at different issue pages.

[avl]

Hmm, I wonder if this is a duplicate of #131474 ?

The problem is still present in the latest nightly, but maybe the fix for the above issue is not yet in nightly?

[jieyouxu]

I think this should have been addressed by #131498, but I'm not sure on the nightly timing

EDIT: no, this example is not covered by #131498

[avl]

I think this should have be addressed by #131498, but I'm not sure on the nightly timing, can you check if you are on latest nightly?

Yeah, I think this whole bug report is a duplicate. Unless the fix from #131498 for some reason doesn't work in this particular case?

How can I determine if I have the fix or not?

[jieyouxu]

Let me double check the commits

[avl]

I'm using rustc 1.83.0-nightly (6b9676b45 2024-10-12) , which seems to be the latest nightly at the moment.

[jieyouxu]

The nightly commit is 6b9676b which is later than 7e05da8 so I feel like the nightly should've already contained #131498.

[avl]

The nightly commit is 6b9676b which is later than 7e05da8 so I feel like the nightly should've already contained #131498.

Ok, then maybe #131498 doesn't solve this particular case. It's not entirely identical to #131474, which is what #131498 was to solve.

[jieyouxu]

I'll double-check the logic in the lint, I think this might not be the exact same thing yeah

[avl]

I tried rebuilding rustc from source, using latest 'master' (commit ef4e825), and the problem exists on this version as well.

[jieyouxu]

My basic understanding is that unlike #131474

struct Test;
const _: () = {
    const _: () = {
        impl Default for Test {
            fn default() -> Test {
                Test
            }
        }
    };
};

the example in this issue

const _: () = {
    const _: () = {
        impl Callable for Dummy {}
    };
    pub trait Callable {}
    struct Dummy;
};

is different: the #131474 example produces a non-local definition inside nested anon consts for a outermost-level type Test outside of the anon consts. But Dummy in this example is not outermost-level which escapes the exception added by #131498

rust/compiler/rustc_lint/src/non_local_def.rs

Lines 141 to 170 in ecf2d1f

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

[jieyouxu]

Remark: this would be fixed by #131660 once that is beta-backported.

[clarfonthey]

Also adding that yes, the original issue I mentioned in #131474 is still triggering because part of it is inside a proc macro. I thought it was because the nightly hadn't yet included the change but I checked and confirmed this myself.

[jieyouxu]

Also adding that yes, the original issue I mentioned in #131474 is still triggering because part of it is inside a proc macro. I thought it was because the nightly hadn't yet included the change but I checked and confirmed this myself.

To clarify, do you mean the fix for #131474 wasn't sufficient? Could you check if that's still a problem once #131660 is available in nightly, and if so open a new issue (or reopen if it's the same code pattern)?

[clarfonthey]

Yup, that's the plan: I just wanted to clarify that yes, the issue isn't fully resolved yet.

[Urgau]

#131660 was beta backported, this issue is no longer reproducible, closing as fixed

if there are more issue(s), please open a new issue

[leighmcculloch]

After updating to rustc 1.84.0-nightly (759e07f06 2024-10-30), I am still seeing the warning when an impl is defined inside a const _: () = { ... } block is in code generated by a macro.

error: non-local `impl` definition, `impl` blocks should be written at the same level as their item
  --> /Users/leighmcculloch/Code/rs-soroban-sdk/soroban-sdk/src/auth.rs:11:1
   |
11 | #[contracttype(crate_path = "crate", export = false)]
   | ^----------------------------------------------------
   | |
   | `Arbitrary` is not local
   | move the `impl` block outside of this constant `_` and up 2 bodies
12 | pub enum Context {
   |          ------- `ArbitraryContext` is not local
   |
   = note: the derive macro `crate::testutils::arbitrary::arbitrary::Arbitrary` defines the non-local `impl`, and may need to be changed
   = note: the derive macro `crate::testutils::arbitrary::arbitrary::Arbitrary` may come from an old version of the `derive_arbitrary` crate, try updating your dependency with `cargo update -p derive_arbitrary`
   = 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: `-D non-local-definitions` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(non_local_definitions)]`
   = note: this error originates in the derive macro `crate::testutils::arbitrary::arbitrary::Arbitrary` which comes from the expansion of the attribute macro `contracttype` (in Nightly builds, run with -Z macro-backtrace for more info)

The macro is generated by:
https://github.com/stellar/rs-soroban-sdk/blob/main/soroban-sdk-macros/src/arbitrary.rs#L320-L339

[avl]

The original problem also persists in 'savefile'. However, I've added #[allow(non_local_definitions)] in recent versions, so the problem is hidden for users.

Here is a new minimal reproducer:

mod some_module {
    const _: () = {
        const _: () = {
            impl Callable for Dummy {}
        };
        pub trait Callable {}
        struct Dummy;
    };
}

It's reproducible in nightly on the playground:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=00a58a82e7102f5b187eeb90845be1b3