On Wed, Sep 27, 2023 at 7:25 AM Victor Toso <victort...@redhat.com> 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 like StrOrNull and BlockdevRefOrNull. For
> this case, we translate Null with a boolean value in a field called
> IsNull. 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.
>
> Example:
>
> qapi:
> | { 'alternate': 'StrOrNull',
> | 'data': { 's': 'str',
> | 'n': 'null' } }
>
> go:
> | type StrOrNull struct {
> | S *string
> | IsNull bool
> | }
>
> Signed-off-by: Victor Toso <victort...@redhat.com>
> ---
> scripts/qapi/golang.py | 188 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 185 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
> index 87081cdd05..43dbdde14c 100644
> --- a/scripts/qapi/golang.py
> +++ b/scripts/qapi/golang.py
> @@ -16,10 +16,11 @@
> from __future__ import annotations
>
> import os
> -from typing import List, Optional
> +from typing import Tuple, List, Optional
>
> from .schema import (
> QAPISchema,
> + QAPISchemaAlternateType,
> QAPISchemaType,
> QAPISchemaVisitor,
> QAPISchemaEnumMember,
> @@ -38,6 +39,76 @@
> )
> '''
>
> +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 {
> + dec := json.NewDecoder(strings.NewReader(string(from)))
> + dec.DisallowUnknownFields()
> +
> + if err := dec.Decode(into); err != nil {
> + return err
> + }
> + return nil
> +}
> +'''
> +
> +TEMPLATE_ALTERNATE = '''
> +// Only implemented on Alternate types that can take JSON NULL as value.
> +//
> +// This is a helper for the marshalling code. It should return true only when
> +// the Alternate is empty (no members are set), otherwise it returns false
> and
> +// the member set to be Marshalled.
> +type AbsentAlternate interface {
> + ToAnyOrAbsent() (any, bool)
> +}
> +'''
> +
> +TEMPLATE_ALTERNATE_NULLABLE_CHECK = '''
> + }} else if s.{var_name} != nil {{
> + return *s.{var_name}, false'''
> +
> +TEMPLATE_ALTERNATE_MARSHAL_CHECK = '''
> + if s.{var_name} != nil {{
> + return json.Marshal(s.{var_name})
> + }} else '''
> +
> +TEMPLATE_ALTERNATE_UNMARSHAL_CHECK = '''
> + // Check for {var_type}
> + {{
> + s.{var_name} = new({var_type})
> + if err := StrictDecode(s.{var_name}, data); err == nil {{
> + return nil
> + }}
> + s.{var_name} = nil
> + }}
> +'''
> +
> +TEMPLATE_ALTERNATE_NULLABLE = '''
> +func (s *{name}) ToAnyOrAbsent() (any, bool) {{
> + if s != nil {{
> + if s.IsNull {{
> + return nil, false
> +{absent_check_fields}
> + }}
> + }}
> +
> + return nil, true
> +}}
> +'''
> +
> +TEMPLATE_ALTERNATE_METHODS = '''
> +func (s {name}) MarshalJSON() ([]byte, error) {{
> + {marshal_check_fields}
> + return {marshal_return_default}
> +}}
> +
> +func (s *{name}) UnmarshalJSON(data []byte) error {{
> + {unmarshal_check_fields}
> + return fmt.Errorf("Can't convert to {name}: %s", string(data))
> +}}
> +'''
>
> def gen_golang(schema: QAPISchema,
> output_dir: str,
> @@ -46,27 +117,135 @@ def gen_golang(schema: QAPISchema,
> schema.visit(vis)
> vis.write(output_dir)
>
> +def qapi_to_field_name(name: str) -> str:
> + return name.title().replace("_", "").replace("-", "")
>
> def qapi_to_field_name_enum(name: str) -> str:
> return name.title().replace("-", "")
>
> +def qapi_schema_type_to_go_type(qapitype: str) -> str:
> + schema_types_to_go = {
> + 'str': 'string', 'null': 'nil', 'bool': 'bool', 'number':
> + 'float64', 'size': 'uint64', 'int': 'int64', 'int8': 'int8',
> + 'int16': 'int16', 'int32': 'int32', 'int64': 'int64', 'uint8':
> + 'uint8', 'uint16': 'uint16', 'uint32': 'uint32', 'uint64':
> + 'uint64', 'any': 'any', 'QType': 'QType',
> + }
> +
> + prefix = ""
> + if qapitype.endswith("List"):
> + prefix = "[]"
> + qapitype = qapitype[:-4]
> +
> + qapitype = schema_types_to_go.get(qapitype, qapitype)
> + return prefix + qapitype
> +
> +def qapi_field_to_go_field(member_name: str, type_name: str) -> Tuple[str,
> str, str]:
> + # Nothing to generate on null types. We update some
> + # variables to handle json-null on marshalling methods.
> + if type_name == "null":
> + return "IsNull", "bool", ""
> +
> + # This function is called on Alternate, so fields should be ptrs
> + return qapi_to_field_name(member_name),
> qapi_schema_type_to_go_type(type_name), "*"
> +
> +# Helper function for boxed or self contained structures.
> +def generate_struct_type(type_name, args="") -> str:
> + args = args if len(args) == 0 else f"\n{args}\n"
> + with_type = f"\ntype {type_name}" if len(type_name) > 0 else ""
> + return f'''{with_type} struct {{{args}}}
> +'''
> +
> +def generate_template_alternate(self: QAPISchemaGenGolangVisitor,
> + name: str,
> + variants: Optional[QAPISchemaVariants]) ->
> str:
> + absent_check_fields = ""
> + variant_fields = ""
> + # to avoid having to check accept_null_types
> + nullable = False
> + if name in self.accept_null_types:
> + # In QEMU QAPI schema, only StrOrNull and BlockdevRefOrNull.
> + nullable = True
> + marshal_return_default = '''[]byte("{}"), nil'''
> + marshal_check_fields = '''
> + if s.IsNull {
> + return []byte("null"), nil
> + } else '''
> + unmarshal_check_fields = '''
> + // Check for json-null first
> + if string(data) == "null" {
> + s.IsNull = true
> + return nil
> + }
> + '''
> + else:
> + marshal_return_default = f'nil, errors.New("{name} has empty
> fields")'
> + marshal_check_fields = ""
> + unmarshal_check_fields = f'''
> + // Check for json-null first
> + if string(data) == "null" {{
> + return errors.New(`null not supported for {name}`)
> + }}
> + '''
> +
> + for var in variants.variants:
qapi/golang.py:213: error: Item "None" of "QAPISchemaVariants | None"
has no attribute "variants" [union-attr]
if variants is None ( variants: Optional[QAPISchemaVariants]) we
can't iterate over it without checking to see if it's present first or
not. It may make more sense to change this field to always be an
Iterable and have it default to an empty iterable, but as it is
currently typed we need to check if it's None first.
> + var_name, var_type, isptr = qapi_field_to_go_field(var.name,
> var.type.name)
> + variant_fields += f"\t{var_name} {isptr}{var_type}\n"
> +
> + # Null is special, handled first
> + if var.type.name == "null":
> + assert nullable
> + continue
> +
> + if nullable:
> + absent_check_fields +=
> TEMPLATE_ALTERNATE_NULLABLE_CHECK.format(var_name=var_name)[1:]
> + marshal_check_fields +=
> TEMPLATE_ALTERNATE_MARSHAL_CHECK.format(var_name=var_name)
> + unmarshal_check_fields +=
> TEMPLATE_ALTERNATE_UNMARSHAL_CHECK.format(var_name=var_name,
> +
> var_type=var_type)[1:]
> +
> + content = generate_struct_type(name, variant_fields)
> + if nullable:
> + content += TEMPLATE_ALTERNATE_NULLABLE.format(name=name,
> +
> absent_check_fields=absent_check_fields)
> + content += TEMPLATE_ALTERNATE_METHODS.format(name=name,
> +
> marshal_check_fields=marshal_check_fields[1:-5],
> +
> marshal_return_default=marshal_return_default,
> +
> unmarshal_check_fields=unmarshal_check_fields[1:])
> + return content
> +
>
> class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
>
> def __init__(self, _: str):
> super().__init__()
> - types = ["enum"]
> + types = ["alternate", "enum", "helper"]
> self.target = {name: "" for name in types}
> + self.objects_seen = {}
qapi/golang.py:256: error: Need type annotation for "objects_seen"
(hint: "objects_seen: Dict[<type>, <type>] = ...") [var-annotated]
self.objects_seen: Dict[str, bool] = {}
> self.schema = None
> self.golang_package_name = "qapi"
> + self.accept_null_types = []
qapi/golang.py:259: error: Need type annotation for
"accept_null_types" (hint: "accept_null_types: List[<type>] = ...")
[var-annotated]
self.accept_null_types: List[str] = []
>
> def visit_begin(self, schema):
> self.schema = schema
>
> + # We need to be aware of any types that accept JSON NULL
> + for name, entity in self.schema._entity_dict.items():
We're reaching into the schema's private fields here. I know you
avoided touching the core which Markus liked, but we should look into
giving this a proper interface that we can rely on.
OR, if we all agree that this is fine, and all of this code is going
to *continue living in the same repository for the foreseeable
future*, you can just silence this warning.
jsnow@scv ~/s/q/scripts (review)> pylint qapi --rcfile=qapi/pylintrc
************* Module qapi.golang
qapi/golang.py:265:28: W0212: Access to a protected member
_entity_dict of a client class (protected-access)
for name, entity in self.schema._entity_dict.items(): # pylint:
disable=protected-access
> + if not isinstance(entity, QAPISchemaAlternateType):
> + # Assume that only Alternate types accept JSON NULL
> + continue
> +
> + for var in entity.variants.variants:
> + if var.type.name == 'null':
> + self.accept_null_types.append(name)
> + break
> +
> # Every Go file needs to reference its package name
> for target in self.target:
> self.target[target] = f"package {self.golang_package_name}\n"
>
> + self.target["helper"] += TEMPLATE_HELPER
> + self.target["alternate"] += TEMPLATE_ALTERNATE
> +
> def visit_end(self):
> self.schema = None
>
> @@ -88,7 +267,10 @@ def visit_alternate_type(self: QAPISchemaGenGolangVisitor,
> features: List[QAPISchemaFeature],
> variants: QAPISchemaVariants
> ) -> None:
> - pass
> + assert name not in self.objects_seen
> + self.objects_seen[name] = True
> +
> + self.target["alternate"] += generate_template_alternate(self, name,
> variants)
>
> def visit_enum_type(self: QAPISchemaGenGolangVisitor,
> name: str,
> --
> 2.41.0
>
flake8 is a little unhappy on this patch. My line numbers here won't
match up because I've been splicing in my own fixes/edits, but here's
the gist:
qapi/golang.py:62:1: W191 indentation contains tabs
qapi/golang.py:62:1: E101 indentation contains mixed spaces and tabs
qapi/golang.py:111:1: E302 expected 2 blank lines, found 1
qapi/golang.py:118:1: E302 expected 2 blank lines, found 1
qapi/golang.py:121:1: E302 expected 2 blank lines, found 1
qapi/golang.py:124:1: E302 expected 2 blank lines, found 1
qapi/golang.py:141:1: E302 expected 2 blank lines, found 1
qapi/golang.py:141:80: E501 line too long (85 > 79 characters)
qapi/golang.py:148:80: E501 line too long (87 > 79 characters)
qapi/golang.py:151:1: E302 expected 2 blank lines, found 1
qapi/golang.py:157:1: E302 expected 2 blank lines, found 1
qapi/golang.py:190:80: E501 line too long (83 > 79 characters)
qapi/golang.py:199:80: E501 line too long (98 > 79 characters)
qapi/golang.py:200:80: E501 line too long (90 > 79 characters)
qapi/golang.py:201:80: E501 line too long (94 > 79 characters)
qapi/golang.py:202:80: E501 line too long (98 > 79 characters)
qapi/golang.py:207:80: E501 line too long (94 > 79 characters)
qapi/golang.py:209:80: E501 line too long (97 > 79 characters)
qapi/golang.py:210:80: E501 line too long (95 > 79 characters)
qapi/golang.py:211:80: E501 line too long (99 > 79 characters)
qapi/golang.py:236:23: E271 multiple spaces after keyword
qapi/golang.py:272:80: E501 line too long (85 > 79 characters)
qapi/golang.py:289:80: E501 line too long (82 > 79 characters)
Mostly just lines being too long and so forth, nothing functional. You
can fix all of this by running black - I didn't use black when I
toured qapi last, but it's grown on me since and I think it does a
reasonable job at applying a braindead standard that you don't have to
think about.
Try it:
jsnow@scv ~/s/q/scripts (review)> black --line-length 79 qapi/golang.py
reformatted qapi/golang.py
All done! ✨ 🍰 ✨
1 file reformatted.
jsnow@scv ~/s/q/scripts (review)> flake8 qapi/golang.py
qapi/golang.py:62:1: W191 indentation contains tabs
qapi/golang.py:62:1: E101 indentation contains mixed spaces and tabs
The remaining tab stuff happens in your templates/comments and doesn't
seem to bother anything, but I think you should fix it for the sake of
the linter tooling in Python if you can.
This is pretty disruptive to the formatting you've chosen so far, but
I think it's a reasonable standard for new code and removes a lot of
the thinking. (like gofmt, right?)
Just keep in mind that our line length limitation for QEMU is a bit
shorter than black's default, so you'll have to tell it the line
length you want each time. It can be made shorter with "-l 79".
