Re: [Qemu-devel] [PATCH v7 09/14] qapi: Track owner of each object member

[Markus Armbruster]

Eric Blake <address@hidden> 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.  Rather than making all the check()
> methods have to pass around additional information, it is easier
> to have each member track the name of the type that owns it in
> the first place.

Making members track their owner is easy enough (your patch is proof),
but asserting it's easier suggests you tried the other approach, too.
Did you?

In fact, passing the owner to the check() methods would be easy enough.
QAPISchemaObjectType.check() passes self to its local members' .check().
Covers non-variant members.  Variant members take a bit more effort,
because more classes (and thus more check() methods) are involved.
QAPISchemaObjectType.check() and QAPISchemaAlternateType.check() pass
self to QAPISchemaObjectTypeVariants.check(), which passes it on to
QAPISchemaObjectTypeVariant.check(), which passes it on to
QAPISchemaObjectTypeMember.check().

I suspect the technique becomes cumbersome only when you start passing
members to helper functions: you have to pass the owner, too.  If
members know their owner, passing a member suffices.

>                   The new member.owner field is set when
> registering the members and variants arrays with an object or
> variant type.  We 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' (member of foo arguments)".
>
> To make the human-readable name of implicit types work without
> duplicating efforts, the name of implicit types is tweaked
> after the ':obj-' prefix, so that we can just trim off the
> prefix.  This required updates to the testsuite.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> 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                         | 41 ++++++++++++---
>  tests/qapi-schema/args-member-array.out |  4 +-
>  tests/qapi-schema/args-name-clash.out   |  4 +-
>  tests/qapi-schema/ident-with-escape.out |  4 +-
>  tests/qapi-schema/qapi-schema-test.out  | 88 
> ++++++++++++++++-----------------
>  tests/qapi-schema/union-clash-data.out  |  4 +-
>  6 files changed, 87 insertions(+), 58 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index e49f72b..11ffc49 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -961,8 +961,16 @@ 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))
> +            assert not m.owner
> +            m.owner = name
> +        if variants is not None:
> +            assert isinstance(variants, QAPISchemaObjectTypeVariants)
> +            if variants.tag_member:
> +                assert not variants.tag_member.owner
> +                variants.tag_member.owner = name
> +            for v in variants.variants:
> +                assert not v.owner
> +                v.owner = name

Works, but rummaging in instances of other classes is not so nice.
Could instead do

        for m in local_members:
            m.set_owner(name)
        if variants is not None:
            variants.set_owner(name)

with the obvious set_owner() methods in QAPISchemaObjectTypeMember,
QAPISchemaObjectTypeVariants, QAPISchemaObjectTypeVariant.

>          self._base_name = base
>          self.base = None
>          self.local_members = local_members
> @@ -1023,8 +1031,10 @@ class QAPISchemaObjectTypeMember(object):
>          self._type_name = typ
>          self.type = None
>          self.optional = optional
> +        self.owner = None   # will be filled by owner's init
>
>      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
> @@ -1034,6 +1044,15 @@ class QAPISchemaObjectTypeMember(object):
>      def c_name(self):
>          return c_name(self.name)
>
> +    def describe(self):
> +        source = self.owner
> +        if source.startswith(':obj-'):
> +            source = source[5:]
> +        return "'%s' (%s of %s)" % (self.name, self._describe(), source)
> +
> +    def _describe(self):
> +        return 'member'

A simple class variable would do, wouldn't it?

> +
>
>  # TODO Drop this class once we no longer have the 'type'/'kind' mismatch
>  class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
> @@ -1042,6 +1061,9 @@ class 
> QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>          assert self.type.is_implicit(QAPISchemaEnumType)
>          return 'kind'
>
> +    def describe(self):
> +        return "'kind' (implicit tag of %s)" % self.owner
> +

Let's compare to the inherited describe() with this one.

Why no need to strip a ':obj-' prefix here?

This prints "'kind' (implicit tag of ...)".  The inherited describe()
would print "'type' (member of ...).

The 'kind' vs. 'type' difference doesn't matter, because neither name
occurs in the schema anyway.

"implicit tag of" is an improvement over "member of".

However, the inherited describe() already has a hook to replace the part
before the "of".  Why do we need to override it wholesale anyway?

>
>  class QAPISchemaObjectTypeVariants(object):
>      def __init__(self, tag_name, tag_member, variants):
> @@ -1086,12 +1108,19 @@ class 
> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>              return self.type.members[0].type
>          return None
>
> +    def _describe(self):
> +        return 'branch'
> +
>
>  class QAPISchemaAlternateType(QAPISchemaType):
>      def __init__(self, name, info, variants):
>          QAPISchemaType.__init__(self, name, info)
>          assert isinstance(variants, QAPISchemaObjectTypeVariants)
> -        assert not variants.tag_name
> +        assert variants.tag_member and not variants.tag_member.owner
> +        variants.tag_member.owner = name
> +        for v in variants.variants:
> +            assert not v.owner
> +            v.owner = name

With the set_owner() discussed above, this becomes a one-liner:

        variants.set_owner(name)

>          self.variants = variants
>
>      def check(self, schema):
> @@ -1222,7 +1251,7 @@ class QAPISchema(object):
>      def _make_implicit_object_type(self, name, info, role, members):
>          if not members:
>              return None
> -        name = ':obj-%s-%s' % (name, role)
> +        name = ':obj-%s %s' % (name, role)
>          if not self.lookup_entity(name, QAPISchemaObjectType):
>              self._def_entity(QAPISchemaObjectType(name, info, None,
>                                                    members, None))

I know I suggested this, but I'm having second thoughts about spaces in
name.  The test output (visible below) becomes a bit confusing, and
unnecessarily hard to parse by ad hoc scripts (yes, I've done such
things from time to time).

We could keep

        name = ':obj-%s-%s' % (name, role)

here, and replace the '-' by ' ' in describe().  Problematic if name or
role can also contain '-'.  Use a more suitable character to separate
the two then.

> @@ -1312,7 +1341,7 @@ class QAPISchema(object):
>          success_response = expr.get('success-response', True)
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
> -                name, info, 'arg', self._make_members(data, info))
> +                name, info, 'arguments', self._make_members(data, info))
>          if isinstance(rets, list):
>              assert len(rets) == 1
>              rets = self._make_array_type(rets[0], info)
> @@ -1324,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))
>          self._def_entity(QAPISchemaEvent(name, info, data))
>
>      def _def_exprs(self):
> diff --git a/tests/qapi-schema/args-member-array.out 
> b/tests/qapi-schema/args-member-array.out
> index b3b92df..c822309 100644
> --- a/tests/qapi-schema/args-member-array.out
> +++ b/tests/qapi-schema/args-member-array.out
> @@ -1,9 +1,9 @@
>  object :empty
> -object :obj-okay-arg
> +object :obj-okay arguments
>      member member1: intList optional=False
>      member member2: defList optional=False
>  enum abc ['a', 'b', 'c']
>  object def
>      member array: abcList optional=False
> -command okay :obj-okay-arg -> None
> +command okay :obj-okay arguments -> None
>     gen=True success_response=True
> diff --git a/tests/qapi-schema/args-name-clash.out 
> b/tests/qapi-schema/args-name-clash.out
> index 9b2f6e4..0e986b6 100644
> --- a/tests/qapi-schema/args-name-clash.out
> +++ b/tests/qapi-schema/args-name-clash.out
> @@ -1,6 +1,6 @@
>  object :empty
> -object :obj-oops-arg
> +object :obj-oops arguments
>      member a-b: str optional=False
>      member a_b: str optional=False
> -command oops :obj-oops-arg -> None
> +command oops :obj-oops arguments -> None
>     gen=True success_response=True
> diff --git a/tests/qapi-schema/ident-with-escape.out 
> b/tests/qapi-schema/ident-with-escape.out
> index f4542b1..4544777 100644
> --- a/tests/qapi-schema/ident-with-escape.out
> +++ b/tests/qapi-schema/ident-with-escape.out
> @@ -1,5 +1,5 @@
>  object :empty
> -object :obj-fooA-arg
> +object :obj-fooA arguments
>      member bar1: str optional=False
> -command fooA :obj-fooA-arg -> None
> +command fooA :obj-fooA arguments -> None
>     gen=True success_response=True
> diff --git a/tests/qapi-schema/qapi-schema-test.out 
> b/tests/qapi-schema/qapi-schema-test.out
> index 2a8c82e..c666481 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -1,56 +1,56 @@
>  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
>      member enum3: EnumOne optional=True
> -object :obj-__org.qemu_x-command-arg
> +object :obj-__org.qemu_x-command arguments
>      member a: __org.qemu_x-EnumList optional=False
>      member b: __org.qemu_x-StructList optional=False
>      member c: __org.qemu_x-Union2 optional=False
>      member d: __org.qemu_x-Alt optional=False
> -object :obj-anyList-wrapper
> +object :obj-anyList wrapper
>      member data: anyList optional=False
> -object :obj-boolList-wrapper
> +object :obj-boolList wrapper
>      member data: boolList optional=False
> -object :obj-guest-sync-arg
> +object :obj-guest-sync arguments
>      member arg: any optional=False
> -object :obj-int16List-wrapper
> +object :obj-int16List wrapper
>      member data: int16List optional=False
> -object :obj-int32List-wrapper
> +object :obj-int32List wrapper
>      member data: int32List optional=False
> -object :obj-int64List-wrapper
> +object :obj-int64List wrapper
>      member data: int64List optional=False
> -object :obj-int8List-wrapper
> +object :obj-int8List wrapper
>      member data: int8List optional=False
> -object :obj-intList-wrapper
> +object :obj-intList wrapper
>      member data: intList optional=False
> -object :obj-numberList-wrapper
> +object :obj-numberList wrapper
>      member data: numberList optional=False
> -object :obj-sizeList-wrapper
> +object :obj-sizeList wrapper
>      member data: sizeList optional=False
> -object :obj-str-wrapper
> +object :obj-str wrapper
>      member data: str optional=False
> -object :obj-strList-wrapper
> +object :obj-strList wrapper
>      member data: strList optional=False
> -object :obj-uint16List-wrapper
> +object :obj-uint16List wrapper
>      member data: uint16List optional=False
> -object :obj-uint32List-wrapper
> +object :obj-uint32List wrapper
>      member data: uint32List optional=False
> -object :obj-uint64List-wrapper
> +object :obj-uint64List wrapper
>      member data: uint64List optional=False
> -object :obj-uint8List-wrapper
> +object :obj-uint8List wrapper
>      member data: uint8List optional=False
> -object :obj-user_def_cmd1-arg
> +object :obj-user_def_cmd1 arguments
>      member ud1a: UserDefOne optional=False
> -object :obj-user_def_cmd2-arg
> +object :obj-user_def_cmd2 arguments
>      member ud1a: UserDefOne optional=False
>      member ud1b: UserDefOne optional=True
> -object :obj-user_def_cmd3-arg
> +object :obj-user_def_cmd3 arguments
>      member a: int optional=False
>      member b: int optional=True
>  alternate AltIntNum
> @@ -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
> @@ -123,20 +123,20 @@ object UserDefFlatUnion2
>      case value2: UserDefB
>      case value3: UserDefA
>  object UserDefNativeListUnion
> -    case integer: :obj-intList-wrapper
> -    case s8: :obj-int8List-wrapper
> -    case s16: :obj-int16List-wrapper
> -    case s32: :obj-int32List-wrapper
> -    case s64: :obj-int64List-wrapper
> -    case u8: :obj-uint8List-wrapper
> -    case u16: :obj-uint16List-wrapper
> -    case u32: :obj-uint32List-wrapper
> -    case u64: :obj-uint64List-wrapper
> -    case number: :obj-numberList-wrapper
> -    case boolean: :obj-boolList-wrapper
> -    case string: :obj-strList-wrapper
> -    case sizes: :obj-sizeList-wrapper
> -    case any: :obj-anyList-wrapper
> +    case integer: :obj-intList wrapper
> +    case s8: :obj-int8List wrapper
> +    case s16: :obj-int16List wrapper
> +    case s32: :obj-int32List wrapper
> +    case s64: :obj-int64List wrapper
> +    case u8: :obj-uint8List wrapper
> +    case u16: :obj-uint16List wrapper
> +    case u32: :obj-uint32List wrapper
> +    case u64: :obj-uint64List wrapper
> +    case number: :obj-numberList wrapper
> +    case boolean: :obj-boolList wrapper
> +    case string: :obj-strList wrapper
> +    case sizes: :obj-sizeList wrapper
> +    case any: :obj-anyList wrapper
>  enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32', 's64', 'u8', 
> 'u16', 'u32', 'u64', 'number', 'boolean', 'string', 'sizes', 'any']
>  object UserDefOne
>      base UserDefZero
> @@ -178,21 +178,21 @@ object __org.qemu_x-Struct
>  object __org.qemu_x-Struct2
>      member array: __org.qemu_x-Union1List optional=False
>  object __org.qemu_x-Union1
> -    case __org.qemu_x-branch: :obj-str-wrapper
> +    case __org.qemu_x-branch: :obj-str wrapper
>  enum __org.qemu_x-Union1Kind ['__org.qemu_x-branch']
>  object __org.qemu_x-Union2
>      base __org.qemu_x-Base
>      tag __org.qemu_x-member1
>      case __org.qemu_x-value: __org.qemu_x-Struct2
> -command __org.qemu_x-command :obj-__org.qemu_x-command-arg -> 
> __org.qemu_x-Union1
> +command __org.qemu_x-command :obj-__org.qemu_x-command arguments -> 
> __org.qemu_x-Union1
>     gen=True success_response=True
> -command guest-sync :obj-guest-sync-arg -> any
> +command guest-sync :obj-guest-sync arguments -> any
>     gen=True success_response=True
>  command user_def_cmd None -> None
>     gen=True success_response=True
> -command user_def_cmd1 :obj-user_def_cmd1-arg -> None
> +command user_def_cmd1 :obj-user_def_cmd1 arguments -> None
>     gen=True success_response=True
> -command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
> +command user_def_cmd2 :obj-user_def_cmd2 arguments -> UserDefTwo
>     gen=True success_response=True
> -command user_def_cmd3 :obj-user_def_cmd3-arg -> int
> +command user_def_cmd3 :obj-user_def_cmd3 arguments -> int
>     gen=True success_response=True
> diff --git a/tests/qapi-schema/union-clash-data.out 
> b/tests/qapi-schema/union-clash-data.out
> index 6277239..4c5903f 100644
> --- a/tests/qapi-schema/union-clash-data.out
> +++ b/tests/qapi-schema/union-clash-data.out
> @@ -1,6 +1,6 @@
>  object :empty
> -object :obj-int-wrapper
> +object :obj-int wrapper
>      member data: int optional=False
>  object TestUnion
> -    case data: :obj-int-wrapper
> +    case data: :obj-int wrapper
>  enum TestUnionKind ['data']