| Message ID | 1443760312-656-9-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 defer 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 Which patch was that again? > 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> > > --- > v6: rebase to earlier testsuite improvements, fold in cleanup > of flat-union-clash-type > --- > scripts/qapi.py | 46 +++++++++------------------ > 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, 19 insertions(+), 35 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 1acf02b..e58c5a8 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,15 +577,9 @@ 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 > @@ -611,11 +590,6 @@ def check_union(expr, expr_info): # 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. Should this comment be updated for the code removal below? 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 +664,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=[]): > @@ -1046,16 +1018,28 @@ 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, > + self.tag_name is not None) > > > class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): > def __init__(self, name, typ, owner): > QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner) > > - 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: Any way to avoid the conditional? I've tried hard to make simple unions mere sugar for flat ones. There are a few special cases left where we need to distinguish the two, but they're all marked TODO. Can we have a brief comment explaing what we're checking here? We generally don't have such comments in check() methods so far. Sort of okay as long as they merely assert, but you're now starting to move semantic analysis into them, which raises the bar. > + self.type.check(schema) Uh, careful. It's always okay for a check() method to call the check() of a child in the abstract syntax tree, e.g. QAPISchemaObjectType.check() calling m.check() for its members. A tree has no cycles. Calling it on anything else requires a non-trivial correctness argument. Example: QAPISchemaObjectType.check() calls self.base.check(). Okay, because no type may be a base of itself, and therefore a cycle would be an error. The code takes care to detect cycles. I think the correctness argument here would be "no type contain itself as variant member, and therefore a cycle would be an error." Do we detect cycles? > + assert isinstance(self.type.members, list) Implied by QAPISchemaObjectType.check()'s postcondition. Feels redundant here, in particular since you iterate over it next anyway. > + 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" Hanging indentation would avoid the split string. > + % (m.name, self.name, > + seen[m.c_name()].describe())) > > def describe(self): > return "'%s' (branch of %s)" % (self.name, self._owner) > 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/02/2015 08:00 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 defer 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 > > Which patch was that again? v7a 6/18, http://repo.or.cz/qemu/ericb.git/commit/ede42e8e [uggh - gnu.org archives are still quite lagging, or I'd point to a mail] > >> 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> >> >> @@ -611,11 +590,6 @@ def check_union(expr, expr_info): > # 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. > > Should this comment be updated for the code removal below? Oh, right, revert both hunks of the earlier commit since we are now doing it in a less hacky manner. >> + v.check(schema, info, self.tag_member.type, vseen, >> + self.tag_name is not None) >> >> >> class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): >> def __init__(self, name, typ, owner): >> QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner) >> >> - 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: > > Any way to avoid the conditional? Not that I could come up with when I first tried, but it's been a while, so I can try again. I remember running into an assertion failure about nested ObjectType.check() calls if I did it unconditionally, since we allow the following: { 'struct': 'Fork', 'data': { '*fork': 'Union' } } { 'union': 'Union', 'data': { 'branch': 'Fork' } } We must NOT call self.type.check() on Fork (because we are not embedding the members of Fork into the simple union) - but then again, what we are really doing in the QAPISchema is visiting 'branch':':obj-Union-branch-wrapper', not 'Fork'. And then there's alternates, where branches aren't necessarily QAPISchemaObjectType in the first place. > > I've tried hard to make simple unions mere sugar for flat ones. There > are a few special cases left where we need to distinguish the two, but > they're all marked TODO. > > Can we have a brief comment explaing what we're checking here? We > generally don't have such comments in check() methods so far. Sort of > okay as long as they merely assert, but you're now starting to move > semantic analysis into them, which raises the bar. Yeah, I agree with adding more documentation about the checks. > >> + self.type.check(schema) > > Uh, careful. > > It's always okay for a check() method to call the check() of a child in > the abstract syntax tree, e.g. QAPISchemaObjectType.check() calling > m.check() for its members. A tree has no cycles. > > Calling it on anything else requires a non-trivial correctness argument. > Example: QAPISchemaObjectType.check() calls self.base.check(). Okay, > because no type may be a base of itself, and therefore a cycle would be > an error. The code takes care to detect cycles. > > I think the correctness argument here would be "no type contain itself > as variant member, and therefore a cycle would be an error." Do we > detect cycles? Not in this patch, but the assertion failure I ran into is the very reason I wrote a cycle detection patch next (11/12 in this series); maybe I should rebase that patch first? But you are correct that a flat union must not include itself as one of the branch types. I originally tried to test that in v5 3/46; your review convinced me it is no different than any other branch type that injects the same QMP members into the flat union, so I revamped the test in v6 to be a self-inheritance test instead, and then we dropped it after v7. (Search for "fork" in https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05733.html) > >> + assert isinstance(self.type.members, list) > > Implied by QAPISchemaObjectType.check()'s postcondition. Feels > redundant here, in particular since you iterate over it next anyway. Probably redundant, but I added it when I hit a cycle loop, to make sure that I wasn't still hitting a cycle (the 'if flat:' condition was the only way I could figure at the time to ensure I didn't hit this assertion).
diff --git a/scripts/qapi.py b/scripts/qapi.py index 1acf02b..e58c5a8 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,15 +577,9 @@ 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 @@ -611,11 +590,6 @@ def check_union(expr, 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 +664,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=[]): @@ -1046,16 +1018,28 @@ 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, + self.tag_name is not None) class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def __init__(self, name, typ, owner): QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner) - 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: + self.type.check(schema) + assert isinstance(self.type.members, list) + 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())) def describe(self): return "'%s' (branch of %s)" % (self.name, self._owner) 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 defer 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> --- v6: rebase to earlier testsuite improvements, fold in cleanup of flat-union-clash-type --- scripts/qapi.py | 46 +++++++++------------------ 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, 19 insertions(+), 35 deletions(-)