Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()

[Markus Armbruster <armbru@redhat.com>]

First reply: commit message review only.  Patch review to follow.

Eric Blake <eblake@redhat.com> 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 move all of the parse time semantic checking into
> the newer schema code.  The check_member_clash() function is
> no longer needed.
>
> Checking variants is tricky:

Indeed.

Struct types aren't as tricky, but tricky enough to warrant spelling
them out, so let me do that.

QMP: The member names of the JSON object are exactly the member names of
the struct type.  Thus, members can clash with each other (d'oh!).

C: The member names of the C struct are exactly the C names of the *own*
(non-inherited) member names of the struct type, plus 'base' if it has a
base type, plus a has_FOO flag for each optional local member with C
name FOO.  Therefore, local members can clash with each other (d'oh
again!), and additionally with 'base' and the has_FOOs.

The additional clashes are self-inflicted pain:

* Less foolish names for the flags, i.e. ones that cannot occur as
  member names, would eliminate the has_FOO clashes.

* Unboxing base types like we do for unions would eliminate the 'base'
  clash.  Heck, even renaming base to something that cannot occur as
  member name would.

If we check for clashes with *inherited* member names, too, then
checking for C clashes suffices, because when the QMP member names
clash, the C member names clash, too.

>                              we need to check that the branch
> name will not cause a collision (important for C code, but
> no bearing on QMP).  Then, if the variant belongs to a union
> (flat or simple), we need to check that QMP members of that
> type will not collide with non-variant QMP members (but do
> not care if they collide with C branch names).

Union types.

QMP: The member names of the JSON object are exactly the names of the
union type's non-variant members (this includes the tag member; a simple
union's implicit tag is named 'type') plus the names of a single case's
variant members.  Branch names occurs only as (tag) value, not as key.

Thus, members can clash with each other, except for variant members from
different cases.

C: The member names of the C struct are

* the C names of the non-variant members (this includes the tag member;
  a simple union's implicit tag is named 'kind' now, soon 'type')

* a has_FOO for each optional non-variant member with C name FOO

* the branch names, wrapped in an unnamed union

* 'data', wrapped in the same unnamed union

Therefore, non-variant members can clash with each other as for struct
types (except here the inherited members *are* unboxed already), and
additionally

* branch names can clash with each other (but that's caught when
  checking the tag enumeration, which has the branch names as values)

* branch names can clash with non-variant members

* 'data' can clash with branch names and non-variant members

The additional clashes are all self-inflicted pain: either give the
union a name that cannot clash with a non-variant member, or unbox the
cases and rename or kill 'data'.

If we check for clashes between the non-variant members and each single
case's variant members, too, then checking for C clashes suffices,
because when the QMP member names clash, the C member names clash, too.

>                                                 Each call to
> variant.check() has a 'seen' that contains all [*] non-variant
> C names (which includes all non-variant QMP names), but does

What exactly do you mean by the parenthesis?

> not add to that array (QMP members of one branch do not cause
> collisions with other branches).  This means that there is
> one form of collision we still miss: when two branch names
> collide.  However, that will be dealt with in the next patch.

Well, it's already dealt with.  The next patch merely moves the dealing
into the .check().

> [*] Exception - the 'seen' array doesn't contain 'base', which
> is currently a non-variant C member of structs; but since
> structs don't contain variants, it doesn't hurt. Besides, a
> later patch will be unboxing structs the way flat unions
> have already been unboxed.
>
> The wording of several error messages has changed, but in many
> cases feels like an improvement rather than a regression.  The
> recent change (commit 7b2a5c2) to avoid an assertion failure
> when a flat union branch name collides with its discriminator
> name is also 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.

Only simple?

I've come to the conclusion that we should get rid of the self-inflicted
pain before we attempt to detect all collisions.

> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>