Re: [PATCH 14/19] qapi/schema: assert QAPISchemaVariants are QAPISchemaObjectType

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> I'm actually not too sure about this one, it seems to hold up at runtime
> but instead of lying and coming up with an elaborate ruse as a commit
> message I'm just going to admit I just cribbed my own notes from the
> last time I typed schema.py and I no longer remember why or if this is
> correct.
>
> Cool!
>
> With more seriousness, variants are only guaranteed to house a
> QAPISchemaType as per the definition of QAPISchemaObjectTypeMember but
> the only classes/types that have a check_clash method are descendents of
> QAPISchemaMember and the QAPISchemaVariants class itself.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/schema.py | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 476b19aed61..ce5b01b3182 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -717,6 +717,7 @@ def check_clash(self, info, seen):
>          for v in self.variants:
>              # Reset seen map for each variant, since qapi names from one
>              # branch do not affect another branch
> +            assert isinstance(v.type, QAPISchemaObjectType)  # I think, 
> anyway?
>              v.type.check_clash(info, dict(seen))

Have a look at .check() right above:

       def check(
           self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
       ) -> None:
           [...]
           if not self.variants:
               raise QAPISemError(self.info, "union has no branches")
           for v in self.variants:
               v.check(schema)
               # Union names must match enum values; alternate names are
               # checked separately. Use 'seen' to tell the two apart.
               if seen:
                   if v.name not in self.tag_member.type.member_names():
                       raise QAPISemError(
                           self.info,
                           "branch '%s' is not a value of %s"
                           % (v.name, self.tag_member.type.describe()))
--->               if not isinstance(v.type, QAPISchemaObjectType):
--->                   raise QAPISemError(
                           self.info,
                           "%s cannot use %s"
                           % (v.describe(self.info), v.type.describe()))
                   v.type.check(schema)

Since .check() runs before .check_clash(), your assertion holds.

Clearer now?