From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44628) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zlr6w-00027D-GS for qemu-devel@nongnu.org; Tue, 13 Oct 2015 00:22:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zlr6u-0005oB-NW for qemu-devel@nongnu.org; Tue, 13 Oct 2015 00:22:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39562) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zlr6u-0005nh-Cn for qemu-devel@nongnu.org; Tue, 13 Oct 2015 00:22:48 -0400 From: Eric Blake Date: Mon, 12 Oct 2015 22:22:35 -0600 Message-Id: <1444710158-8723-16-git-send-email-eblake@redhat.com> In-Reply-To: <1444710158-8723-1-git-send-email-eblake@redhat.com> References: <1444710158-8723-1-git-send-email-eblake@redhat.com> Subject: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: armbru@redhat.com, Michael Roth 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. Checking variants is tricky: we need to check that the branch name will not cause a collision (important for C code, but no bearing on QMP). Then, if the variant belongs to a union (flat or simple), we need to check that QMP members of that type will not collide with non-variant QMP members (but do not care if they collide with C branch names). Each call to variant.check() has a 'seen' that contains all [*] non-variant C names (which includes all non-variant QMP names), but does not add to that array (QMP members of one branch do not cause collisions with other branches). This means that there is one form of collision we still miss: when two branch names collide. However, that will be dealt with in the next patch. [*] Exception - the 'seen' array doesn't contain 'base', which is currently a non-variant C member of structs; but since structs don't contain variants, it doesn't hurt. Besides, a later patch will be unboxing structs the way flat unions have already been unboxed. The wording of several error messages has changed, but in many cases feels like an improvement rather than a regression. The recent change (commit 7b2a5c2) 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 --- v8: decide whether to inline members based on union vs. alternate, not on flat vs. simple, and fix logic to avoid breaking union-clash-data in the process; add comments; assumes pull-qapi-2015-10-12 will go in without modifying commit ids v7: comment improvements, retitle subject v6: rebase to earlier testsuite improvements, fold in cleanup of flat-union-clash-type --- scripts/qapi.py | 70 ++++++++++++--------------- 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, 36 insertions(+), 42 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 58c4bb3..144dd4a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -496,21 +496,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'] @@ -589,30 +574,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: @@ -687,8 +660,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=[]): @@ -982,7 +953,7 @@ class QAPISchemaObjectType(QAPISchemaType): for m in self.local_members: m.check(schema, self.info, members, seen) if self.variants: - self.variants.check(schema, self.info, members, seen) + self.variants.check(schema, self.info, members, seen, True) self.members = members def is_implicit(self): @@ -1094,7 +1065,7 @@ class QAPISchemaObjectTypeVariants(object): for v in self.variants: v.set_owner(name) - def check(self, schema, info, members, seen): + def check(self, schema, info, members, seen, union): if self.tag_name: self.tag_member = seen[c_name(self.tag_name)] assert self.tag_name == self.tag_member.name @@ -1102,18 +1073,41 @@ class QAPISchemaObjectTypeVariants(object): self.tag_member.check(schema, info, members, seen) 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, dict(seen), union) class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def __init__(self, name, typ): QAPISchemaObjectTypeMember.__init__(self, name, typ, False) - def check(self, schema, info, tag_type, seen): - QAPISchemaObjectTypeMember.check(self, schema, info, [], seen) + # TODO remove union argument once alternate types can be distinguished + # solely by their different tag_type + def check(self, schema, info, tag_type, seen, union): + # Check that the branch name does not collide with non-variant C + # members, without modifying caller's copy of seen + # TODO Detect collisions between branch names in C - easiest + # will be to check instead for collisions in the corresponding + # enum type + QAPISchemaObjectTypeMember.check(self, schema, info, [], dict(seen)) assert self.name in tag_type.values + # Additionally, for unions (flat or simple), the QMP members of the + # (possibly implicit) variant type are flattened into the owner + # object, and must not collide with any non-variant members. For + # alternates, no flattening occurs. No type can contain itself + # directly as a variant. + if union: + assert isinstance(self.type, QAPISchemaObjectType) + 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 def simple_union_type(self): @@ -1136,7 +1130,7 @@ class QAPISchemaAlternateType(QAPISchemaType): self.variants = variants def check(self, schema): - self.variants.check(schema, self.info, [], {}) + self.variants.check(schema, self.info, [], {}, False) def json_type(self): return 'value' 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) -- 2.4.3