Re: [PATCH v2 11/11] docs: add notes on Golang code generator

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Hi,
>
> On Wed, Oct 18, 2023 at 01:47:56PM +0200, Markus Armbruster wrote:
>> Victor Toso <victortoso@redhat.com> writes:
>> 
>> > The goal of this patch is converge discussions into a documentation,
>> > to make it easy and explicit design decisions, known issues and what
>> > else might help a person interested in how the Go module is generated.
>> >
>> > Signed-off-by: Victor Toso <victortoso@redhat.com>
>> > ---
>> >  docs/devel/index-build.rst          |   1 +
>> >  docs/devel/qapi-golang-code-gen.rst | 376 ++++++++++++++++++++++++++++
>> >  2 files changed, 377 insertions(+)
>> >  create mode 100644 docs/devel/qapi-golang-code-gen.rst
>> >
>> > diff --git a/docs/devel/index-build.rst b/docs/devel/index-build.rst
>> > index 57e8d39d98..8f7c6f5dc7 100644
>> > --- a/docs/devel/index-build.rst
>> > +++ b/docs/devel/index-build.rst
>> > @@ -15,5 +15,6 @@ the basics if you are adding new files and targets to 
>> > the build.
>> >     qtest
>> >     ci
>> >     qapi-code-gen
>> > +   qapi-golang-code-gen
>> >     fuzzing
>> >     control-flow-integrity
>> 
>> Let's not worry whether and how this should be integrated with
>> qapi-code-gen.rst for now.
>> 
>> I'm a Go ignoramus.  I hope my comments are at least somewhat
>> helpful all the same.
>
> They always are.

Thanks!

>> > diff --git a/docs/devel/qapi-golang-code-gen.rst 
>> > b/docs/devel/qapi-golang-code-gen.rst
>> > new file mode 100644
>> > index 0000000000..b62daf3bad
>> > --- /dev/null
>> > +++ b/docs/devel/qapi-golang-code-gen.rst
>> > @@ -0,0 +1,376 @@
>> > +==========================
>> > +QAPI Golang code generator
>> > +==========================
>> > +
>> > +..
>> > +   Copyright (C) 2023 Red Hat, Inc.
>> > +
>> > +   This work is licensed under the terms of the GNU GPL, version 2 or
>> > +   later.  See the COPYING file in the top-level directory.
>> > +
>> > +
>> > +Introduction
>> > +============
>> > +
>> > +This document provides information of how the generated Go code maps
>> > +with the QAPI specification, clarifying design decisions when needed.
>> > +
>> > +
>> > +Scope of the generated Go code
>> > +==============================
>> 
>> What do you mean by "scope"?
>
> To build an application to talk with QEMU over QMP, this
> generated code is not enough. What I mean is that, this is just
> the first layer. We still need a library on top to do the work of
> connecting, sending/receiving messages, etc.
>
> Any recommendations on how to word this better?

Yes: try working your explanation of layers into the text.

>> > +
>> > +The scope is limited to data structures that can interpret and be used
>> > +to generate valid QMP messages. These data structures are generated
>> > +from a QAPI schema and should be able to handle QMP messages from the
>> > +same schema.
>> > +
>> > +The generated Go code is a Go module with data structs that uses Go
>> > +standard library ``encoding/json``, implementing its field tags and
>> > +Marshal interface whenever needed.
>> > +
>> > +
>> > +QAPI types to Go structs
>> > +========================
>> > +
>> > +Enum
>> > +----
>> > +
>> > +Enums are mapped as strings in Go, using a specified string type per
>> > +Enum to help with type safety in the Go application.
>> > +
>> > +::
>> > +
>> > +    { 'enum': 'HostMemPolicy',
>> > +      'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
>> > +
>> > +.. code-block:: go
>> > +
>> > +    type HostMemPolicy string
>> > +
>> > +    const (
>> > +        HostMemPolicyDefault    HostMemPolicy = "default"
>> > +        HostMemPolicyPreferred  HostMemPolicy = "preferred"
>> > +        HostMemPolicyBind       HostMemPolicy = "bind"
>> > +        HostMemPolicyInterleave HostMemPolicy = "interleave"
>> > +    )
>> > +
>> > +
>> > +Struct
>> > +------
>> > +
>> > +The mapping between a QAPI struct in Go struct is very straightforward.
>> > + - Each member of the QAPI struct has its own field in a Go struct.
>> > + - Optional members are pointers type with 'omitempty' field tag set
>> > +
>> > +One important design decision was to _not_ embed base struct, copying
>> > +the base members to the original struct. This reduces the complexity
>> > +for the Go application.
>> > +
>> > +::
>> > +
>> > +    { 'struct': 'BlockExportOptionsNbdBase',
>> > +      'data': { '*name': 'str', '*description': 'str' } }
>> > +
>> > +    { 'struct': 'BlockExportOptionsNbd',
>> > +      'base': 'BlockExportOptionsNbdBase',
>> > +      'data': { '*bitmaps': ['BlockDirtyBitmapOrStr'],
>> > +                '*allocation-depth': 'bool' } }
>> > +
>> > +.. code-block:: go
>> > +
>> > +    type BlockExportOptionsNbd struct {
>> > +        Name        *string `json:"name,omitempty"`
>> > +        Description *string `json:"description,omitempty"`
>> > +
>> > +        Bitmaps         []BlockDirtyBitmapOrStr `json:"bitmaps,omitempty"`
>> > +        AllocationDepth *bool                   
>> > `json:"allocation-depth,omitempty"`
>> > +    }
>> 
>> Is there a need to convert from type to base type?
>
> Do you mean why we don't embed a base struct and do a copy of its
> fields instead? If yes, the main reason is aesthetic. See this
> issue: https://github.com/golang/go/issues/29438
>
> So, we could have a situation where we have a embed type, inside
> a embed type, inside of embed type making it horrible to write
> the code for using it.

Yes, avoiding such horrors is why we copy the base type's members
generated C.

In object oriented languages with inheritance, you can accomplish the
same by inheriting the base type.  These languages commonly let you
convert to the base type then.

When copying the base type's members, converting to the base type may
require extra code.  It doesn't in C:

>> In C, we get away with (BlockExportOptionsNbdBase *)obj.  We hide it in
>> an inline function, though.

I don't actually know whether it does in Go.  Only matters if there's an
actual need for such a conversion, hence my question.

>> > +
>> > +
>> > +Union
>> > +-----
>> > +
>> > +Unions in QAPI are binded to a Enum type which provides all possible
>> 
>> bound to an Enum
>
> Fixed.
>
>> > +branches of the union. The most important caveat here is that the Union
>> > +does not need to have a complex type implemented for all possible
>> > +branches of the Enum. Receiving a enum value of a unimplemented branch
>> 
>> I'd call that branch empty, not unimplemented.
>
> Ok, changed them all to empty branch.
>
>> > +is valid.
>> > +
>> > +For this reason, the generated Go struct will define a field for each
>> > +Enum value. The Go type defined for unbranched Enum values is bool
>> 
>> Important design decision: since Go sucks at sum types, you elect to add
>> all the variant members to the struct.  Only one of them may be used at
>> any time.  Please spell that out more clearly.
>
> What about:
>
>     The generated Go struct will then define a field for each
>     Enum value. The type for Enum values of empty branch is bool.
>     Only one field can be set at time.

Better.

We'll polish the prose later.

>> > +
>> > +Go struct and also implement the Marshal interface.
>> 
>> Blank line in the middle of a sentence?  Or two sentences?  Can't make
>> sense of them.
>
> Leftover when rewriting it. Removed it.
>
>> What do you mean by "unbranched" Enum value?  Enum value with an
>> implicit (empty) branch?
>
> Yes, changing it to empty branch or empty branched.
>
>> > +
>> > +As each Union Go struct type has both the discriminator field and
>> > +optional fields, it is important to note that when converting Go struct
>> > +to JSON, we only consider the discriminator field if no optional field
>> > +member was set. In practice, the user should use the optional fields if
>> > +the QAPI Union type has defined them, otherwise the user can set the
>> > +discriminator field for the unbranched enum value.
>> 
>> I don't think I get this paragraph.
>
> Sorry, leftover again. I've removed it entirely.
>
> This bit was when we had a Discriminator field, we don't have it
> anymore.
>
>> > +
>> > +::
>> > +
>> > +    { 'union': 'ImageInfoSpecificQCow2Encryption',
>> > +      'base': 'ImageInfoSpecificQCow2EncryptionBase',
>> > +      'discriminator': 'format',
>> > +      'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
>> 
>> The example is hard to understand without additional context, namely:
>> 
>>        { 'struct': 'ImageInfoSpecificQCow2EncryptionBase',
>>          'data': { 'format': 'BlockdevQcow2EncryptionFormat'}}
>> 
>>        { 'enum': 'BlockdevQcow2EncryptionFormat',
>>          'data': [ 'aes', 'luks' ] }
>
> Added.
>
>> > +
>> > +.. code-block:: go
>> > +
>> > +    type ImageInfoSpecificQCow2Encryption struct {
>> > +        // Variants fields
>> > +        Luks *QCryptoBlockInfoLUKS `json:"-"`
>> > +        // Unbranched enum fields
>> > +        Aes bool `json:"-"`
>> > +    }
>> 
>> The members of the base type are the common members, and the members of
>> the branch's type are the variant members.
>> 
>> Your example shows the variant member: 'luks'.
>> 
>> The common member @format isn't there.  I guess you're eliding it
>> because you can derive its value from other members.
>
> Correct. We can define @format value based on user's setting Aes
> or Luks.
>
>> If there were other common members, where would they go?
>
> They should all be at the top of the struct, for example:
>
>     { 'union': 'ExpirePasswordOptions',
>       'base': { 'protocol': 'DisplayProtocol',
>                 'time': 'str' },
>       'discriminator': 'protocol',
>       'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
>
>     { 'enum': 'DisplayProtocol',
>       'data': [ 'vnc', 'spice' ] }
>
> generates:
>
>     type ExpirePasswordOptions struct {
>         Time string `json:"time"`
>         // Variants fields
>         Vnc *ExpirePasswordOptionsVnc `json:"-"`
>         // Unbranched enum fields
>         Spice bool `json:"-"`
>     }
>
> if you want to navigate over it:
>
>     
> https://gitlab.com/victortoso/qapi-go/-/blob/qapi-golang-v2-by-tags/pkg/qapi/unions.go?ref_type=heads#L4516

Recommend to use an example that lets us show Go for such common members.

[...]

>
> Many thanks for the review,
> Victor

You're welcome!