Re: [Qemu-devel] [PATCH v5 12/46] qapi: Track location that created an implicit type

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/28/2015 06:56 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> A future patch will enable error detection in the various
>>> QapiSchema check() methods.  But since all errors have to
>>> have an associated 'info' location, we need a location to
>>> be associated with all implicit types.  Easiest is to reuse
>>> the location of the enclosing entity that includes the
>>> dictionary defining the implicit type.
>>>
>>> While at it, we were always passing None as the location of
>>> array types, making that parameter useless; sharing the
>>> location (if any) of the underlying element type makes sense.
>> 
>> The parameter is useless only because all array types are implicit.
>> Once we change that, it won't be anymore.
>
> I guess it depends whether I pass in the existing info when creating the
> array or determine the info when resolving the string name of the array
> element during check() (it will ultimately be the same info either way,
> it's just a question of which approach looks cleaner for getting the
> information set).

The latter leaves info None until check().  Unhealthy state.

I suspect the appropriate info is readily available in all the places
where we create array types, so let's just pass it to
_make_array_type().

>>> @@ -917,6 +917,7 @@ class QAPISchemaArrayType(QAPISchemaType):
>>>      def check(self, schema):
>>>          self.element_type = schema.lookup_type(self._element_type_name)
>>>          assert self.element_type
>>> +        self._info = self.element_type._info
>>>
>>>      def json_type(self):
>>>          return 'array'
>> 
>> Implicit array type's info is the element type's info.  Okay.
>> 
>>> @@ -928,6 +929,7 @@ class QAPISchemaArrayType(QAPISchemaType):
>>>  class QAPISchemaObjectType(QAPISchemaType):
>>>      def __init__(self, name, info, base, local_members, variants):
>>>          QAPISchemaType.__init__(self, name, info)
>>> +        assert info or name == ':empty'
>> 
>> I think what we really want to assert is "we got info unless this is a
>> built-in entity", in QAPISchemaEntity.__init__().
>
> To do that, arrays would have to pass info in to __init__() rather than
> deferring to check() as I did above.
>
>> 
>> Built-in entities are exactly the types defined by
>> QAPISchema._def_predefineds(), currently the built-in types and
>> ':empty'.
>
> I'm still wondering how best to test that, but agree that hoisting the
> assert into QAPISchemaEntity instead of just in QAPISchemaObjectType
> would be nice.  Maybe some sort of boolean switch, initially off, then
> turned on after _def_predefineds() is called, and assuming that no types
> other than predefineds are initialized prior to that point.

Either that, or have a special info value for built-in types.  Oh wait,
we got one already: None :)

Back to serious.  If we have to work for the assertion, we should
consider the assertion's value: how likely are the actual mistakes it
can catch?

Can legitimate errors be reported for built-in types?

>> 
>> Missing: implicit enum types' info.
>
> I'll add it; should be the info of the containing union type that caused
> the implicit enum.

Yup, just like for the union's wrapper objects.