Issue between non_local_definitions and soroban-sdk

[Urgau]

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

Originally posted by @leighmcculloch in #131643

[Urgau]

@leighmcculloch could you give us a reproduction, ideally a small and self-contained one.

And/or if you can, the result of cargo expand for this particular derive would be quite helpful too.

[leighmcculloch]

The issue is occurring in an area of code that is generated by a proc-macro, and the generated code inserts a #[derive(Arbitrary)] into a generated type, that references the original type. The proc-macro wraps the generated type in a const _: () = { ... }, and the Arbitrary derive also uses a const block, so the notable thing I see in the expanded code is that there are two nested const blocks.

I've tried to produce a minimal example, but all the minimal examples I have created have built just fine. So unfortunately all I have to offer is the project the issue was seen in.

The following commands demonstrate the issue:

Setup:

git clone https://github.com/stellar/rs-soroban-sdk
cd rs-soroban-sdk
git checkout b7b8255ab27dc0d85570b5deed697f4c90079661 # main

Build the crate withnightly-2024-11-01, and the warning occurs:

❯ cargo +nightly-2024-11-01 build --package soroban-sdk --features testutils
...
warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
  --> 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: `#[warn(non_local_definitions)]` on by default
   = note: this warning 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)
...

Build the crate with nightly-2024-10-10, and see no warnings:

❯ cargo +nightly-2024-10-10 build --package soroban-sdk --features testutils
...
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 27.91s

The code that is triggering the warning is here:

#[contracttype(crate_path = "crate", export = false)]
pub enum Context {
    /// Contract invocation.
    Contract(ContractContext),
    /// Contract that has a constructor with no arguments is created.
    CreateContractHostFn(CreateContractHostFnContext),
    /// Contract that has a constructor with 1 or more arguments is created.
    CreateContractWithCtorHostFn(CreateContractWithConstructorHostFnContext),
}

Ref: https://github.com/stellar/rs-soroban-sdk/blob/b7b8255ab27dc0d85570b5deed697f4c90079661/soroban-sdk/src/auth.rs#L11-L19

The #[contracttype] is generating this code:

        const _: () = {
            // derive(Arbitrary) expects these two to be in scope
            use #path::testutils::arbitrary::std;
            use #path::testutils::arbitrary::arbitrary;

            #[derive(#path::testutils::arbitrary::arbitrary::Arbitrary)]
            #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd)]
            #vis #arbitrary_type_decl

            impl #path::testutils::arbitrary::SorobanArbitrary for #ident {
                type Prototype = #arbitrary_type_ident;
            }

            impl #path::TryFromVal<#path::Env, #arbitrary_type_ident> for #ident {
                type Error = #path::ConversionError;
                fn try_from_val(env: &#path::Env, v: &#arbitrary_type_ident) -> std::result::Result<Self, Self::Error> {
                    Ok(#arbitrary_ctor)
                }
            }
        };

Ref: https://github.com/stellar/rs-soroban-sdk/blob/b7b8255ab27dc0d85570b5deed697f4c90079661/soroban-sdk-macros/src/arbitrary.rs#L320-L339

The #[derive(#path::testutils::arbitrary::arbitrary::Arbitrary)] is the arbitrary::Arbitrary trait derive from the arbitrary crate, it's just re-exported via the #path crate.

The full expand for the crate is pretty huge, about 33k lines. I've included the whole thing below, because I didn't want to make an assumption on the scope to include. Search for "enum Context" and that's the type the code is being generated for, and then look for the const block that follows soon after it.

cargo expand output.rs.zip

(GitHub wouldn't let me attach a .rs file, so it's zipped.)

[Urgau]

Thanks for the expanded code. I've managed to minimized it.

pub mod auth {
    const _: () = {
        pub enum ArbitraryContext {}

        const _: () = {
            impl ArbitraryContext {}
        };
    };
}

[leighmcculloch]

Ah fantastic. So there's some interaction of the nested const having been used inside a mod. In the examples I had experimented with I didn't include the mod and they were working. Thanks @Urgau!

[Urgau]

Yeah, item are considered to be transparent to const-anon and modules, but were impl were only considered transparent to const-anon (given the interaction between modules and const-anon), but this creates a discrepancy between the parent of the item and the parent of the impl, so I've applied in #132453 the same "peal" as for const-anon.

[avl]

Here is a minimized version of what the 'savefile-derive'-library does (which was also affected by the similar bug #131643):

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

In savefile-derive this pattern occurs if you use the derive-macros for a type that is not a the top level of the crate. Something that's a very reasonable thing to do.

I'm guessing this is basically the exact same problem, but it would be nice to ensure this also works!

[Urgau]

@avl yeah, it's the same issue, and it will also be fixed by #132453.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-low

[Urgau]

#132453 was beta-backported, closing.