Re: [PATCH v1 1/9] qapi: golang: Generate qapi's enum types in Go

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Sep 27, 2023 at 01:25:36PM +0200, Victor Toso wrote:
>> This patch handles QAPI enum types and generates its equivalent in Go.
>> 
>> Basically, Enums are being handled as strings in Golang.
>> 
>> 1. For each QAPI enum, we will define a string type in Go to be the
>>    assigned type of this specific enum.
>> 
>> 2. Naming: CamelCase will be used in any identifier that we want to
>>    export [0], which is everything.
>> 
>> [0] https://go.dev/ref/spec#Exported_identifiers
>> 
>> Example:
>> 
>> qapi:
>>   | { 'enum': 'DisplayProtocol',
>>   |   'data': [ 'vnc', 'spice' ] }
>> 
>> go:
>>   | type DisplayProtocol string
>>   |
>>   | const (
>>   |     DisplayProtocolVnc   DisplayProtocol = "vnc"
>>   |     DisplayProtocolSpice DisplayProtocol = "spice"
>>   | )
>> 
>> Signed-off-by: Victor Toso <victortoso@redhat.com>
>> ---
>>  scripts/qapi/golang.py | 140 +++++++++++++++++++++++++++++++++++++++++
>>  scripts/qapi/main.py   |   2 +
>>  2 files changed, 142 insertions(+)
>>  create mode 100644 scripts/qapi/golang.py
>> 
>> diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
>> new file mode 100644
>> index 0000000000..87081cdd05
>> --- /dev/null
>> +++ b/scripts/qapi/golang.py
>> @@ -0,0 +1,140 @@
>> +"""
>> +Golang QAPI generator
>> +"""
>> +# Copyright (c) 2023 Red Hat Inc.
>> +#
>> +# Authors:
>> +#  Victor Toso <victortoso@redhat.com>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2.
>> +# See the COPYING file in the top-level directory.
>> +
>> +# due QAPISchemaVisitor interface
>> +# pylint: disable=too-many-arguments
>> +
>> +# Just for type hint on self
>> +from __future__ import annotations
>> +
>> +import os
>> +from typing import List, Optional
>> +
>> +from .schema import (
>> +    QAPISchema,
>> +    QAPISchemaType,
>> +    QAPISchemaVisitor,
>> +    QAPISchemaEnumMember,
>> +    QAPISchemaFeature,
>> +    QAPISchemaIfCond,
>> +    QAPISchemaObjectType,
>> +    QAPISchemaObjectTypeMember,
>> +    QAPISchemaVariants,
>> +)
>> +from .source import QAPISourceInfo
>> +
>> +TEMPLATE_ENUM = '''
>> +type {name} string
>> +const (
>> +{fields}
>> +)
>> +'''
>> +
>> +
>> +def gen_golang(schema: QAPISchema,
>> +               output_dir: str,
>> +               prefix: str) -> None:
>> +    vis = QAPISchemaGenGolangVisitor(prefix)
>> +    schema.visit(vis)
>> +    vis.write(output_dir)
>> +
>> +
>> +def qapi_to_field_name_enum(name: str) -> str:
>> +    return name.title().replace("-", "")
>> +
>> +
>> +class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
>> +
>> +    def __init__(self, _: str):
>> +        super().__init__()
>> +        types = ["enum"]
>> +        self.target = {name: "" for name in types}
>> +        self.schema = None
>> +        self.golang_package_name = "qapi"
>> +
>> +    def visit_begin(self, schema):
>> +        self.schema = schema
>> +
>> +        # Every Go file needs to reference its package name
>> +        for target in self.target:
>> +            self.target[target] = f"package {self.golang_package_name}\n"
>> +
>> +    def visit_end(self):
>> +        self.schema = None
>> +
>> +    def visit_object_type(self: QAPISchemaGenGolangVisitor,
>> +                          name: str,
>> +                          info: Optional[QAPISourceInfo],
>> +                          ifcond: QAPISchemaIfCond,
>> +                          features: List[QAPISchemaFeature],
>> +                          base: Optional[QAPISchemaObjectType],
>> +                          members: List[QAPISchemaObjectTypeMember],
>> +                          variants: Optional[QAPISchemaVariants]
>> +                          ) -> None:
>> +        pass
>> +
>> +    def visit_alternate_type(self: QAPISchemaGenGolangVisitor,
>> +                             name: str,
>> +                             info: Optional[QAPISourceInfo],
>> +                             ifcond: QAPISchemaIfCond,
>> +                             features: List[QAPISchemaFeature],
>> +                             variants: QAPISchemaVariants
>> +                             ) -> None:
>> +        pass
>> +
>> +    def visit_enum_type(self: QAPISchemaGenGolangVisitor,
>> +                        name: str,
>> +                        info: Optional[QAPISourceInfo],
>> +                        ifcond: QAPISchemaIfCond,
>> +                        features: List[QAPISchemaFeature],
>> +                        members: List[QAPISchemaEnumMember],
>> +                        prefix: Optional[str]
>> +                        ) -> None:
>> +
>> +        value = qapi_to_field_name_enum(members[0].name)
>> +        fields = ""
>> +        for member in members:
>> +            value = qapi_to_field_name_enum(member.name)
>> +            fields += f'''\t{name}{value} {name} = "{member.name}"\n'''
>> +
>> +        self.target["enum"] += TEMPLATE_ENUM.format(name=name, 
>> fields=fields[:-1])
>
> Here you are formatting the enums as you visit them, appending to
> the output buffer. The resulting enums appear in whatever order we
> visited them with, which is pretty arbitrary.

We visit in source order, not in arbitrary order.

> Browsing the generated Go code to understand it, I find myself
> wishing that it was emitted in alphabetical order.

If that's easier to read in generated Go, then I suspect it would also
be easier to read in the QAPI schema and in generated C.

> This could be done if we worked in two phase. In the visit phase,
> we collect the bits of data we need, and then add a format phase
> then generates the formatted output, having first sorted by enum
> name.
>
> Same thought for the other types/commands.
>
>> +
>> +    def visit_array_type(self, name, info, ifcond, element_type):
>> +        pass
>> +
>> +    def visit_command(self,
>> +                      name: str,
>> +                      info: Optional[QAPISourceInfo],
>> +                      ifcond: QAPISchemaIfCond,
>> +                      features: List[QAPISchemaFeature],
>> +                      arg_type: Optional[QAPISchemaObjectType],
>> +                      ret_type: Optional[QAPISchemaType],
>> +                      gen: bool,
>> +                      success_response: bool,
>> +                      boxed: bool,
>> +                      allow_oob: bool,
>> +                      allow_preconfig: bool,
>> +                      coroutine: bool) -> None:
>> +        pass
>> +
>> +    def visit_event(self, name, info, ifcond, features, arg_type, boxed):
>> +        pass
>> +
>> +    def write(self, output_dir: str) -> None:
>> +        for module_name, content in self.target.items():
>> +            go_module = module_name + "s.go"
>> +            go_dir = "go"
>> +            pathname = os.path.join(output_dir, go_dir, go_module)
>> +            odir = os.path.dirname(pathname)
>> +            os.makedirs(odir, exist_ok=True)
>> +
>> +            with open(pathname, "w", encoding="ascii") as outfile:
>
> IIUC, we defacto consider the .qapi json files to be UTF-8, and thus
> in theory we could have non-ascii characters in there somewhere. I'd
> suggest we using utf8 encoding when outputting to avoid surprises.

Seconded.  QAPIGen.write() already uses encoding='utf-8' for writing
generated files.

>> +                outfile.write(content)
>
>
> With regards,
> Daniel