Re: [Qemu-devel] [PATCH v8 13/18] qapi: Track owner of each object member

[Markus Armbruster <armbru@redhat.com>]

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