Eric Blake <ebl...@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 <ebl...@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
