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

[Victor Toso]

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.

Cheers,
Victor

signature.asc
Description: PGP signature