| Message ID | 1446791754-23823-24-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Right now, our ad hoc parser ensures that we cannot have a > flat union that introduces any qapi member names that would > conflict with the non-variant qapi members already present > from the union's base type (see flat-union-clash-member.json). > We want QAPISchemaObjectType.check() to make the same check, > so we can later reduce some of the ad hoc checks. > > We already ensure that all branches of a flat union are qapi > structs with no variants, at which point those members appear > in the same JSON object as all non-variant members. And we > already have a map 'seen' of all non-variant members. All > we need is a new QAPISchemaObjectTypeVariants.check_clash(), > which clones the seen map then checks for clashes with each > member of the variant's qapi type. > > Note that the clone of seen inside Variants.check_clash() > resembles the one we just removed from Variants.check(); the > difference here is that we are now checking for clashes > among the qapi members of the variant type, rather than for > a single clash with the variant tag name itself. > > In general, a type used as a branch of a flat union cannot > also be the base type of the flat union, so even though we are > adding a call to variant.type.check() in order to populate > variant.type.members, this is merely a case of gaining > topological sorting of how types are visited (and type.check() > is already set up to allow multiple calls due to base types). Yes, a type cannot contain itself, neither as base nor as variant. We have tests covering attempts to do the former (struct-cycle-direct.json, struct-cycle-indirect.json). As far as I can see, we don't have tests covering the latter. Do we catch it? > For simple unions, the same code happens to work by design, > because of our synthesized wrapper classes (however, the > wrapper has a single member 'data' which will never collide > with the one non-variant member 'type', so it doesn't really > matter). > > There is no impact to alternates, which intentionally do not > need to call variants.check_clash() (there, at most one of > the variant's branches will be an ObjectType, and even if one > exists, we are not inlining the qapi members of that object > into a parent object, the way we do for unions). > > No change to generated code. > > Signed-off-by: Eric Blake <eblake@redhat.com> Patch looks good.
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: > >> Right now, our ad hoc parser ensures that we cannot have a >> flat union that introduces any qapi member names that would >> conflict with the non-variant qapi members already present >> from the union's base type (see flat-union-clash-member.json). >> We want QAPISchemaObjectType.check() to make the same check, >> so we can later reduce some of the ad hoc checks. >> >> We already ensure that all branches of a flat union are qapi >> structs with no variants, at which point those members appear >> in the same JSON object as all non-variant members. And we >> already have a map 'seen' of all non-variant members. All >> we need is a new QAPISchemaObjectTypeVariants.check_clash(), >> which clones the seen map then checks for clashes with each >> member of the variant's qapi type. >> >> Note that the clone of seen inside Variants.check_clash() >> resembles the one we just removed from Variants.check(); the >> difference here is that we are now checking for clashes >> among the qapi members of the variant type, rather than for >> a single clash with the variant tag name itself. >> >> In general, a type used as a branch of a flat union cannot >> also be the base type of the flat union, so even though we are >> adding a call to variant.type.check() in order to populate >> variant.type.members, this is merely a case of gaining >> topological sorting of how types are visited (and type.check() >> is already set up to allow multiple calls due to base types). > > Yes, a type cannot contain itself, neither as base nor as variant. > > We have tests covering attempts to do the former > (struct-cycle-direct.json, struct-cycle-indirect.json). As far as I can > see, we don't have tests covering the latter. Do we catch it? > >> For simple unions, the same code happens to work by design, >> because of our synthesized wrapper classes (however, the >> wrapper has a single member 'data' which will never collide >> with the one non-variant member 'type', so it doesn't really >> matter). >> >> There is no impact to alternates, which intentionally do not >> need to call variants.check_clash() (there, at most one of >> the variant's branches will be an ObjectType, and even if one >> exists, we are not inlining the qapi members of that object >> into a parent object, the way we do for unions). Yes. QAPISchemaObjectTypeVariants.check_clash() checks for each variant's members clashing with other members in the same name space. For alternates, there are no such other members. That said, should we add a comment to QAPISchemaAlternateType.check()? Perhaps: # Not calling self.variant.check_clash(), because there's # nothing to clash with. >> No change to generated code. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Patch looks good.
On 11/09/2015 08:13 AM, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > >> Eric Blake <eblake@redhat.com> writes: >> >>> Right now, our ad hoc parser ensures that we cannot have a >>> flat union that introduces any qapi member names that would >>> conflict with the non-variant qapi members already present >>> from the union's base type (see flat-union-clash-member.json). >>> We want QAPISchemaObjectType.check() to make the same check, >>> so we can later reduce some of the ad hoc checks. >>> >>> There is no impact to alternates, which intentionally do not >>> need to call variants.check_clash() (there, at most one of >>> the variant's branches will be an ObjectType, and even if one >>> exists, we are not inlining the qapi members of that object >>> into a parent object, the way we do for unions). > > Yes. QAPISchemaObjectTypeVariants.check_clash() checks for each > variant's members clashing with other members in the same name space. > For alternates, there are no such other members. > > That said, should we add a comment to QAPISchemaAlternateType.check()? > Perhaps: > > # Not calling self.variant.check_clash(), because there's > # nothing to clash with. Sure; if there's another reason for a respin, I can add it here; otherwise, it's already slated to be added in my respin of the patches for redoing how alternates are laid out.
diff --git a/scripts/qapi.py b/scripts/qapi.py index e057408..4c56935 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -992,6 +992,7 @@ class QAPISchemaObjectType(QAPISchemaType): if self.variants: self.variants.check(schema, seen) assert self.variants.tag_member in self.members + self.variants.check_clash(schema, seen) def is_implicit(self): # See QAPISchema._make_implicit_object_type() @@ -1057,6 +1058,17 @@ class QAPISchemaObjectTypeVariants(object): for v in self.variants: v.check(schema, self.tag_member.type) + def check_clash(self, schema, seen): + for v in self.variants: + # Reset seen map for each variant, since qapi names from one + # branch do not affect another branch + vseen = dict(seen) + assert isinstance(v.type, QAPISchemaObjectType) + assert not v.type.variants # not implemented + v.type.check(schema) + for m in v.type.members: + m.check_clash(vseen) + class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def __init__(self, name, typ):
Right now, our ad hoc parser ensures that we cannot have a flat union that introduces any qapi member names that would conflict with the non-variant qapi members already present from the union's base type (see flat-union-clash-member.json). We want QAPISchemaObjectType.check() to make the same check, so we can later reduce some of the ad hoc checks. We already ensure that all branches of a flat union are qapi structs with no variants, at which point those members appear in the same JSON object as all non-variant members. And we already have a map 'seen' of all non-variant members. All we need is a new QAPISchemaObjectTypeVariants.check_clash(), which clones the seen map then checks for clashes with each member of the variant's qapi type. Note that the clone of seen inside Variants.check_clash() resembles the one we just removed from Variants.check(); the difference here is that we are now checking for clashes among the qapi members of the variant type, rather than for a single clash with the variant tag name itself. In general, a type used as a branch of a flat union cannot also be the base type of the flat union, so even though we are adding a call to variant.type.check() in order to populate variant.type.members, this is merely a case of gaining topological sorting of how types are visited (and type.check() is already set up to allow multiple calls due to base types). For simple unions, the same code happens to work by design, because of our synthesized wrapper classes (however, the wrapper has a single member 'data' which will never collide with the one non-variant member 'type', so it doesn't really matter). There is no impact to alternates, which intentionally do not need to call variants.check_clash() (there, at most one of the variant's branches will be an ObjectType, and even if one exists, we are not inlining the qapi members of that object into a parent object, the way we do for unions). No change to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> --- v10: create new Variants.check_clash() rather than piggybacking on .check() v9: new patch, split off from v8 7/17 --- scripts/qapi.py | 12 ++++++++++++ 1 file changed, 12 insertions(+)