Re: [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 10/02/2015 03:50 AM, Markus Armbruster wrote:
>> 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, we need 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 who owns it in the first place.
>>>
>>> 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)".
>> 
>> Peeking ahead a bit, I see two describe(), one for ordinary members
>> returning
>> 
>>     "'%s' (member of %s)" % (self.name, self._owner)
>> 
>> and one for variant members returning
>> 
>>     "'%s' (branch of %s)" % (self.name, self._owner)
>> 
>> The name _owner makes me expect it's the owning types name, but that's
>> not always the case, as we shall see.  How is it related to info and to
>> the owning type's name then?
>> 
>> In your example (implicit arguments type):
>> 
>>     arg_type.members[0]._owner is 'foo arguments'.
>> 
>>     arg_type.members[0] has no info.
>
> Well, none of the members have info - they are not subclassed from
> QAPISchemaEntity.
>
>> 
>>     arg_type.name is ':obj-foo-arg'
>> 
>>     arg_type.info is something like
>> 
>>         info {'line': 10, 'parent': None, 'file': 'example-schema.json'}
>> 
>>     pointing to the definition of command 'foo'.  It's actually the
>>     command's info, inherited by its implicit argument type.
>> 
>> Here, _owner is merely a variation of the owning type's name geared for
>> human readers.
>
> Oh, I think I see where you are going with this - why is 'owner' a
> string, rather than the actual QAPISchemaType python object?  And is it
> worth following the pattern used in other classes, where __init__ gets a
> string naming the type, and then check() resolves that name to the
> actual type?  At which point, we could do member.owner.info to access
> the info of the type that owns the member?

We generally refer to types by their name until check().  In check(), we
then do things like

        self.type = schema.lookup_type(self._type_name)

Done that way because in the general case, you can't resolve type names
to types before check(), and doing it that way even in cases where you
could keeps things nicely regular.

We don't currently create back-references from member to type.  Instead,
we rely on context.  Tends to be simpler as long as the context is
readily available.  If we find the lack of back-references complicates
things, we can of course add them.  Slightly spooky, because Python
lacks a real garbage collector, and cyclic references can make it leak.

> But there's a chicken-and-egg situation - we don't know what the type
> will be named until we call _make_implicit_object_type(), but that
> function requires that we have already pre-constructed the
> QAPISchemaObjectTypeMembers array (which means we can't pre-construct
> the members with the type name embedded).
>
> We'd have to refactor things to generate the type name, then construct
> the members, then construct the type (doable, but probably involves
> splitting this patch for ease of review).

Not the only way, see below.

>> Example of explicit arguments type:
>> 
>>     { 'struct': 'BarArgs', 'data': { 'string': 'str' } }
>>     { 'command': 'bar', 'data': 'BarArgs' }
>> 
>> Here, we get:
>> 
>>     arg_type.members[0]._owner is 'BarArgs'.
>> 
>>     arg_type.members[0] has no info.
>
> Again, because NO members have info.
>
>> 
>>     arg_type.name is 'BarArgs'
>> 
>>     arg_type.info is something like
>> 
>>         info {'line': 12, 'parent': None, 'file': 'example-schema.json'}
>> 
>>     pointing to the definition of command 'bar.  Again, it's the
>>     command's info, inherited by its implicit argument type.
>> 
>> Here, _owner *is* the owning type's name.
>> 
>> So, _owner is a more readable name we make up when the other name for
>> the same thing isn't readable.  However, we make up that other name,
>> too!  Begs the question why we don't simply make it readable right away.
>> 
>> Naturally, we still need to make up names collision-free.  But as far as
>> I can tell, nothing stops us from picking ':obj-foo arguments' instead
>> of ':obj-foo-arg', and when we talk to users strip off the common prefix
>> ':obj-' we prepend to avoid collisions.
>
> Might be doable, but then we'd have to generate the implicit object name
> prior to creating its Member objects (thus splitting
> _make_implicit_object_type() into two parts).

    def _make_implicit_object_type(self, name, role, members):
        if not members:
            return None
(1)     name = ':obj-%s-%s' % (name, role)
        if not self.lookup_entity(name, QAPISchemaObjectType):
(2)         self._def_entity(QAPISchemaObjectType(name, None, None,
                                                  members, None))
        return name

We create the implicit object type name at (1), and we associate the
type and its members in (2), by creating references from type to
members.  Isn't that the natural place for creating back-references,
too?  Assuming we need them.

>>> Where implicit types are involved, the code intentionally tries
>>> to pick the name of the owner of that implicit type, rather than
>>> the type name itself (a user reading the error message should be
>>> able to grep for the problem in their original file, but will not
>>> be able to locate a generated implicit name).
>>>
>>> No change to generated code.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>> 
>> Let's discuss the above before I review the actual patch closely.
>
> What do you think - would that refactoring be worth it?