Eric Blake <ebl...@redhat.com> writes:
> 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. Doing this was made
> easier by adding a member.c_name() helper function.
This gains protection against colliding C names. It keeps protection
against colliding QMP names as long as QMP name collision implies C name
collision. I think that's the case, but it's definitely worth spelling
out in the code, and possibly in the commit message.
> As this is the first error raised within the QAPISchema*.check()
> methods, we also have to pass 'info' through the call stack, and
> fix the overall 'try' to display errors detected during the
> check() phase.
Could also be a separate patch, if the parts are easier to review.
> This fixes a couple of previously-broken tests, and the
Just two.
> resulting error messages demonstrate the utility of the
> .describe() method added previously. No change to generated
> code.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v6: rebase to earlier testsuite and info improvements
> ---
> scripts/qapi.py | 46
> ++++++++++++++++----------
> 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, 40 insertions(+), 47 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 880de94..1acf02b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -103,6 +103,7 @@ class QAPISchemaError(Exception):
> class QAPIExprError(Exception):
> def __init__(self, expr_info, msg):
> Exception.__init__(self)
> + assert expr_info
> self.info = expr_info
> self.msg = msg
>
Assertion should hold before the patch. Could therefore be added in a
separate patch before this one. Only if we have enough material for
a separate patch, or it makes this one materially easier to review.
> @@ -964,11 +965,12 @@ class QAPISchemaObjectType(QAPISchemaType):
> members = []
> seen = {}
> for m in members:
> - seen[m.name] = m
> + assert m.c_name() not in seen
> + seen[m.c_name()] = m
Assertion should hold before the patch.
> 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):
> @@ -1004,12 +1006,19 @@ class QAPISchemaObjectTypeMember(object):
> self.optional = optional
> self._owner = owner
>
> - def check(self, schema, all_members, seen):
> - assert self.name not in seen
> + def check(self, schema, info, all_members, seen):
> + if self.c_name() in seen:
> + raise QAPIExprError(info,
> + "%s collides with %s"
> + % (self.describe(),
> + seen[self.c_name()].describe()))
> self.type = schema.lookup_type(self._type_name)
> assert self.type
> all_members.append(self)
> - seen[self.name] = self
> + seen[self.c_name()] = self
> +
> + def c_name(self):
> + return c_name(self.name)
Why wrap function c_name() in a method? Why not simply call the
function?
It's method in QAPISchemaEntity only because this lets us add special
cases in a neat way.
>
> def describe(self):
> return "'%s' (member of %s)" % (self.name, self._owner)
> @@ -1028,23 +1037,24 @@ class QAPISchemaObjectTypeVariants(object):
> self.tag_member = tag_member
> self.variants = variants
>
> - 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
Assertion should hold before the patch, but it's kind of trivial then.
> 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, owner):
> QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)
>
> - 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
>
> def describe(self):
> @@ -1069,7 +1079,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'
> @@ -1128,14 +1138,14 @@ class QAPISchema(object):
> def __init__(self, fname):
> try:
> self.exprs = check_exprs(QAPISchemaParser(open(fname,
> "r")).exprs)
> + self._entity_dict = {}
> + self._def_predefineds()
> + QAPISchema.predefined_initialized = True
> + self._def_exprs()
> + self.check()
> except (QAPISchemaError, QAPIExprError), err:
> print >>sys.stderr, err
> exit(1)
> - self._entity_dict = {}
> - self._def_predefineds()
> - QAPISchema.predefined_initialized = True
> - self._def_exprs()
> - self.check()
Moving code into the try wholesale is okay because we catch only our own
exceptions.
>
> def _def_entity(self, ent):
> assert ent.name not in self._entity_dict
> diff --git a/tests/qapi-schema/args-name-clash.err
> b/tests/qapi-schema/args-name-clash.err
> index e69de29..66f609c 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' (member of oops arguments)
> collides with 'a-b' (member of oops arguments)
> 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
I'm fine with not splitting this patch. I'd be also fine with splitting
it up. Your choice.
