[Qemu-devel] [PATCH 0/2] qapi: allow flat unions with empty branches

[Qemu-devel] [PATCH 0/2] qapi: allow flat unions with empty branches

[Anton Nefedov]

hej,

inspired by this thread
http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg04802.html

Seems that nobody got around to this since (did anybody?),
so I thought I'd give a try.

There's a generator patch and usage example.

Anton Nefedov (2):
  qapi: allow flat unions with empty branches
  qapi: avoid empty CpuInfoOther type

 qapi/misc.json                 | 48 ++++--------------------------------------
 cpus.c                         |  2 --
 scripts/qapi/common.py         | 18 ++++++++++------
 scripts/qapi/doc.py            |  2 +-
 scripts/qapi/types.py          |  2 +-
 scripts/qapi/visit.py          | 12 ++++++-----
 tests/qapi-schema/test-qapi.py |  2 +-
 7 files changed, 26 insertions(+), 60 deletions(-)

-- 
2.7.4

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

[Anton Nefedov]

The patch makes possible to avoid introducing dummy empty types
for the flat union branches that have no data.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 scripts/qapi/common.py         | 18 ++++++++++++------
 scripts/qapi/doc.py            |  2 +-
 scripts/qapi/types.py          |  2 +-
 scripts/qapi/visit.py          | 12 +++++++-----
 tests/qapi-schema/test-qapi.py |  2 +-
 5 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index a032cec..ec5cf28 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -721,6 +721,7 @@ def check_union(expr, info):
     name = expr['union']
     base = expr.get('base')
     discriminator = expr.get('discriminator')
+    partial = expr.get('data-partial', False)
     members = expr['data']
 
     # Two types of unions, determined by discriminator.
@@ -783,7 +784,7 @@ def check_union(expr, info):
                                    % (key, enum_define['enum']))
 
     # If discriminator is user-defined, ensure all values are covered
-    if enum_define:
+    if enum_define and not partial:
         for value in enum_define['data']:
             if value not in members.keys():
                 raise QAPISemError(info, "Union '%s' data missing '%s' branch"
@@ -909,7 +910,7 @@ def check_exprs(exprs):
         elif 'union' in expr:
             meta = 'union'
             check_keys(expr_elem, 'union', ['data'],
-                       ['base', 'discriminator'])
+                       ['base', 'discriminator', 'data-partial'])
             union_types[expr[meta]] = expr
         elif 'alternate' in expr:
             meta = 'alternate'
@@ -1035,7 +1036,7 @@ class QAPISchemaVisitor(object):
     def visit_array_type(self, name, info, element_type):
         pass
 
-    def visit_object_type(self, name, info, base, members, variants):
+    def visit_object_type(self, name, info, base, members, variants, partial):
         pass
 
     def visit_object_type_flat(self, name, info, members, variants):
@@ -1192,7 +1193,8 @@ class QAPISchemaArrayType(QAPISchemaType):
 
 
 class QAPISchemaObjectType(QAPISchemaType):
-    def __init__(self, name, info, doc, base, local_members, variants):
+    def __init__(self, name, info, doc, base, local_members, variants,
+                 partial = False):
         # struct has local_members, optional base, and no variants
         # flat union has base, variants, and no local_members
         # simple union has local_members, variants, and no base
@@ -1209,6 +1211,7 @@ class QAPISchemaObjectType(QAPISchemaType):
         self.local_members = local_members
         self.variants = variants
         self.members = None
+        self.partial = partial
 
     def check(self, schema):
         if self.members is False:               # check for cycles
@@ -1269,7 +1272,8 @@ class QAPISchemaObjectType(QAPISchemaType):
 
     def visit(self, visitor):
         visitor.visit_object_type(self.name, self.info,
-                                  self.base, self.local_members, self.variants)
+                                  self.base, self.local_members, self.variants,
+                                  self.partial)
         visitor.visit_object_type_flat(self.name, self.info,
                                        self.members, self.variants)
 
@@ -1636,6 +1640,7 @@ class QAPISchema(object):
         name = expr['union']
         data = expr['data']
         base = expr.get('base')
+        partial = expr.get('data-partial', False)
         tag_name = expr.get('discriminator')
         tag_member = None
         if isinstance(base, dict):
@@ -1656,7 +1661,8 @@ class QAPISchema(object):
             QAPISchemaObjectType(name, info, doc, base, members,
                                  QAPISchemaObjectTypeVariants(tag_name,
                                                               tag_member,
-                                                              variants)))
+                                                              variants),
+                                 partial))
 
     def _def_alternate_type(self, expr, info, doc):
         name = expr['alternate']
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 9b312b2..40dffc4 100644
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -211,7 +211,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
                                body=texi_entity(doc, 'Values',
                                                 member_func=texi_enum_value)))
 
-    def visit_object_type(self, name, info, base, members, variants):
+    def visit_object_type(self, name, info, base, members, variants, partial):
         doc = self.cur_doc
         if base and base.is_implicit():
             base = None
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 64d9c0f..e805509 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -215,7 +215,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
         self._genh.add(gen_array(name, element_type))
         self._gen_type_cleanup(name)
 
-    def visit_object_type(self, name, info, base, members, variants):
+    def visit_object_type(self, name, info, base, members, variants, partial):
         # Nothing to do for the special empty builtin
         if name == 'q_empty':
             return
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 5d72d89..3ee64bb 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -34,7 +34,7 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
                  c_name=c_name(name))
 
 
-def gen_visit_object_members(name, base, members, variants):
+def gen_visit_object_members(name, base, members, variants, partial):
     ret = mcgen('''
 
 void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
@@ -93,9 +93,10 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
 
         ret += mcgen('''
     default:
-        abort();
+        %(action)s
     }
-''')
+''',
+                     action="break;" if partial else "abort();")
 
     # 'goto out' produced for base, for each member, and if variants were
     # present
@@ -309,12 +310,13 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
         self._genh.add(gen_visit_decl(name))
         self._genc.add(gen_visit_list(name, element_type))
 
-    def visit_object_type(self, name, info, base, members, variants):
+    def visit_object_type(self, name, info, base, members, variants, partial):
         # Nothing to do for the special empty builtin
         if name == 'q_empty':
             return
         self._genh.add(gen_visit_members_decl(name))
-        self._genc.add(gen_visit_object_members(name, base, members, variants))
+        self._genc.add(gen_visit_object_members(name, base, members, variants,
+                                                partial))
         # TODO Worth changing the visitor signature, so we could
         # directly use rather than repeat type.is_implicit()?
         if not name.startswith('q_'):
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index c1a144b..95cd575 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -28,7 +28,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
         if prefix:
             print('    prefix %s' % prefix)
 
-    def visit_object_type(self, name, info, base, members, variants):
+    def visit_object_type(self, name, info, base, members, variants, partial):
         print('object %s' % name)
         if base:
             print('    base %s' % base.name)
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] qapi: avoid empty CpuInfoOther type

[Anton Nefedov]

the patch provides an example of a previously introduced data-partial
qapi union tag

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 qapi/misc.json | 48 ++++--------------------------------------------
 cpus.c         |  2 --
 2 files changed, 4 insertions(+), 46 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index f5988cc..2059852 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -387,14 +387,14 @@
            'qom_path': 'str', 'thread_id': 'int',
            '*props': 'CpuInstanceProperties', 'arch': 'CpuInfoArch' },
   'discriminator': 'arch',
+  'data-partial': true,
   'data': { 'x86': 'CpuInfoX86',
             'sparc': 'CpuInfoSPARC',
             'ppc': 'CpuInfoPPC',
             'mips': 'CpuInfoMIPS',
             'tricore': 'CpuInfoTricore',
             's390': 'CpuInfoS390',
-            'riscv': 'CpuInfoRISCV',
-            'other': 'CpuInfoOther' } }
+            'riscv': 'CpuInfoRISCV' } }
 
 ##
 # @CpuInfoX86:
@@ -465,16 +465,6 @@
 { 'struct': 'CpuInfoRISCV', 'data': { 'pc': 'int' } }
 
 ##
-# @CpuInfoOther:
-#
-# No additional information is available about the virtual CPU
-#
-# Since: 2.6
-#
-##
-{ 'struct': 'CpuInfoOther', 'data': { } }
-
-##
 # @CpuS390State:
 #
 # An enumeration of cpu states that can be assumed by a virtual
@@ -575,38 +565,8 @@
                       'arch'         : 'CpuInfoArch',
                       'target'       : 'SysEmuTarget' },
   'discriminator' : 'target',
-  'data'          : { 'aarch64'      : 'CpuInfoOther',
-                      'alpha'        : 'CpuInfoOther',
-                      'arm'          : 'CpuInfoOther',
-                      'cris'         : 'CpuInfoOther',
-                      'hppa'         : 'CpuInfoOther',
-                      'i386'         : 'CpuInfoOther',
-                      'lm32'         : 'CpuInfoOther',
-                      'm68k'         : 'CpuInfoOther',
-                      'microblaze'   : 'CpuInfoOther',
-                      'microblazeel' : 'CpuInfoOther',
-                      'mips'         : 'CpuInfoOther',
-                      'mips64'       : 'CpuInfoOther',
-                      'mips64el'     : 'CpuInfoOther',
-                      'mipsel'       : 'CpuInfoOther',
-                      'moxie'        : 'CpuInfoOther',
-                      'nios2'        : 'CpuInfoOther',
-                      'or1k'         : 'CpuInfoOther',
-                      'ppc'          : 'CpuInfoOther',
-                      'ppc64'        : 'CpuInfoOther',
-                      'ppcemb'       : 'CpuInfoOther',
-                      'riscv32'      : 'CpuInfoOther',
-                      'riscv64'      : 'CpuInfoOther',
-                      's390x'        : 'CpuInfoS390',
-                      'sh4'          : 'CpuInfoOther',
-                      'sh4eb'        : 'CpuInfoOther',
-                      'sparc'        : 'CpuInfoOther',
-                      'sparc64'      : 'CpuInfoOther',
-                      'tricore'      : 'CpuInfoOther',
-                      'unicore32'    : 'CpuInfoOther',
-                      'x86_64'       : 'CpuInfoOther',
-                      'xtensa'       : 'CpuInfoOther',
-                      'xtensaeb'     : 'CpuInfoOther' } }
+  'data-partial'  : true,
+  'data'          : { 's390x'        : 'CpuInfoS390' } }
 
 ##
 # @query-cpus-fast:
diff --git a/cpus.c b/cpus.c
index 5bcd3ec..bd0de44 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2273,8 +2273,6 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
         info->value->target = target;
         if (target == SYS_EMU_TARGET_S390X) {
             cpustate_to_cpuinfo_s390(&info->value->u.s390x, cpu);
-        } else {
-            /* do nothing for @CpuInfoOther */
         }
 
         if (!cur_item) {
-- 
2.7.4

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

[Markus Armbruster]

Anton Nefedov <anton.nefedov@virtuozzo.com> writes:

> The patch makes possible to avoid introducing dummy empty types
> for the flat union branches that have no data.
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  scripts/qapi/common.py         | 18 ++++++++++++------
>  scripts/qapi/doc.py            |  2 +-
>  scripts/qapi/types.py          |  2 +-
>  scripts/qapi/visit.py          | 12 +++++++-----
>  tests/qapi-schema/test-qapi.py |  2 +-
>  5 files changed, 22 insertions(+), 14 deletions(-)

Doesn't docs/devel/qapi-code-gen.txt need an update?

> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index a032cec..ec5cf28 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -721,6 +721,7 @@ def check_union(expr, info):
>      name = expr['union']
>      base = expr.get('base')
>      discriminator = expr.get('discriminator')
> +    partial = expr.get('data-partial', False)

Yes, it does.

Discussing a proposed new QAPI language feature is much easier when the
patch starts with a documentation update specifying the new feature.  If
you were a seasoned QAPI developer, my review would stop right here :)
Since you're not, I'll try to make sense of it.

>      members = expr['data']
>  
>      # Two types of unions, determined by discriminator.
> @@ -783,7 +784,7 @@ def check_union(expr, info):
>                                     % (key, enum_define['enum']))
>  
>      # If discriminator is user-defined, ensure all values are covered
> -    if enum_define:
> +    if enum_define and not partial:

data-partial supresses the check for "union lists all discriminator
values".  Makes sense given your commit message.

>          for value in enum_define['data']:
>              if value not in members.keys():
>                  raise QAPISemError(info, "Union '%s' data missing '%s' branch"
> @@ -909,7 +910,7 @@ def check_exprs(exprs):
>          elif 'union' in expr:
>              meta = 'union'
>              check_keys(expr_elem, 'union', ['data'],
> -                       ['base', 'discriminator'])
> +                       ['base', 'discriminator', 'data-partial'])

This accepts key 'data-partial'.  Missing: we also need to check its
value, in check_keys().

>              union_types[expr[meta]] = expr
>          elif 'alternate' in expr:
>              meta = 'alternate'
> @@ -1035,7 +1036,7 @@ class QAPISchemaVisitor(object):
>      def visit_array_type(self, name, info, element_type):
>          pass
>  
> -    def visit_object_type(self, name, info, base, members, variants):
> +    def visit_object_type(self, name, info, base, members, variants, partial):

As we'll see below, @partial is needed just for qapi/visit.py's
gen_visit_object_members().

>          pass
>  
>      def visit_object_type_flat(self, name, info, members, variants):
> @@ -1192,7 +1193,8 @@ class QAPISchemaArrayType(QAPISchemaType):
>  
>  
>  class QAPISchemaObjectType(QAPISchemaType):
> -    def __init__(self, name, info, doc, base, local_members, variants):
> +    def __init__(self, name, info, doc, base, local_members, variants,
> +                 partial = False):
>          # struct has local_members, optional base, and no variants
>          # flat union has base, variants, and no local_members
>          # simple union has local_members, variants, and no base
> @@ -1209,6 +1211,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>          self.local_members = local_members
>          self.variants = variants
>          self.members = None
> +        self.partial = partial
>  
>      def check(self, schema):
>          if self.members is False:               # check for cycles
> @@ -1269,7 +1272,8 @@ class QAPISchemaObjectType(QAPISchemaType):
>  
>      def visit(self, visitor):
>          visitor.visit_object_type(self.name, self.info,
> -                                  self.base, self.local_members, self.variants)
> +                                  self.base, self.local_members, self.variants,
> +                                  self.partial)
>          visitor.visit_object_type_flat(self.name, self.info,
>                                         self.members, self.variants)
>  
> @@ -1636,6 +1640,7 @@ class QAPISchema(object):
>          name = expr['union']
>          data = expr['data']
>          base = expr.get('base')
> +        partial = expr.get('data-partial', False)
>          tag_name = expr.get('discriminator')
>          tag_member = None
>          if isinstance(base, dict):

Flat union; @partial applies.

               base = (self._make_implicit_object_type(
                   name, info, doc, 'base', self._make_members(base, info)))
           if tag_name:
               variants = [self._make_variant(key, value)
                           for (key, value) in data.items()]
               members = []

Note: if @partial, then variants no longer cover all tag values.  As
we'll see below, this necessitates changes to some back ends.

Our general strategy is to make the front end normalize away convenience
features as much as possible.  It looks quite possible here: add the
implicit variants.  We need a type for them, obviously.  Perhaps we can
use the internal 'q_empty'.

           else:

Simple union; @partial is meaningless here.

               variants = [self._make_simple_variant(key, value, info)
                           for (key, value) in data.items()]
               typ = self._make_implicit_enum_type(name, info,
                                                   [v.name for v in variants])
               tag_member = QAPISchemaObjectTypeMember('type', typ, False)
               members = [tag_member]
           self._def_entity(
               QAPISchemaObjectType(name, info, doc, base, members,
                                    QAPISchemaObjectTypeVariants(tag_name,
                                                                 tag_member,
                                                                 variants)))

> @@ -1656,7 +1661,8 @@ class QAPISchema(object):
>              QAPISchemaObjectType(name, info, doc, base, members,
>                                   QAPISchemaObjectTypeVariants(tag_name,
>                                                                tag_member,
> -                                                              variants)))
> +                                                              variants),
> +                                 partial))
>  
>      def _def_alternate_type(self, expr, info, doc):
>          name = expr['alternate']

Okay, that's all for the front end.

As far as I can tell, 'data-partial' the front end accept and ignores
'data-partial' with simple unions.  It should reject it instead.

> diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
> index 9b312b2..40dffc4 100644
> --- a/scripts/qapi/doc.py
> +++ b/scripts/qapi/doc.py
> @@ -211,7 +211,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
>                                 body=texi_entity(doc, 'Values',
>                                                  member_func=texi_enum_value)))
>  
> -    def visit_object_type(self, name, info, base, members, variants):
> +    def visit_object_type(self, name, info, base, members, variants, partial):
>          doc = self.cur_doc
>          if base and base.is_implicit():
>              base = None
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 64d9c0f..e805509 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -215,7 +215,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
>          self._genh.add(gen_array(name, element_type))
>          self._gen_type_cleanup(name)
>  
> -    def visit_object_type(self, name, info, base, members, variants):
> +    def visit_object_type(self, name, info, base, members, variants, partial):
>          # Nothing to do for the special empty builtin
>          if name == 'q_empty':
>              return
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 5d72d89..3ee64bb 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -34,7 +34,7 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
>                   c_name=c_name(name))
>  
>  
> -def gen_visit_object_members(name, base, members, variants):
> +def gen_visit_object_members(name, base, members, variants, partial):
>      ret = mcgen('''
>  
>  void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
> @@ -93,9 +93,10 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>  
>          ret += mcgen('''
>      default:
> -        abort();
> +        %(action)s
>      }
> -''')
> +''',
> +                     action="break;" if partial else "abort();")

No change when partial=False: the switch cases cover all valid
discriminator values, and default aborts on invalid ones.

When partial=True, the cases cover only the values listed in the union,
and default does nothing.  This isn't quite right.  We should do nothing
only for the values not listed in the union, and still abort for invalid
ones.

If the front end normalizes away the convenience feature, we should get
this behavior without any changes here.

>  
>      # 'goto out' produced for base, for each member, and if variants were
>      # present
> @@ -309,12 +310,13 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
>          self._genh.add(gen_visit_decl(name))
>          self._genc.add(gen_visit_list(name, element_type))
>  
> -    def visit_object_type(self, name, info, base, members, variants):
> +    def visit_object_type(self, name, info, base, members, variants, partial):
>          # Nothing to do for the special empty builtin
>          if name == 'q_empty':
>              return
>          self._genh.add(gen_visit_members_decl(name))
> -        self._genc.add(gen_visit_object_members(name, base, members, variants))
> +        self._genc.add(gen_visit_object_members(name, base, members, variants,
> +                                                partial))
>          # TODO Worth changing the visitor signature, so we could
>          # directly use rather than repeat type.is_implicit()?
>          if not name.startswith('q_'):
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index c1a144b..95cd575 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -28,7 +28,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>          if prefix:
>              print('    prefix %s' % prefix)
>  
> -    def visit_object_type(self, name, info, base, members, variants):
> +    def visit_object_type(self, name, info, base, members, variants, partial):
>          print('object %s' % name)
>          if base:
>              print('    base %s' % base.name)

Okay, now let's take a step back and review the problem this patch
solves, and the design space.  The perfect patch would do that in the
commit message, but we're not expecting perfection, only honest effort
:)

Some unions have no variant members for certain discriminator values.
We currently have to use an empty type there, and we've accumulated
several just for the purpose, which is annoying.

QAPI language design alternatives:

1. Having unions cover all discriminator values explicitly is useful.
All we need is a more convenient way to denote empty types.  Eric
proposed {}, as in 'foo: {}.

2. Having unions repeat all the discriminator values explicitly is not
useful.  All we need is replacing the code enforcing that by code
defaulting missing ones to the empty type.

3. We can't decide, so we do both (this patch).

Preferences?

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

[Eric Blake]

On 05/14/2018 01:08 PM, Markus Armbruster wrote:
> Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
> 
>> The patch makes possible to avoid introducing dummy empty types
>> for the flat union branches that have no data.
>>

> 
> Some unions have no variant members for certain discriminator values.
> We currently have to use an empty type there, and we've accumulated
> several just for the purpose, which is annoying.
> 
> QAPI language design alternatives:
> 
> 1. Having unions cover all discriminator values explicitly is useful.
> All we need is a more convenient way to denote empty types.  Eric
> proposed {}, as in 'foo: {}.

And even posted a patch for it once:
https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00311.html

although it was late enough in that series with other churn in the 
meantime that it would need serious revisiting to apply today.

> 
> 2. Having unions repeat all the discriminator values explicitly is not
> useful.  All we need is replacing the code enforcing that by code
> defaulting missing ones to the empty type.
> 
> 3. We can't decide, so we do both (this patch).
> 
> Preferences?

Here's some tradeoffs to consider:

Allowing 'foo':{} makes your intent explicit - "I know this branch of 
the union exists, but it specifically adds no further members".  But 
it's a lot of redundant typing - "I already had to type all the branch 
names when declaring the enum type, why do it again here?"

Allowing an easy way to mark all non-listed members of an enum default 
to the empty type is compact - "I told you the enum, so you are 
permitted to fill in an empty type with every branch that does not 
actually need further members; and I had to opt in to that behavior, so 
that I purposefully get an error if I did not opt in but forgot an enum 
member".  But if the enum is likely to change, it can lead to forgotten 
additions later on - "I'm adding member 'bar' to an enum that already 
has member 'foo'; therefore, grepping for 'foo' should tell me all 
places where I should add handling for 'bar', except that it doesn't 
work when handling for 'foo' was implicit but handling for 'bar' should 
not be".

Having said that, my personal preference is that opting in to an 
explicit way to do less typing ("all branches of the enum listed here 
are valid, and add no further members") seems like a nice syntactic 
sugar trick; and the easiest way to actually implement it is to probably 
already have support for an explicit 'foo':{} branch notation.  That 
way, you can always change your mind later and undo the opt-in 
"data-partial" flag and rewrite the union with explicit empty branches 
when needed, to match how the sugar was expanded on your behalf.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/14/2018 01:08 PM, Markus Armbruster wrote:
>> Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
>>
>>> The patch makes possible to avoid introducing dummy empty types
>>> for the flat union branches that have no data.
>>>
>
>>
>> Some unions have no variant members for certain discriminator values.
>> We currently have to use an empty type there, and we've accumulated
>> several just for the purpose, which is annoying.
>>
>> QAPI language design alternatives:
>>
>> 1. Having unions cover all discriminator values explicitly is useful.
>> All we need is a more convenient way to denote empty types.  Eric
>> proposed {}, as in 'foo: {}.
>
> And even posted a patch for it once:
> https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00311.html
>
> although it was late enough in that series with other churn in the
> meantime that it would need serious revisiting to apply today.
>
>>
>> 2. Having unions repeat all the discriminator values explicitly is not
>> useful.  All we need is replacing the code enforcing that by code
>> defaulting missing ones to the empty type.
>>
>> 3. We can't decide, so we do both (this patch).
>>
>> Preferences?
>
> Here's some tradeoffs to consider:
>
> Allowing 'foo':{} makes your intent explicit - "I know this branch of
> the union exists, but it specifically adds no further members".

Yes.

>                                                                  But
> it's a lot of redundant typing - "I already had to type all the branch
> names when declaring the enum type, why do it again here?"

Redundancy isn't bad per se, but it needs to serve a useful purpose.
What's the purpose of repeating the tag values?

You give one below: grep.

> Allowing an easy way to mark all non-listed members of an enum default
> to the empty type is compact - "I told you the enum, so you are
> permitted to fill in an empty type with every branch that does not
> actually need further members; and I had to opt in to that behavior,
> so that I purposefully get an error if I did not opt in but forgot an
> enum member".

Syntactic options aren't bad per se, but they need to serve a useful
purpose.  What's the purpose of having both unions that require all tag
values, and unions that default omitted tag values to "no variant
members"?

>                But if the enum is likely to change, it can lead to
> forgotten additions later on - "I'm adding member 'bar' to an enum
> that already has member 'foo'; therefore, grepping for 'foo' should
> tell me all places where I should add handling for 'bar', except that
> it doesn't work when handling for 'foo' was implicit but handling for
> 'bar' should not be".

If your code still compiles when you forget to add members to a struct
or union, you obviously don't need the members you forgot :)

For what it's worth, grepping for the enum type's name finds the union
just fine, and is less likely to find unrelated stuff.

> Having said that, my personal preference is that opting in to an
> explicit way to do less typing ("all branches of the enum listed here
> are valid, and add no further members") seems like a nice syntactic
> sugar trick; and the easiest way to actually implement it is to
> probably already have support for an explicit 'foo':{} branch
> notation.  That way, you can always change your mind later and undo
> the opt-in "data-partial" flag and rewrite the union with explicit
> empty branches when needed, to match how the sugar was expanded on
> your behalf.

Is that 1. combined with 3.?

I dislike 3. because I feel it adds more complexity than the other
options.  More apparent once you add the necessary test coverage
(another reason why requiring test coverage is such a good idea; having
to cover every bell & whistle tends to make people reconsider which ones
they actually need).

I'd prefer a more opinionated design here.

Either we opine making people repeat the tag values is an overall win.
Then make them repeat them always, don't give them the option to not
repeat them.  Drop this patch.  To reduce verbosity, we can add a
suitable way to denote a predefined empty type.

Or we opine it's not.  Then let them not repeat them, don't give them
the option to make themselves repeat them.  Evolve this patch into one
that makes flat union variants optional and default to empty.  If we
later find we still want a way to denote a predefined empty type, we can
add it then.

My personal opinion is it's not.

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

[Eric Blake]

On 05/15/2018 02:01 AM, Markus Armbruster wrote:

>>> QAPI language design alternatives:
>>>
>>> 1. Having unions cover all discriminator values explicitly is useful.

>>> 2. Having unions repeat all the discriminator values explicitly is not
>>> useful.  All we need is replacing the code enforcing that by code
>>> defaulting missing ones to the empty type.
>>>
>>> 3. We can't decide, so we do both (this patch).
>>>

> I'd prefer a more opinionated design here.
> 
> Either we opine making people repeat the tag values is an overall win.
> Then make them repeat them always, don't give them the option to not
> repeat them.  Drop this patch.  To reduce verbosity, we can add a
> suitable way to denote a predefined empty type.
> 
> Or we opine it's not.  Then let them not repeat them, don't give them
> the option to make themselves repeat them.  Evolve this patch into one
> that makes flat union variants optional and default to empty.  If we
> later find we still want a way to denote a predefined empty type, we can
> add it then.
> 
> My personal opinion is it's not.

I followed the arguments up to the last sentence, but then I got lost on 
whether you meant:

This patch is not an overall win, so let's drop it and keep status quo 
and/or implement a way to write 'branch':{} (option 1 above)

or

Forcing repetition is not an overall win, so let's drop that requirement 
by using something similar to this patch (option 2 above) but without 
adding a 'partial-data' key.

But you've convinced me that option 3 (supporting a compact branch 
representation AND supporting missing branches as defaulting to an empty 
type) is more of a maintenance burden (any time there is more than one 
way to write something, it requires more testing that both ways continue 
to work) and thus not worth doing without strong evidence that we need 
both ways (which we do not currently have).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/15/2018 02:01 AM, Markus Armbruster wrote:
>
>>>> QAPI language design alternatives:
>>>>
>>>> 1. Having unions cover all discriminator values explicitly is useful.
>
>>>> 2. Having unions repeat all the discriminator values explicitly is not
>>>> useful.  All we need is replacing the code enforcing that by code
>>>> defaulting missing ones to the empty type.
>>>>
>>>> 3. We can't decide, so we do both (this patch).
>>>>
>
>> I'd prefer a more opinionated design here.
>>
>> Either we opine making people repeat the tag values is an overall win.
>> Then make them repeat them always, don't give them the option to not
>> repeat them.  Drop this patch.  To reduce verbosity, we can add a
>> suitable way to denote a predefined empty type.
>>
>> Or we opine it's not.  Then let them not repeat them, don't give them
>> the option to make themselves repeat them.  Evolve this patch into one
>> that makes flat union variants optional and default to empty.  If we
>> later find we still want a way to denote a predefined empty type, we can
>> add it then.
>>
>> My personal opinion is it's not.
>
> I followed the arguments up to the last sentence, but then I got lost
> on whether you meant:
>
> This patch is not an overall win, so let's drop it and keep status quo
> and/or implement a way to write 'branch':{} (option 1 above)
>
> or
>
> Forcing repetition is not an overall win, so let's drop that
> requirement by using something similar to this patch (option 2 above)
> but without adding a 'partial-data' key.

Sorry about that.  I meant the latter.

> But you've convinced me that option 3 (supporting a compact branch
> representation AND supporting missing branches as defaulting to an
> empty type) is more of a maintenance burden (any time there is more
> than one way to write something, it requires more testing that both
> ways continue to work) and thus not worth doing without strong
> evidence that we need both ways (which we do not currently have).

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

[Anton Nefedov]

On 15/5/2018 8:40 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 05/15/2018 02:01 AM, Markus Armbruster wrote:
>>
>>>>> QAPI language design alternatives:
>>>>>
>>>>> 1. Having unions cover all discriminator values explicitly is useful.
>>
>>>>> 2. Having unions repeat all the discriminator values explicitly is not
>>>>> useful.  All we need is replacing the code enforcing that by code
>>>>> defaulting missing ones to the empty type.
>>>>>
>>>>> 3. We can't decide, so we do both (this patch).
>>>>>
>>
>>> I'd prefer a more opinionated design here.
>>>
>>> Either we opine making people repeat the tag values is an overall win.
>>> Then make them repeat them always, don't give them the option to not
>>> repeat them.  Drop this patch.  To reduce verbosity, we can add a
>>> suitable way to denote a predefined empty type.
>>>
>>> Or we opine it's not.  Then let them not repeat them, don't give them
>>> the option to make themselves repeat them.  Evolve this patch into one
>>> that makes flat union variants optional and default to empty.  If we
>>> later find we still want a way to denote a predefined empty type, we can
>>> add it then.
>>>
>>> My personal opinion is it's not.
>>
>> I followed the arguments up to the last sentence, but then I got lost
>> on whether you meant:
>>
>> This patch is not an overall win, so let's drop it and keep status quo
>> and/or implement a way to write 'branch':{} (option 1 above)
>>
>> or
>>
>> Forcing repetition is not an overall win, so let's drop that
>> requirement by using something similar to this patch (option 2 above)
>> but without adding a 'partial-data' key.
> 
> Sorry about that.  I meant the latter.
> 
>> But you've convinced me that option 3 (supporting a compact branch
>> representation AND supporting missing branches as defaulting to an
>> empty type) is more of a maintenance burden (any time there is more
>> than one way to write something, it requires more testing that both
>> ways continue to work) and thus not worth doing without strong
>> evidence that we need both ways (which we do not currently have).

I agree that neither option 3 nor this patch are the best way to handle
this, so it's 1 or 2.

(2) sure brings some prettiness into jsons; I wonder when it might harm;
e.g. a person adds another block driver: it would be difficult to get
round BlockdevOptionsFoo, and what can be missed is something
optional like BlockdevStatsFoo, which is harmless if left empty and
probably would be made an empty branch anyway. The difference is that
an empty branch is something one might notice during a review and
object.

I think I'd vote for 2 (never enforce all-branches coverage) as well.

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

[Markus Armbruster]

Anton Nefedov <anton.nefedov@virtuozzo.com> writes:

> On 15/5/2018 8:40 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 05/15/2018 02:01 AM, Markus Armbruster wrote:
>>>
>>>>>> QAPI language design alternatives:
>>>>>>
>>>>>> 1. Having unions cover all discriminator values explicitly is useful.
>>>
>>>>>> 2. Having unions repeat all the discriminator values explicitly is not
>>>>>> useful.  All we need is replacing the code enforcing that by code
>>>>>> defaulting missing ones to the empty type.
>>>>>>
>>>>>> 3. We can't decide, so we do both (this patch).
>>>>>>
>>>
>>>> I'd prefer a more opinionated design here.
>>>>
>>>> Either we opine making people repeat the tag values is an overall win.
>>>> Then make them repeat them always, don't give them the option to not
>>>> repeat them.  Drop this patch.  To reduce verbosity, we can add a
>>>> suitable way to denote a predefined empty type.
>>>>
>>>> Or we opine it's not.  Then let them not repeat them, don't give them
>>>> the option to make themselves repeat them.  Evolve this patch into one
>>>> that makes flat union variants optional and default to empty.  If we
>>>> later find we still want a way to denote a predefined empty type, we can
>>>> add it then.
>>>>
>>>> My personal opinion is it's not.
>>>
>>> I followed the arguments up to the last sentence, but then I got lost
>>> on whether you meant:
>>>
>>> This patch is not an overall win, so let's drop it and keep status quo
>>> and/or implement a way to write 'branch':{} (option 1 above)
>>>
>>> or
>>>
>>> Forcing repetition is not an overall win, so let's drop that
>>> requirement by using something similar to this patch (option 2 above)
>>> but without adding a 'partial-data' key.
>>
>> Sorry about that.  I meant the latter.
>>
>>> But you've convinced me that option 3 (supporting a compact branch
>>> representation AND supporting missing branches as defaulting to an
>>> empty type) is more of a maintenance burden (any time there is more
>>> than one way to write something, it requires more testing that both
>>> ways continue to work) and thus not worth doing without strong
>>> evidence that we need both ways (which we do not currently have).
>
> I agree that neither option 3 nor this patch are the best way to handle
> this, so it's 1 or 2.
>
> (2) sure brings some prettiness into jsons; I wonder when it might harm;
> e.g. a person adds another block driver: it would be difficult to get
> round BlockdevOptionsFoo, and what can be missed is something
> optional like BlockdevStatsFoo, which is harmless if left empty and
> probably would be made an empty branch anyway. The difference is that
> an empty branch is something one might notice during a review and
> object.

Yes, forcing reviewer attention is the one real advantage of 1. I can
see.  I agree with you that reviewers missing the "main" union (such as
BlockdevOptions) is unlikely.  Missing "secondary" unions is more
plausible.  Let's see how many of unions sharing a tag enumeration type
we have.  A quick hack to introspect.py coughs up:

    Flat union type                  Tag enum type
    --------------------------------------------------------------
    BlockdevCreateOptions            BlockdevDriver
    BlockdevOptions                  BlockdevDriver
    BlockdevQcow2Encryption          BlockdevQcow2EncryptionFormat
    ImageInfoSpecificQCow2Encryption BlockdevQcow2EncryptionFormat
    BlockdevQcowEncryption           BlockdevQcowEncryptionFormat
    CpuInfo                          CpuInfoArch
    GuestPanicInformation            GuestPanicInformationType
    QCryptoBlockCreateOptions        QCryptoBlockFormat
    SchemaInfo                       SchemaMetaType
    SheepdogRedundancy               SheepdogRedundancyType
    SocketAddress                    SocketAddressType
    SshHostKeyCheck                  SshHostKeyCheckMode
    CpuInfoFast                      SysEmuTarget
    UsernetConnection                UsernetType

Two pairs.  We'll live.

> I think I'd vote for 2 (never enforce all-branches coverage) as well.

Eric, what do you think?

One more thought: if we ever get around to provide more convenient flat
union syntax so users don't have to enumerate the variant names twice,
we'll need a way to say "empty branch".  Let's worry about that problem
when we have it.

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

[Eric Blake]

On 05/17/2018 03:05 AM, Markus Armbruster wrote:
>>>>>>> QAPI language design alternatives:
>>>>>>>
>>>>>>> 1. Having unions cover all discriminator values explicitly is useful.
>>>>
>>>>>>> 2. Having unions repeat all the discriminator values explicitly is not
>>>>>>> useful.  All we need is replacing the code enforcing that by code
>>>>>>> defaulting missing ones to the empty type.
>>>>>>>

>> I think I'd vote for 2 (never enforce all-branches coverage) as well.
> 
> Eric, what do you think?

I'm sold. Let's go ahead and make the change that for any flat union, a 
branch not listed defaults to the empty type (no added fields) rather 
than being an error, then simplify a couple of the existing flat unions 
that benefit from that.

> 
> One more thought: if we ever get around to provide more convenient flat
> union syntax so users don't have to enumerate the variant names twice,
> we'll need a way to say "empty branch".  Let's worry about that problem
> when we have it.

In other words, our current "simple" unions act in a manner that 
declares an implicit enum type - if we ever add an implicit enum to a 
flat union (where you don't have to declare a pre-existing enum type), 
then you need some syntax to declare additional acceptable enum values 
that form an empty branch.  Indeed, not a problem to be solved right now.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/17/2018 03:05 AM, Markus Armbruster wrote:
>>>>>>>> QAPI language design alternatives:
>>>>>>>>
>>>>>>>> 1. Having unions cover all discriminator values explicitly is useful.
>>>>>
>>>>>>>> 2. Having unions repeat all the discriminator values explicitly is not
>>>>>>>> useful.  All we need is replacing the code enforcing that by code
>>>>>>>> defaulting missing ones to the empty type.
>>>>>>>>
>
>>> I think I'd vote for 2 (never enforce all-branches coverage) as well.
>>
>> Eric, what do you think?
>
> I'm sold. Let's go ahead and make the change that for any flat union,
> a branch not listed defaults to the empty type (no added fields)
> rather than being an error, then simplify a couple of the existing
> flat unions that benefit from that.

Anton, would you like to give this a try?

[...]

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

[Anton Nefedov]

On 18/5/2018 9:45 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 05/17/2018 03:05 AM, Markus Armbruster wrote:
>>>>>>>>> QAPI language design alternatives:
>>>>>>>>>
>>>>>>>>> 1. Having unions cover all discriminator values explicitly is useful.
>>>>>>
>>>>>>>>> 2. Having unions repeat all the discriminator values explicitly is not
>>>>>>>>> useful.  All we need is replacing the code enforcing that by code
>>>>>>>>> defaulting missing ones to the empty type.
>>>>>>>>>
>>
>>>> I think I'd vote for 2 (never enforce all-branches coverage) as well.
>>>
>>> Eric, what do you think?
>>
>> I'm sold. Let's go ahead and make the change that for any flat union,
>> a branch not listed defaults to the empty type (no added fields)
>> rather than being an error, then simplify a couple of the existing
>> flat unions that benefit from that.
> 
> Anton, would you like to give this a try?
> 
> [...]
> 

Sure, I can try this next week.