Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go

[Andrea Bolognani]

On Mon, Oct 16, 2023 at 05:26:57PM +0200, Victor Toso wrote:
> This patch handles QAPI alternate types and generates data structures
> in Go that handles it.
>
> Alternate types are similar to Union but without a discriminator that
> can be used to identify the underlying value on the wire. It is needed
> to infer it. In Go, most of the types [*] are mapped as optional
> fields and Marshal and Unmarshal methods will be handling the data
> checks.
>
> Example:
>
> qapi:
>   | { 'alternate': 'BlockdevRef',
>   |   'data': { 'definition': 'BlockdevOptions',
>   |             'reference': 'str' } }
>
> go:
>   | type BlockdevRef struct {
>   |         Definition *BlockdevOptions
>   |         Reference  *string
>   | }
>
> usage:
>   | input := `{"driver":"qcow2","data-file":"/some/place/my-image"}`
>   | k := BlockdevRef{}
>   | err := json.Unmarshal([]byte(input), &k)
>   | if err != nil {
>   |     panic(err)
>   | }
>   | // *k.Definition.Qcow2.DataFile.Reference == "/some/place/my-image"
>
> [*] The exception for optional fields as default is to Types that can
> accept JSON Null as a value. For this case, we translate NULL to a
> member type called IsNull, which is boolean in Go.  This will be
> explained better in the documentation patch of this series but the
> main rationale is around Marshaling to and from JSON and Go data
> structures.

These usage examples are great; in fact, I think they're too good to
be relegated to the commit messages. I would like to see them in the
actual documentation.

At the same time, the current documentation seems to focus a lot on
internals rather than usage. I think we really need two documents
here:

  * one for users of the library, with lots of usage examples
    (ideally at least one for JSON->Go and one for Go->JSON for each
    class of QAPI object) and little-to-no peeking behind the
    curtains;

  * one for QEMU developers / QAPI maintainers, which goes into
    detail regarding the internals and explains the various design
    choices and trade-offs.

Some parts of the latter should probably be code comments instead. A
reasonable balance will have to be found.

> diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
> +TEMPLATE_HELPER = """
> +// Creates a decoder that errors on unknown Fields
> +// Returns nil if successfully decoded @from payload to @into type
> +// Returns error if failed to decode @from payload to @into type
> +func StrictDecode(into interface{}, from []byte) error {
> +\tdec := json.NewDecoder(strings.NewReader(string(from)))
> +\tdec.DisallowUnknownFields()
> +
> +\tif err := dec.Decode(into); err != nil {
> +\t\treturn err
> +\t}
> +\treturn nil
> +}
> +"""

I think the use of \t here makes things a lot less readable. Can't
you just do

  TEMPLATE_HELPER = """
  func StrictDecode() {
      dec := ...
      if err := ... {
         return err
      }
      return nil
  }
  """

I would actually recommend the use of textwrap.dedent() to make
things less awkward:

  TEMPLATE_HELPER = textwrap.dedent("""
      func StrictDecode() {
          dec := ...
          if err := ... {
             return err
          }
          return nil
      }
  """

This is particularly useful for blocks of Go code that are not
declared at the top level...

> +        unmarshal_check_fields = f"""
> +\t// Check for json-null first
> +\tif string(data) == "null" {{
> +\t\treturn errors.New(`null not supported for {name}`)
> +\t}}"""

... such as this one, which could be turned into:

  unmarshal_check_fields = textwrap.dedent(f"""
      // Check for json-null first
      if string(data) == "null" {{
          return errors.New(`null not supported for {name}`)
      }}
  """)

Much more manageable, don't you think? :)


On a partially related note: while I haven't yet looked closely at
how much effort you've dedicated to producing pretty output, from a
quick look at generate_struct_type() it seems that the answer is "not
zero". I think it would be fine to simplify things there and produce
ugly output, under the assumption that gofmt will be called on the
generated code immediately afterwards. The C generator doesn't have
this luxury, but we should take advantage of it.

-- 
Andrea Bolognani / Red Hat / Virtualization