[v10,27/30] qapi: Track owner of each object member

Future commits will migrate semantic checking away from parsing
and over to the various QAPISchema*.check() methods.  But to
report an error message about an incorrect semantic use of a
member of an object type, it helps to know which type, command,
or event owns the member.  In particular, when a member is
inherited from a base type, it is desirable to associate the
member name with the base type (and not the type calling
member.check()).

Rather than packing additional information into the seen array
passed to each member.check() (as in seen[m.name] = {'member':m,
'owner':type}), it is easier to have each member track the name
of the owner type in the first place (keeping things simpler
with the existing seen[m.name] = m).  The new member.owner field
is set via a new set_owner() method, called when registering
the members and variants arrays with an object or variant type.
Track only a name, and not the actual type object, to avoid
creating a circular python reference chain.

Note that Variants.set_owner() method does not set the owner
for the tag_member field; this field is set earlier either as
part of an object's non-variant members, or explicitly by
alternates.

The source information is intended for human consumption in
error messages, and a new describe() method is added to access
the resulting information.  For example, given the qapi:
  { 'command': 'foo', 'data': { 'string': 'str' } }
an implementation of visit_command() that calls
  arg_type.members[0].describe()
will see "'string' (argument of foo)".

To make the human-readable name of implicit types work without
duplicating efforts, the describe() method has to reverse the
name of implicit types, via the helper _pretty_owner(), plus a
tweak to report event data separately from command arguments.

No change to generated code.

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

---
v10 (now in subset C): rebase to latest, set alternate tag_member
owner from alternate
v9 (now in subset D): rebase to earlier changes, hoist 'role' to top
of class, split out _pretty_helper(), manage owner when tag_member
appears as part of local_members for unions
v8: don't munge implicit type names [except for event data], and
instead make describe() create nicer messages. Add set_owner(), and
use field 'role' instead of method _describe()
v7: total rewrite: rework implicit object names, assign owner
when initializing owner type rather than when creating member
python object
v6: rebase on new lazy array creation and simple union 'type'
motion; tweak commit message
---
 scripts/qapi.py                        | 44 +++++++++++++++++++++++++++++++---
 tests/qapi-schema/qapi-schema-test.out |  8 +++----
 2 files changed, 45 insertions(+), 7 deletions(-)

Eric Blake <eblake@redhat.com> writes:

> Future commits will migrate semantic checking away from parsing
> and over to the various QAPISchema*.check() methods.  But to
> report an error message about an incorrect semantic use of a
> member of an object type, it helps to know which type, command,
> or event owns the member.  In particular, when a member is
> inherited from a base type, it is desirable to associate the
> member name with the base type (and not the type calling
> member.check()).
>
> Rather than packing additional information into the seen array
> passed to each member.check() (as in seen[m.name] = {'member':m,
> 'owner':type}), it is easier to have each member track the name
> of the owner type in the first place (keeping things simpler
> with the existing seen[m.name] = m).  The new member.owner field
> is set via a new set_owner() method, called when registering
> the members and variants arrays with an object or variant type.
> Track only a name, and not the actual type object, to avoid
> creating a circular python reference chain.
>
> Note that Variants.set_owner() method does not set the owner
> for the tag_member field; this field is set earlier either as
> part of an object's non-variant members, or explicitly by
> alternates.
>
> The source information is intended for human consumption in
> error messages, and a new describe() method is added to access
> the resulting information.  For example, given the qapi:
>   { 'command': 'foo', 'data': { 'string': 'str' } }
> an implementation of visit_command() that calls
>   arg_type.members[0].describe()
> will see "'string' (argument of foo)".
>
> To make the human-readable name of implicit types work without
> duplicating efforts, the describe() method has to reverse the
> name of implicit types, via the helper _pretty_owner(), plus a
> tweak to report event data separately from command arguments.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v10 (now in subset C): rebase to latest, set alternate tag_member
> owner from alternate
> v9 (now in subset D): rebase to earlier changes, hoist 'role' to top
> of class, split out _pretty_helper(), manage owner when tag_member
> appears as part of local_members for unions
> v8: don't munge implicit type names [except for event data], and
> instead make describe() create nicer messages. Add set_owner(), and
> use field 'role' instead of method _describe()
> v7: total rewrite: rework implicit object names, assign owner
> when initializing owner type rather than when creating member
> python object
> v6: rebase on new lazy array creation and simple union 'type'
> motion; tweak commit message
> ---
>  scripts/qapi.py                        | 44 +++++++++++++++++++++++++++++++---
>  tests/qapi-schema/qapi-schema-test.out |  8 +++----
>  2 files changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index b914785..b3af973 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -957,8 +957,10 @@ class QAPISchemaObjectType(QAPISchemaType):
>          assert base is None or isinstance(base, str)
>          for m in local_members:
>              assert isinstance(m, QAPISchemaObjectTypeMember)
> -        assert (variants is None or
> -                isinstance(variants, QAPISchemaObjectTypeVariants))
> +            m.set_owner(name)
> +        if variants is not None:
> +            assert isinstance(variants, QAPISchemaObjectTypeVariants)
> +            variants.set_owner(name)
>          self._base_name = base
>          self.base = None
>          self.local_members = local_members
> @@ -1012,6 +1014,8 @@ class QAPISchemaObjectType(QAPISchemaType):
>
>
>  class QAPISchemaObjectTypeMember(object):
> +    role = 'member'
> +
>      def __init__(self, name, typ, optional):
>          assert isinstance(name, str)
>          assert isinstance(typ, str)
> @@ -1020,8 +1024,14 @@ class QAPISchemaObjectTypeMember(object):
>          self._type_name = typ
>          self.type = None
>          self.optional = optional
> +        self.owner = None
> +
> +    def set_owner(self, name):
> +        assert not self.owner
> +        self.owner = name
>
>      def check(self, schema):
> +        assert self.owner
>          self.type = schema.lookup_type(self._type_name)
>          assert self.type
>
> @@ -1030,6 +1040,25 @@ class QAPISchemaObjectTypeMember(object):
>          assert self.name not in seen
>          seen[self.name] = self
>
> +    def _pretty_owner(self):
> +        # See QAPISchema._make_implicit_object_type() - reverse the
> +        # mapping there to create a nice human-readable description

This comment confused me briefly.  It applies to the following
conditionals if-part, but not its else-part.  Move it into the if-part?

> +        owner = self.owner
> +        if owner.startswith(':obj-'):
> +            owner = owner[5:]
> +            if owner.endswith('-arg'):
> +                return '(argument of %s)' % owner[:-4]
> +            elif owner.endswith('-data'):
> +                return '(data of %s)' % owner[:-5]
> +            else:
> +                assert owner.endswith('-wrapper')
> +                return '(branch of %s)' % owner[:-8]

See last remark.

> +        else:
> +            return '(%s of %s)' % (self.role, owner)
> +
> +    def describe(self):
> +        return "'%s' %s" % (self.name, self._pretty_owner())
> +
>
>  class QAPISchemaObjectTypeVariants(object):
>      def __init__(self, tag_name, tag_member, variants):
> @@ -1046,6 +1075,10 @@ class QAPISchemaObjectTypeVariants(object):
>          self.tag_member = tag_member
>          self.variants = variants
>
> +    def set_owner(self, name):
> +        for v in self.variants:
> +            v.set_owner(name)
> +
>      def check(self, schema, seen):
>          if self.tag_name:    # flat union
>              self.tag_member = seen[self.tag_name]
> @@ -1063,6 +1096,8 @@ class QAPISchemaObjectTypeVariants(object):
>
>
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> +    role = 'branch'
> +
>      def __init__(self, name, typ):
>          QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>
> @@ -1082,9 +1117,11 @@ class QAPISchemaAlternateType(QAPISchemaType):
>          QAPISchemaType.__init__(self, name, info)
>          assert isinstance(variants, QAPISchemaObjectTypeVariants)
>          assert not variants.tag_name
> +        variants.set_owner(name)
>          self.variants = variants
>
>      def check(self, schema):
> +        self.variants.tag_member.set_owner(self.name)
>          self.variants.tag_member.check(schema)
>          self.variants.check(schema, {})
>

Odd: all other .set_owner() calls are in .__init__() or .set_owner().
Can we move this one to __init__() for consistency?

I think we can: __init__() requires its variants argument to have a
tag_member (it even asserts not variants.tag_name).

> @@ -1212,6 +1249,7 @@ class QAPISchema(object):
>      def _make_implicit_object_type(self, name, info, role, members):
>          if not members:
>              return None
> +        # See also QAPISchemaObjectTypeMember.describe()

Should this point to ._pretty_owner() instead?

>          name = ':obj-%s-%s' % (name, role)
>          if not self.lookup_entity(name, QAPISchemaObjectType):
>              self._def_entity(QAPISchemaObjectType(name, info, None,
> @@ -1315,7 +1353,7 @@ class QAPISchema(object):
>          data = expr.get('data')
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
> -                name, info, 'arg', self._make_members(data, info))
> +                name, info, 'data', self._make_members(data, info))

This is necessary only to make .pretty_owner() say 'data of EVENT_NAME'
instead of 'argument of EVENT_NAME'.  Do we really need different
wording for commands and events?

I'd make it say 'parameter of' for both commands and events.  I
habitually use "parameter" for formal parameters, and "argument" for
actual arguments.  Guess I've worked with the C standard too much...

For what it's worth, the QAPI schema language uses 'data' for both (and
many other things, too), and the introspection schema uses 'arg-type'
for both.

>          self._def_entity(QAPISchemaEvent(name, info, data))
>
>      def _def_exprs(self):
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 786024e..f78ef04 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -1,9 +1,9 @@
>  object :empty
> -object :obj-EVENT_C-arg
> +object :obj-EVENT_C-data
>      member a: int optional=True
>      member b: UserDefOne optional=True
>      member c: str optional=False
> -object :obj-EVENT_D-arg
> +object :obj-EVENT_D-data
>      member a: EventStructOne optional=False
>      member b: str optional=False
>      member c: str optional=True
> @@ -79,8 +79,8 @@ alternate AltStrNum
>  enum AltStrNumKind ['s', 'n']
>  event EVENT_A None
>  event EVENT_B None
> -event EVENT_C :obj-EVENT_C-arg
> -event EVENT_D :obj-EVENT_D-arg
> +event EVENT_C :obj-EVENT_C-data
> +event EVENT_D :obj-EVENT_D-data
>  object Empty1
>  object Empty2
>      base Empty1

On 11/09/2015 07:26 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Future commits will migrate semantic checking away from parsing
>> and over to the various QAPISchema*.check() methods.  But to
>> report an error message about an incorrect semantic use of a
>> member of an object type, it helps to know which type, command,
>> or event owns the member.  In particular, when a member is
>> inherited from a base type, it is desirable to associate the
>> member name with the base type (and not the type calling
>> member.check()).
>>

>>
>> +    def _pretty_owner(self):
>> +        # See QAPISchema._make_implicit_object_type() - reverse the
>> +        # mapping there to create a nice human-readable description
> 
> This comment confused me briefly.  It applies to the following
> conditionals if-part, but not its else-part.  Move it into the if-part?

Done.


>> @@ -1082,9 +1117,11 @@ class QAPISchemaAlternateType(QAPISchemaType):
>>          QAPISchemaType.__init__(self, name, info)
>>          assert isinstance(variants, QAPISchemaObjectTypeVariants)
>>          assert not variants.tag_name
>> +        variants.set_owner(name)
>>          self.variants = variants
>>
>>      def check(self, schema):
>> +        self.variants.tag_member.set_owner(self.name)
>>          self.variants.tag_member.check(schema)
>>          self.variants.check(schema, {})
>>
> 
> Odd: all other .set_owner() calls are in .__init__() or .set_owner().
> Can we move this one to __init__() for consistency?

Yes, done.

> 
> I think we can: __init__() requires its variants argument to have a
> tag_member (it even asserts not variants.tag_name).
> 
>> @@ -1212,6 +1249,7 @@ class QAPISchema(object):
>>      def _make_implicit_object_type(self, name, info, role, members):
>>          if not members:
>>              return None
>> +        # See also QAPISchemaObjectTypeMember.describe()
> 
> Should this point to ._pretty_owner() instead?

Good call.

> 
>>          name = ':obj-%s-%s' % (name, role)
>>          if not self.lookup_entity(name, QAPISchemaObjectType):
>>              self._def_entity(QAPISchemaObjectType(name, info, None,
>> @@ -1315,7 +1353,7 @@ class QAPISchema(object):
>>          data = expr.get('data')
>>          if isinstance(data, OrderedDict):
>>              data = self._make_implicit_object_type(
>> -                name, info, 'arg', self._make_members(data, info))
>> +                name, info, 'data', self._make_members(data, info))
> 
> This is necessary only to make .pretty_owner() say 'data of EVENT_NAME'
> instead of 'argument of EVENT_NAME'.  Do we really need different
> wording for commands and events?
> 
> I'd make it say 'parameter of' for both commands and events.  I
> habitually use "parameter" for formal parameters, and "argument" for
> actual arguments.  Guess I've worked with the C standard too much...

'parameter' sounds better for both, at which point the messages no
longer differ,...

> 
> For what it's worth, the QAPI schema language uses 'data' for both (and
> many other things, too), and the introspection schema uses 'arg-type'
> for both.
> 
>>          self._def_entity(QAPISchemaEvent(name, info, data))
>>
>>      def _def_exprs(self):
>> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
>> index 786024e..f78ef04 100644
>> --- a/tests/qapi-schema/qapi-schema-test.out
>> +++ b/tests/qapi-schema/qapi-schema-test.out
>> @@ -1,9 +1,9 @@
>>  object :empty
>> -object :obj-EVENT_C-arg
>> +object :obj-EVENT_C-data
>>      member a: int optional=True
>>      member b: UserDefOne optional=True
>>      member c: str optional=False
>> -object :obj-EVENT_D-arg
>> +object :obj-EVENT_D-data

...so I no longer need to rename the implicit struct for events to allow
the differentiation.  v11 is thus a smaller patch :)