Hi,
On Mon, Oct 02, 2023 at 04:36:11PM -0400, John Snow wrote:
> 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.
Sure,
> > + 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
Right. Here I knew it was somewhat bad. It is up to you/Markus. I
can introduce a proper interface in the schema as a preparatory
patch to this one, or we mark this as a problem for the future,
for sure there is no intention to detach this from this repo,
specifically scripts/qapi.
It depends on what you think is best.
> > + 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".
Awesome. I didn't know this tool. I'll apply it to all patches
for v2, fixing all python tooling that you've mention that throws
a warning at me.
Thanks for the patience!
Cheers,
Victor
signature.asc
Description: PGP signature
