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

[Daniel P . Berrangé]

On Fri, Feb 17, 2023 at 04:48:59PM +0530, Het Gala wrote:
> 
> On 14/02/23 3:46 pm, Markus Armbruster wrote:
> > 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?
> 
> Completely understood everything. Thankyou for the wonderful explanation.
> Looking forward to implement the missing parts in QAPI schema language.

I cc'd you on a patch that implements this missing feature a couple
of days ago, and its on Markus' review todo list. So we should be
able to decide how to move forward sometime next week.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|