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

Activity

1. avl commented on Feb 4, 2024

   

   avl

   on Feb 4, 2024

   Owner

   More actions

   Thank you for the notice.

   This will be fixed tomorrow.
2. avl commented on Feb 5, 2024

   

   avl

   on Feb 5, 2024

   Last edited by avl

   Owner

   More actions

   @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!
3. Urgau commented on Feb 5, 2024

   

   Urgau

   on Feb 5, 2024

   Last edited by Urgau

   Author

   More actions

   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. ↩ ↩²

   👍React with 👍1Reacted by Anders Musikka
4. avl commented on Feb 5, 2024

   

   avl

   on Feb 5, 2024

   Owner

   More actions

   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!
5. avl

   closed this as completedon Feb 5, 2024
6. Urgau commented on Feb 5, 2024

   

   Urgau

   on Feb 5, 2024

   Author

   More actions

   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.

   👍React with 👍1Reacted by Anders Musikka