| Message ID | 20231116014350.653792-14-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
John Snow <jsnow@redhat.com> writes: > There are two related changes here: > > (1) We need to perform type narrowing for resolving the type of > tag_member during check(), and > > (2) tag_member is a delayed initialization field, but we can hide it > behind a property that raises an Exception if it's called too > early. This simplifies the typing in quite a few places and avoids > needing to assert that the "tag_member is not None" at a dozen > callsites, which can be confusing and suggest the wrong thing to a > drive-by contributor. > > Signed-off-by: John Snow <jsnow@redhat.com> Without looking closely: review of PATCH 10 applies, doesn't it?
On Wed, Nov 22, 2023 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote: > > John Snow <jsnow@redhat.com> writes: > > > There are two related changes here: > > > > (1) We need to perform type narrowing for resolving the type of > > tag_member during check(), and > > > > (2) tag_member is a delayed initialization field, but we can hide it > > behind a property that raises an Exception if it's called too > > early. This simplifies the typing in quite a few places and avoids > > needing to assert that the "tag_member is not None" at a dozen > > callsites, which can be confusing and suggest the wrong thing to a > > drive-by contributor. > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > Without looking closely: review of PATCH 10 applies, doesn't it? > Yep!
On Wed, Nov 22, 2023 at 11:02 AM John Snow <jsnow@redhat.com> wrote: > > On Wed, Nov 22, 2023 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote: > > > > John Snow <jsnow@redhat.com> writes: > > > > > There are two related changes here: > > > > > > (1) We need to perform type narrowing for resolving the type of > > > tag_member during check(), and > > > > > > (2) tag_member is a delayed initialization field, but we can hide it > > > behind a property that raises an Exception if it's called too > > > early. This simplifies the typing in quite a few places and avoids > > > needing to assert that the "tag_member is not None" at a dozen > > > callsites, which can be confusing and suggest the wrong thing to a > > > drive-by contributor. > > > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > > > Without looking closely: review of PATCH 10 applies, doesn't it? > > > > Yep! Hm, actually, maybe not quite as cleanly. The problem is we *are* initializing that field immediately with whatever we were passed in during __init__, which means the field is indeed Optional. Later, during check(), we happen to eliminate that usage of None. To remove the use of the @property trick here, we could: ... declare the field, then only initialize it if we were passed a non-None value. But then check() would need to rely on something like hasattr to check if it was set or not, which is maybe an unfortunate code smell. So I think you'd still wind up needing a ._tag_member field which is Optional and always gets set during __init__, then setting a proper .tag_member field during check(). Or I could just leave this one as-is. Or something else. I think the dirt has to get swept somewhere, because we don't *always* have enough information to fully initialize it at __init__ time, it's a conditional delayed initialization, unlike the others which are unconditionally delayed. --js
John Snow <jsnow@redhat.com> writes: > On Wed, Nov 22, 2023 at 11:02 AM John Snow <jsnow@redhat.com> wrote: >> >> On Wed, Nov 22, 2023 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote: >> > >> > John Snow <jsnow@redhat.com> writes: >> > >> > > There are two related changes here: >> > > >> > > (1) We need to perform type narrowing for resolving the type of >> > > tag_member during check(), and >> > > >> > > (2) tag_member is a delayed initialization field, but we can hide it >> > > behind a property that raises an Exception if it's called too >> > > early. This simplifies the typing in quite a few places and avoids >> > > needing to assert that the "tag_member is not None" at a dozen >> > > callsites, which can be confusing and suggest the wrong thing to a >> > > drive-by contributor. >> > > >> > > Signed-off-by: John Snow <jsnow@redhat.com> >> > >> > Without looking closely: review of PATCH 10 applies, doesn't it? >> > >> >> Yep! > > Hm, actually, maybe not quite as cleanly. > > The problem is we *are* initializing that field immediately with > whatever we were passed in during __init__, which means the field is > indeed Optional. Later, during check(), we happen to eliminate that > usage of None. You're right. QAPISchemaVariants.__init__() takes @tag_name and @tag_member. Exactly one of them must be None. When creating a union's QAPISchemaVariants, it's tag_member, and when creating an alternate's, it's tag_name. Why? A union's tag is an ordinary member selected by name via 'discriminator': TAG_NAME. We can't resolve the name at this time, because it may be buried arbitrarily deep in the base type chain. An alternate's tag is an implicitly created "member" of type 'QType'. "Member" in scare-quotes, because is special: it exists in C, but not on the wire, and not in introspection. Historical note: simple unions also had an implictly created tag member, and its type was the implicit enum type enumerating the branches. So _def_union_type() passes TAG_NAME to .__init__(), and _def_alternate_type() creates and passes the implicit tag member. Hardly elegant, but it works. > To remove the use of the @property trick here, we could: > > ... declare the field, then only initialize it if we were passed a > non-None value. But then check() would need to rely on something like > hasattr to check if it was set or not, which is maybe an unfortunate > code smell. > So I think you'd still wind up needing a ._tag_member field which is > Optional and always gets set during __init__, then setting a proper > .tag_member field during check(). > > Or I could just leave this one as-is. Or something else. I think the > dirt has to get swept somewhere, because we don't *always* have enough > information to fully initialize it at __init__ time, it's a > conditional delayed initialization, unlike the others which are > unconditionally delayed. Yes. Here's a possible "something else": 1. Drop parameter .__init__() parameter @tag_member, and leave .tag_member unset there. 2. Set .tag_member in .check(): if .tag_name, look up that member (no change). Else, it's an alternate; create the alternate's implicit tag member. Drawback: before, we create AST in just one place, namely QAPISchema._def_exprs(). Now we also create some in .check(). Here's another "something else": 1. Fuse parameters .__init__() @tag_member and @tag_name. The type becomes Union. Store for .check(). 2. Set .tag_member in .check(): if we stored a name, look up that member, else we must have stored an implicit member, so use that. 3. We check "is this a union?" like if self._tag_name. Needs adjustment. Feels a bit awkward to me. We can also do nothing, as you said. We don't *have* to express ".check() resolves unresolved tag member" in the type system. We can just live with .tag_member remaining Optional. Differently awkward, I guess. Thoughts?
On Wed, Jan 10, 2024 at 2:53 AM Markus Armbruster <armbru@redhat.com> wrote: > > John Snow <jsnow@redhat.com> writes: > > > On Wed, Nov 22, 2023 at 11:02 AM John Snow <jsnow@redhat.com> wrote: > >> > >> On Wed, Nov 22, 2023 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote: > >> > > >> > John Snow <jsnow@redhat.com> writes: > >> > > >> > > There are two related changes here: > >> > > > >> > > (1) We need to perform type narrowing for resolving the type of > >> > > tag_member during check(), and > >> > > > >> > > (2) tag_member is a delayed initialization field, but we can hide it > >> > > behind a property that raises an Exception if it's called too > >> > > early. This simplifies the typing in quite a few places and avoids > >> > > needing to assert that the "tag_member is not None" at a dozen > >> > > callsites, which can be confusing and suggest the wrong thing to a > >> > > drive-by contributor. > >> > > > >> > > Signed-off-by: John Snow <jsnow@redhat.com> > >> > > >> > Without looking closely: review of PATCH 10 applies, doesn't it? > >> > > >> > >> Yep! > > > > Hm, actually, maybe not quite as cleanly. > > > > The problem is we *are* initializing that field immediately with > > whatever we were passed in during __init__, which means the field is > > indeed Optional. Later, during check(), we happen to eliminate that > > usage of None. > > You're right. > > QAPISchemaVariants.__init__() takes @tag_name and @tag_member. Exactly > one of them must be None. When creating a union's QAPISchemaVariants, > it's tag_member, and when creating an alternate's, it's tag_name. > > Why? > > A union's tag is an ordinary member selected by name via > 'discriminator': TAG_NAME. We can't resolve the name at this time, > because it may be buried arbitrarily deep in the base type chain. > > An alternate's tag is an implicitly created "member" of type 'QType'. > "Member" in scare-quotes, because is special: it exists in C, but not on > the wire, and not in introspection. > > Historical note: simple unions also had an implictly created tag member, > and its type was the implicit enum type enumerating the branches. > > So _def_union_type() passes TAG_NAME to .__init__(), and > _def_alternate_type() creates and passes the implicit tag member. > Hardly elegant, but it works. > > > To remove the use of the @property trick here, we could: > > > > ... declare the field, then only initialize it if we were passed a > > non-None value. But then check() would need to rely on something like > > hasattr to check if it was set or not, which is maybe an unfortunate > > code smell. > > So I think you'd still wind up needing a ._tag_member field which is > > Optional and always gets set during __init__, then setting a proper > > .tag_member field during check(). > > > > Or I could just leave this one as-is. Or something else. I think the > > dirt has to get swept somewhere, because we don't *always* have enough > > information to fully initialize it at __init__ time, it's a > > conditional delayed initialization, unlike the others which are > > unconditionally delayed. > > Yes. > > Here's a possible "something else": > > 1. Drop parameter .__init__() parameter @tag_member, and leave > .tag_member unset there. > > 2. Set .tag_member in .check(): if .tag_name, look up that member (no > change). Else, it's an alternate; create the alternate's implicit tag > member. > > Drawback: before, we create AST in just one place, namely > QAPISchema._def_exprs(). Now we also create some in .check(). I suppose I don't have a concrete argument against this beyond "It doesn't seem prettier than using the @property getter." > > Here's another "something else": > > 1. Fuse parameters .__init__() @tag_member and @tag_name. The type > becomes Union. Store for .check(). > > 2. Set .tag_member in .check(): if we stored a name, look up that > member, else we must have stored an implicit member, so use that. > > 3. We check "is this a union?" like if self._tag_name. Needs > adjustment. > > Feels a bit awkward to me. Yeah, a little. Mechanically simple, though, I think. > > We can also do nothing, as you said. We don't *have* to express > ".check() resolves unresolved tag member" in the type system. We can > just live with .tag_member remaining Optional. This is the only option I'm sure I don't want - it's misleading to users of the API for the purposes of new generators using a fully realized schema object. I think it's important to remove Optional[] where possible to avoid the question "When will this be set to None?" if the answer is just "For your purposes, never." It's an implementation detail of object initialization leaking out. (Also, I just counted and leaving the field as Optional adds 22 new type errors; that's a lot of callsites to bandage with new conditions. nah.) The *other* way to not do anything is to just leave the @property solution in O:-) > > Differently awkward, I guess. > > Thoughts? Partial to the getter, unless option 1 or 2 leads to simplification of the check() code, which I haven't really experimented with. If that's something you'd really rather avoid, I might ask for you to decide on your preferred alternative - I don't have strong feelings between 'em. (Not helpful, oops. Thanks for your feedback and review, though. You've successfully talked me down to a much smaller series over time :p) --js
John Snow <jsnow@redhat.com> writes: > On Wed, Jan 10, 2024 at 2:53 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> John Snow <jsnow@redhat.com> writes: >> >> > On Wed, Nov 22, 2023 at 11:02 AM John Snow <jsnow@redhat.com> wrote: >> >> >> >> On Wed, Nov 22, 2023 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> > >> >> > John Snow <jsnow@redhat.com> writes: >> >> > >> >> > > There are two related changes here: >> >> > > >> >> > > (1) We need to perform type narrowing for resolving the type of >> >> > > tag_member during check(), and >> >> > > >> >> > > (2) tag_member is a delayed initialization field, but we can hide it >> >> > > behind a property that raises an Exception if it's called too >> >> > > early. This simplifies the typing in quite a few places and avoids >> >> > > needing to assert that the "tag_member is not None" at a dozen >> >> > > callsites, which can be confusing and suggest the wrong thing to a >> >> > > drive-by contributor. >> >> > > >> >> > > Signed-off-by: John Snow <jsnow@redhat.com> >> >> > >> >> > Without looking closely: review of PATCH 10 applies, doesn't it? >> >> > >> >> >> >> Yep! >> > >> > Hm, actually, maybe not quite as cleanly. >> > >> > The problem is we *are* initializing that field immediately with >> > whatever we were passed in during __init__, which means the field is >> > indeed Optional. Later, during check(), we happen to eliminate that >> > usage of None. >> >> You're right. >> >> QAPISchemaVariants.__init__() takes @tag_name and @tag_member. Exactly >> one of them must be None. When creating a union's QAPISchemaVariants, >> it's tag_member, and when creating an alternate's, it's tag_name. >> >> Why? >> >> A union's tag is an ordinary member selected by name via >> 'discriminator': TAG_NAME. We can't resolve the name at this time, >> because it may be buried arbitrarily deep in the base type chain. >> >> An alternate's tag is an implicitly created "member" of type 'QType'. >> "Member" in scare-quotes, because is special: it exists in C, but not on >> the wire, and not in introspection. >> >> Historical note: simple unions also had an implictly created tag member, >> and its type was the implicit enum type enumerating the branches. >> >> So _def_union_type() passes TAG_NAME to .__init__(), and >> _def_alternate_type() creates and passes the implicit tag member. >> Hardly elegant, but it works. >> >> > To remove the use of the @property trick here, we could: >> > >> > ... declare the field, then only initialize it if we were passed a >> > non-None value. But then check() would need to rely on something like >> > hasattr to check if it was set or not, which is maybe an unfortunate >> > code smell. >> > So I think you'd still wind up needing a ._tag_member field which is >> > Optional and always gets set during __init__, then setting a proper >> > .tag_member field during check(). >> > >> > Or I could just leave this one as-is. Or something else. I think the >> > dirt has to get swept somewhere, because we don't *always* have enough >> > information to fully initialize it at __init__ time, it's a >> > conditional delayed initialization, unlike the others which are >> > unconditionally delayed. >> >> Yes. >> >> Here's a possible "something else": >> >> 1. Drop parameter .__init__() parameter @tag_member, and leave >> .tag_member unset there. >> >> 2. Set .tag_member in .check(): if .tag_name, look up that member (no >> change). Else, it's an alternate; create the alternate's implicit tag >> member. >> >> Drawback: before, we create AST in just one place, namely >> QAPISchema._def_exprs(). Now we also create some in .check(). > > I suppose I don't have a concrete argument against this beyond "It > doesn't seem prettier than using the @property getter." > >> >> Here's another "something else": >> >> 1. Fuse parameters .__init__() @tag_member and @tag_name. The type >> becomes Union. Store for .check(). >> >> 2. Set .tag_member in .check(): if we stored a name, look up that >> member, else we must have stored an implicit member, so use that. >> >> 3. We check "is this a union?" like if self._tag_name. Needs >> adjustment. >> >> Feels a bit awkward to me. > > Yeah, a little. Mechanically simple, though, I think. > >> We can also do nothing, as you said. We don't *have* to express >> ".check() resolves unresolved tag member" in the type system. We can >> just live with .tag_member remaining Optional. > > This is the only option I'm sure I don't want - it's misleading to > users of the API for the purposes of new generators using a fully > realized schema object. I think it's important to remove Optional[] > where possible to avoid the question "When will this be set to None?" > if the answer is just "For your purposes, never." > > It's an implementation detail of object initialization leaking out. > > (Also, I just counted and leaving the field as Optional adds 22 new > type errors; that's a lot of callsites to bandage with new conditions. > nah.) > > The *other* way to not do anything is to just leave the @property > solution in O:-) > >> >> Differently awkward, I guess. >> >> Thoughts? > > Partial to the getter, unless option 1 or 2 leads to simplification of > the check() code, Put a pin into that. > which I haven't really experimented with. If that's > something you'd really rather avoid, I might ask for you to decide on > your preferred alternative - I don't have strong feelings between 'em. All the solutions so far give me slight "there has to be a better way" vibes. Staring at QAPISchemaVariants, I realized: more than half of the actual code are under "if union / else" conditionals. So I tried the more object-oriented solution: classes instead of conditionals. Diff appended. Shedding the conditionals does lead to slightly simple .check(), so maybe you like it. Once this is done, we can narrow .tag_member's type from Optional[QAPISchemaObjectTypeMember] to QAPISchemaObjectTypeMember the exact same way as QAPISchemaObjectTypeMember.type: replace self.tag_member = None by self.tag_member: QAPISchemaObjectTypeMember. Admittedly a bit more churn that your solution, but the result has slightly less code, and feels slitghly cleaner to me. Thoughts? > (Not helpful, oops. Thanks for your feedback and review, though. > You've successfully talked me down to a much smaller series over time > :p) diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index c38df61a6d..e5cea8004e 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -26,6 +26,7 @@ from .gen import QAPISchemaMonolithicCVisitor from .schema import ( QAPISchema, + QAPISchemaAlternatives, QAPISchemaArrayType, QAPISchemaBuiltinType, QAPISchemaEntity, @@ -343,12 +344,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo], def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo], ifcond: QAPISchemaIfCond, features: List[QAPISchemaFeature], - variants: QAPISchemaVariants) -> None: + alternatives: QAPISchemaAlternatives) -> None: self._gen_tree( name, 'alternate', {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond) - for m in variants.variants]}, + for m in alternatives.variants]}, ifcond, features ) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 0d9a70ab4c..949ee6bfd4 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -563,8 +563,7 @@ class QAPISchemaAlternateType(QAPISchemaType): def __init__(self, name, info, doc, ifcond, features, variants): super().__init__(name, info, doc, ifcond, features) - assert isinstance(variants, QAPISchemaVariants) - assert variants.tag_member + assert isinstance(variants, QAPISchemaAlternatives) variants.set_defined_in(name) variants.tag_member.set_defined_in(self.name) self.variants = variants @@ -625,19 +624,12 @@ def visit(self, visitor): self.name, self.info, self.ifcond, self.features, self.variants) -class QAPISchemaVariants: - def __init__(self, tag_name, info, tag_member, variants): - # Unions pass tag_name but not tag_member. - # Alternates pass tag_member but not tag_name. - # After check(), tag_member is always set. - assert bool(tag_member) != bool(tag_name) - assert (isinstance(tag_name, str) or - isinstance(tag_member, QAPISchemaObjectTypeMember)) +class QAPISchemaVariantsBase: + def __init__(self, info, variants): for v in variants: assert isinstance(v, QAPISchemaVariant) - self._tag_name = tag_name self.info = info - self.tag_member = tag_member + self.tag_member = None self.variants = variants def set_defined_in(self, name): @@ -645,48 +637,6 @@ def set_defined_in(self, name): v.set_defined_in(name) def check(self, schema, seen): - if self._tag_name: # union - self.tag_member = seen.get(c_name(self._tag_name)) - base = "'base'" - # Pointing to the base type when not implicit would be - # nice, but we don't know it here - if not self.tag_member or self._tag_name != self.tag_member.name: - raise QAPISemError( - self.info, - "discriminator '%s' is not a member of %s" - % (self._tag_name, base)) - # Here we do: - base_type = schema.resolve_type(self.tag_member.defined_in) - if not base_type.is_implicit(): - base = "base type '%s'" % self.tag_member.defined_in - if not isinstance(self.tag_member.type, QAPISchemaEnumType): - raise QAPISemError( - self.info, - "discriminator member '%s' of %s must be of enum type" - % (self._tag_name, base)) - if self.tag_member.optional: - raise QAPISemError( - self.info, - "discriminator member '%s' of %s must not be optional" - % (self._tag_name, base)) - if self.tag_member.ifcond.is_present(): - raise QAPISemError( - self.info, - "discriminator member '%s' of %s must not be conditional" - % (self._tag_name, base)) - else: # alternate - assert isinstance(self.tag_member.type, QAPISchemaEnumType) - assert not self.tag_member.optional - assert not self.tag_member.ifcond.is_present() - if self._tag_name: # union - # branches that are not explicitly covered get an empty type - cases = {v.name for v in self.variants} - for m in self.tag_member.type.members: - if m.name not in cases: - v = QAPISchemaVariant(m.name, self.info, - 'q_empty', m.ifcond) - v.set_defined_in(self.tag_member.defined_in) - self.variants.append(v) if not self.variants: raise QAPISemError(self.info, "union has no branches") for v in self.variants: @@ -713,6 +663,65 @@ def check_clash(self, info, seen): v.type.check_clash(info, dict(seen)) +class QAPISchemaVariants(QAPISchemaVariantsBase): + def __init__(self, info, variants, tag_name): + assert isinstance(tag_name, str) + super().__init__(info, variants) + self._tag_name = tag_name + + def check(self, schema, seen): + self.tag_member = seen.get(c_name(self._tag_name)) + base = "'base'" + # Pointing to the base type when not implicit would be + # nice, but we don't know it here + if not self.tag_member or self._tag_name != self.tag_member.name: + raise QAPISemError( + self.info, + "discriminator '%s' is not a member of %s" + % (self._tag_name, base)) + # Here we do: + base_type = schema.resolve_type(self.tag_member.defined_in) + if not base_type.is_implicit(): + base = "base type '%s'" % self.tag_member.defined_in + if not isinstance(self.tag_member.type, QAPISchemaEnumType): + raise QAPISemError( + self.info, + "discriminator member '%s' of %s must be of enum type" + % (self._tag_name, base)) + if self.tag_member.optional: + raise QAPISemError( + self.info, + "discriminator member '%s' of %s must not be optional" + % (self._tag_name, base)) + if self.tag_member.ifcond.is_present(): + raise QAPISemError( + self.info, + "discriminator member '%s' of %s must not be conditional" + % (self._tag_name, base)) + # branches that are not explicitly covered get an empty type + cases = {v.name for v in self.variants} + for m in self.tag_member.type.members: + if m.name not in cases: + v = QAPISchemaVariant(m.name, self.info, + 'q_empty', m.ifcond) + v.set_defined_in(self.tag_member.defined_in) + self.variants.append(v) + super().check(schema, seen) + + +class QAPISchemaAlternatives(QAPISchemaVariantsBase): + def __init__(self, info, variants, tag_member): + assert isinstance(tag_member, QAPISchemaObjectTypeMember) + super().__init__(info, variants) + self.tag_member = tag_member + + def check(self, schema, seen): + assert isinstance(self.tag_member.type, QAPISchemaEnumType) + assert not self.tag_member.optional + assert not self.tag_member.ifcond.is_present() + super().check(schema, seen) + + class QAPISchemaMember: """ Represents object members, enum members and features """ role = 'member' @@ -1184,7 +1193,7 @@ def _def_union_type(self, expr: QAPIExpression): QAPISchemaObjectType(name, info, expr.doc, ifcond, features, base, members, QAPISchemaVariants( - tag_name, info, None, variants))) + info, variants, tag_name))) def _def_alternate_type(self, expr: QAPIExpression): name = expr['alternate'] @@ -1202,7 +1211,7 @@ def _def_alternate_type(self, expr: QAPIExpression): self._def_definition( QAPISchemaAlternateType( name, info, expr.doc, ifcond, features, - QAPISchemaVariants(None, info, tag_member, variants))) + QAPISchemaAlternatives(info, variants, tag_member))) def _def_command(self, expr: QAPIExpression): name = expr['command'] diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index c39d054d2c..05da30b855 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -23,6 +23,7 @@ ) from .schema import ( QAPISchema, + QAPISchemaAlternatives, QAPISchemaEnumMember, QAPISchemaFeature, QAPISchemaIfCond, @@ -369,11 +370,11 @@ def visit_alternate_type(self, info: Optional[QAPISourceInfo], ifcond: QAPISchemaIfCond, features: List[QAPISchemaFeature], - variants: QAPISchemaVariants) -> None: + alternatives: QAPISchemaAlternatives) -> None: with ifcontext(ifcond, self._genh): self._genh.preamble_add(gen_fwd_object_or_array(name)) self._genh.add(gen_object(name, ifcond, None, - [variants.tag_member], variants)) + [alternatives.tag_member], alternatives)) with ifcontext(ifcond, self._genh, self._genc): self._gen_type_cleanup(name) diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index c56ea4d724..725bfcef50 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -28,6 +28,7 @@ ) from .schema import ( QAPISchema, + QAPISchemaAlternatives, QAPISchemaEnumMember, QAPISchemaEnumType, QAPISchemaFeature, @@ -222,7 +223,8 @@ def gen_visit_enum(name: str) -> str: c_name=c_name(name)) -def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str: +def gen_visit_alternate(name: str, + alternatives: QAPISchemaAlternatives) -> str: ret = mcgen(''' bool visit_type_%(c_name)s(Visitor *v, const char *name, @@ -244,7 +246,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str: ''', c_name=c_name(name)) - for var in variants.variants: + for var in alternatives.variants: ret += var.ifcond.gen_if() ret += mcgen(''' case %(case)s: @@ -414,10 +416,10 @@ def visit_alternate_type(self, info: Optional[QAPISourceInfo], ifcond: QAPISchemaIfCond, features: List[QAPISchemaFeature], - variants: QAPISchemaVariants) -> None: + alternatives: QAPISchemaAlternatives) -> None: with ifcontext(ifcond, self._genh, self._genc): self._genh.add(gen_visit_decl(name)) - self._genc.add(gen_visit_alternate(name, variants)) + self._genc.add(gen_visit_alternate(name, alternatives)) def gen_visit(schema: QAPISchema,
Hmm, there's more union-specific code to move out of the base. Revised
patch:
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 658c288f8f..4a2e62d919 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -328,7 +328,8 @@ def visit_object_type(self, name, info, ifcond, features,
+ self._nodes_for_sections(doc)
+ self._nodes_for_if_section(ifcond))
- def visit_alternate_type(self, name, info, ifcond, features, variants):
+ def visit_alternate_type(self, name, info, ifcond, features,
+ alternatives):
doc = self._cur_doc
self._add_doc('Alternate',
self._nodes_for_members(doc, 'Members')
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index c38df61a6d..e5cea8004e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -26,6 +26,7 @@
from .gen import QAPISchemaMonolithicCVisitor
from .schema import (
QAPISchema,
+ QAPISchemaAlternatives,
QAPISchemaArrayType,
QAPISchemaBuiltinType,
QAPISchemaEntity,
@@ -343,12 +344,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
ifcond: QAPISchemaIfCond,
features: List[QAPISchemaFeature],
- variants: QAPISchemaVariants) -> None:
+ alternatives: QAPISchemaAlternatives) -> None:
self._gen_tree(
name, 'alternate',
{'members': [Annotated({'type': self._use_type(m.type)},
m.ifcond)
- for m in variants.variants]},
+ for m in alternatives.variants]},
ifcond, features
)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 0d9a70ab4c..f64e337ba2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -187,7 +187,8 @@ def visit_object_type_flat(self, name, info, ifcond, features,
members, variants):
pass
- def visit_alternate_type(self, name, info, ifcond, features, variants):
+ def visit_alternate_type(self, name, info, ifcond, features,
+ alternatives):
pass
def visit_command(self, name, info, ifcond, features,
@@ -563,8 +564,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
def __init__(self, name, info, doc, ifcond, features, variants):
super().__init__(name, info, doc, ifcond, features)
- assert isinstance(variants, QAPISchemaVariants)
- assert variants.tag_member
+ assert isinstance(variants, QAPISchemaAlternatives)
variants.set_defined_in(name)
variants.tag_member.set_defined_in(self.name)
self.variants = variants
@@ -625,19 +625,12 @@ def visit(self, visitor):
self.name, self.info, self.ifcond, self.features, self.variants)
-class QAPISchemaVariants:
- def __init__(self, tag_name, info, tag_member, variants):
- # Unions pass tag_name but not tag_member.
- # Alternates pass tag_member but not tag_name.
- # After check(), tag_member is always set.
- assert bool(tag_member) != bool(tag_name)
- assert (isinstance(tag_name, str) or
- isinstance(tag_member, QAPISchemaObjectTypeMember))
+class QAPISchemaVariantsBase:
+ def __init__(self, info, variants):
for v in variants:
assert isinstance(v, QAPISchemaVariant)
- self._tag_name = tag_name
self.info = info
- self.tag_member = tag_member
+ self.tag_member = None
self.variants = variants
def set_defined_in(self, name):
@@ -645,66 +638,8 @@ def set_defined_in(self, name):
v.set_defined_in(name)
def check(self, schema, seen):
- if self._tag_name: # union
- self.tag_member = seen.get(c_name(self._tag_name))
- base = "'base'"
- # Pointing to the base type when not implicit would be
- # nice, but we don't know it here
- if not self.tag_member or self._tag_name != self.tag_member.name:
- raise QAPISemError(
- self.info,
- "discriminator '%s' is not a member of %s"
- % (self._tag_name, base))
- # Here we do:
- base_type = schema.resolve_type(self.tag_member.defined_in)
- if not base_type.is_implicit():
- base = "base type '%s'" % self.tag_member.defined_in
- if not isinstance(self.tag_member.type, QAPISchemaEnumType):
- raise QAPISemError(
- self.info,
- "discriminator member '%s' of %s must be of enum type"
- % (self._tag_name, base))
- if self.tag_member.optional:
- raise QAPISemError(
- self.info,
- "discriminator member '%s' of %s must not be optional"
- % (self._tag_name, base))
- if self.tag_member.ifcond.is_present():
- raise QAPISemError(
- self.info,
- "discriminator member '%s' of %s must not be conditional"
- % (self._tag_name, base))
- else: # alternate
- assert isinstance(self.tag_member.type, QAPISchemaEnumType)
- assert not self.tag_member.optional
- assert not self.tag_member.ifcond.is_present()
- if self._tag_name: # union
- # branches that are not explicitly covered get an empty type
- cases = {v.name for v in self.variants}
- for m in self.tag_member.type.members:
- if m.name not in cases:
- v = QAPISchemaVariant(m.name, self.info,
- 'q_empty', m.ifcond)
- v.set_defined_in(self.tag_member.defined_in)
- self.variants.append(v)
- 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)
def check_clash(self, info, seen):
for v in self.variants:
@@ -713,6 +648,79 @@ def check_clash(self, info, seen):
v.type.check_clash(info, dict(seen))
+class QAPISchemaVariants(QAPISchemaVariantsBase):
+ def __init__(self, info, variants, tag_name):
+ assert isinstance(tag_name, str)
+ super().__init__(info, variants)
+ self._tag_name = tag_name
+
+ def check(self, schema, seen):
+ self.tag_member = seen.get(c_name(self._tag_name))
+ base = "'base'"
+ # Pointing to the base type when not implicit would be
+ # nice, but we don't know it here
+ if not self.tag_member or self._tag_name != self.tag_member.name:
+ raise QAPISemError(
+ self.info,
+ "discriminator '%s' is not a member of %s"
+ % (self._tag_name, base))
+ # Here we do:
+ base_type = schema.resolve_type(self.tag_member.defined_in)
+ if not base_type.is_implicit():
+ base = "base type '%s'" % self.tag_member.defined_in
+ if not isinstance(self.tag_member.type, QAPISchemaEnumType):
+ raise QAPISemError(
+ self.info,
+ "discriminator member '%s' of %s must be of enum type"
+ % (self._tag_name, base))
+ if self.tag_member.optional:
+ raise QAPISemError(
+ self.info,
+ "discriminator member '%s' of %s must not be optional"
+ % (self._tag_name, base))
+ if self.tag_member.ifcond.is_present():
+ raise QAPISemError(
+ self.info,
+ "discriminator member '%s' of %s must not be conditional"
+ % (self._tag_name, base))
+ # branches that are not explicitly covered get an empty type
+ cases = {v.name for v in self.variants}
+ for m in self.tag_member.type.members:
+ if m.name not in cases:
+ v = QAPISchemaVariant(m.name, self.info,
+ 'q_empty', m.ifcond)
+ v.set_defined_in(self.tag_member.defined_in)
+ self.variants.append(v)
+ if not self.variants:
+ raise QAPISemError(self.info, "union has no branches")
+ super().check(schema, seen)
+ for v in self.variants:
+ 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)
+
+
+class QAPISchemaAlternatives(QAPISchemaVariantsBase):
+ def __init__(self, info, variants, tag_member):
+ assert isinstance(tag_member, QAPISchemaObjectTypeMember)
+ super().__init__(info, variants)
+ self.tag_member = tag_member
+
+ def check(self, schema, seen):
+ super().check(schema, seen)
+ assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+ assert not self.tag_member.optional
+ assert not self.tag_member.ifcond.is_present()
+
+
class QAPISchemaMember:
""" Represents object members, enum members and features """
role = 'member'
@@ -1184,7 +1192,7 @@ def _def_union_type(self, expr: QAPIExpression):
QAPISchemaObjectType(name, info, expr.doc, ifcond, features,
base, members,
QAPISchemaVariants(
- tag_name, info, None, variants)))
+ info, variants, tag_name)))
def _def_alternate_type(self, expr: QAPIExpression):
name = expr['alternate']
@@ -1202,7 +1210,7 @@ def _def_alternate_type(self, expr: QAPIExpression):
self._def_definition(
QAPISchemaAlternateType(
name, info, expr.doc, ifcond, features,
- QAPISchemaVariants(None, info, tag_member, variants)))
+ QAPISchemaAlternatives(info, variants, tag_member)))
def _def_command(self, expr: QAPIExpression):
name = expr['command']
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index c39d054d2c..05da30b855 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -23,6 +23,7 @@
)
from .schema import (
QAPISchema,
+ QAPISchemaAlternatives,
QAPISchemaEnumMember,
QAPISchemaFeature,
QAPISchemaIfCond,
@@ -369,11 +370,11 @@ def visit_alternate_type(self,
info: Optional[QAPISourceInfo],
ifcond: QAPISchemaIfCond,
features: List[QAPISchemaFeature],
- variants: QAPISchemaVariants) -> None:
+ alternatives: QAPISchemaAlternatives) -> None:
with ifcontext(ifcond, self._genh):
self._genh.preamble_add(gen_fwd_object_or_array(name))
self._genh.add(gen_object(name, ifcond, None,
- [variants.tag_member], variants))
+ [alternatives.tag_member], alternatives))
with ifcontext(ifcond, self._genh, self._genc):
self._gen_type_cleanup(name)
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index c56ea4d724..725bfcef50 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -28,6 +28,7 @@
)
from .schema import (
QAPISchema,
+ QAPISchemaAlternatives,
QAPISchemaEnumMember,
QAPISchemaEnumType,
QAPISchemaFeature,
@@ -222,7 +223,8 @@ def gen_visit_enum(name: str) -> str:
c_name=c_name(name))
-def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
+def gen_visit_alternate(name: str,
+ alternatives: QAPISchemaAlternatives) -> str:
ret = mcgen('''
bool visit_type_%(c_name)s(Visitor *v, const char *name,
@@ -244,7 +246,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
''',
c_name=c_name(name))
- for var in variants.variants:
+ for var in alternatives.variants:
ret += var.ifcond.gen_if()
ret += mcgen('''
case %(case)s:
@@ -414,10 +416,10 @@ def visit_alternate_type(self,
info: Optional[QAPISourceInfo],
ifcond: QAPISchemaIfCond,
features: List[QAPISchemaFeature],
- variants: QAPISchemaVariants) -> None:
+ alternatives: QAPISchemaAlternatives) -> None:
with ifcontext(ifcond, self._genh, self._genc):
self._genh.add(gen_visit_decl(name))
- self._genc.add(gen_visit_alternate(name, variants))
+ self._genc.add(gen_visit_alternate(name, alternatives))
def gen_visit(schema: QAPISchema,
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 14f7b62a44..b66ceb81b8 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -61,9 +61,10 @@ def visit_object_type(self, name, info, ifcond, features,
self._print_if(ifcond)
self._print_features(features)
- def visit_alternate_type(self, name, info, ifcond, features, variants):
+ def visit_alternate_type(self, name, info, ifcond, features,
+ alternatives):
print('alternate %s' % name)
- self._print_variants(variants)
+ self._print_variants(alternatives)
self._print_if(ifcond)
self._print_features(features)
Still more...
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 658c288f8f..4a2e62d919 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -328,7 +328,8 @@ def visit_object_type(self, name, info, ifcond, features,
+ self._nodes_for_sections(doc)
+ self._nodes_for_if_section(ifcond))
- def visit_alternate_type(self, name, info, ifcond, features, variants):
+ def visit_alternate_type(self, name, info, ifcond, features,
+ alternatives):
doc = self._cur_doc
self._add_doc('Alternate',
self._nodes_for_members(doc, 'Members')
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index c38df61a6d..e5cea8004e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -26,6 +26,7 @@
from .gen import QAPISchemaMonolithicCVisitor
from .schema import (
QAPISchema,
+ QAPISchemaAlternatives,
QAPISchemaArrayType,
QAPISchemaBuiltinType,
QAPISchemaEntity,
@@ -343,12 +344,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
ifcond: QAPISchemaIfCond,
features: List[QAPISchemaFeature],
- variants: QAPISchemaVariants) -> None:
+ alternatives: QAPISchemaAlternatives) -> None:
self._gen_tree(
name, 'alternate',
{'members': [Annotated({'type': self._use_type(m.type)},
m.ifcond)
- for m in variants.variants]},
+ for m in alternatives.variants]},
ifcond, features
)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 0d9a70ab4c..f18aac7199 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -187,7 +187,8 @@ def visit_object_type_flat(self, name, info, ifcond, features,
members, variants):
pass
- def visit_alternate_type(self, name, info, ifcond, features, variants):
+ def visit_alternate_type(self, name, info, ifcond, features,
+ alternatives):
pass
def visit_command(self, name, info, ifcond, features,
@@ -563,8 +564,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
def __init__(self, name, info, doc, ifcond, features, variants):
super().__init__(name, info, doc, ifcond, features)
- assert isinstance(variants, QAPISchemaVariants)
- assert variants.tag_member
+ assert isinstance(variants, QAPISchemaAlternatives)
variants.set_defined_in(name)
variants.tag_member.set_defined_in(self.name)
self.variants = variants
@@ -625,19 +625,12 @@ def visit(self, visitor):
self.name, self.info, self.ifcond, self.features, self.variants)
-class QAPISchemaVariants:
- def __init__(self, tag_name, info, tag_member, variants):
- # Unions pass tag_name but not tag_member.
- # Alternates pass tag_member but not tag_name.
- # After check(), tag_member is always set.
- assert bool(tag_member) != bool(tag_name)
- assert (isinstance(tag_name, str) or
- isinstance(tag_member, QAPISchemaObjectTypeMember))
+class QAPISchemaVariantsBase:
+ def __init__(self, info, variants):
for v in variants:
assert isinstance(v, QAPISchemaVariant)
- self._tag_name = tag_name
self.info = info
- self.tag_member = tag_member
+ self.tag_member = None
self.variants = variants
def set_defined_in(self, name):
@@ -645,66 +638,68 @@ def set_defined_in(self, name):
v.set_defined_in(name)
def check(self, schema, seen):
- if self._tag_name: # union
- self.tag_member = seen.get(c_name(self._tag_name))
- base = "'base'"
- # Pointing to the base type when not implicit would be
- # nice, but we don't know it here
- if not self.tag_member or self._tag_name != self.tag_member.name:
- raise QAPISemError(
- self.info,
- "discriminator '%s' is not a member of %s"
- % (self._tag_name, base))
- # Here we do:
- base_type = schema.resolve_type(self.tag_member.defined_in)
- if not base_type.is_implicit():
- base = "base type '%s'" % self.tag_member.defined_in
- if not isinstance(self.tag_member.type, QAPISchemaEnumType):
- raise QAPISemError(
- self.info,
- "discriminator member '%s' of %s must be of enum type"
- % (self._tag_name, base))
- if self.tag_member.optional:
- raise QAPISemError(
- self.info,
- "discriminator member '%s' of %s must not be optional"
- % (self._tag_name, base))
- if self.tag_member.ifcond.is_present():
- raise QAPISemError(
- self.info,
- "discriminator member '%s' of %s must not be conditional"
- % (self._tag_name, base))
- else: # alternate
- assert isinstance(self.tag_member.type, QAPISchemaEnumType)
- assert not self.tag_member.optional
- assert not self.tag_member.ifcond.is_present()
- if self._tag_name: # union
- # branches that are not explicitly covered get an empty type
- cases = {v.name for v in self.variants}
- for m in self.tag_member.type.members:
- if m.name not in cases:
- v = QAPISchemaVariant(m.name, self.info,
- 'q_empty', m.ifcond)
- v.set_defined_in(self.tag_member.defined_in)
- self.variants.append(v)
- 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)
+
+
+class QAPISchemaVariants(QAPISchemaVariantsBase):
+ def __init__(self, info, variants, tag_name):
+ assert isinstance(tag_name, str)
+ super().__init__(info, variants)
+ self._tag_name = tag_name
+
+ def check(self, schema, seen):
+ self.tag_member = seen.get(c_name(self._tag_name))
+ base = "'base'"
+ # Pointing to the base type when not implicit would be
+ # nice, but we don't know it here
+ if not self.tag_member or self._tag_name != self.tag_member.name:
+ raise QAPISemError(
+ self.info,
+ "discriminator '%s' is not a member of %s"
+ % (self._tag_name, base))
+ # Here we do:
+ base_type = schema.resolve_type(self.tag_member.defined_in)
+ if not base_type.is_implicit():
+ base = "base type '%s'" % self.tag_member.defined_in
+ if not isinstance(self.tag_member.type, QAPISchemaEnumType):
+ raise QAPISemError(
+ self.info,
+ "discriminator member '%s' of %s must be of enum type"
+ % (self._tag_name, base))
+ if self.tag_member.optional:
+ raise QAPISemError(
+ self.info,
+ "discriminator member '%s' of %s must not be optional"
+ % (self._tag_name, base))
+ if self.tag_member.ifcond.is_present():
+ raise QAPISemError(
+ self.info,
+ "discriminator member '%s' of %s must not be conditional"
+ % (self._tag_name, base))
+ # branches that are not explicitly covered get an empty type
+ cases = {v.name for v in self.variants}
+ for m in self.tag_member.type.members:
+ if m.name not in cases:
+ v = QAPISchemaVariant(m.name, self.info,
+ 'q_empty', m.ifcond)
+ v.set_defined_in(self.tag_member.defined_in)
+ self.variants.append(v)
+ if not self.variants:
+ raise QAPISemError(self.info, "union has no branches")
+ super().check(schema, seen)
+ for v in self.variants:
+ 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)
def check_clash(self, info, seen):
for v in self.variants:
@@ -713,6 +708,19 @@ def check_clash(self, info, seen):
v.type.check_clash(info, dict(seen))
+class QAPISchemaAlternatives(QAPISchemaVariantsBase):
+ def __init__(self, info, variants, tag_member):
+ assert isinstance(tag_member, QAPISchemaObjectTypeMember)
+ super().__init__(info, variants)
+ self.tag_member = tag_member
+
+ def check(self, schema, seen):
+ super().check(schema, seen)
+ assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+ assert not self.tag_member.optional
+ assert not self.tag_member.ifcond.is_present()
+
+
class QAPISchemaMember:
""" Represents object members, enum members and features """
role = 'member'
@@ -1184,7 +1192,7 @@ def _def_union_type(self, expr: QAPIExpression):
QAPISchemaObjectType(name, info, expr.doc, ifcond, features,
base, members,
QAPISchemaVariants(
- tag_name, info, None, variants)))
+ info, variants, tag_name)))
def _def_alternate_type(self, expr: QAPIExpression):
name = expr['alternate']
@@ -1202,7 +1210,7 @@ def _def_alternate_type(self, expr: QAPIExpression):
self._def_definition(
QAPISchemaAlternateType(
name, info, expr.doc, ifcond, features,
- QAPISchemaVariants(None, info, tag_member, variants)))
+ QAPISchemaAlternatives(info, variants, tag_member)))
def _def_command(self, expr: QAPIExpression):
name = expr['command']
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index c39d054d2c..05da30b855 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -23,6 +23,7 @@
)
from .schema import (
QAPISchema,
+ QAPISchemaAlternatives,
QAPISchemaEnumMember,
QAPISchemaFeature,
QAPISchemaIfCond,
@@ -369,11 +370,11 @@ def visit_alternate_type(self,
info: Optional[QAPISourceInfo],
ifcond: QAPISchemaIfCond,
features: List[QAPISchemaFeature],
- variants: QAPISchemaVariants) -> None:
+ alternatives: QAPISchemaAlternatives) -> None:
with ifcontext(ifcond, self._genh):
self._genh.preamble_add(gen_fwd_object_or_array(name))
self._genh.add(gen_object(name, ifcond, None,
- [variants.tag_member], variants))
+ [alternatives.tag_member], alternatives))
with ifcontext(ifcond, self._genh, self._genc):
self._gen_type_cleanup(name)
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index c56ea4d724..725bfcef50 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -28,6 +28,7 @@
)
from .schema import (
QAPISchema,
+ QAPISchemaAlternatives,
QAPISchemaEnumMember,
QAPISchemaEnumType,
QAPISchemaFeature,
@@ -222,7 +223,8 @@ def gen_visit_enum(name: str) -> str:
c_name=c_name(name))
-def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
+def gen_visit_alternate(name: str,
+ alternatives: QAPISchemaAlternatives) -> str:
ret = mcgen('''
bool visit_type_%(c_name)s(Visitor *v, const char *name,
@@ -244,7 +246,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
''',
c_name=c_name(name))
- for var in variants.variants:
+ for var in alternatives.variants:
ret += var.ifcond.gen_if()
ret += mcgen('''
case %(case)s:
@@ -414,10 +416,10 @@ def visit_alternate_type(self,
info: Optional[QAPISourceInfo],
ifcond: QAPISchemaIfCond,
features: List[QAPISchemaFeature],
- variants: QAPISchemaVariants) -> None:
+ alternatives: QAPISchemaAlternatives) -> None:
with ifcontext(ifcond, self._genh, self._genc):
self._genh.add(gen_visit_decl(name))
- self._genc.add(gen_visit_alternate(name, variants))
+ self._genc.add(gen_visit_alternate(name, alternatives))
def gen_visit(schema: QAPISchema,
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 14f7b62a44..b66ceb81b8 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -61,9 +61,10 @@ def visit_object_type(self, name, info, ifcond, features,
self._print_if(ifcond)
self._print_features(features)
- def visit_alternate_type(self, name, info, ifcond, features, variants):
+ def visit_alternate_type(self, name, info, ifcond, features,
+ alternatives):
print('alternate %s' % name)
- self._print_variants(variants)
+ self._print_variants(alternatives)
self._print_if(ifcond)
self._print_features(features)
On Wed, Jan 17, 2024 at 5:53 AM Markus Armbruster <armbru@redhat.com> wrote: > > Still more... > Would you hate me if I suggested that we punt this to after type checking is applied? i.e. let's do the stupid @property thing for now, and we'll rebase this proposal and put it in right afterwards. (Admittedly, it's just easier for me to review the impact on the typing work if I already have that baseline to work from...) > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py > index 658c288f8f..4a2e62d919 100644 > --- a/docs/sphinx/qapidoc.py > +++ b/docs/sphinx/qapidoc.py > @@ -328,7 +328,8 @@ def visit_object_type(self, name, info, ifcond, features, > + self._nodes_for_sections(doc) > + self._nodes_for_if_section(ifcond)) > > - def visit_alternate_type(self, name, info, ifcond, features, variants): > + def visit_alternate_type(self, name, info, ifcond, features, > + alternatives): > doc = self._cur_doc > self._add_doc('Alternate', > self._nodes_for_members(doc, 'Members') > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index c38df61a6d..e5cea8004e 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -26,6 +26,7 @@ > from .gen import QAPISchemaMonolithicCVisitor > from .schema import ( > QAPISchema, > + QAPISchemaAlternatives, > QAPISchemaArrayType, > QAPISchemaBuiltinType, > QAPISchemaEntity, > @@ -343,12 +344,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo], > def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo], > ifcond: QAPISchemaIfCond, > features: List[QAPISchemaFeature], > - variants: QAPISchemaVariants) -> None: > + alternatives: QAPISchemaAlternatives) -> None: > self._gen_tree( > name, 'alternate', > {'members': [Annotated({'type': self._use_type(m.type)}, > m.ifcond) > - for m in variants.variants]}, > + for m in alternatives.variants]}, > ifcond, features > ) > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index 0d9a70ab4c..f18aac7199 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -187,7 +187,8 @@ def visit_object_type_flat(self, name, info, ifcond, features, > members, variants): > pass > > - def visit_alternate_type(self, name, info, ifcond, features, variants): > + def visit_alternate_type(self, name, info, ifcond, features, > + alternatives): > pass > > def visit_command(self, name, info, ifcond, features, > @@ -563,8 +564,7 @@ class QAPISchemaAlternateType(QAPISchemaType): > > def __init__(self, name, info, doc, ifcond, features, variants): > super().__init__(name, info, doc, ifcond, features) > - assert isinstance(variants, QAPISchemaVariants) > - assert variants.tag_member > + assert isinstance(variants, QAPISchemaAlternatives) > variants.set_defined_in(name) > variants.tag_member.set_defined_in(self.name) > self.variants = variants > @@ -625,19 +625,12 @@ def visit(self, visitor): > self.name, self.info, self.ifcond, self.features, self.variants) > > > -class QAPISchemaVariants: > - def __init__(self, tag_name, info, tag_member, variants): > - # Unions pass tag_name but not tag_member. > - # Alternates pass tag_member but not tag_name. > - # After check(), tag_member is always set. > - assert bool(tag_member) != bool(tag_name) > - assert (isinstance(tag_name, str) or > - isinstance(tag_member, QAPISchemaObjectTypeMember)) > +class QAPISchemaVariantsBase: > + def __init__(self, info, variants): > for v in variants: > assert isinstance(v, QAPISchemaVariant) > - self._tag_name = tag_name > self.info = info > - self.tag_member = tag_member > + self.tag_member = None > self.variants = variants > > def set_defined_in(self, name): > @@ -645,66 +638,68 @@ def set_defined_in(self, name): > v.set_defined_in(name) > > def check(self, schema, seen): > - if self._tag_name: # union > - self.tag_member = seen.get(c_name(self._tag_name)) > - base = "'base'" > - # Pointing to the base type when not implicit would be > - # nice, but we don't know it here > - if not self.tag_member or self._tag_name != self.tag_member.name: > - raise QAPISemError( > - self.info, > - "discriminator '%s' is not a member of %s" > - % (self._tag_name, base)) > - # Here we do: > - base_type = schema.resolve_type(self.tag_member.defined_in) > - if not base_type.is_implicit(): > - base = "base type '%s'" % self.tag_member.defined_in > - if not isinstance(self.tag_member.type, QAPISchemaEnumType): > - raise QAPISemError( > - self.info, > - "discriminator member '%s' of %s must be of enum type" > - % (self._tag_name, base)) > - if self.tag_member.optional: > - raise QAPISemError( > - self.info, > - "discriminator member '%s' of %s must not be optional" > - % (self._tag_name, base)) > - if self.tag_member.ifcond.is_present(): > - raise QAPISemError( > - self.info, > - "discriminator member '%s' of %s must not be conditional" > - % (self._tag_name, base)) > - else: # alternate > - assert isinstance(self.tag_member.type, QAPISchemaEnumType) > - assert not self.tag_member.optional > - assert not self.tag_member.ifcond.is_present() > - if self._tag_name: # union > - # branches that are not explicitly covered get an empty type > - cases = {v.name for v in self.variants} > - for m in self.tag_member.type.members: > - if m.name not in cases: > - v = QAPISchemaVariant(m.name, self.info, > - 'q_empty', m.ifcond) > - v.set_defined_in(self.tag_member.defined_in) > - self.variants.append(v) > - 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) > + > + > +class QAPISchemaVariants(QAPISchemaVariantsBase): > + def __init__(self, info, variants, tag_name): > + assert isinstance(tag_name, str) > + super().__init__(info, variants) > + self._tag_name = tag_name > + > + def check(self, schema, seen): > + self.tag_member = seen.get(c_name(self._tag_name)) > + base = "'base'" > + # Pointing to the base type when not implicit would be > + # nice, but we don't know it here > + if not self.tag_member or self._tag_name != self.tag_member.name: > + raise QAPISemError( > + self.info, > + "discriminator '%s' is not a member of %s" > + % (self._tag_name, base)) > + # Here we do: > + base_type = schema.resolve_type(self.tag_member.defined_in) > + if not base_type.is_implicit(): > + base = "base type '%s'" % self.tag_member.defined_in > + if not isinstance(self.tag_member.type, QAPISchemaEnumType): > + raise QAPISemError( > + self.info, > + "discriminator member '%s' of %s must be of enum type" > + % (self._tag_name, base)) > + if self.tag_member.optional: > + raise QAPISemError( > + self.info, > + "discriminator member '%s' of %s must not be optional" > + % (self._tag_name, base)) > + if self.tag_member.ifcond.is_present(): > + raise QAPISemError( > + self.info, > + "discriminator member '%s' of %s must not be conditional" > + % (self._tag_name, base)) > + # branches that are not explicitly covered get an empty type > + cases = {v.name for v in self.variants} > + for m in self.tag_member.type.members: > + if m.name not in cases: > + v = QAPISchemaVariant(m.name, self.info, > + 'q_empty', m.ifcond) > + v.set_defined_in(self.tag_member.defined_in) > + self.variants.append(v) > + if not self.variants: > + raise QAPISemError(self.info, "union has no branches") > + super().check(schema, seen) > + for v in self.variants: > + 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) > > def check_clash(self, info, seen): > for v in self.variants: > @@ -713,6 +708,19 @@ def check_clash(self, info, seen): > v.type.check_clash(info, dict(seen)) > > > +class QAPISchemaAlternatives(QAPISchemaVariantsBase): > + def __init__(self, info, variants, tag_member): > + assert isinstance(tag_member, QAPISchemaObjectTypeMember) > + super().__init__(info, variants) > + self.tag_member = tag_member > + > + def check(self, schema, seen): > + super().check(schema, seen) > + assert isinstance(self.tag_member.type, QAPISchemaEnumType) > + assert not self.tag_member.optional > + assert not self.tag_member.ifcond.is_present() > + > + > class QAPISchemaMember: > """ Represents object members, enum members and features """ > role = 'member' > @@ -1184,7 +1192,7 @@ def _def_union_type(self, expr: QAPIExpression): > QAPISchemaObjectType(name, info, expr.doc, ifcond, features, > base, members, > QAPISchemaVariants( > - tag_name, info, None, variants))) > + info, variants, tag_name))) > > def _def_alternate_type(self, expr: QAPIExpression): > name = expr['alternate'] > @@ -1202,7 +1210,7 @@ def _def_alternate_type(self, expr: QAPIExpression): > self._def_definition( > QAPISchemaAlternateType( > name, info, expr.doc, ifcond, features, > - QAPISchemaVariants(None, info, tag_member, variants))) > + QAPISchemaAlternatives(info, variants, tag_member))) > > def _def_command(self, expr: QAPIExpression): > name = expr['command'] > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > index c39d054d2c..05da30b855 100644 > --- a/scripts/qapi/types.py > +++ b/scripts/qapi/types.py > @@ -23,6 +23,7 @@ > ) > from .schema import ( > QAPISchema, > + QAPISchemaAlternatives, > QAPISchemaEnumMember, > QAPISchemaFeature, > QAPISchemaIfCond, > @@ -369,11 +370,11 @@ def visit_alternate_type(self, > info: Optional[QAPISourceInfo], > ifcond: QAPISchemaIfCond, > features: List[QAPISchemaFeature], > - variants: QAPISchemaVariants) -> None: > + alternatives: QAPISchemaAlternatives) -> None: > with ifcontext(ifcond, self._genh): > self._genh.preamble_add(gen_fwd_object_or_array(name)) > self._genh.add(gen_object(name, ifcond, None, > - [variants.tag_member], variants)) > + [alternatives.tag_member], alternatives)) > with ifcontext(ifcond, self._genh, self._genc): > self._gen_type_cleanup(name) > > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py > index c56ea4d724..725bfcef50 100644 > --- a/scripts/qapi/visit.py > +++ b/scripts/qapi/visit.py > @@ -28,6 +28,7 @@ > ) > from .schema import ( > QAPISchema, > + QAPISchemaAlternatives, > QAPISchemaEnumMember, > QAPISchemaEnumType, > QAPISchemaFeature, > @@ -222,7 +223,8 @@ def gen_visit_enum(name: str) -> str: > c_name=c_name(name)) > > > -def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str: > +def gen_visit_alternate(name: str, > + alternatives: QAPISchemaAlternatives) -> str: > ret = mcgen(''' > > bool visit_type_%(c_name)s(Visitor *v, const char *name, > @@ -244,7 +246,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str: > ''', > c_name=c_name(name)) > > - for var in variants.variants: > + for var in alternatives.variants: > ret += var.ifcond.gen_if() > ret += mcgen(''' > case %(case)s: > @@ -414,10 +416,10 @@ def visit_alternate_type(self, > info: Optional[QAPISourceInfo], > ifcond: QAPISchemaIfCond, > features: List[QAPISchemaFeature], > - variants: QAPISchemaVariants) -> None: > + alternatives: QAPISchemaAlternatives) -> None: > with ifcontext(ifcond, self._genh, self._genc): > self._genh.add(gen_visit_decl(name)) > - self._genc.add(gen_visit_alternate(name, variants)) > + self._genc.add(gen_visit_alternate(name, alternatives)) > > > def gen_visit(schema: QAPISchema, > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py > index 14f7b62a44..b66ceb81b8 100755 > --- a/tests/qapi-schema/test-qapi.py > +++ b/tests/qapi-schema/test-qapi.py > @@ -61,9 +61,10 @@ def visit_object_type(self, name, info, ifcond, features, > self._print_if(ifcond) > self._print_features(features) > > - def visit_alternate_type(self, name, info, ifcond, features, variants): > + def visit_alternate_type(self, name, info, ifcond, features, > + alternatives): > print('alternate %s' % name) > - self._print_variants(variants) > + self._print_variants(alternatives) > self._print_if(ifcond) > self._print_features(features) > >
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 200bc0730d6..476b19aed61 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -627,25 +627,39 @@ def __init__(self, tag_name, info, tag_member, variants): assert isinstance(v, QAPISchemaVariant) self._tag_name = tag_name self.info = info - self.tag_member = tag_member + self._tag_member: Optional[QAPISchemaObjectTypeMember] = tag_member self.variants = variants + @property + def tag_member(self) -> 'QAPISchemaObjectTypeMember': + if self._tag_member is None: + raise RuntimeError( + "QAPISchemaVariants has no tag_member property until " + "after check() has been run." + ) + return self._tag_member + def set_defined_in(self, name): for v in self.variants: v.set_defined_in(name) def check(self, schema, seen): if self._tag_name: # union - self.tag_member = seen.get(c_name(self._tag_name)) + # We need to narrow the member type: + tmp = seen.get(c_name(self._tag_name)) + assert tmp is None or isinstance(tmp, QAPISchemaObjectTypeMember) + self._tag_member = tmp + base = "'base'" # Pointing to the base type when not implicit would be # nice, but we don't know it here - if not self.tag_member or self._tag_name != self.tag_member.name: + if not self._tag_member or self._tag_name != self._tag_member.name: raise QAPISemError( self.info, "discriminator '%s' is not a member of %s" % (self._tag_name, base)) # Here we do: + assert self.tag_member.defined_in base_type = schema.lookup_type(self.tag_member.defined_in) assert base_type if not base_type.is_implicit(): @@ -666,11 +680,13 @@ def check(self, schema, seen): "discriminator member '%s' of %s must not be conditional" % (self._tag_name, base)) else: # alternate + assert self._tag_member assert isinstance(self.tag_member.type, QAPISchemaEnumType) assert not self.tag_member.optional assert not self.tag_member.ifcond.is_present() if self._tag_name: # union # branches that are not explicitly covered get an empty type + assert self.tag_member.defined_in cases = {v.name for v in self.variants} for m in self.tag_member.type.members: if m.name not in cases:
There are two related changes here: (1) We need to perform type narrowing for resolving the type of tag_member during check(), and (2) tag_member is a delayed initialization field, but we can hide it behind a property that raises an Exception if it's called too early. This simplifies the typing in quite a few places and avoids needing to assert that the "tag_member is not None" at a dozen callsites, which can be confusing and suggest the wrong thing to a drive-by contributor. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/schema.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)