Eric Blake <ebl...@redhat.com> writes: > On 09/28/2015 06:56 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> 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.
