[v8,13/18] 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() function, 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.

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.

No change to generated code.

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

---
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                        | 48 +++++++++++++++++++++++++++++++---
 tests/qapi-schema/qapi-schema-test.out |  8 +++---
 2 files changed, 49 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() function, called when registering

method

> 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.
>
> 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.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> 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                        | 48 +++++++++++++++++++++++++++++++---
>  tests/qapi-schema/qapi-schema-test.out |  8 +++---
>  2 files changed, 49 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index c9ce9ee..8b29e11 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -952,8 +952,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
> @@ -1014,8 +1016,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, all_members, seen):
> +        assert self.owner
>          assert self.name not in seen
>          self.type = schema.lookup_type(self._type_name)
>          assert self.type
> @@ -1025,6 +1033,25 @@ class QAPISchemaObjectTypeMember(object):
>      def c_name(self):
>          return c_name(self.name)
>
> +    def describe(self):
> +        owner = self.owner
> +        # See QAPISchema._make_implicit_object_type() - reverse the
> +        # mapping there to create a nice human-readable description
> +        if owner.startswith(':obj-'):
> +            owner = owner[5:]
> +            if owner.endswith('-arg'):
> +                source = '(argument of %s)' % owner[:4]
> +            elif owner.endswith('-data'):
> +                source = '(data of %s)' % owner[:5]
> +            else:
> +                assert owner.endswith('-wrapper')
> +                source = '(branch of %s)' % owner[:8]
> +        else:
> +            source = '(%s of %s)' % (self.role, owner)
> +        return "'%s' %s" % (self.name, source)

Perhaps not exactly pretty, but it gets the job done with minimal fuss.
Sold.

> +
> +    role = 'member'
> +
>

Class variables are usually defined first, before methods.

>  # TODO Drop this class once we no longer have the 'type'/'kind' mismatch
>  class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
> @@ -1034,6 +1061,11 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>          assert self.type.is_implicit()
>          return 'kind'
>
> +    def describe(self):
> +        # Must override superclass describe() because self.name is 'type'

I don't find this argument convincing.  I think 'kind' is exactly as
unhelpful as 'type' is.  Specifically, should we report a QMP clash,
calling it 'kind' is confusing (it actually clashes with 'type').
Conversely 'type' is confusing when we report a C clash.

The helpful part...

> +        assert self.owner[0] != ':'
> +        return "'kind' (implicit tag of %s)" % self.owner
> +

... is (implicit tag of FOO).

Fortunately, you're working towards killing the kind vs. type confusion.
Suggest to either rephrase the comment, or simply drop it.

>
>  class QAPISchemaObjectTypeVariants(object):
>      def __init__(self, tag_name, tag_member, variants):
> @@ -1050,6 +1082,12 @@ class QAPISchemaObjectTypeVariants(object):
>          self.tag_member = tag_member
>          self.variants = variants
>
> +    def set_owner(self, name):
> +        if self.tag_member:
> +            self.tag_member.set_owner(name)
> +        for v in self.variants:
> +            v.set_owner(name)
> +
>      def check(self, schema, members, seen):
>          if self.tag_name:
>              self.tag_member = seen[self.tag_name]
> @@ -1079,12 +1117,15 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>              return self.type.members[0].type
>          return None
>
> +    role = 'branch'
> +
>

Define this first in the class.

>  class QAPISchemaAlternateType(QAPISchemaType):
>      def __init__(self, name, info, variants):
>          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):
> @@ -1216,6 +1257,7 @@ class QAPISchema(object):
>      def _make_implicit_object_type(self, name, info, role, members):
>          if not members:
>              return None
> +        # See also QAPISchemaObjectTypeMember.describe()
>          name = ':obj-%s-%s' % (name, role)
>          if not self.lookup_entity(name, QAPISchemaObjectType):
>              self._def_entity(QAPISchemaObjectType(name, info, None,
> @@ -1318,7 +1360,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))
>          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 a6c80e0..d837475 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
>  enum EnumOne ['value1', 'value2', 'value3']
>  object EventStructOne
>      member struct1: UserDefOne optional=False

On 10/13/2015 07:14 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 describe(self):
>> +        owner = self.owner
>> +        # See QAPISchema._make_implicit_object_type() - reverse the
>> +        # mapping there to create a nice human-readable description
>> +        if owner.startswith(':obj-'):
>> +            owner = owner[5:]
>> +            if owner.endswith('-arg'):
>> +                source = '(argument of %s)' % owner[:4]
>> +            elif owner.endswith('-data'):
>> +                source = '(data of %s)' % owner[:5]
>> +            else:
>> +                assert owner.endswith('-wrapper')
>> +                source = '(branch of %s)' % owner[:8]
>> +        else:
>> +            source = '(%s of %s)' % (self.role, owner)
>> +        return "'%s' %s" % (self.name, source)
> 
> Perhaps not exactly pretty, but it gets the job done with minimal fuss.
> Sold.

Ouch - I think these should be owner[:-4] (and similar) - I want the
slice that excludes the last four bytes, and not one that is four bytes
long.  (It happened to work in my tests because I was testing on
:obj-oops-arg, where owner[:4] and owner[:-4] gave the same 'oops' string).

It's an easy fixup to squash in, so I'm not yet sure if I need a full v9
respin.

> 
>> +
>> +    role = 'member'
>> +
>>
> 
> Class variables are usually defined first, before methods.

Sure, can do. pep8 didn't complain, and I didn't check if pylint warns
about it.

> 
>>  # TODO Drop this class once we no longer have the 'type'/'kind' mismatch
>>  class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>> @@ -1034,6 +1061,11 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>>          assert self.type.is_implicit()
>>          return 'kind'
>>
>> +    def describe(self):
>> +        # Must override superclass describe() because self.name is 'type'
> 
> I don't find this argument convincing.  I think 'kind' is exactly as
> unhelpful as 'type' is.  Specifically, should we report a QMP clash,
> calling it 'kind' is confusing (it actually clashes with 'type').
> Conversely 'type' is confusing when we report a C clash.
> 
> The helpful part...
> 
>> +        assert self.owner[0] != ':'
>> +        return "'kind' (implicit tag of %s)" % self.owner
>> +
> 
> ... is (implicit tag of FOO).
> 
> Fortunately, you're working towards killing the kind vs. type confusion.
> Suggest to either rephrase the comment, or simply drop it.

Drop the comment is fine by me.

I personally thought the message needed to report 'kind', because 16/18
ends up reporting:

+tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion)
collides with 'kind' (implicit tag of TestUnion)

If I didn't override describe() at all, it would report:

...: 'kind' (branch of TestUnion) collides with 'type' (member of TestUnion)

If all I did was override 'role = "implicit tag"' instead of describe(),
it would report:

...: 'kind' (branch of TestUnion) collides with 'type' (implicit tag of
TestUnion)

Of course when a later subset fixes the kind/type misnomer, the error
message changes:

...: 'type' (branch of TestUnion) collides with 'type' (member of TestUnion)

where we lose the "implicit tag" because we completely lose the subclass.

Any preferences on which error message to prefer, and therefore how much
(or how little) we need to override in this temporary subclass?  Or is
this an argument for making the subclass permanent (with a 'role =
"implicit tag"') even after the kind/type rename?

Eric Blake <eblake@redhat.com> writes:

> On 10/13/2015 07:14 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 describe(self):
>>> +        owner = self.owner
>>> +        # See QAPISchema._make_implicit_object_type() - reverse the
>>> +        # mapping there to create a nice human-readable description
>>> +        if owner.startswith(':obj-'):
>>> +            owner = owner[5:]
>>> +            if owner.endswith('-arg'):
>>> +                source = '(argument of %s)' % owner[:4]
>>> +            elif owner.endswith('-data'):
>>> +                source = '(data of %s)' % owner[:5]
>>> +            else:
>>> +                assert owner.endswith('-wrapper')
>>> +                source = '(branch of %s)' % owner[:8]
>>> +        else:
>>> +            source = '(%s of %s)' % (self.role, owner)
>>> +        return "'%s' %s" % (self.name, source)
>> 
>> Perhaps not exactly pretty, but it gets the job done with minimal fuss.
>> Sold.
>
> Ouch - I think these should be owner[:-4] (and similar) - I want the
> slice that excludes the last four bytes, and not one that is four bytes
> long.  (It happened to work in my tests because I was testing on
> :obj-oops-arg, where owner[:4] and owner[:-4] gave the same 'oops' string).
>
> It's an easy fixup to squash in, so I'm not yet sure if I need a full v9
> respin.

If we decide we can do without a respin, I'll want the incremental patch
to squash in.

>>> +
>>> +    role = 'member'
>>> +
>>>
>> 
>> Class variables are usually defined first, before methods.
>
> Sure, can do. pep8 didn't complain, and I didn't check if pylint warns
> about it.

PEP8 (the document) doesn't ask for it, but the examples I found in the
Python tutorial are consistently putting them first.

>>>  # TODO Drop this class once we no longer have the 'type'/'kind' mismatch
>>>  class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>>> @@ -1034,6 +1061,11 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>>>          assert self.type.is_implicit()
>>>          return 'kind'
>>>
>>> +    def describe(self):
>>> +        # Must override superclass describe() because self.name is 'type'
>> 
>> I don't find this argument convincing.  I think 'kind' is exactly as
>> unhelpful as 'type' is.  Specifically, should we report a QMP clash,
>> calling it 'kind' is confusing (it actually clashes with 'type').
>> Conversely 'type' is confusing when we report a C clash.
>> 
>> The helpful part...
>> 
>>> +        assert self.owner[0] != ':'
>>> +        return "'kind' (implicit tag of %s)" % self.owner
>>> +
>> 
>> ... is (implicit tag of FOO).
>> 
>> Fortunately, you're working towards killing the kind vs. type confusion.
>> Suggest to either rephrase the comment, or simply drop it.
>
> Drop the comment is fine by me.
>
> I personally thought the message needed to report 'kind', because 16/18
> ends up reporting:
>
> +tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion)
> collides with 'kind' (implicit tag of TestUnion)
>
> If I didn't override describe() at all, it would report:
>
> ...: 'kind' (branch of TestUnion) collides with 'type' (member of TestUnion)
>
> If all I did was override 'role = "implicit tag"' instead of describe(),
> it would report:
>
> ...: 'kind' (branch of TestUnion) collides with 'type' (implicit tag of
> TestUnion)

Good enough for me considering we'll clean up the 'kind' vs. 'type' mess
soon.

> Of course when a later subset fixes the kind/type misnomer, the error
> message changes:
>
> ...: 'type' (branch of TestUnion) collides with 'type' (member of TestUnion)
>
> where we lose the "implicit tag" because we completely lose the subclass.

Still good enough for me.

We could perhaps leverage is_implicit() to make it (implicit member of
TestUnion).

> Any preferences on which error message to prefer, and therefore how much
> (or how little) we need to override in this temporary subclass?  Or is
> this an argument for making the subclass permanent (with a 'role =
> "implicit tag"') even after the kind/type rename?

I think I'd prefer the simplest solution that still gives understandable
error messages.

I'd even accept temporary regressions in error message quality if it
simplifies coding or review, or reduces churn.

When a little effort can yield significantly better error messages in
the final state, feel free to go for it, of course.