| Message ID | 1443930073-19359-11-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@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. 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 <eblake@redhat.com> > > --- > 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 11ffc49..30f1483 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -993,11 +993,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): > @@ -1033,13 +1033,19 @@ class QAPISchemaObjectTypeMember(object): > self.optional = optional > self.owner = None # will be filled by owner's init > > - 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) > @@ -1080,23 +1086,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 > 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 > @@ -1124,7 +1131,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..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) "(argument of oops)" would be better, but "(member of oops arguments)" will do. > 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 0e986b6..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 arguments > - member a-b: str optional=False > - member a_b: str optional=False > -command oops :obj-oops arguments -> 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
On 10/09/2015 08:11 AM, Markus Armbruster wrote: > Eric Blake <eblake@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. 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 <eblake@redhat.com> >> >> +++ 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) > > "(argument of oops)" would be better, but "(member of oops arguments)" > will do. May be possible to tweak the describe() function in the previous commit along those lines, especially since I'm already tweaking it to not have space in the generated name. I'll have to play with it.
Eric Blake <eblake@redhat.com> writes: > On 10/09/2015 08:11 AM, Markus Armbruster wrote: >> Eric Blake <eblake@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. 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 <eblake@redhat.com> >>> > >>> +++ 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) >> >> "(argument of oops)" would be better, but "(member of oops arguments)" >> will do. > > May be possible to tweak the describe() function in the previous commit > along those lines, especially since I'm already tweaking it to not have > space in the generated name. I'll have to play with it. At some point, implementation simplicity trumps error message polish. Use your judgement.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 11ffc49..30f1483 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -993,11 +993,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): @@ -1033,13 +1033,19 @@ class QAPISchemaObjectTypeMember(object): self.optional = optional self.owner = None # will be filled by owner's init - 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) @@ -1080,23 +1086,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 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 @@ -1124,7 +1131,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..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 0e986b6..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 arguments - member a-b: str optional=False - member a_b: str optional=False -command oops :obj-oops arguments -> 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
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 <eblake@redhat.com> --- 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(-)