Re: [PATCH v1 5/9] qapi: golang: Generate qapi's union types in Go

[Victor Toso]

Hi,

On Fri, Sep 29, 2023 at 03:41:27PM +0200, Victor Toso wrote:
> Hi,
> 
> On Thu, Sep 28, 2023 at 03:21:59PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Sep 27, 2023 at 01:25:40PM +0200, Victor Toso wrote:
> > > This patch handles QAPI union types and generates the equivalent data
> > > structures and methods in Go to handle it.
> > > 
> > > The QAPI union type has two types of fields: The @base and the
> > > @Variants members. The @base fields can be considered common members
> > > for the union while only one field maximum is set for the @Variants.
> > > 
> > > In the QAPI specification, it defines a @discriminator field, which is
> > > an Enum type. The purpose of the  @discriminator is to identify which
> > > @variant type is being used.
> > > 
> > > Not that @discriminator's enum might have more values than the union's
> > > data struct. This is fine. The union does not need to handle all cases
> > > of the enum, but it should accept them without error. For this
> > > specific case, we keep the @discriminator field in every union type.
> > 
> > I still tend think the @discriminator field should not be
> > present in the union structs. It feels like we're just trying
> > to directly copy the C code in Go and so smells wrong from a
> > Go POV.
> > 
> > For most of the unions the @discriminator field will be entirely
> > redundant, becasue the commonm case is that a @variant field
> > exists for every possible @discriminator value.
> 
> You are correct.
> 
> > To take one example
> > 
> >   type SocketAddress struct {
> >         Type SocketAddressType `json:"type"`
> > 
> >         // Variants fields
> >         Inet  *InetSocketAddress  `json:"-"`
> >         Unix  *UnixSocketAddress  `json:"-"`
> >         Vsock *VsockSocketAddress `json:"-"`
> >         Fd    *String             `json:"-"`
> >   }
> > 
> > If one was just writing Go code without the pre-existing knowledge
> > of the QAPI C code, 'Type' is not something a Go programmer would
> > be inclined add IMHO.
> 
> You don't need previous knowledge in the QAPI C code to see that
> having optional field members and a discriminator field feels
> very very suspicious. I wasn't too happy to add it.
> 
> > And yet you are right that we need a way to represent a
> > @discriminator value that has no corresponding @variant, since
> > QAPI allows for that scenario.
> 
> Thank Markus for that, really nice catch :)
> 
> 
> > To deal with that I would suggest we just use an empty
> > interface type. eg
> > 
> >   type SocketAddress struct {
> >         Type SocketAddressType `json:"type"`
> > 
> >         // Variants fields
> >         Inet  *InetSocketAddress  `json:"-"`
> >         Unix  *UnixSocketAddress  `json:"-"`
> >         Vsock *VsockSocketAddress `json:"-"`
> >         Fd    *String             `json:"-"`
> >     Fish  *interface{}        `json:"-"`
> >     Food  *interface()        `json:"-"`
> >   }
> > 
> > the pointer value for 'Fish' and 'Food' fields here merely needs to
> > be non-NULL, it doesn't matter what the actual thing assigned is.
> 
> I like this idea. What happens if Fish becomes a handled in the
> future?
> 
> Before:
> 
>     type SocketAddress struct {
>         // Variants fields
>         Inet  *InetSocketAddress  `json:"-"`
>         Unix  *UnixSocketAddress  `json:"-"`
>         Vsock *VsockSocketAddress `json:"-"`
>         Fd    *String             `json:"-"`
> 
>         // Unhandled enum branches
>         Fish  *interface{}        `json:"-"`
>         Food  *interface{}        `json:"-"`
>     }
> 
> to
> 
>     type SocketAddress struct {
>         // Variants fields
>         Inet  *InetSocketAddress  `json:"-"`
>         Unix  *UnixSocketAddress  `json:"-"`
>         Vsock *VsockSocketAddress `json:"-"`
>         Fd    *String             `json:"-"`
>         Fish  *FishSocketAddress  `json:"-"`
> 
>         // Unhandled enum branches
>         Food  *interface{}        `json:"-"`
>     }
> 
> An application that hat s.Fish = &something, will now error on
> compile due something type not being FishSocketAddress. I think
> this is acceptable. Very corner case scenario and the user
> probably want to use the right struct now.
> 
> If you agree with above, I'd instead like to try a boolean
> instead of *interface{}. s.Fish = true seems better and false is
> simply ignored.

I was just double checking that the Union's value for each enum
branch needs to be a Struct. So I think it is fine to use boolean
for unhandled enum branches. I'll be doing that in the next
iteration.

docs/devel/qapi-code-gen.rst: 350
    The BRANCH's value defines the branch's properties, in particular its
    type.  The type must a struct type.  The form TYPE-REF_ is shorthand
    for :code:`{ 'type': TYPE-REF }`.

Cheers,
Victor

signature.asc
Description: PGP signature