| Message ID | 1443930073-19359-12-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > With the previous commit, we have two different locations for > detecting member name clashes - one at parse time, and another > at QAPISchema*.check() time. Consolidate some of the checks > into a single place, which is also in line with our TODO to > eventually move all of the parse time semantic checking into > the newer schema code. The check_member_clash() function is > no longer needed. > > The wording of several error messages has changed, but in many > cases feels like an improvement rather than a regression. The > recent change to avoid an assertion failure when a flat union > branch name collides with its discriminator name is also > handled nicely by this code; but there is more work needed > before we can detect all collisions in simple union branch > names done by the old code. > > No change to generated code. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v7: comment improvements, retitle subject > v6: rebase to earlier testsuite improvements, fold in cleanup > of flat-union-clash-type > --- > scripts/qapi.py | 51 ++++++++++----------------- > tests/qapi-schema/flat-union-clash-member.err | 2 +- > tests/qapi-schema/flat-union-clash-type.err | 2 +- > tests/qapi-schema/struct-base-clash-deep.err | 2 +- > tests/qapi-schema/struct-base-clash.err | 2 +- > 5 files changed, 22 insertions(+), 37 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 30f1483..9f98413 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -499,21 +499,6 @@ def check_type(expr_info, source, value, allow_array=False, > 'enum']) > > > -def check_member_clash(expr_info, base_name, data, source=""): > - base = find_struct(base_name) > - assert base > - base_members = base['data'] > - for key in data.keys(): > - if key.startswith('*'): > - key = key[1:] > - if key in base_members or "*" + key in base_members: > - raise QAPIExprError(expr_info, > - "Member name '%s'%s clashes with base '%s'" > - % (key, source, base_name)) > - if base.get('base'): > - check_member_clash(expr_info, base['base'], data, source) > - > - > def check_command(expr, expr_info): > name = expr['command'] > > @@ -592,30 +577,18 @@ def check_union(expr, expr_info): > for (key, value) in members.items(): > check_name(expr_info, "Member of union '%s'" % name, key) > > - # Each value must name a known type; furthermore, in flat unions, > - # branches must be a struct with no overlapping member names > + # Each value must name a known type > check_type(expr_info, "Member '%s' of union '%s'" % (key, name), > value, allow_array=not base, allow_metas=allow_metas) > - if base: > - branch_struct = find_struct(value) > - assert branch_struct > - check_member_clash(expr_info, base, branch_struct['data'], > - " of branch '%s'" % key) > > # If the discriminator names an enum type, then all members > - # of 'data' must also be members of the enum type, which in turn > - # must not collide with the discriminator name. > + # of 'data' must also be members of the enum type. > if enum_define: > if key not in enum_define['enum_values']: > raise QAPIExprError(expr_info, > "Discriminator value '%s' is not found in " > "enum '%s'" % > (key, enum_define["enum_name"])) > - if discriminator in enum_define['enum_values']: > - raise QAPIExprError(expr_info, > - "Discriminator name '%s' collides with " > - "enum value in '%s'" % > - (discriminator, enum_define["enum_name"])) > > # Otherwise, check for conflicts in the generated enum > else: > @@ -690,8 +663,6 @@ def check_struct(expr, expr_info): > allow_dict=True, allow_optional=True) > check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'), > allow_metas=['struct']) > - if expr.get('base'): > - check_member_clash(expr_info, expr['base'], expr['data']) > > > def check_keys(expr_elem, meta, required, optional=[]): > @@ -1095,16 +1066,30 @@ class QAPISchemaObjectTypeVariants(object): > assert isinstance(self.tag_member.type, QAPISchemaEnumType) > for v in self.variants: > vseen = dict(seen) > - v.check(schema, info, self.tag_member.type, vseen) > + v.check(schema, info, self.tag_member.type, vseen, > + bool(self.tag_name)) > > > class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): > def __init__(self, name, typ): > QAPISchemaObjectTypeMember.__init__(self, name, typ, False) > > - def check(self, schema, info, tag_type, seen): > + def check(self, schema, info, tag_type, seen, flat): > QAPISchemaObjectTypeMember.check(self, schema, info, [], seen) > assert self.name in tag_type.values > + if flat: > + # For flat unions, check that each QMP member does not > + # collide with any non-variant members. No type can > + # contain itself as a flat variant > + self.type.check(schema) > + assert not self.type.variants # not implemented > + for m in self.type.members: > + if m.c_name() in seen: > + raise QAPIExprError(info, > + "Member '%s' of branch '%s' collides " > + "with %s" > + % (m.name, self.name, > + seen[m.c_name()].describe())) If our data structures were right, we wouldn't need the conditional. Three cases: 1. Flat union self is a flat union object type's variant case. self.name is a tag value, and self.type is an object type providing the members for this tag value. It has no variants. Example: UserDefFlatUnion's first case, self.name is 'value1', self.type is type UserDefA. We need to check that the case's variant members (i.e. self.type's members) don't clash with non-variant members, because in QMP, they become members of the same JSON object. 2. Simple union self is a simple union object type's variant case. self.name is a tag value, and self.type is an object type providing the members for this tag value. It has no variants. Exactly the same as 1. The fact that the tag member, its type and all the variant types are implicitly defined is immaterial. Example: __org.qemu_x-Union1's first case, self.name is '__org.qemu_x-branch', self.type is the implicitly defined type :obj-str-wrapper. It's only member is named 'data'. The case's variant members (i.e. self.type's members) don't clash with non-variant members by construction. Since simple unions can't have a base, the only non-variant member is the implicitly defined tag. Its name is 'type'. Each case has exactly one variant member named 'data'. Therefore, there should be no need to special-case simple unions here: the check() for flat unions should work fine, it just shouldn't find clashes. 3. Alternate self is an alternate type's case. self.name names the case. It is not a tag value in QMP, because alternates are implicitly tagged in QMP. It is a tag value in C. self.type is a type. It needn't be an object type. Example: UserDefAlternate's first case, self.name is 'uda', self.type is type UserDefA. The case's members (if any) can't clash with anything. However, the check() for flat unions doesn't quite work. First, it assumes self.type is an object type without variants. That's not true for alternate cases. Second, it puts the implicit tag member into seen, even though it's not actually on the wire. So far the theory, now practice: if I hack the code to test "not alternate" (i.e. case 1 and 2) instead of "flat union" (just case 1), "make check" passes except for union-clash-data.json. There, we now report a clash we didn't report before: union-clash-data.json:6: Member 'data' of branch 'data' collides with 'data' (branch of TestUnion) The FIXME in union-clash-data.json actually asks for an error, because 'data' clashes with our stupid filler to avoid empty unions. But that's not what this error message reports! I think what happens is this: QAPISchemaObjectTypeVariant.check() adds the case name self.name (here: 'data') to seen. When we then check the case's 'data': 'int' member, it finds the with the case name, and reports a clash. I can't see why it should. This clashing business is awfully confusing, because we have to consider (flat unions, simple unions, alternates) times (QMP, C). It would be simpler if we could make C clash only when QMP clashes. I've been trying to make simple unions a genuine special case of flat unions in part to get shrink this matrix. We might want to separate out alternates. Dunno. > # This function exists to support ugly simple union special cases > # TODO get rid of them, and drop the function > diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err > index 2f0397a..57dd478 100644 > --- a/tests/qapi-schema/flat-union-clash-member.err > +++ b/tests/qapi-schema/flat-union-clash-member.err > @@ -1 +1 @@ > -tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base' > +tests/qapi-schema/flat-union-clash-member.json:11: Member 'name' of branch 'value1' collides with 'name' (member of Base) > diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err > index b44dd40..3f3248b 100644 > --- a/tests/qapi-schema/flat-union-clash-type.err > +++ b/tests/qapi-schema/flat-union-clash-type.err > @@ -1 +1 @@ > -tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum' > +tests/qapi-schema/flat-union-clash-type.json:11: 'type' (branch of TestUnion) collides with 'type' (member of Base) > diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err > index f7a25a3..e2d7943 100644 > --- a/tests/qapi-schema/struct-base-clash-deep.err > +++ b/tests/qapi-schema/struct-base-clash-deep.err > @@ -1 +1 @@ > -tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base' > +tests/qapi-schema/struct-base-clash-deep.json:10: 'name' (member of Sub) collides with 'name' (member of Base) > diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err > index 3a9f66b..c52f33d 100644 > --- a/tests/qapi-schema/struct-base-clash.err > +++ b/tests/qapi-schema/struct-base-clash.err > @@ -1 +1 @@ > -tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base' > +tests/qapi-schema/struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)
On 10/12/2015 09:53 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> With the previous commit, we have two different locations for >> detecting member name clashes - one at parse time, and another >> at QAPISchema*.check() time. Consolidate some of the checks >> into a single place, which is also in line with our TODO to >> eventually move all of the parse time semantic checking into >> the newer schema code. The check_member_clash() function is >> no longer needed. >> >> + def check(self, schema, info, tag_type, seen, flat): >> QAPISchemaObjectTypeMember.check(self, schema, info, [], seen) >> assert self.name in tag_type.values >> + if flat: >> + # For flat unions, check that each QMP member does not >> + # collide with any non-variant members. No type can >> + # contain itself as a flat variant >> + self.type.check(schema) >> + assert not self.type.variants # not implemented >> + for m in self.type.members: >> + if m.c_name() in seen: >> + raise QAPIExprError(info, >> + "Member '%s' of branch '%s' collides " >> + "with %s" >> + % (m.name, self.name, >> + seen[m.c_name()].describe())) > > If our data structures were right, we wouldn't need the conditional. > Okay, you've made a good point. The only conditional that is appropriate here is 'if not alternate', because you are right that flat and simple unions should behave identically (and merely that simple unions won't detect any collisions, because flattening their implicit struct with a single 'data' member shouldn't introduce a conflict in QMP). > So far the theory, now practice: if I hack the code to test "not > alternate" (i.e. case 1 and 2) instead of "flat union" (just case 1), > "make check" passes except for union-clash-data.json. There, we now > report a clash we didn't report before: > > union-clash-data.json:6: Member 'data' of branch 'data' collides with 'data' (branch of TestUnion) Hmm, I'll have to investigate before posting v8. At one point, I wired in debug statements into the generator to show the contents of seen[] through each call to check(), to see where collisions are coming from; looks like I get to play with that again. > > The FIXME in union-clash-data.json actually asks for an error, because > 'data' clashes with our stupid filler to avoid empty unions. But that's > not what this error message reports! I think what happens is this: > QAPISchemaObjectTypeVariant.check() adds the case name self.name (here: > 'data') to seen. When we then check the case's 'data': 'int' member, it > finds the with the case name, and reports a clash. I can't see why it > should. > > This clashing business is awfully confusing, because we have to consider > (flat unions, simple unions, alternates) times (QMP, C). > > It would be simpler if we could make C clash only when QMP clashes. If a QMP clash always caused a C clash, life would be a bit simpler. We aren't going to get there (because in a flat union, hiding the members of the branch type behind a single pointer means those members don't clash with any C names of the overall union), but I can certainly try to make the comments explain what is going on. > > We might want to separate out alternates. Dunno. And to some extent, subset C already does some of that separation (because I try to convert from 'FooKind type' to 'qtype_code type' without even creating an implicit 'FooKind' enum). It sounds like you're okay with any well-documented TODO warts related to alternates for the purposes of this subset B, especially if I can then clean those warts back up in subset C. But what v8 of subset B needs to avoid is any warts based on simple vs. flat unions. I can live with that. I'm still trying to figure out if hoisting the kind=>type rename into subset B makes life easier or harder (it certainly causes me more rebase churn, but that's not necessarily a bad thing).
On 10/12/2015 10:22 AM, Eric Blake wrote: > >>> + def check(self, schema, info, tag_type, seen, flat): >>> QAPISchemaObjectTypeMember.check(self, schema, info, [], seen) seen gets modified here... >>> assert self.name in tag_type.values >>> + if flat: >>> + # For flat unions, check that each QMP member does not >>> + # collide with any non-variant members. No type can >>> + # contain itself as a flat variant >>> + self.type.check(schema) >>> + assert not self.type.variants # not implemented >>> + for m in self.type.members: >>> + if m.c_name() in seen: ...and it is the modified seen here that causes... >> union-clash-data.json:6: Member 'data' of branch 'data' collides with 'data' (branch of TestUnion) ...the incorrect error. >> >> The FIXME in union-clash-data.json actually asks for an error, because >> 'data' clashes with our stupid filler to avoid empty unions. But that's >> not what this error message reports! I think what happens is this: >> QAPISchemaObjectTypeVariant.check() adds the case name self.name (here: >> 'data') to seen. When we then check the case's 'data': 'int' member, it >> finds the with the case name, and reports a clash. I can't see why it >> should. >> >> This clashing business is awfully confusing, because we have to consider >> (flat unions, simple unions, alternates) times (QMP, C). >> >> It would be simpler if we could make C clash only when QMP clashes. > > If a QMP clash always caused a C clash, life would be a bit simpler. We > aren't going to get there (because in a flat union, hiding the members > of the branch type behind a single pointer means those members don't > clash with any C names of the overall union), but I can certainly try to > make the comments explain what is going on. I've managed to fix it with a dict(seen) and some comments, and will post a v8. Great catch, by the way. > >> >> We might want to separate out alternates. Dunno. > > And to some extent, subset C already does some of that separation > (because I try to convert from 'FooKind type' to 'qtype_code type' > without even creating an implicit 'FooKind' enum). It sounds like > you're okay with any well-documented TODO warts related to alternates > for the purposes of this subset B, especially if I can then clean those > warts back up in subset C. But what v8 of subset B needs to avoid is > any warts based on simple vs. flat unions. I can live with that. > > I'm still trying to figure out if hoisting the kind=>type rename into > subset B makes life easier or harder (it certainly causes me more rebase > churn, but that's not necessarily a bad thing). At this point, it was easier to live with a temporary hack for QAPISchemaObjectTypeUnionTag than it was to rebase my kind=>type rename patch to be this early in the overall series. We'll get there, but I agree with the approach we've been taking of one subset at a time.
Eric Blake <eblake@redhat.com> writes: > On 10/12/2015 09:53 AM, Markus Armbruster wrote: [...] >> It would be simpler if we could make C clash only when QMP clashes. > > If a QMP clash always caused a C clash, life would be a bit simpler. We > aren't going to get there (because in a flat union, hiding the members > of the branch type behind a single pointer means those members don't > clash with any C names of the overall union), but I can certainly try to > make the comments explain what is going on. (1) If QMP clashed exactly when C clashed, we'd have to think only about one of them, and the C compiler would check for clashes statically. (2) If a QMP clash implied a C clash, we'd only have to think about C, and the C compiler would check statically. (3) If a C clash implied a QMP clash, we'd only have to think about QMP. (4) If they can clash independently, we need to think about both independently. We currently have (4), and having to juggle both QMP and C in my mind has been taxing. My remark was about (3): C clashes only when QMP clashes <=> C clash implies QMP clash. Your remark was about (2). More useful than (3), but as you say not feasible. Do you think we can get to (3)? >> We might want to separate out alternates. Dunno. > > And to some extent, subset C already does some of that separation > (because I try to convert from 'FooKind type' to 'qtype_code type' > without even creating an implicit 'FooKind' enum). It sounds like > you're okay with any well-documented TODO warts related to alternates > for the purposes of this subset B, especially if I can then clean those > warts back up in subset C. But what v8 of subset B needs to avoid is > any warts based on simple vs. flat unions. I can live with that. I'm prepared to accept rephrased existing warts and additional temporary warts when I understand why simply fixing them isn't practical or economical. A certain amount of hand-waving is okay. > I'm still trying to figure out if hoisting the kind=>type rename into > subset B makes life easier or harder (it certainly causes me more rebase > churn, but that's not necessarily a bad thing). Thanks!
On 10/13/2015 01:08 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 10/12/2015 09:53 AM, Markus Armbruster wrote: > [...] >>> It would be simpler if we could make C clash only when QMP clashes. >> >> If a QMP clash always caused a C clash, life would be a bit simpler. We >> aren't going to get there (because in a flat union, hiding the members >> of the branch type behind a single pointer means those members don't >> clash with any C names of the overall union), but I can certainly try to >> make the comments explain what is going on. > > (1) If QMP clashed exactly when C clashed, we'd have to think only about > one of them, and the C compiler would check for clashes statically. Although deferring the error about the clash until compile time is a bit long, compared to reporting the clash at generator time. > > (2) If a QMP clash implied a C clash, we'd only have to think about C, > and the C compiler would check statically. > > (3) If a C clash implied a QMP clash, we'd only have to think about QMP. > > (4) If they can clash independently, we need to think about both > independently. > > We currently have (4), and having to juggle both QMP and C in my mind > has been taxing. > > My remark was about (3): C clashes only when QMP clashes <=> C clash > implies QMP clash. > > Your remark was about (2). More useful than (3), but as you say not > feasible. > > Do you think we can get to (3)? C clash that does not imply a QMP clash: { 'union':'U', 'data':{ 'a':'int', 'A':'str' } } C clash (the implicit enum has two U_KIND_A values) but not a QMP clash ('a' and 'A' don't even appear in QMP; and even if they did, they are distinct in C as branch names). Might be possible to avoid the C clash by munging case labels of the generated enum differently, but I'm not sure it is worth the effort. QMP clash that does not (currently) imply a C clash (using abbreviated notation): { 'union':'U', 'base':{ 'type':'Enum', 'member':'int' }, 'discriminator':'type', 'data':{ 'branch':{ 'member':'str' } } } QMP clash (the field 'member' is part of the base type, and also part of the 'branch' variant type), but not a C clash (because the C layout accesses the branch through a box named 'branch'). But we cannot just remove the boxing, either, because then we'd have a clash in C that is NOT a clash in QMP: { 'union':'U', 'base':{ 'type':'Enum' }, 'discriminator':'type', 'data':{ 'branch1':{ 'member':'str' }, 'branch2': { 'member':'int' } } } If the two 'member' fields were at the same C level as 'type', then we could use C collisions to detect a QMP clash between a variant's members and the non-variant fields - but it also has the drawback of declaring a C collision between the two branches. So no, I don't think we can get to (3).
Eric Blake <eblake@redhat.com> writes: > On 10/13/2015 01:08 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 10/12/2015 09:53 AM, Markus Armbruster wrote: >> [...] >>>> It would be simpler if we could make C clash only when QMP clashes. >>> >>> If a QMP clash always caused a C clash, life would be a bit simpler. We >>> aren't going to get there (because in a flat union, hiding the members >>> of the branch type behind a single pointer means those members don't >>> clash with any C names of the overall union), but I can certainly try to >>> make the comments explain what is going on. >> >> (1) If QMP clashed exactly when C clashed, we'd have to think only about >> one of them, and the C compiler would check for clashes statically. > > Although deferring the error about the clash until compile time is a bit > long, compared to reporting the clash at generator time. I'd view the C compiler as insurance. The generators should still check. >> (2) If a QMP clash implied a C clash, we'd only have to think about C, >> and the C compiler would check statically. >> >> (3) If a C clash implied a QMP clash, we'd only have to think about QMP. >> >> (4) If they can clash independently, we need to think about both >> independently. >> >> We currently have (4), and having to juggle both QMP and C in my mind >> has been taxing. >> >> My remark was about (3): C clashes only when QMP clashes <=> C clash >> implies QMP clash. >> >> Your remark was about (2). More useful than (3), but as you say not >> feasible. >> >> Do you think we can get to (3)? > > C clash that does not imply a QMP clash: > > { 'union':'U', 'data':{ 'a':'int', 'A':'str' } } > > C clash (the implicit enum has two U_KIND_A values) but not a QMP clash > ('a' and 'A' don't even appear in QMP; and even if they did, they are > distinct in C as branch names). Might be possible to avoid the C clash > by munging case labels of the generated enum differently, but I'm not > sure it is worth the effort. > > QMP clash that does not (currently) imply a C clash (using abbreviated > notation): > > { 'union':'U', 'base':{ 'type':'Enum', 'member':'int' }, > 'discriminator':'type', > 'data':{ 'branch':{ 'member':'str' } } } > > QMP clash (the field 'member' is part of the base type, and also part of > the 'branch' variant type), but not a C clash (because the C layout > accesses the branch through a box named 'branch'). > > But we cannot just remove the boxing, either, because then we'd have a > clash in C that is NOT a clash in QMP: > > { 'union':'U', 'base':{ 'type':'Enum' }, 'discriminator':'type', > 'data':{ 'branch1':{ 'member':'str' }, 'branch2': { 'member':'int' } } } > > If the two 'member' fields were at the same C level as 'type', then we > could use C collisions to detect a QMP clash between a variant's members > and the non-variant fields - but it also has the drawback of declaring a > C collision between the two branches. > > So no, I don't think we can get to (3). Scratch that then. My head hurts.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 30f1483..9f98413 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -499,21 +499,6 @@ def check_type(expr_info, source, value, allow_array=False, 'enum']) -def check_member_clash(expr_info, base_name, data, source=""): - base = find_struct(base_name) - assert base - base_members = base['data'] - for key in data.keys(): - if key.startswith('*'): - key = key[1:] - if key in base_members or "*" + key in base_members: - raise QAPIExprError(expr_info, - "Member name '%s'%s clashes with base '%s'" - % (key, source, base_name)) - if base.get('base'): - check_member_clash(expr_info, base['base'], data, source) - - def check_command(expr, expr_info): name = expr['command'] @@ -592,30 +577,18 @@ def check_union(expr, expr_info): for (key, value) in members.items(): check_name(expr_info, "Member of union '%s'" % name, key) - # Each value must name a known type; furthermore, in flat unions, - # branches must be a struct with no overlapping member names + # Each value must name a known type check_type(expr_info, "Member '%s' of union '%s'" % (key, name), value, allow_array=not base, allow_metas=allow_metas) - if base: - branch_struct = find_struct(value) - assert branch_struct - check_member_clash(expr_info, base, branch_struct['data'], - " of branch '%s'" % key) # If the discriminator names an enum type, then all members - # of 'data' must also be members of the enum type, which in turn - # must not collide with the discriminator name. + # of 'data' must also be members of the enum type. if enum_define: if key not in enum_define['enum_values']: raise QAPIExprError(expr_info, "Discriminator value '%s' is not found in " "enum '%s'" % (key, enum_define["enum_name"])) - if discriminator in enum_define['enum_values']: - raise QAPIExprError(expr_info, - "Discriminator name '%s' collides with " - "enum value in '%s'" % - (discriminator, enum_define["enum_name"])) # Otherwise, check for conflicts in the generated enum else: @@ -690,8 +663,6 @@ def check_struct(expr, expr_info): allow_dict=True, allow_optional=True) check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'), allow_metas=['struct']) - if expr.get('base'): - check_member_clash(expr_info, expr['base'], expr['data']) def check_keys(expr_elem, meta, required, optional=[]): @@ -1095,16 +1066,30 @@ class QAPISchemaObjectTypeVariants(object): assert isinstance(self.tag_member.type, QAPISchemaEnumType) for v in self.variants: vseen = dict(seen) - v.check(schema, info, self.tag_member.type, vseen) + v.check(schema, info, self.tag_member.type, vseen, + bool(self.tag_name)) class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def __init__(self, name, typ): QAPISchemaObjectTypeMember.__init__(self, name, typ, False) - def check(self, schema, info, tag_type, seen): + def check(self, schema, info, tag_type, seen, flat): QAPISchemaObjectTypeMember.check(self, schema, info, [], seen) assert self.name in tag_type.values + if flat: + # For flat unions, check that each QMP member does not + # collide with any non-variant members. No type can + # contain itself as a flat variant + self.type.check(schema) + assert not self.type.variants # not implemented + for m in self.type.members: + if m.c_name() in seen: + raise QAPIExprError(info, + "Member '%s' of branch '%s' collides " + "with %s" + % (m.name, self.name, + seen[m.c_name()].describe())) # This function exists to support ugly simple union special cases # TODO get rid of them, and drop the function diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err index 2f0397a..57dd478 100644 --- a/tests/qapi-schema/flat-union-clash-member.err +++ b/tests/qapi-schema/flat-union-clash-member.err @@ -1 +1 @@ -tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base' +tests/qapi-schema/flat-union-clash-member.json:11: Member 'name' of branch 'value1' collides with 'name' (member of Base) diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err index b44dd40..3f3248b 100644 --- a/tests/qapi-schema/flat-union-clash-type.err +++ b/tests/qapi-schema/flat-union-clash-type.err @@ -1 +1 @@ -tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum' +tests/qapi-schema/flat-union-clash-type.json:11: 'type' (branch of TestUnion) collides with 'type' (member of Base) diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err index f7a25a3..e2d7943 100644 --- a/tests/qapi-schema/struct-base-clash-deep.err +++ b/tests/qapi-schema/struct-base-clash-deep.err @@ -1 +1 @@ -tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base' +tests/qapi-schema/struct-base-clash-deep.json:10: 'name' (member of Sub) collides with 'name' (member of Base) diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err index 3a9f66b..c52f33d 100644 --- a/tests/qapi-schema/struct-base-clash.err +++ b/tests/qapi-schema/struct-base-clash.err @@ -1 +1 @@ -tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base' +tests/qapi-schema/struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)
With the previous commit, we have two different locations for detecting member name clashes - one at parse time, and another at QAPISchema*.check() time. Consolidate some of the checks into a single place, which is also in line with our TODO to eventually move all of the parse time semantic checking into the newer schema code. The check_member_clash() function is no longer needed. The wording of several error messages has changed, but in many cases feels like an improvement rather than a regression. The recent change to avoid an assertion failure when a flat union branch name collides with its discriminator name is also handled nicely by this code; but there is more work needed before we can detect all collisions in simple union branch names done by the old code. No change to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> --- v7: comment improvements, retitle subject v6: rebase to earlier testsuite improvements, fold in cleanup of flat-union-clash-type --- scripts/qapi.py | 51 ++++++++++----------------- tests/qapi-schema/flat-union-clash-member.err | 2 +- tests/qapi-schema/flat-union-clash-type.err | 2 +- tests/qapi-schema/struct-base-clash-deep.err | 2 +- tests/qapi-schema/struct-base-clash.err | 2 +- 5 files changed, 22 insertions(+), 37 deletions(-)