| Message ID | 1442872682-6523-13-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@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. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > scripts/qapi.py | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 1dc7641..e982970 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -908,8 +908,8 @@ class QAPISchemaEnumType(QAPISchemaType): > > > class QAPISchemaArrayType(QAPISchemaType): > - def __init__(self, name, info, element_type): > - QAPISchemaType.__init__(self, name, info) > + def __init__(self, name, element_type): > + QAPISchemaType.__init__(self, name, None) > assert isinstance(element_type, str) > self._element_type_name = element_type > self.element_type = None > @@ -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__(). Built-in entities are exactly the types defined by QAPISchema._def_predefineds(), currently the built-in types and ':empty'. > assert base is None or isinstance(base, str) > for m in local_members: > assert isinstance(m, QAPISchemaObjectTypeMember) > @@ -1165,15 +1167,15 @@ class QAPISchema(object): > def _make_array_type(self, element_type): > name = element_type + 'List' > if not self.lookup_type(name): > - self._def_entity(QAPISchemaArrayType(name, None, element_type)) > + self._def_entity(QAPISchemaArrayType(name, element_type)) > return name > > - def _make_implicit_object_type(self, name, role, members): > + def _make_implicit_object_type(self, name, info, role, members): > if not members: > return None > name = ':obj-%s-%s' % (name, role) > if not self.lookup_entity(name, QAPISchemaObjectType): > - self._def_entity(QAPISchemaObjectType(name, None, None, > + self._def_entity(QAPISchemaObjectType(name, info, None, > members, None)) > return name > > @@ -1210,11 +1212,11 @@ class QAPISchema(object): > def _make_variant(self, case, typ): > return QAPISchemaObjectTypeVariant(case, typ) > > - def _make_simple_variant(self, case, typ): > + def _make_simple_variant(self, info, case, typ): > if isinstance(typ, list): > assert len(typ) == 1 > typ = self._make_array_type(typ[0]) > - typ = self._make_implicit_object_type(typ, 'wrapper', > + typ = self._make_implicit_object_type(typ, info, 'wrapper', > [self._make_member('data', typ)]) > return QAPISchemaObjectTypeVariant(case, typ) > > @@ -1232,7 +1234,7 @@ class QAPISchema(object): > variants = [self._make_variant(key, value) > for (key, value) in data.iteritems()] > else: > - variants = [self._make_simple_variant(key, value) > + variants = [self._make_simple_variant(info, key, value) > for (key, value) in data.iteritems()] > tag_enum = self._make_tag_enum(name, variants) > self._def_entity( A simple union type's implicit wrapper types' info is the simple union type's info. Okay. > @@ -1263,7 +1265,7 @@ class QAPISchema(object): > gen = expr.get('gen', True) > success_response = expr.get('success-response', True) > if isinstance(data, OrderedDict): > - data = self._make_implicit_object_type(name, 'arg', > + data = self._make_implicit_object_type(name, info, 'arg', > self._make_members(data)) > if isinstance(rets, list): > assert len(rets) == 1 A command's implicit argument type's info is the command's info. Okay. > @@ -1275,7 +1277,7 @@ class QAPISchema(object): > name = expr['event'] > data = expr.get('data') > if isinstance(data, OrderedDict): > - data = self._make_implicit_object_type(name, 'arg', > + data = self._make_implicit_object_type(name, info, 'arg', > self._make_members(data)) > self._def_entity(QAPISchemaEvent(name, info, data)) An event's implicit argument type's info is the event's info. Okay. Missing: implicit enum types' info.
On 09/28/2015 06:56 AM, Markus Armbruster wrote: > Eric Blake <eblake@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). >> @@ -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. > > Missing: implicit enum types' info. I'll add it; should be the info of the containing union type that caused the implicit enum.
Eric Blake <eblake@redhat.com> writes: > On 09/28/2015 06:56 AM, Markus Armbruster wrote: >> Eric Blake <eblake@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.
On 09/29/2015 02:02 AM, Markus Armbruster wrote: >>>> 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(). I just realized why I left it None until check(). We can have forward references to arrays, as in: { 'struct':'A', 'data':{ 'list': ['B'] } } { 'struct':'B', 'data':{ 'value': 'int' } } Normally, BList gets created at the same time as B (and so logically shares the location of B), but with forward references, we have the problem that _make_member() has no access to info (unless I plumb it in), and even if it did, it would be the info for A, whereas my check() methodology used the info for B. That is, _make_array_type() does not yet have ready access to info in all call sites. On the other hand, we have the big TODO about whether pre-creating array types is even necessary, and want to consider lazy array creation where they are only instantiated if actually used by a member type or command return value. I guess it's time for me to play with doing that cleanup first, and then I'd be happy to have an array's info be the info of its first instantiation, rather than the info of its element type. (And that means that ['int'] would have an info, if an array of ints is used anywhere, even though 'int' does not.) > 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? I don't think so (and in fact have assertions along those lines) - we can only report a semantic error about something in the user's QAPI file, but their file didn't define the builtin types, so I don't see how we would ever want to report a semantic error at the location tracked by a builtin type. So we should be safe if builtin types use info=None.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 1dc7641..e982970 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -908,8 +908,8 @@ class QAPISchemaEnumType(QAPISchemaType): class QAPISchemaArrayType(QAPISchemaType): - def __init__(self, name, info, element_type): - QAPISchemaType.__init__(self, name, info) + def __init__(self, name, element_type): + QAPISchemaType.__init__(self, name, None) assert isinstance(element_type, str) self._element_type_name = element_type self.element_type = None @@ -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' @@ -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' assert base is None or isinstance(base, str) for m in local_members: assert isinstance(m, QAPISchemaObjectTypeMember) @@ -1165,15 +1167,15 @@ class QAPISchema(object): def _make_array_type(self, element_type): name = element_type + 'List' if not self.lookup_type(name): - self._def_entity(QAPISchemaArrayType(name, None, element_type)) + self._def_entity(QAPISchemaArrayType(name, element_type)) return name - def _make_implicit_object_type(self, name, role, members): + def _make_implicit_object_type(self, name, info, role, members): if not members: return None name = ':obj-%s-%s' % (name, role) if not self.lookup_entity(name, QAPISchemaObjectType): - self._def_entity(QAPISchemaObjectType(name, None, None, + self._def_entity(QAPISchemaObjectType(name, info, None, members, None)) return name @@ -1210,11 +1212,11 @@ class QAPISchema(object): def _make_variant(self, case, typ): return QAPISchemaObjectTypeVariant(case, typ) - def _make_simple_variant(self, case, typ): + def _make_simple_variant(self, info, case, typ): if isinstance(typ, list): assert len(typ) == 1 typ = self._make_array_type(typ[0]) - typ = self._make_implicit_object_type(typ, 'wrapper', + typ = self._make_implicit_object_type(typ, info, 'wrapper', [self._make_member('data', typ)]) return QAPISchemaObjectTypeVariant(case, typ) @@ -1232,7 +1234,7 @@ class QAPISchema(object): variants = [self._make_variant(key, value) for (key, value) in data.iteritems()] else: - variants = [self._make_simple_variant(key, value) + variants = [self._make_simple_variant(info, key, value) for (key, value) in data.iteritems()] tag_enum = self._make_tag_enum(name, variants) self._def_entity( @@ -1263,7 +1265,7 @@ class QAPISchema(object): gen = expr.get('gen', True) success_response = expr.get('success-response', True) if isinstance(data, OrderedDict): - data = self._make_implicit_object_type(name, 'arg', + data = self._make_implicit_object_type(name, info, 'arg', self._make_members(data)) if isinstance(rets, list): assert len(rets) == 1 @@ -1275,7 +1277,7 @@ class QAPISchema(object): name = expr['event'] data = expr.get('data') if isinstance(data, OrderedDict): - data = self._make_implicit_object_type(name, 'arg', + data = self._make_implicit_object_type(name, info, 'arg', self._make_members(data)) self._def_entity(QAPISchemaEvent(name, info, data))
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. Signed-off-by: Eric Blake <eblake@redhat.com> --- scripts/qapi.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)