Future incompatibility with Rust RFC 3373: Avoid non-local definitions in functions

[Urgau]

Rust RFC 3373: Avoid non-local definitions in functions was accepted and it's implementation at rust-lang/rust#120393 found that this crate would be affected by it.

To be more precise users of this crate would be affected by it, in the form of a warn-by-default lint: non_local_definitions. This is because the derive macros from this crate use impl in a local context, const _IMPL_SAVEFILE_REPRC_FOR_???:

savefile/savefile-derive/src/lib.rs

Lines 1135 to 1136 in 2c5b36b

	#[allow(non_upper_case_globals)]
	const #dummy_const: () = {

Fortunately a simple fix exist for this crate, by using a const-anon instead of named one:

         #[allow(non_upper_case_globals)]
-        const #dummy_const: () = {
+        const _: () = {

Be-aware, there are many instances of this pattern in the code, don't forget to replace them all.

---

I would suggest applying some form of the patch above as well as releasing a patch version of this crate, as to have a fix available for users as soon as possible.

cc @avl

[avl]

Thank you for the notice.

This will be fixed tomorrow.

[avl]

@Urgau I've now analysed this a bit more. I don't see how your proposed fix would help?

Surely the problem in Savefile isn't the #dummy_const's themselves? They are just regular const, they are not themselves defined inside functions.

This is how it looks when it is expanded:

   #[allow(non_upper_case_globals)]
    const _IMPL_SAVEFILE_SERIALIZE_FOR_EnumVer1: () = {
        extern crate savefile as _savefile;
        impl _savefile::prelude::Serialize for EnumVer1 {
            #[allow(unused_comparisons, unused_variables)]
            fn serialize(
                &self,
                serializer: &mut _savefile::prelude::Serializer<impl std::io::Write>,
            ) -> Result<(), _savefile::prelude::SavefileError> {
                match self {
                    EnumVer1::Variant1 => serializer.write_u8(0u8)?,
                    EnumVer1::Variant2 => serializer.write_u8(1u8)?,
                }
                Ok(())
            }
        }
    };

Surely there's nothing wrong with the constant _IMPL_SAVEFILE_SERIALIZE_FOR_EnumVer1? Isn't the problem that its initializer contains an implementation of a trait?

The reason it's done this way is to be able to have a local extern crate savefile as _savefile; so that the derived trait implementation doesn't require the user to have the right extern-declarations, and also doesn't risk conflicting with any extern-declarations used by the user.

That said, I just checked how serde does this. It seems to use the same trick (I think I originally copied it from serde), but with your fix applied.

Is it perhaps the case that serde will also trigger this warning?

Anyway, using anonymous constans (like serde already does) is simply better, so I will implement the fix as you suggested!

Thanks again for getting in touch!

[Urgau]

Surely the problem in Savefile isn't the #dummy_const's themselves? They are just regular const, they are not themselves defined inside functions.

The RFC name is ill-defined, it's every "block" (functions, enum discriminant, arrays initializer, consts, statics, ...) and every nested-"block".

Surely there's nothing wrong with the constant _IMPL_SAVEFILE_SERIALIZE_FOR_EnumVer1? Isn't the problem that its initializer contains an implementation of a trait?

Yes, the issue is the implementation of the trait Serialize for EnumVer1, both of them are non-local in the const.

The reason it's done this way is to be able to have a local extern crate savefile as _savefile; so that the derived trait implementation doesn't require the user to have the right extern-declarations, and also doesn't risk conflicting with any extern-declarations used by the user.

Yes and this is why const _ is special, it's a lang hack, and it is specialized (for now¹) to be considered as a transparent parent. This means with const _, impl Serialize for EnumVer1 and EnumVer1 and considered to be at the same nesting, even through it isn't really the case.

Is it perhaps the case that serde will also trigger this warning?

Not for now ¹, not with const _ hack.

Thanks again for getting in touch!

You're welcome @avl.

Footnotes

1. It's technically a "unresolved question" from the RFC, but given the number of users of serde and others, I doubt we will warn on them any time soon. ↩ ↩²

[avl]

Yes and this is why const _ is special, it's a lang hack, and it is specialized (for now1) to be considered as a transparent parent. This means with const _, impl Serialize for EnumVer1 and EnumVer1 and considered to be at the same nesting, even through it isn't really the case.

Ah, this was the information I was missing!

If serde does it this way, I think it's reasonable that savefile will do it the same way. However, I'm interested to know if there is some other, better way.

I have released savefile 0.16.5 with the "const _"-hack.

Thanks again for getting in touch - I'll close this as fixed now!

[Urgau]

I'm interested to know if there is some other, better way.

I'm not aware of a better way to do it, but I'm also not an expert in derive macros.
If I hear something, I will try to transmit it to you and any other derive macro authors.