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  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.

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

[Markus Armbruster]

Eric Blake  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

[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]

Anton Nefedov  writes:

> On 15/5/2018 8:40 PM, Markus Armbruster wrote:
>> Eric Blake  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
--
BlockdevCreateOptionsBlockdevDriver
BlockdevOptions  BlockdevDriver
BlockdevQcow2Encryption  BlockdevQcow2EncryptionFormat
ImageInfoSpecificQCow2Encryption BlockdevQcow2EncryptionFormat
BlockdevQcowEncryption   BlockdevQcowEncryptionFormat
CpuInfo  CpuInfoArch
GuestPanicInformationGuestPanicInformationType
QCryptoBlockCreateOptionsQCryptoBlockFormat
SchemaInfo   SchemaMetaType
SheepdogRedundancy   SheepdogRedundancyType
SocketAddressSocketAddressType
SshHostKeyCheck  SshHostKeyCheckMode
CpuInfoFast  SysEmuTarget
UsernetConnectionUsernetType

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

[Anton Nefedov]

On 15/5/2018 8:40 PM, Markus Armbruster wrote:

Eric Blake  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]

Eric Blake  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

[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  writes:

> On 05/14/2018 01:08 PM, Markus Armbruster wrote:
>> Anton Nefedov  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/14/2018 01:08 PM, Markus Armbruster wrote:

Anton Nefedov  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]

Anton Nefedov  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 
> ---
>  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.