Re: QAPI unions as branches / unifying struct and union types

[Markus Armbruster]

Het Gala <het.gala@nutanix.com> writes:

> On 10/02/23 12:54 pm, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> [...]
>>
>>>> +##
>>>> +# @MigrateAddress:
>>>> +#
>>>> +# The options available for communication transport mechanisms for 
>>>> migration
>>>> +#
>>>> +# Since 8.0
>>>> +##
>>>> +{ 'union' : 'MigrateAddress',
>>>> +  'base' : { 'transport' : 'MigrateTransport'},
>>>> +  'discriminator' : 'transport',
>>>> +  'data' : {
>>>> +    'socket' : 'MigrateSocketAddr',
>>>> +    'exec' : 'MigrateExecAddr',
>>>> +    'rdma': 'MigrateRdmaAddr' } }
>>>
>>> Ideally this would be
>>>
>>>     'data' : {
>>>       'socket' : 'SocketAddress',
>>>       'exec' : 'MigrateCommand',
>>>       'rdma': 'InetSocketAddress' } }
>>>
>>> though the first SocketAddress isn't possible unless it is easy to
>>> lift the QAPI limitation.
>>
>> Context: SocketAddress is a QAPI union, and "the QAPI limitation" is
>>
>>      scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79:
>>      ../qapi/migration.json: In union 'MigrateAddress':
>>      ../qapi/migration.json:1505: branch 'socket' cannot use union type 
>> 'SocketAddress'
>>
>> Emitted by schema.py like this:
>>
>>                  if (not isinstance(v.type, QAPISchemaObjectType)
>>                          or v.type.variants):
>>                      raise QAPISemError(
>>                          self.info,
>>                          "%s cannot use %s"
>>                          % (v.describe(self.info), v.type.describe()))
>>
>> This enforces docs/devel/qapi-code-gen.rst's clause
>>
>>      The BRANCH's value defines the branch's properties, in particular its
>>      type.  The type must a struct type.  [...]
>>
>> Next paragraph:
>>
>>      In the Client JSON Protocol, a union is represented by an object with
>>      the common members (from the base type) and the selected branch's
>>      members.  The two sets of member names must be disjoint.
>>
>> So, we're splicing in the members of the branch's JSON object.  For that
>> to even make sense, the branch type needs to map to a JSON object.  This
>> is fundamental.  It's the first part of the condition in the code
>> snippet above.
>>
>> We have two kinds of QAPI types that map to a JSON object: struct and
>> union.  The second part of the condition restricts to struct.  Unless
>> I'm missing something (imperfect memory...), this is *not* fundamental,
>> just a matter of implementing it.  But I'd have to try to be sure.
>>
>>
>> Instead of simply allowing unions in addition to structs here, I'd like
>> to go one step further, and fuse the two into "objects".  Let me
>> explain.
>>
>> If we abstract from syntax, structs have become almost a special kind of
>> union.  Unions have a set of common members and sets of variant members,
>> and a special common member (the tag) selects the set of variant
>> members.  Structs are unions with zero variants and no tag.
>>
>> The generator code actually represents both structs and unions as a
>> common QAPISchemaObjectType already.  QAPI/QMP introspection does the
>> same: it uses a single meta type 'object' for both.
>>
>>
>> There is another spot where only structs are allowed: a struct or
>> union's base type.  That restriction will be awkward to lift, as I made
>> the mistake of baking the assumption "object type has at most one tag
>> member" into QAPI/QMP introspection .
>
> Hi Markus, thankyou for explaning in such detail. I tried to understand of 
> what you explained.
>
> So IIUC, you mentioned the QAPI generator treats both structs and unions 
> same, but basically in the schema.py checks is where it tries to distinguish 
> between the two ? and because of the fact that docs/devel/qapi-code-gen.rst 
> states that for a union, it's branches must be 'struct', and that's the 
> reason it gives an error ?

Permit me a brief digression into history.

The initial QAPI design language provided product types (structs) and
sum types (unions containing exactly one of several types, and a tag
member that tells which one).  The two are orthogonal.

These unions turned out rather awkward.

The unions we have today are more general.  They have common members,
and one of them is the tag member, of enumeration type.  For each tag
value, they have variant members.  Both the common members and each tag
value's variant members are given as struct types.

What if the tag's enumeration type is empty, i.e. has no values?  We get
a union with no variant members, only common ones.  Isn't that a struct?

Not quite.  To get a struct, we also have to drop the tag member.  It
has no possible values anyway.

You see, struct types are almost a special case of today's union types.
To overcome "almost", we can introduce the notion of "object type":

* An object type has common members, one of them can be a tag member, of
  enumeration type, not empty.  For each tag value, it additionally has
  variant members.

* A union type is an object type with a tag member and variant members.

* A struct type is an object type without tag member and variant
  members.

The QAPI generator code already made the jump to this object type
notion.  It transform the special cases into the general case at first
opportunity, in QAPISchema._def_struct_type() and ._def_union_type().

*Except* we haven't implemented support for variant members in a few
places where they cannot occur now, e.g. as a tag value's variant.  This
is the restriction you ran into.

I'd like to make the jump to object type in the QAPI schema language,
too.  But that's not a prerequisite to lifting the restriction.

> If that's the case, can we improve on our checks and allow union as a part of 
> branch of a union ? or something else ?

I believe we can implement the missing parts and relax the checks.  But
to be sure, we need to try.

> or I may have completely misunderstood most of the part 😅. Please let me know

More questions?