Re: [Qemu-devel] [PATCH v7 14/15] qapi: Allow anonymous branch types in flat union

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/16/2016 08:33 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> Recent commits added support for an anonymous type as the base
>>> of a flat union; with a bit more work, we can also allow an
>>> anonymous struct as a branch of a flat union.  This probably
>>> most useful when a branch adds no additional members beyond the
>>> common elements of the base (that is, the branch struct is '{}'),
>>> but can be used for any struct in the same way we allow for an
>>> anonymous struct for a command.
>>>
>>> The generator has to do a bit of special-casing for the fact that
>>> we do not emit a '_empty' struct nor a 'visit_type__empty_members()'
>>> corresponding to the special ':empty' type; but when the branch
>>> is truly empty, there's nothing to do.
>> 
>> Well, it could emit them, if it makes things easier.
>> 
>>> The testsuite gets an update to use the new feature, and to ensure
>>> that we can still detect invalid collisions of QMP names.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
>
>>> @@ -1061,6 +1063,9 @@ class QAPISchemaMember(object):
>>>                  return '(parameter of %s)' % owner[:-4]
>>>              elif owner.endswith('-base'):
>>>                  return '(base of %s)' % owner[:-5]
>>> +            elif owner.endswith('-branch'):
>>> +                return ('(member of %s branch %s)'
>>> +                        % tuple(owner[:-7].split(':')))
>> 
>> I think we should point to the spot that puts in the colon, and back.
>> 
>> Do we really need the "of %s" part?
>> 
>
> If you think the message reads okay without it, then we can avoid...
>
>>>              else:
>>>                  assert owner.endswith('-wrapper')
>>>                  # Unreachable and not implemented
>>> @@ -1335,7 +1340,11 @@ class QAPISchema(object):
>>>                                                self._make_members(data, 
>>> info),
>>>                                                None))
>>>
>>> -    def _make_variant(self, case, typ):
>>> +    def _make_variant(self, case, typ, info, owner):
>>> +        if isinstance(typ, dict):
>>> +            typ = self._make_implicit_object_type(
>>> +                "%s:%s" % (owner, case), info, 'branch',
>>> +                self._make_members(typ, info)) or 'q_empty'
>> 
>> This is the spot.
>> 
>
>>> @@ -1485,7 +1494,7 @@ def c_enum_const(type_name, const_name, prefix=None):
>>>          type_name = prefix
>>>      return camel_to_upper(type_name) + '_' + c_name(const_name, 
>>> False).upper()
>>>
>>> -c_name_trans = string.maketrans('.-', '__')
>>> +c_name_trans = string.maketrans('.-:', '___')
>> 
>> Because you use the colon as separator.  Hmm.
>> 
>
> ...the need to transliterate : into _.  More below[1]
>
>
>>> +++ b/scripts/qapi-types.py
>>> @@ -61,7 +61,8 @@ def gen_object(name, base, members, variants):
>>    def gen_object(name, base, members, variants):
>>        if name in objects_seen:
>>            return ''
>>        objects_seen.add(name)
>> 
>>>      ret = ''
>>>      if variants:
>>>          for v in variants.variants:
>>> -            if isinstance(v.type, QAPISchemaObjectType):
>>> +            if (isinstance(v.type, QAPISchemaObjectType)
>>> +                    and not (v.type.is_implicit() and v.type.is_empty())):
>>>                  ret += gen_object(v.type.name, v.type.base,
>>>                                    v.type.local_members, v.type.variants)
>>>
>> 
>> This is the recursion that ensures an object type's variant member types
>> are emitted before the object type.
>> 
>> We can't simply .type == schema.the_empty_object_type like
>> qapi-introspect.py does, because we don't have schema handy here.  Hmm.
>> 
>> Do we really need this change?  Note that gen_object() does nothing for
>> name in objects_seen, and we do this in visit_begin():
>> 
>>         # gen_object() is recursive, ensure it doesn't visit the empty type
>>         objects_seen.add(schema.the_empty_object_type.name)
>
> Cool! Remember, these patches have been through a lot of rebase churn -
> I did indeed originally have to add this check to work around the empty
> type (back when it was based on v5 of the "unboxed visits" series), but
> that was before incorporating your suggestions in commit 7ce106a9 of the
> nicer recursion prevention (v6 of the "unboxed visits", which is what
> got pulled).  You are indeed correct that even without this hunk, things
> still work just fine - I just never noticed that across all the rebasing.
>
>> 
>>> @@ -123,11 +124,14 @@ def gen_variants(variants):
>>>                  c_name=c_name(variants.tag_member.name))
>>>
>>>      for var in variants.variants:
>> 
>> Here, we emit the C union member for a variant:
>> 
>>> +        typ = var.type.c_unboxed_type()
>>> +        if (isinstance(var.type, QAPISchemaObjectType) and
>>> +                var.type.is_empty() and var.type.is_implicit()):
>>> +            typ = 'char'
>>>          ret += mcgen('''
>>>          %(c_type)s %(c_name)s;
>>>  ''',
>>> -                     c_type=var.type.c_unboxed_type(),
>>> -                     c_name=c_name(var.name))
>>> +                     c_type=typ, c_name=c_name(var.name))
>> 
>> Your change replaces the C type when var.type is the empty type.
>> Without that, we'd get 'q_empty', I guess.
>> 
>> Do we need the union member?  Hmm, we may want to avoid empty unions, to
>> avoid pedantic warnings.
>> 
>> Should we make the_empty_object_type.c_unboxed_type() return a suitable
>> C type instead of 'q_empty'?
>
> Sounds like something worthwhile to try, and simpler to handle if it works.
>
>> 
>>>
>>>      ret += mcgen('''
>>>      } u;
>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>>> index 07ae6d1..46f8b39 100644
>>> --- a/scripts/qapi-visit.py
>>> +++ b/scripts/qapi-visit.py
>>> @@ -79,13 +79,19 @@ void visit_type_%(c_name)s_members(Visitor *v, 
>>> %(c_name)s *obj, Error **errp)
>>>          for var in variants.variants:
>>>              ret += mcgen('''
>>>      case %(case)s:
>> 
>> Here, we emit the visit of the "active" C union member:
>> 
>>> -        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),
>>> -                         c_type=var.type.c_name(), c_name=c_name(var.name))
>>> +                                           
>>> variants.tag_member.type.prefix))
>>> +            if (not isinstance(var.type, QAPISchemaObjectType) or
>>> +                    not var.type.is_empty()):
>>> +                ret += mcgen('''
>>> +        visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
>>> +''',
>>> +                             c_type=var.type.c_name(), 
>>> c_name=c_name(var.name))
>>> +            ret += mcgen('''
>>> +        break;
>>> +''')
>> 
>> Your change suppresses the visit_type_FOO_members() call when var.type
>> is the empty type.  Without that, we'd get visit_type_q_empty_members(),
>> which doesn't exist.
>> 
>> Why not make it exist?
>
> If .c_unboxed_type() returns 'char' instead of the name of an empty
> type, then we are not only avoiding 'q_empty branch;' where we need to
> avoid a missing visit_type_q_empty_members(), but we would also be
> changing CpuInfo's 'CpuInfoOther other' to 'char other', and would want
> to avoid generating visit_type_CpuInfoOther_members().  Then again, one
> of the end goals is getting rid of the need for an explicit empty type
> in the .json files (CpuInfoOther disappears, to be replaced by an
> anonymous branch).
>
> I guess it's a tradeoff for whether special casing the empty type makes
> things easier or harder for the rest of the code.  But commit 7ce106a9
> documented some reasons for why making visit_type_q_empty_members()
> exist is rather hard to achieve - as a built-in type, we would have to
> solve problems on how to avoid it having multiple declarations between
> the main QMP use and the QGA use.

We already emit code for built-in types that is to be shared between QMP
and QGA, both to .h and .c.  The way we do it isn't exactly pretty (I
dislike it in fact), but it works.

>>> +++ b/tests/qapi-schema/flat-union-inline.err
>>> @@ -1 +1 @@
>>> -tests/qapi-schema/flat-union-inline.json:7: Member 'value1' of union 
>>> 'TestUnion' should be a type name
>>> +tests/qapi-schema/flat-union-inline.json:6: 'kind' (member of TestUnion 
>>> branch value2) collides with 'kind' (member of Base)
>>> diff --git a/tests/qapi-schema/flat-union-inline.json 
>>> b/tests/qapi-schema/flat-union-inline.json
>>> index 62c7cda..a049ec8 100644
>>> --- a/tests/qapi-schema/flat-union-inline.json
>>> +++ b/tests/qapi-schema/flat-union-inline.json
>>> @@ -1,5 +1,4 @@
>>> -# we require branches to be a struct name
>>> -# TODO: should we allow anonymous inline branch types?
>>> +# we allow anonymous union branches, but they must not have clashing names
>>>  { 'enum': 'TestEnum',
>>>    'data': [ 'value1', 'value2' ] }
>>>  { 'struct': 'Base',
>>> @@ -8,4 +7,4 @@
>>>    'base': 'Base',
>>>    'discriminator': 'enum1',
>>>    'data': { 'value1': { 'string': 'str' },
>>> -            'value2': { 'integer': 'int' } } }
>>> +            'value2': { 'kind': 'int' } } }
>
> [1] Here's an example of the error message that would be changed if we
> simplify the '%s:%s' string notation mentioned above.  Let's try the
> shorter version:
>
> 'kind' (member of branch value2) collides with 'kind' (member of Base)
>
> Okay, that might work.  But the other thing we have to worry about is
> collision prevention.  When a branch type is anonymous, we synthesize a
> visit_type_FOO_members() call - and so we must ensure FOO is unique.  If
> two different flat unions both have a branch named 'bar', the current
> patch proposal gives:
>
> visit_type_q_obj_Foo1_bar_branch_members()
> visit_type_q_obj_Foo2_bar_branch_members()
>
> where the type owning the branch is the only thing different; but
> changing the generated name to avoid the colon would produce collisions:
>
> visit_type_q_obj_bar_branch_members() // from Foo1
> visit_type_q_obj_bar_branch_members() // from Foo2
>
> So we really DO want both the type name and the branch name included in
> the generated name; what's more, since both type names AND branch names
> can include '-' (and even '_'), we need a third character to split on if
> we are going to have _pretty_owner() do a proper split back into legible
> error messages.

I'm afraid we're once again growing beyond what our hacks can reasonably
support.  Having _pretty_owner key on a few suffixes is a tolerable
hack.  Taking off the suffix then splitting the remainder is a bit much.

We use .owner as key for case_whitelist, and for _pretty_owner().
Should it be an object instead of a string?

>>> @@ -81,7 +81,8 @@
>>>    'base': { '*integer': 'int', 'string': 'str', 'enum1': 'QEnumTwo' },
>>>    'discriminator': 'enum1',
>>>    'data': { 'value1' : 'UserDefC', # intentional forward reference
>>> -            'value2' : 'UserDefB' } }
>> 
>> You're deleting the equally intentional case of a backward reference :)
>
> Okay, I'll add the new code as a new branch, rather than deleting an
> existing one.