From patchwork Tue Oct 13 04:22:34 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 529571 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 6ABFB140273 for ; Tue, 13 Oct 2015 15:34:05 +1100 (AEDT) Received: from localhost ([::1]:60519 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZlrHn-0002OZ-2w for incoming@patchwork.ozlabs.org; Tue, 13 Oct 2015 00:34:03 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44606) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zlr6w-00026X-7W 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-0005ni-A4 for qemu-devel@nongnu.org; Tue, 13 Oct 2015 00:22:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39560) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zlr6t-0005nJ-T1 for qemu-devel@nongnu.org; Tue, 13 Oct 2015 00:22:48 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 843DEC0C9A6D; Tue, 13 Oct 2015 04:22:47 +0000 (UTC) Received: from red.redhat.com (ovpn-113-98.phx2.redhat.com [10.3.113.98]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t9D4MeFC022380; Tue, 13 Oct 2015 00:22:47 -0400 From: Eric Blake To: qemu-devel@nongnu.org Date: Mon, 12 Oct 2015 22:22:34 -0600 Message-Id: <1444710158-8723-15-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> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: armbru@redhat.com, Michael Roth Subject: [Qemu-devel] [PATCH v8 14/18] qapi: Detect collisions in C member names X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Detect attempts to declare two object members that would result in the same C member name, by keying the 'seen' dictionary off of the C name rather than the qapi name. It also requires passing info through some of the check() methods. This fixes two previously-broken tests, and the resulting error messages demonstrate the utility of the .describe() method added previously. No change to generated code. Signed-off-by: Eric Blake --- v8: rebase to earlier changes v7: split out error reporting prep and member.c_name() addition v6: rebase to earlier testsuite and info improvements --- scripts/qapi.py | 33 ++++++++++++++++---------- tests/qapi-schema/args-name-clash.err | 1 + tests/qapi-schema/args-name-clash.exit | 2 +- tests/qapi-schema/args-name-clash.json | 6 ++--- tests/qapi-schema/args-name-clash.out | 6 ----- tests/qapi-schema/flat-union-clash-branch.err | 1 + tests/qapi-schema/flat-union-clash-branch.exit | 2 +- tests/qapi-schema/flat-union-clash-branch.json | 9 +++---- tests/qapi-schema/flat-union-clash-branch.out | 14 ----------- 9 files changed, 32 insertions(+), 42 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 8b29e11..58c4bb3 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -978,11 +978,11 @@ class QAPISchemaObjectType(QAPISchemaType): seen = {} for m in members: assert m.c_name() not in seen - seen[m.name] = m + seen[m.c_name()] = m for m in self.local_members: - m.check(schema, members, seen) + m.check(schema, self.info, members, seen) if self.variants: - self.variants.check(schema, members, seen) + self.variants.check(schema, self.info, members, seen) self.members = members def is_implicit(self): @@ -1022,13 +1022,19 @@ class QAPISchemaObjectTypeMember(object): assert not self.owner self.owner = name - def check(self, schema, all_members, seen): + def check(self, schema, info, all_members, seen): assert self.owner - assert self.name not in seen self.type = schema.lookup_type(self._type_name) assert self.type + # Check that there is no collision in generated C names (which + # also ensures no collisions in QMP names) + if self.c_name() in seen: + raise QAPIExprError(info, + "%s collides with %s" + % (self.describe(), + seen[self.c_name()].describe())) all_members.append(self) - seen[self.name] = self + seen[self.c_name()] = self def c_name(self): return c_name(self.name) @@ -1088,23 +1094,24 @@ class QAPISchemaObjectTypeVariants(object): for v in self.variants: v.set_owner(name) - def check(self, schema, members, seen): + def check(self, schema, info, members, seen): if self.tag_name: - self.tag_member = seen[self.tag_name] + self.tag_member = seen[c_name(self.tag_name)] + assert self.tag_name == self.tag_member.name else: - self.tag_member.check(schema, members, seen) + 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, self.tag_member.type, vseen) + v.check(schema, info, self.tag_member.type, vseen) 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) + def check(self, schema, info, tag_type, seen): + QAPISchemaObjectTypeMember.check(self, schema, info, [], seen) assert self.name in tag_type.values # This function exists to support ugly simple union special cases @@ -1129,7 +1136,7 @@ class QAPISchemaAlternateType(QAPISchemaType): self.variants = variants def check(self, schema): - self.variants.check(schema, [], {}) + self.variants.check(schema, self.info, [], {}) def json_type(self): return 'value' diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err index e69de29..2735217 100644 --- a/tests/qapi-schema/args-name-clash.err +++ b/tests/qapi-schema/args-name-clash.err @@ -0,0 +1 @@ +tests/qapi-schema/args-name-clash.json:5: 'a_b' (argument of oops) collides with 'a-b' (argument of oops) diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/args-name-clash.exit +++ b/tests/qapi-schema/args-name-clash.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json index 9e8f889..3fe4ea5 100644 --- a/tests/qapi-schema/args-name-clash.json +++ b/tests/qapi-schema/args-name-clash.json @@ -1,5 +1,5 @@ # C member name collision -# FIXME - This parses, but fails to compile, because the C struct is given -# two 'a_b' members. Either reject this at parse time, or munge the C names -# to avoid the collision. +# Reject members that clash when mapped to C names (we would have two 'a_b' +# members). It would also be possible to munge the C names to avoid the +# collision, but unlikely to be worth the effort. { 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } } diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out index 9b2f6e4..e69de29 100644 --- a/tests/qapi-schema/args-name-clash.out +++ b/tests/qapi-schema/args-name-clash.out @@ -1,6 +0,0 @@ -object :empty -object :obj-oops-arg - member a-b: str optional=False - member a_b: str optional=False -command oops :obj-oops-arg -> None - gen=True success_response=True diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err index e69de29..0190d79 100644 --- a/tests/qapi-schema/flat-union-clash-branch.err +++ b/tests/qapi-schema/flat-union-clash-branch.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-clash-branch.json:15: 'c-d' (branch of TestUnion) collides with 'c_d' (member of Base) diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/flat-union-clash-branch.exit +++ b/tests/qapi-schema/flat-union-clash-branch.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json index e593336..a6c302f 100644 --- a/tests/qapi-schema/flat-union-clash-branch.json +++ b/tests/qapi-schema/flat-union-clash-branch.json @@ -1,8 +1,9 @@ # Flat union branch name collision -# FIXME: this parses, but then fails to compile due to a duplicate 'c_d' -# (one from the base member, the other from the branch name). We should -# either reject the collision at parse time, or munge the generated branch -# name to allow this to compile. +# Reject attempts to use a branch name that would clash with a non-variant +# member, when mapped to C names (here, we would have two 'c_d' members, +# one from the base member, the other from the branch name). +# TODO: We could munge the generated branch name to avoid the collision and +# allow this to compile. { 'enum': 'TestEnum', 'data': [ 'base', 'c-d' ] } { 'struct': 'Base', diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out index 8e0da73..e69de29 100644 --- a/tests/qapi-schema/flat-union-clash-branch.out +++ b/tests/qapi-schema/flat-union-clash-branch.out @@ -1,14 +0,0 @@ -object :empty -object Base - member enum1: TestEnum optional=False - member c_d: str optional=True -object Branch1 - member string: str optional=False -object Branch2 - member value: int optional=False -enum TestEnum ['base', 'c-d'] -object TestUnion - base Base - tag enum1 - case base: Branch1 - case c-d: Branch2