| Message ID | 20231116014350.653792-15-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
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?
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
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))
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(+)