Support serialization as another type without casting #12712

1. 
   sipa commented at 11:34 pm on March 17, 2018: member

   With the new AsType function it’s possible to serialize an object as another (compatible) type, and is intended for invoking the serializer of a parent class.

   Writing AsType<Parent>(child) will work in any context:

   • READWRITE(AsType<Parent>(child))
   • s << AsType<Parent>(child)
   • s >> AsType<Parent>(child)

   In case child is const, the result will be a reference to a const Parent type, resulting in const-correct behavior.

   For now this primitive isn’t very useful, as the constness is statically known in each of the instances (so a simple cast would work). However, in #10785 I plan to modify the serialization code to have a single implementation which works on a const object when serializing and non-const when deserializing. AsType behaves correctly in this case, maintaining the constness of the argument. It’s also safer, as this only involves automatic type conversions, and no explicit casts.

2. Support serialization as another type without casting

   With the new AsType function it's possible to serialize an object as another
   (compatible) type, and is intended for invoking the serializer of a parent
   class.
   
   Writing AsType<Parent>(child) will work in any context:
   * READWRITE(AsType<Parent>(child))
   * s << AsType<Parent>(child)
   * s >> AsType<Parent>(child)
   In case child is const, the result will be a reference to a const
   Parent type, resulting in const-correct behavior.

   a6e5ddc0df
3. fanquake added the label Refactoring on Mar 17, 2018
4. 
   jonasschnelli commented at 5:03 am on March 18, 2018: contributor

   utACK a6e5ddc0df2ad86ca0352076a656dadab5345abb
5. 
   meshcollider commented at 10:10 am on March 18, 2018: contributor

   utACK https://github.com/bitcoin/bitcoin/pull/12712/commits/a6e5ddc0df2ad86ca0352076a656dadab5345abb
6. 
   donaloconnor commented at 7:36 pm on March 18, 2018: contributor

   utACK https://github.com/bitcoin/bitcoin/commit/a6e5ddc0df2ad86ca0352076a656dadab5345abb
7. 
   jimpo commented at 11:26 pm on March 18, 2018: contributor

   -0

   This adds a new codebase specific function for which a perfectly adequate language-standard one exists. It’s just one more thing for people new to the codebase to see and ask themselves “but why not static_cast”? Converting the invocations from *static_cast<X*>(this) to static_cast<X&>(*this) seems fine and marginally better though.

8. 
   sipa commented at 11:32 pm on March 18, 2018: member

   @jimpo That doesn’t work when the constness of the argument isn’t known.

   I guess the context isn’t entirely clear in this PR, but the goal is to have serialization functions that use single implementation that taken a const argument when serializing and non-const argument when deserializing. Simple static casts aren’t enough, as they don’t maintain constness, but I’d be happy to learn about another approach.

   EDIT: I’ve added a rationale to the PR description; you’re very right to point out it wasn’t sufficient.

9. 
   ryanofsky commented at 3:48 pm on March 19, 2018: member

   utACK a6e5ddc0df2ad86ca0352076a656dadab5345abb, with caveat that I think @jimpo makes a good point about downsides of adding a new cast. IMO, it would be clearer to drop AsType and replace:

   • READWRITE(AsType<Parent>(child))
   • s << AsType<Parent>(child)
   • s >> AsType<Parent>(child)

   with:

   • READWRITEAS(child, Parent)
   • s << static_cast<const Parent&>(child)
   • s >> static_cast<Parent&>(child)

   I don’t think AsType actually serves a useful function except when it’s being used in combination with the READWRITE macro, and in that case, it could just be built in to the macro. In other contexts, AsType could even be dangerous since it potentially returns references to temporaries.

10. 
    sipa commented at 5:14 pm on March 19, 2018: member

    @ryanofsky I was trying to avoid as much macro magic as possible.

    In other contexts, AsType could even be dangerous since it potentially returns references to temporaries.

    Can you give an example?

11. 
    ryanofsky commented at 5:55 pm on March 19, 2018: member

    @ryanofsky I was trying to avoid as much macro magic as possible.

    To be clear, I’m not suggesting replacing AsType with a macro, I’m just suggesting using it internally in READWRITEAS and not exposing it to callers.

    I don’t think macro magic calls like READWRITEAS(child, Parent) are great, but they seem preferable to a macro magic calls combined with template magic like READWRITE(AsType<Parent>(child)).

    Can you give an example?

    0const Base& i = CastAs<Base>(Derived());
    

    It just seems like the CastAs construct is unusual and unsafe and something that doesn’t really make sense outside of the context of our READWRITE macro.

12. 
    sipa commented at 6:10 pm on March 19, 2018: member

    const Base& i = CastAs<Base>(Derived());

    Sure, but that has nothing to do with the AsType construction, I think? This works just as well:

    const Derived& i = Derived();

    Or even:

    const Base& i = Derived();

    It just seems like the CastAs construct is unusual and unsafe and something that doesn’t really make sense outside of the context of our READWRITE macro.

    0
    1~~~My point is that `AsType` never makes unsafe constructions that were illegal legal, and is thus much less risky to use than `static_cast`.~~~
    2
    3EDIT: I see your point. `const Base& i = Derived()` extends the lifetime of the temporary, while `const Base& i = AsType<Base>(Derived());` does not, making it unsafe to use.
    

13. 
    ryanofsky commented at 6:58 pm on March 19, 2018: member

    Yes, but it can’t participate in a READWRITE with multiple arguments at once.

    That’s a good point. I can see how it is a benefit of the current approach. I’m not sure it actually justifies exposing the cast construct, but makes sense.

    const Derived& i = Derived();

    This is safe due to reference lifetime extension (https://abseil.io/tips/107). My example wasn’t safe because lifetime extension can’t work in that case. To be sure, I don’t actually think this is a big deal. It’s just a general reason to be wary of functions like AsType which return references that might not have clear lifetimes.

14. 
    sipa commented at 7:37 pm on March 19, 2018: member

    I’m not sure it actually justifies exposing the cast construct, but makes sense.

    Right, I guess that’s the question.

    After the discussion above with @jimpo and @ryanofsky I’m fine with either the READWRITEAS(type, obj) or the READWRITE(AsType<type>(obj)) approach (this PR). If we choose the former, I’ll close this PR and structure things differently.

15. 
    ryanofsky commented at 8:05 pm on March 19, 2018: member

    Can’t speak for @jimpo. I prefer READWRITEAS to AsType, but I think you should choose whichever approach you like better.
16. 
    jimpo commented at 8:12 pm on March 19, 2018: contributor

    I also prefer READWRITEAS because it contains the scope of the logic. But if you go with AsType I would elaborate on the in-code comments to explain that it is designed for use with READWRITE and not as a general-purpose cast.
17. 
    sipa commented at 8:15 pm on March 19, 2018: member

    Thanks, good points.

    I’ll redo this.

18. sipa closed this on Mar 19, 2018

    ---

19. laanwj referenced this in commit 0a8054e7cd on Apr 10, 2018
20. PastaPastaPasta referenced this in commit 0e304dc860 on Jun 10, 2020
21. PastaPastaPasta referenced this in commit 5856b50d16 on Jun 10, 2020
22. PastaPastaPasta referenced this in commit 498e5dfe11 on Jun 10, 2020
23. PastaPastaPasta referenced this in commit 7ed4ecfdab on Jun 11, 2020
24. PastaPastaPasta referenced this in commit 9cca51a72a on Jun 11, 2020
25. PastaPastaPasta referenced this in commit eadc85e288 on Jun 11, 2020
26. PastaPastaPasta referenced this in commit 96e2c5a25f on Jun 12, 2020
27. MarcoFalke locked this on Sep 8, 2021

Contributors
sipa jonasschnelli meshcollider donaloconnor jimpo ryanofsky

Labels
Refactoring