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() 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 <ebl...@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
