Re: [Qemu-devel] [PATCH v6 08/12] qapi: Defer duplicate member checks to schema check()

[Eric Blake]

On 10/02/2015 08:00 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> With the previous commit, we have two different locations for
>> detecting member name clashes - one at parse time, and another
>> at QAPISchema*.check() time.  Consolidate some of the checks
>> into a single place, which is also in line with our TODO to
>> eventually defer all of the parse time semantic checking into
>> the newer schema code.  The check_member_clash() function is
>> no longer needed.
>>
>> The wording of several error messages has changed, but in many
>> cases feels like an improvement rather than a regression.  The
>> recent change to avoid an assertion failure when a flat union
>> branch name collides with its discriminator name is also
> 
> Which patch was that again?

v7a 6/18, http://repo.or.cz/qemu/ericb.git/commit/ede42e8e
[uggh - gnu.org archives are still quite lagging, or I'd point to a mail]

> 
>> handled nicely by this code; but there is more work needed
>> before we can detect all collisions in simple union branch
>> names done by the old code.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>
>> @@ -611,11 +590,6 @@ def check_union(expr, expr_info):
>            # If the discriminator names an enum type, then all members
>            # of 'data' must also be members of the enum type, which in turn
>            # must not collide with the discriminator name.
> 
> Should this comment be updated for the code removal below?

Oh, right, revert both hunks of the earlier commit since we are now
doing it in a less hacky manner.

>> +            v.check(schema, info, self.tag_member.type, vseen,
>> +                    self.tag_name is not None)
>>
>>
>>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>>      def __init__(self, name, typ, owner):
>>          QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)
>>
>> -    def check(self, schema, info, tag_type, seen):
>> +    def check(self, schema, info, tag_type, seen, flat):
>>          QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
>>          assert self.name in tag_type.values
>> +        if flat:
> 
> Any way to avoid the conditional?

Not that I could come up with when I first tried, but it's been a while,
so I can try again.  I remember running into an assertion failure about
nested ObjectType.check() calls if I did it unconditionally, since we
allow the following:

{ 'struct': 'Fork', 'data': { '*fork': 'Union' } }
{ 'union': 'Union', 'data': { 'branch': 'Fork' } }

We must NOT call self.type.check() on Fork (because we are not embedding
the members of Fork into the simple union) - but then again, what we are
really doing in the QAPISchema is visiting
'branch':':obj-Union-branch-wrapper', not 'Fork'.  And then there's
alternates, where branches aren't necessarily QAPISchemaObjectType in
the first place.

> 
> I've tried hard to make simple unions mere sugar for flat ones.  There
> are a few special cases left where we need to distinguish the two, but
> they're all marked TODO.
> 
> Can we have a brief comment explaing what we're checking here?  We
> generally don't have such comments in check() methods so far.  Sort of
> okay as long as they merely assert, but you're now starting to move
> semantic analysis into them, which raises the bar.

Yeah, I agree with adding more documentation about the checks.

> 
>> +            self.type.check(schema)
> 
> Uh, careful.
> 
> It's always okay for a check() method to call the check() of a child in
> the abstract syntax tree, e.g. QAPISchemaObjectType.check() calling
> m.check() for its members.  A tree has no cycles.
> 
> Calling it on anything else requires a non-trivial correctness argument.
> Example: QAPISchemaObjectType.check() calls self.base.check().  Okay,
> because no type may be a base of itself, and therefore a cycle would be
> an error.  The code takes care to detect cycles.
> 
> I think the correctness argument here would be "no type contain itself
> as variant member, and therefore a cycle would be an error."  Do we
> detect cycles?

Not in this patch, but the assertion failure I ran into is the very
reason I wrote a cycle detection patch next (11/12 in this series);
maybe I should rebase that patch first?

But you are correct that a flat union must not include itself as one of
the branch types.  I originally tried to test that in v5 3/46; your
review convinced me it is no different than any other branch type that
injects the same QMP members into the flat union, so I revamped the test
in v6 to be a self-inheritance test instead, and then we dropped it
after v7.  (Search for "fork" in
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05733.html)

> 
>> +            assert isinstance(self.type.members, list)
> 
> Implied by QAPISchemaObjectType.check()'s postcondition.  Feels
> redundant here, in particular since you iterate over it next anyway.

Probably redundant, but I added it when I hit a cycle loop, to make sure
that I wasn't still hitting a cycle (the 'if flat:' condition was the
only way I could figure at the time to ensure I didn't hit this assertion).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature