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

[John Snow]

On Thu, Nov 23, 2023 at 8:51 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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?
>

OK, I think this just needs a better commit message and comment, then.

--js