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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 10/13/2015 07:14 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, 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()).
>>>
>
>>>
>>> +    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.
>
> Ouch - I think these should be owner[:-4] (and similar) - I want the
> slice that excludes the last four bytes, and not one that is four bytes
> long.  (It happened to work in my tests because I was testing on
> :obj-oops-arg, where owner[:4] and owner[:-4] gave the same 'oops' string).
>
> It's an easy fixup to squash in, so I'm not yet sure if I need a full v9
> respin.

If we decide we can do without a respin, I'll want the incremental patch
to squash in.

>>> +
>>> +    role = 'member'
>>> +
>>>
>> 
>> Class variables are usually defined first, before methods.
>
> Sure, can do. pep8 didn't complain, and I didn't check if pylint warns
> about it.

PEP8 (the document) doesn't ask for it, but the examples I found in the
Python tutorial are consistently putting them first.

>>>  # 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.
>
> Drop the comment is fine by me.
>
> I personally thought the message needed to report 'kind', because 16/18
> ends up reporting:
>
> +tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion)
> collides with 'kind' (implicit tag of TestUnion)
>
> If I didn't override describe() at all, it would report:
>
> ...: 'kind' (branch of TestUnion) collides with 'type' (member of TestUnion)
>
> If all I did was override 'role = "implicit tag"' instead of describe(),
> it would report:
>
> ...: 'kind' (branch of TestUnion) collides with 'type' (implicit tag of
> TestUnion)

Good enough for me considering we'll clean up the 'kind' vs. 'type' mess
soon.

> Of course when a later subset fixes the kind/type misnomer, the error
> message changes:
>
> ...: 'type' (branch of TestUnion) collides with 'type' (member of TestUnion)
>
> where we lose the "implicit tag" because we completely lose the subclass.

Still good enough for me.

We could perhaps leverage is_implicit() to make it (implicit member of
TestUnion).

> Any preferences on which error message to prefer, and therefore how much
> (or how little) we need to override in this temporary subclass?  Or is
> this an argument for making the subclass permanent (with a 'role =
> "implicit tag"') even after the kind/type rename?

I think I'd prefer the simplest solution that still gives understandable
error messages.

I'd even accept temporary regressions in error message quality if it
simplifies coding or review, or reduces churn.

When a little effort can yield significantly better error messages in
the final state, feel free to go for it, of course.