Now that we have separate namespaces for QMP vs. tag values, we can simplify how the QAPISchema*.check() methods check for collisions. Each QAPISchemaObjectTypeMember check() call is given a single set of names it must not collide with; this set is either the QMP names (when this member is used by an ObjectType) or the case names (when this member is used by an ObjectTypeVariants). We no longer need an all_members parameter, as it can be computed by seen.values(). When used by a union, QAPISchemaObjectTypeVariant must also perform a QMP collision check for each member of its corresponding type.
The new ObjectType.check_qmp() is an idempotent subset of check(), and can be called multiple times over different seen sets (useful, since the members of one type can be applied into more than one other location via inheritance or flat union variants). The code needs a temporary hack of passing a 'union' flag through Variants.check(), since we do not inline the branches of an alternate type into a parent QMP object. A later patch will rework how alternates are laid out, by adding a new subclass, and that will allow us to drop the extra parameter. There are no changes to generated code, and we can now add a positive test to qapi-schema-test that proves that alternates can now use names that would previously trigger assertions (see commit 7b2a5c2f for an example of the failure that was still possible for alternates before this commit). Future patches will add more positive tests, improve error message quality on actual collisions, and move collision checks out of ad hoc parse code into the check() methods. Signed-off-by: Eric Blake <ebl...@redhat.com> --- v7: new patch, although it is a much cleaner implementation of what was attempted by subset B v8 15/18 https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg03042.html --- scripts/qapi.py | 55 ++++++++++++++++++++------------- tests/qapi-schema/qapi-schema-test.json | 2 ++ tests/qapi-schema/qapi-schema-test.out | 5 +++ 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index aab2b46..098ba5d 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -974,28 +974,28 @@ class QAPISchemaObjectType(QAPISchemaType): self.variants = variants self.members = None + # Finish construction, and validate that all members are usable def check(self, schema): assert self.members is not False # not running in cycles if self.members: return self.members = False # mark as being checked + seen = OrderedDict() if self._base_name: self.base = schema.lookup_type(self._base_name) - assert isinstance(self.base, QAPISchemaObjectType) - assert not self.base.variants # not implemented - self.base.check(schema) - members = list(self.base.members) - else: - members = [] - seen = {} - for m in members: - assert c_name(m.name) not in seen - seen[m.name] = m + self.base.check_qmp(schema, seen) for m in self.local_members: - m.check(schema, members, seen) + m.check(schema, seen) if self.variants: - self.variants.check(schema, members, seen) - self.members = members + self.variants.check(schema, seen) + self.members = seen.values() + + # Check that this type does not introduce QMP collisions into seen + def check_qmp(self, schema, seen): + self.check(schema) + assert not self.variants # not implemented + for m in self.members: + m.check(schema, seen) def is_implicit(self): # See QAPISchema._make_implicit_object_type() @@ -1029,11 +1029,13 @@ class QAPISchemaObjectTypeMember(object): self.type = None self.optional = optional - def check(self, schema, all_members, seen): + def check(self, schema, seen): + # seen is a map of names we must not collide with (either QMP + # names, when called by ObjectType, or case names, when called + # by Variant). This method is safe to call over multiple 'seen'. assert self.name not in seen self.type = schema.lookup_type(self._type_name) assert self.type - all_members.append(self) seen[self.name] = self @@ -1052,24 +1054,33 @@ class QAPISchemaObjectTypeVariants(object): self.tag_member = tag_member self.variants = variants - def check(self, schema, members, seen): + # TODO drop union once alternates can be distinguished by tag_member + def check(self, schema, seen, union=True): if self.tag_name: self.tag_member = seen[self.tag_name] + assert self.tag_member else: - self.tag_member.check(schema, members, seen) + self.tag_member.check(schema, seen) assert isinstance(self.tag_member.type, QAPISchemaEnumType) + cases = OrderedDict() for v in self.variants: - vseen = dict(seen) - v.check(schema, self.tag_member.type, vseen) + # Reset seen array for each variant, since QMP names from one + # branch do not affect another branch, nor add to all_members + v.check(schema, self.tag_member.type, dict(seen), cases, union) class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def __init__(self, name, typ): QAPISchemaObjectTypeMember.__init__(self, name, typ, False) - def check(self, schema, tag_type, seen): - QAPISchemaObjectTypeMember.check(self, schema, [], seen) + # TODO drop union once alternates can be distinguished by tag_type + def check(self, schema, tag_type, seen, cases, union): + # cases is case names we must not collide with + QAPISchemaObjectTypeMember.check(self, schema, cases) assert self.name in tag_type.values + if union: + # seen is QMP names our members must not collide with + self.type.check_qmp(schema, seen) # This function exists to support ugly simple union special cases # TODO get rid of them, and drop the function @@ -1090,7 +1101,7 @@ class QAPISchemaAlternateType(QAPISchemaType): self.variants = variants def check(self, schema): - self.variants.check(schema, [], {}) + self.variants.check(schema, {}, False) def json_type(self): return 'value' diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index efa7acc..926bd7e 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -120,6 +120,8 @@ { 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'] } } { 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a', 'myList': 'has_a' } } +{ 'alternate': 'AltName', 'data': { 'type': 'int', 'u': 'bool', + 'myKind': 'has_a' } } # testing commands { 'command': 'user_def_cmd', 'data': {} } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 34fb317..1c39a2a 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -61,6 +61,11 @@ alternate AltIntNum case i: int case n: number enum AltIntNumKind ['i', 'n'] +alternate AltName + case type: int + case u: bool + case myKind: has_a +enum AltNameKind ['type', 'u', 'myKind'] alternate AltNumInt case n: number case i: int -- 2.4.3
