Re: [Qemu-devel] [PATCH v4 1/2] qapi: allow empty branches in flat unions

[Anton Nefedov]

On 14/6/2018 10:19 AM, Markus Armbruster wrote:
> Anton Nefedov <address@hidden> writes:
>
> > It often happens that just a few discriminator values imply extra data in
> > a flat union. Existing checks did not make possible to leave other values
> > uncovered. Such cases had to be worked around by either stating a dummy
> > (empty) type or introducing another (subset) discriminator enumeration.
> >
> > Both options create redundant entities in qapi files for little profit.
> >
> > With this patch it is not necessary anymore to add designated union
> > fields for every possible value of a discriminator enumeration.
> >
> > Signed-off-by: Anton Nefedov <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > ---
> >   docs/devel/qapi-code-gen.txt                        |  8 +++++---
> >   tests/qapi-schema/flat-union-incomplete-branch.json |  9 ---------
> >   tests/qapi-schema/qapi-schema-test.json             |  4 ++--
> >   scripts/qapi/common.py                              | 11 ++++-------
> >   scripts/qapi/types.py                               |  4 +++-
> >   scripts/qapi/visit.py                               | 17 +++++++++++++----
> >   tests/Makefile.include                              |  1 -
> >   tests/qapi-schema/flat-union-incomplete-branch.err  |  1 -
> >   tests/qapi-schema/flat-union-incomplete-branch.exit |  1 -
> >   tests/qapi-schema/flat-union-incomplete-branch.out  |  0
> >   tests/qapi-schema/qapi-schema-test.out              |  3 ++-
> >   11 files changed, 29 insertions(+), 30 deletions(-)
> >   delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.json
> >   delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.err
> >   delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.exit
> >   delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.out
> >
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index 1366228..88a70e4 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -496,9 +496,11 @@ Resulting in these JSON objects:
> >   
> >   Notice that in a flat union, the discriminator name is controlled by
> >   the user, but because it must map to a base member with enum type, the
> > -code generator can ensure that branches exist for all values of the
> > -enum (although the order of the keys need not match the declaration of
> > -the enum).  In the resulting generated C data types, a flat union is
> > +code generator ensures that branches match the existing values of the
> > +enum. The order of the keys need not match the declaration of the enum.
> > +The keys need not cover all possible enum values. Omitted enum values
> > +are still valid branches that add no additional members to the data type.
> > +In the resulting generated C data types, a flat union is
> >   represented as a struct with the base members included directly, and
> >   then a union of structures for each branch of the struct.
> >   
> > diff --git a/tests/qapi-schema/flat-union-incomplete-branch.json b/tests/qapi-schema/flat-union-incomplete-branch.json
> > deleted file mode 100644
> > index 25a411b..0000000
> > --- a/tests/qapi-schema/flat-union-incomplete-branch.json
> > +++ /dev/null
> > @@ -1,9 +0,0 @@
> > -# we require all branches of the union to be covered
> > -{ 'enum': 'TestEnum',
> > -  'data': [ 'value1', 'value2' ] }
> > -{ 'struct': 'TestTypeA',
> > -  'data': { 'string': 'str' } }
> > -{ 'union': 'TestUnion',
> > -  'base': { 'type': 'TestEnum' },
> > -  'discriminator': 'type',
> > -  'data': { 'value1': 'TestTypeA' } }
> > diff --git a/tests/qapi-schema/qapi-schema-test.json 
> > b/tests/qapi-schema/qapi-schema-test.json
> > index 46c7282..881b53b 100644
> > --- a/tests/qapi-schema/qapi-schema-test.json
> > +++ b/tests/qapi-schema/qapi-schema-test.json
> > @@ -39,7 +39,7 @@
> >               '*enum1': 'EnumOne' } }   # intentional forward reference
> >   
> >   { 'enum': 'EnumOne',
> > -  'data': [ 'value1', 'value2', 'value3' ] }
> > +  'data': [ 'value1', 'value2', 'value3', 'value4' ] }
> >   
> >   { 'struct': 'UserDefZero',
> >     'data': { 'integer': 'int' } }
> > @@ -76,7 +76,7 @@
> >     'discriminator': 'enum1',
> >     'data': { 'value1' : 'UserDefA',
> >               'value2' : 'UserDefB',
> > -            'value3' : 'UserDefB' } }
> > +            'value3' : 'UserDefB' } } # skip 'value4'
>
> Perhaps
>
>                 'value3' : 'UserDefB'
>                  # 'value4' defaults to empty
>               } }
>
> would be clearer.

Sure

> >   
> >   { 'struct': 'UserDefUnionBase',
> >     'base': 'UserDefZero',
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 2462fc0..0f3d89b 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -779,13 +779,6 @@ def check_union(expr, info):
> >                                      "enum '%s'"
> >                                      % (key, enum_define['enum']))
> >   
> > -    # If discriminator is user-defined, ensure all values are covered
> > -    if enum_define:
> > -        for value in enum_define['data']:
> > -            if value not in members.keys():
> > -                raise QAPISemError(info, "Union '%s' data missing '%s' branch"
> > -                                   % (name, value))
> > -
> >   
> >   def check_alternate(expr, info):
> >       name = expr['alternate']
> > @@ -1644,6 +1637,10 @@ class QAPISchema(object):
> >           if tag_name:
> >               variants = [self._make_variant(key, value)
> >                           for (key, value) in data.items()]
> > +            # branches that are not explicitly covered get an empty type
> > +            variants += [self._make_variant(key, 'q_empty')
> > +                         for key in 
> > discriminator_find_enum_define(expr)['data']
>
> Long line, flagged by pycodestyle --diff.

Done.

> > +                         if key not in data.keys()]
> >               members = []
> >           else:
> >               variants = [self._make_simple_variant(key, value, info)
>
> The way you desugar the union is simple enough, but it uses
> discriminator_find_enum_define() outside check_exprs(), which I'd rather
> avoid.  Not something you could be reasonably expected to know.
>
> We could desugar in QAPISchemaObjectTypeVariants.check(), where we have
> self.tag_member.type.values.
>
>     @@ -1346,10 +1346,17 @@ class QAPISchemaObjectTypeVariants(object):
>                  v.set_owner(name)
>
>          def check(self, schema, seen):
>     -        if not self.tag_member:    # flat union
>     +        if not self.tag_member: # flat union
>                  self.tag_member = seen[c_name(self._tag_name)]
>                  assert self._tag_name == self.tag_member.name
>              assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>     +        if self._tag_name:      # flat union
>     +            cases = set([v.name for v in self.variants])
>     +            for val in self.tag_member.type.values:
>     +                if val.name not in cases:
>     +                    v = QAPISchemaObjectTypeVariant(val.name, 'q_empty')
>     +                    v.set_owner(self.tag_member.owner)
>     +                    self.variants.append(v)
>              for v in self.variants:
>                  v.check(schema)
>                  # Union names must match enum values; alternate names are
>
> Not so simple anymore.
>
> Simpler: desugar in check_union(), i.e. replace your two hunks by just
>
>     @@ -783,8 +783,7 @@ def check_union(expr, info):
>          if enum_define:
>              for value in enum_define['data']:
>                  if value not in members.keys():
>     -                raise QAPISemError(info, "Union '%s' data missing '%s' 
> branch"
>     -                                   % (name, value))
>     +                members[value] = 'q_empty'
>
>
>      def check_alternate(expr, info):
>
> What do you think?

it didn't feel right aesthetically to normalize by modifying the parsed
expressions especially at check_exprs() stage. But maybe using
discriminator_find_enum_define() is more sloppy, I can't quite see

> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> > index 64d9c0f..1fb2b6d 100644
> > --- a/scripts/qapi/types.py
> > +++ b/scripts/qapi/types.py
> > @@ -124,7 +124,9 @@ def gen_variants(variants):
> >   ''',
> >                   c_name=c_name(variants.tag_member.name))
> >   
> > -    for var in variants.variants:
> > +    # filter out the empty types
> > +    for var in filter(lambda var: var.type.name != 'q_empty',
> > +                      variants.variants):
>
> The comment's position suggests it tells what the loop does, but that's
> not the case.  Let's do it the old-fashioned obvious way instead:
>
>         for var in variants.variants:
>             if var.type.name == 'q_empty':
>                 continue

Done.

> Aside: we need to filter out q_empty, because we don't generate a C type
> for it.  Perhaps generating one would be simpler, but that's beyond the
> scope of this patch.
>
> >           ret += mcgen('''
> >           %(c_type)s %(c_name)s;
> >   ''',
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 5d72d89..96bd32c 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -81,14 +81,23 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
> > *obj, Error **errp)
> >                        c_name=c_name(variants.tag_member.name))
> >   
> >           for var in variants.variants:
> > -            ret += mcgen('''
> > +            case_str = c_enum_const(variants.tag_member.type.name,
> > +                                    var.name,
> > +                                    variants.tag_member.type.prefix)
> > +            if var.type.name == 'q_empty':
> > +                # valid variant and nothing to do
> > +                ret += mcgen('''
> > +    case %(case)s:
> > +        break;
> > +''',
> > +                             case=case_str)
> > +            else:
> > +                ret += mcgen('''
>
> Aside: we need this, because we don't generate a visitor for q_empty.
>
> >       case %(case)s:
> >           visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
> >           break;
> >   ''',
> > -                         case=c_enum_const(variants.tag_member.type.name,
> > -                                           var.name,
> > -                                           variants.tag_member.type.prefix),
> > +                         case=case_str,
> >                            c_type=var.type.c_name(), c_name=c_name(var.name))
>
> Indentation's off, flagged by pycodestyle --diff.

Done.

> >   
> >           ret += mcgen('''
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index bb08e37..afdd0db 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -496,7 +496,6 @@ qapi-schema += flat-union-base-any.json
> >   qapi-schema += flat-union-base-union.json
> >   qapi-schema += flat-union-clash-member.json
> >   qapi-schema += flat-union-empty.json
> > -qapi-schema += flat-union-incomplete-branch.json
> >   qapi-schema += flat-union-inline.json
> >   qapi-schema += flat-union-int-branch.json
> >   qapi-schema += flat-union-invalid-branch-key.json
> > diff --git a/tests/qapi-schema/flat-union-incomplete-branch.err 
> > b/tests/qapi-schema/flat-union-incomplete-branch.err
> > deleted file mode 100644
> > index e826bf0..0000000
> > --- a/tests/qapi-schema/flat-union-incomplete-branch.err
> > +++ /dev/null
> > @@ -1 +0,0 @@
> > -tests/qapi-schema/flat-union-incomplete-branch.json:6: Union 'TestUnion' data 
> > missing 'value2' branch
> > diff --git a/tests/qapi-schema/flat-union-incomplete-branch.exit 
> > b/tests/qapi-schema/flat-union-incomplete-branch.exit
> > deleted file mode 100644
> > index d00491f..0000000
> > --- a/tests/qapi-schema/flat-union-incomplete-branch.exit
> > +++ /dev/null
> > @@ -1 +0,0 @@
> > -1
> > diff --git a/tests/qapi-schema/flat-union-incomplete-branch.out 
> > b/tests/qapi-schema/flat-union-incomplete-branch.out
> > deleted file mode 100644
> > index e69de29..0000000
> > diff --git a/tests/qapi-schema/qapi-schema-test.out 
> > b/tests/qapi-schema/qapi-schema-test.out
> > index 542a19c..0dbcdaf 100644
> > --- a/tests/qapi-schema/qapi-schema-test.out
> > +++ b/tests/qapi-schema/qapi-schema-test.out
> > @@ -23,7 +23,7 @@ object UserDefOne
> >       base UserDefZero
> >       member string: str optional=False
> >       member enum1: EnumOne optional=True
> > -enum EnumOne ['value1', 'value2', 'value3']
> > +enum EnumOne ['value1', 'value2', 'value3', 'value4']
> >   object UserDefZero
> >       member integer: int optional=False
> >   object UserDefTwoDictDict
> > @@ -52,6 +52,7 @@ object UserDefFlatUnion
> >       case value1: UserDefA
> >       case value2: UserDefB
> >       case value3: UserDefB
> > +    case value4: q_empty
> >   object UserDefUnionBase
> >       base UserDefZero
> >       member string: str optional=False