Eric Blake <ebl...@redhat.com> writes:
> On 11/27/2015 02:42 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> writes:
>>
>>> We document that members of enums and objects should be
>>> 'lower-case', although we were not enforcing it. We have to
>>> whitelist a few pre-existing entities that violate the norms.
>>> Add three new tests to expose the new error message, each of
>>> which first uses the whitelisted name 'UuidInfo' to prove the
>>> whitelist works, then triggers the failure.
>>>
>>> Note that by adding this check, we have effectively forbidden
>>> an entity with a case-insensitive clash of member names, for
>>> any entity that is not on the whitelist (although there is
>>> still the possibility to clash via '-' vs. '_').
>>>
>>> Signed-off-by: Eric Blake <ebl...@redhat.com>
>> [...]
>>> @@ -1039,6 +1054,10 @@ class QAPISchemaMember(object):
>>>
>>> def check_clash(self, info, seen):
>>> cname = c_name(self.name)
>>> + if cname.lower() != cname and info['name'] not in case_whitelist:
>>> + raise QAPIExprError(info,
>>> + "Member '%s' of '%s' should use lowercase"
Hmm.
tests/qapi-schema/args-member-case.json:3: Member 'Arg' of 'Foo' should use
lowercase
'Foo' *does* use lowercase: 'o'. Easiest fix is "should not use
uppercase".
>>> + % (self.name, info['name']))
>>> if cname in seen:
>>> raise QAPIExprError(info,
>>> "%s collides with %s"
>>
>> As far as I can tell, this is the only use of info['name'] in this
>> series.
>
> Yes, although I may find more uses for it later.
>
>>
>> Can you give an example where info['name'] != self.owner?
>
> Sure; this triggers lots of debug lines before crashing[1]:
>
> diff --git i/scripts/qapi.py w/scripts/qapi.py
> index 6a77db4..ec59682 100644
> --- i/scripts/qapi.py
> +++ w/scripts/qapi.py
> @@ -1054,6 +1054,8 @@ class QAPISchemaMember(object):
>
> def check_clash(self, info, seen):
> cname = c_name(self.name)
> + if info['name'] != self.owner:
> + print ' ** checking differs in %s, owner is %s' % (info['name'],
> self.owner)
Crash is easy to avoid: if info and info['name'] != self.owner.
> if cname.lower() != cname and info['name'] not in case_whitelist:
> raise QAPIExprError(info,
> "Member '%s' of '%s' should use lowercase"
% (self.name, info['name']))
if cname in seen:
raise QAPIExprError(info,
"%s collides with %s"
% (self.describe(), seen[cname].describe()))
Two separate uses of info['name']:
a. Key for the whitelist
b. Error message
> The very first one is:
>
> ** checking differs in block_passwd, owner is :obj-block_passwd-arg
>
> Remember, QAPISchemaMember.owner is the innermost (possibly-implicit)
> type that owns the member, while info['name'] is the name of the
> top-level entity that encloses the member. So the two are not always
> equal. member._pretty_owner() converts from an implicit struct name
> back to the top-level entity, but not directly (it is a human-readable
> phrase, not the plain entity name).
>
> Furthermore, look at CpuInfo's member 'CPU': there, we have two call
> paths (one with info['name'] == 'CpuInfo', the other with it as
> 'CpuInfoBase') but both call paths would see only self.owner ==
> 'CpuInfoBase'. The whitelist covers both struct names. Perhaps
> whitelisting only 'self.owner' names would be sufficient; but then the
> whitelist would have to use implicit type names rather than entity names
> from the .json file.
With the crash avoided, I get 191 distinct lines. I can sort them into
just give buckets:
1. object vs. base
Example: in CpuInfo, owner is CpuInfoBase
self._pretty_owner() returns "(member of CpuInfoBase). Again, this
would do for error message use. For instance,
{ 'struct': 'Foo', 'data': { 'Memb': 'int' } }
would produce
foo.json:1: 'Memb' (member of Foo) should use lowercase
That leaves the whitelist key use. Using .owner as key would work,
as you already explained. The whitelist becomes slightly more
cumbersome, but also slightly tighter.
2. command / event FOO vs. :obj-FOO-arg
Example: in block_passwd, owner is :obj-block_passwd-arg
self._pretty_owner() returns '(parameter of FOO)'. This would do for
error message use, just like it does for the "collides with error
right below.
"%s should use lowercase" % self.describe()
yields something like
tests/qapi-schema/args-member-case.json:3: 'Arg' (parameter of Foo)
should use lowercase
Again, .owner works as whitelist key. This is a case where we'd have
to use implicit type names. However, we don't actually have an
offender to cover.
3. flat union vs. branch
Example: in BlockdevOptions, owner is BlockdevOptionsArchipelago
self._pretty_owner() returns '(member of
BlockdevOptionsArchipelago)'. Error message using that is again
satisfactory:
tests/qapi-schema/union-branch-case.json:3: 'Branch' (branch of Foo)
should use lowercase
Again, .owner works as whitelist key. Saves us whitelist entry
'CpuInfo', as you observed.
4. simple union vs. implicit tag enum
Example: in ChardevBackend, owner is ChardevBackendKind
self._pretty_owner() returns '(branch of ChardevBackend)'. Again,
this would do for error message use. For instance,
{ 'union': 'Foo', 'data': { 'Branch': 'int' } }
would produce
foo.json:1: 'Branch' (branch of Foo) should use lowercase
Again, .owner would work as whitelist key, if we had offenders to
whitelist.
5. simple union FOO vs. member wrapper :obj-BAR-wrapper
Example: in ChardevBackend, owner is :obj-ChardevDummy-wrapper
self._pretty_owner() returns '(branch of BAR)'. This is actually
*wrong*: it's a branch of FOO, not a branch of BAR! We haven't
noticed until now, because we don't actually reach this code. We can
reach it only via .describe(), the only use of .describe() is
.check_clash(), and we check a simple union's implicit enum before
its members. Thus, a simple union member clash will always be found
and reported for the implicit enum, where the error message is
correct, and not for the member list, where it would be wrong.
The obvious knee jerk fix is to replace the incorrect code by an
assertion with a suitable comment.
Once we can actually reach it, we can consider *how* we reach it, and
what the appropriate description for a simple union's wrapped members
may be. There's hope it stays unreachable: implicit stuff like this
should not produce errors.
Bottom line for now: this case does not matter for the problem at
hand, namely enforcing "member names are in lower case".
> [1] The crash is "TypeError: 'NoneType' object has no attribute
> '__getitem__'" at the point where QType is being tested. Normally,
> QType is well-formed, so even though it is a builtin type and therefore
> has info == None, the 'cname.lower() != cname' test never fails and we
> short-circuit past an attempt to dereference None; but not so with my
> temporary print hack.
As an experiment, I used self.owner and dropped "qapi: Populate
info['name'] for each entity". I'll post the result as a reply to your
v14.
