[v4,10/10] qapi: Populate info['name'] for each entity

Every non-implicit entity is associated with an 'info'
dictionary, but it is not easy to reverse-engineer the name of
the top-most entity associated with that 'info'.  Our use of
'case_whitelist' (added in commit 893e1f2) is thus currently
tied to the owner of a member instead; but as the previous patch
showed with CpuInfo, this requires whitelist exceptions to know
how an implicit name will be generated.

While we have a ._pretty_owner() that maps from implicit names
back to a human readable phrase, that produces more than just a
plain top-level entity name.  What's more, the current use of
._pretty_owner() is via .check_clash(), which can be called on
the same member object more than once (once through the
standalone type, and a second time when used as a base class of
a derived tpye); if a clash is only introduced in the derived
class, using ._pretty_owner() to report the error on behalf of
the base class named in member.owner seems wrong.  Therefore,
we need a new mechanism.

Add a new info['name'] field to track the information we need,
allowing us to change 'case_whitelist' to use only names present
in the qapi files.

Unfortunately, there is no one good place to add the mapping:
at the point 'info' is created in QAPISchemaParser.__init__(),
we don't know the name.  Info is then stored into
QAPISchemaParser.exprs, which then then gets fed to
QAPISchema.__init__() via the global check_exprs(), but we want
check_exprs() to go away.  And QAPISchema._def_exprs() sees
every entity, except that the various _def_FOO() helpers don't
return anything.  So we have to modify all of the _def_FOO()
methods.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v4: Change series again :)
[previously posted in subset F]:
v6: sink later in series, and rework commit message
[previously posted in subset D]:
v14: rearrange assignment, improve commit message
v13: new patch
---
 scripts/qapi.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Eric Blake <eblake@redhat.com> writes:

> Every non-implicit entity is associated with an 'info'
> dictionary, but it is not easy to reverse-engineer the name of
> the top-most entity associated with that 'info'.  Our use of
> 'case_whitelist' (added in commit 893e1f2) is thus currently
> tied to the owner of a member instead; but as the previous patch
> showed with CpuInfo, this requires whitelist exceptions to know
> how an implicit name will be generated.

Why is that a problem?

> While we have a ._pretty_owner() that maps from implicit names
> back to a human readable phrase, that produces more than just a
> plain top-level entity name.  What's more, the current use of
> ._pretty_owner() is via .check_clash(), which can be called on
> the same member object more than once (once through the
> standalone type, and a second time when used as a base class of
> a derived tpye); if a clash is only introduced in the derived
> class, using ._pretty_owner() to report the error on behalf of
> the base class named in member.owner seems wrong.  Therefore,
> we need a new mechanism.

Now I'm confused.  Are you fixing suboptimal error messages?

If yes, can you show the message before & after the patch?

> Add a new info['name'] field to track the information we need,
> allowing us to change 'case_whitelist' to use only names present
> in the qapi files.
>
> Unfortunately, there is no one good place to add the mapping:
> at the point 'info' is created in QAPISchemaParser.__init__(),
> we don't know the name.  Info is then stored into
> QAPISchemaParser.exprs, which then then gets fed to
> QAPISchema.__init__() via the global check_exprs(), but we want
> check_exprs() to go away.  And QAPISchema._def_exprs() sees
> every entity, except that the various _def_FOO() helpers don't
> return anything.  So we have to modify all of the _def_FOO()
> methods.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: Change series again :)
> [previously posted in subset F]:
> v6: sink later in series, and rework commit message
> [previously posted in subset D]:
> v14: rearrange assignment, improve commit message
> v13: new patch
> ---
>  scripts/qapi.py | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index ebcd207..6c991c3 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -62,8 +62,8 @@ returns_whitelist = [
>  # Whitelist of entities allowed to violate case conventions
>  case_whitelist = [
>      # From QMP:
> -    ':obj-CpuInfo-base',    # CPU, visible through query-cpu
>      'ACPISlotType',         # DIMM, visible through query-acpi-ospm-status
> +    'CpuInfo',              # CPU and PC, visible through query-cpu
>      'CpuInfoMIPS',          # PC, visible through query-cpu
>      'CpuInfoTricore',       # PC, visible through query-cpu
>      'QapiErrorClass',       # all members, visible through errors
> @@ -1035,7 +1035,7 @@ class QAPISchemaMember(object):
>
>      def check_clash(self, info, seen):
>          cname = c_name(self.name)
> -        if cname.lower() != cname and self.owner not in case_whitelist:
> +        if cname.lower() != cname and info['name'] not in case_whitelist:
>              raise QAPIExprError(info,
>                                  "%s should not use uppercase" % self.describe())
>          if cname in seen:

Doesn't look like you're touching the error message: QAPIExprError()
doesn't use info['name'].

> @@ -1296,7 +1296,7 @@ class QAPISchema(object):
>          return name
>
>      def _def_enum_type(self, expr, info):
> -        name = expr['enum']
> +        name = info['name'] = expr['enum']
>          data = expr['data']
>          prefix = expr.get('prefix')
>          self._def_entity(QAPISchemaEnumType(
> @@ -1317,7 +1317,7 @@ class QAPISchema(object):
>                  for (key, value) in data.iteritems()]
>
>      def _def_struct_type(self, expr, info):
> -        name = expr['struct']
> +        name = info['name'] = expr['struct']
>          base = expr.get('base')
>          data = expr['data']
>          self._def_entity(QAPISchemaObjectType(name, info, base,
> @@ -1336,7 +1336,7 @@ class QAPISchema(object):
>          return QAPISchemaObjectTypeVariant(case, typ)
>
>      def _def_union_type(self, expr, info):
> -        name = expr['union']
> +        name = info['name'] = expr['union']
>          data = expr['data']
>          base = expr.get('base')
>          tag_name = expr.get('discriminator')
> @@ -1362,7 +1362,7 @@ class QAPISchema(object):
>                                                                variants)))
>
>      def _def_alternate_type(self, expr, info):
> -        name = expr['alternate']
> +        name = info['name'] = expr['alternate']
>          data = expr['data']
>          variants = [self._make_variant(key, value)
>                      for (key, value) in data.iteritems()]
> @@ -1374,7 +1374,7 @@ class QAPISchema(object):
>                                                                   variants)))
>
>      def _def_command(self, expr, info):
> -        name = expr['command']
> +        name = info['name'] = expr['command']
>          data = expr.get('data')
>          rets = expr.get('returns')
>          gen = expr.get('gen', True)
> @@ -1389,7 +1389,7 @@ class QAPISchema(object):
>                                             success_response))
>
>      def _def_event(self, expr, info):
> -        name = expr['event']
> +        name = info['name'] = expr['event']
>          data = expr.get('data')
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(

On 03/08/2016 09:46 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Every non-implicit entity is associated with an 'info'
>> dictionary, but it is not easy to reverse-engineer the name of
>> the top-most entity associated with that 'info'.  Our use of
>> 'case_whitelist' (added in commit 893e1f2) is thus currently
>> tied to the owner of a member instead; but as the previous patch
>> showed with CpuInfo, this requires whitelist exceptions to know
>> how an implicit name will be generated.
> 
> Why is that a problem?

Not necessarily a bad problem, but a bit annoying. If a developer
modifies a .json file and adds an improper name, then they will get an
error message that tells them that they need to fix their naming
conventions (hmm, the error message doesn't even point them to the
whitelist - see args-member-case.err).  If the developer figures out
that the whitelist will let them avoid the error, they still have to
figure _what_ name to add to the whitelist, and without this patch, they
have to determine the generated name for the implicit struct (which may
not be constant, since we are discussing about alternative names earlier
in the series other than something that flattens to a public 'struct
_obj_...' in violation of file-local naming scope).  The goal is thus to
make the whitelist tied only to names mentioned in the .json file,
rather than dragging generated implicit names into the mix.

But it is cosmetic; we could live without the patch and stick to
generated names in the whitelist, just as easily.

> 
>> While we have a ._pretty_owner() that maps from implicit names
>> back to a human readable phrase, that produces more than just a
>> plain top-level entity name.  What's more, the current use of
>> ._pretty_owner() is via .check_clash(), which can be called on
>> the same member object more than once (once through the
>> standalone type, and a second time when used as a base class of
>> a derived tpye); if a clash is only introduced in the derived
>> class, using ._pretty_owner() to report the error on behalf of
>> the base class named in member.owner seems wrong.  Therefore,
>> we need a new mechanism.
> 
> Now I'm confused.  Are you fixing suboptimal error messages?

No, I was trying to document why ._pretty_owner() (which is our only
pre-exisiting map from Python objects back to pretty type names) is
insufficient for the task at hand (namely, what is the pretty name of
the type to add to the whitelist, if we don't want generated implicit
names in the whitelist).

Eric Blake <eblake@redhat.com> writes:

> On 03/08/2016 09:46 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Every non-implicit entity is associated with an 'info'
>>> dictionary, but it is not easy to reverse-engineer the name of
>>> the top-most entity associated with that 'info'.  Our use of
>>> 'case_whitelist' (added in commit 893e1f2) is thus currently
>>> tied to the owner of a member instead; but as the previous patch
>>> showed with CpuInfo, this requires whitelist exceptions to know
>>> how an implicit name will be generated.
>> 
>> Why is that a problem?
>
> Not necessarily a bad problem, but a bit annoying. If a developer
> modifies a .json file and adds an improper name, then they will get an
> error message that tells them that they need to fix their naming
> conventions (hmm, the error message doesn't even point them to the
> whitelist - see args-member-case.err).

I'd consider that a feature :)  I don't want anyone adding to that white
list.

>                                         If the developer figures out
> that the whitelist will let them avoid the error, they still have to
> figure _what_ name to add to the whitelist, and without this patch, they
> have to determine the generated name for the implicit struct

Still feature...

>                                                              (which may
> not be constant, since we are discussing about alternative names earlier
> in the series other than something that flattens to a public 'struct
> _obj_...' in violation of file-local naming scope).

Whoever messes with the generated names gets to update the whitelist.

>                                                      The goal is thus to
> make the whitelist tied only to names mentioned in the .json file,
> rather than dragging generated implicit names into the mix.
>
> But it is cosmetic; we could live without the patch and stick to
> generated names in the whitelist, just as easily.

Let's drop this patch.

If we find a truly compelling use for info['name'] later, we can revisit
it if you like, because then it's almost free.

[...]