| Message ID | 20230927112544.85011-3-victortoso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi-go: add generator for Golang interface | expand |
On Wed, Sep 27, 2023 at 01:25:37PM +0200, Victor Toso wrote: > This patch handles QAPI alternate types and generates data structures > in Go that handles it. This file (and most others) needs some imports added. I found the following to be required in order to actually compile this: alternates.go:import ( alternates.go- "encoding/json" alternates.go- "errors" alternates.go- "fmt" alternates.go-) commands.go:import ( commands.go- "encoding/json" commands.go- "errors" commands.go- "fmt" commands.go-) events.go:import ( events.go- "encoding/json" events.go- "errors" events.go- "fmt" events.go-) helpers.go:import ( helpers.go- "encoding/json" helpers.go- "fmt" helpers.go- "strings" helpers.go-) structs.go:import ( structs.go- "encoding/json" structs.go-) unions.go:import ( unions.go- "encoding/json" unions.go- "errors" unions.go- "fmt" unions.go-) With regards, Daniel
Hi, On Thu, Sep 28, 2023 at 03:51:50PM +0100, Daniel P. Berrangé wrote: > On Wed, Sep 27, 2023 at 01:25:37PM +0200, Victor Toso wrote: > > This patch handles QAPI alternate types and generates data structures > > in Go that handles it. > > This file (and most others) needs some imports added. > I found the following to be required in order to > actually compile this: This was by design, I mean, my preference. I decided that the generator should output correct but not necessarly formatted/buildable Go code. The consumer should still use gofmt/goimports. Do you think we should do this in QEMU? What about extra dependencies in QEMU with go binaries? This is how it is done in victortoso/qapi-go module: # to generate toso@tapioca ~> QEMU_REPO=/home/toso/src/qemu go generate ./... # the generation toso@tapioca ~> cat src/qapi-go/pkg/qapi/doc.go //go:generate ../../scripts/generate.sh //go:generate gofmt -w . //go:generate goimports -w . package qapi # script URL="https://gitlab.com/victortoso/qemu.git" BRANCH="qapi-golang" if [[ -z "${QEMU_REPO}" ]]; then git clone --depth 1 --branch $BRANCH $URL QEMU_REPO="$PWD/qemu" fi python3 $QEMU_REPO/scripts/qapi-gen.py -o tmp $QEMU_REPO/qapi/qapi-schema.json mv tmp/go/* . rm -rf tmp qemu Cheers, Victor > alternates.go:import ( > alternates.go- "encoding/json" > alternates.go- "errors" > alternates.go- "fmt" > alternates.go-) > > > commands.go:import ( > commands.go- "encoding/json" > commands.go- "errors" > commands.go- "fmt" > commands.go-) > > > events.go:import ( > events.go- "encoding/json" > events.go- "errors" > events.go- "fmt" > events.go-) > > > helpers.go:import ( > helpers.go- "encoding/json" > helpers.go- "fmt" > helpers.go- "strings" > helpers.go-) > > > structs.go:import ( > structs.go- "encoding/json" > structs.go-) > > > unions.go:import ( > unions.go- "encoding/json" > unions.go- "errors" > unions.go- "fmt" > unions.go-) > > > > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Fri, Sep 29, 2023 at 02:23:22PM +0200, Victor Toso wrote: > Hi, > > On Thu, Sep 28, 2023 at 03:51:50PM +0100, Daniel P. Berrangé wrote: > > On Wed, Sep 27, 2023 at 01:25:37PM +0200, Victor Toso wrote: > > > This patch handles QAPI alternate types and generates data structures > > > in Go that handles it. > > > > This file (and most others) needs some imports added. > > I found the following to be required in order to > > actually compile this: > > This was by design, I mean, my preference. I decided that the > generator should output correct but not necessarly > formatted/buildable Go code. The consumer should still use > gofmt/goimports. > > Do you think we should do this in QEMU? What about extra > dependencies in QEMU with go binaries? IMHO the generator should be omitting well formatted and buildable code, otherwise we need to wrap the generator in a second generator to do the extra missing bits. With regards, Daniel
On Wed, Sep 27, 2023 at 7:25 AM Victor Toso <victortoso@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 <victortoso@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".
On Fri, Sep 29, 2023 at 8:37 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Fri, Sep 29, 2023 at 02:23:22PM +0200, Victor Toso wrote: > > Hi, > > > > On Thu, Sep 28, 2023 at 03:51:50PM +0100, Daniel P. Berrangé wrote: > > > On Wed, Sep 27, 2023 at 01:25:37PM +0200, Victor Toso wrote: > > > > This patch handles QAPI alternate types and generates data structures > > > > in Go that handles it. > > > > > > This file (and most others) needs some imports added. > > > I found the following to be required in order to > > > actually compile this: > > > > This was by design, I mean, my preference. I decided that the > > generator should output correct but not necessarly > > formatted/buildable Go code. The consumer should still use > > gofmt/goimports. > > > > Do you think we should do this in QEMU? What about extra > > dependencies in QEMU with go binaries? > > IMHO the generator should be omitting well formatted and buildable > code, otherwise we need to wrap the generator in a second generator > to do the extra missing bits. > Unless there's some consideration I'm unaware of, I think I agree with Dan here - I don't *think* there's a reason to need to do this in two layers, unless there's some tool that magically fixes/handles dependencies that you want to leverage as part of the pipeline here.
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 <victortoso@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 <victortoso@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
Hi, On Mon, Oct 02, 2023 at 05:48:37PM -0400, John Snow wrote: > On Fri, Sep 29, 2023 at 8:37 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Fri, Sep 29, 2023 at 02:23:22PM +0200, Victor Toso wrote: > > > Hi, > > > > > > On Thu, Sep 28, 2023 at 03:51:50PM +0100, Daniel P. Berrangé wrote: > > > > On Wed, Sep 27, 2023 at 01:25:37PM +0200, Victor Toso wrote: > > > > > This patch handles QAPI alternate types and generates data structures > > > > > in Go that handles it. > > > > > > > > This file (and most others) needs some imports added. > > > > I found the following to be required in order to > > > > actually compile this: > > > > > > This was by design, I mean, my preference. I decided that the > > > generator should output correct but not necessarly > > > formatted/buildable Go code. The consumer should still use > > > gofmt/goimports. > > > > > > Do you think we should do this in QEMU? What about extra > > > dependencies in QEMU with go binaries? > > > > IMHO the generator should be omitting well formatted and buildable > > code, otherwise we need to wrap the generator in a second generator > > to do the extra missing bits. > > > > Unless there's some consideration I'm unaware of, I think I agree with > Dan here - I don't *think* there's a reason to need to do this in two > layers, unless there's some tool that magically fixes/handles > dependencies that you want to leverage as part of the pipeline here. But there is. In the current qapi-go repository, I have 4 line doc.go [0] file: 1 //go:generate ../../scripts/generate.sh 2 //go:generate gofmt -w . 3 //go:generate goimports -w . 4 package qapi With this, anyone can run `go generate` which runs this generator (1), runs gofmt (2) and goimports (3). - gofmt fixes the formatting - goimports adds the imports that are missing Both things were mentioned by Dan as a problem to be fixed but somewhat can be solved by another tool. From what I can see, we have three options to chose: 1. Let this be as is. That means that someone else validates and fixes the generator's output. 2. Fix the indentation and the (very few) imports that are needed. This means gofmt and goimports should do 0 changes, so they would not be needed. 3. Add a post-processing that runs gofmt and goimports from QEMU. I would like to avoid this just to no add go binaries as requirement for QEMU, but perhaps it isn't a big deal. I'm planning to work on (2) for v2 and further discuss if (3) will be needed on top of that. [0] https://gitlab.com/victortoso/qapi-go/-/blob/main/pkg/qapi/doc.go Cheers, Victor
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: + 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 = {} self.schema = None self.golang_package_name = "qapi" + self.accept_null_types = [] 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(): + 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,
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 <victortoso@redhat.com> --- scripts/qapi/golang.py | 188 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 185 insertions(+), 3 deletions(-)