| Message ID | 1443760312-656-7-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> 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. 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. 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. 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. > 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 <eblake@redhat.com> Let's discuss the above before I review the actual patch closely.
On 10/02/2015 03:50 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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? 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). > > 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). > >> 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 <eblake@redhat.com> > > Let's discuss the above before I review the actual patch closely. What do you think - would that refactoring be worth it?
Eric Blake <eblake@redhat.com> writes: > On 10/02/2015 03:50 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> >> >> Let's discuss the above before I review the actual patch closely. > > What do you think - would that refactoring be worth it?
On 10/02/2015 11:05 AM, Markus Armbruster wrote: >> 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. Okay, I don't think we need a backref; a string name is good enough. And I like your idea of making the implicit object name something where the suffix is usable as-is. So I'm currently playing with: class QAPISchemaObjectTypeMember... def __init__... self.owner = None def check... assert self.owner class QAPISchemaObjectType... def __init__(..., members): for m in members: assert isinstance(m, QAPISchemaObjectTypeMember) assert not m.owner m.owner = self.name
diff --git a/scripts/qapi.py b/scripts/qapi.py index 19cca97..880de94 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -993,14 +993,16 @@ class QAPISchemaObjectType(QAPISchemaType): class QAPISchemaObjectTypeMember(object): - def __init__(self, name, typ, optional): + def __init__(self, name, typ, optional, owner): assert isinstance(name, str) assert isinstance(typ, str) assert isinstance(optional, bool) + assert isinstance(owner, str) and ':' not in owner self.name = name self._type_name = typ self.type = None self.optional = optional + self._owner = owner def check(self, schema, all_members, seen): assert self.name not in seen @@ -1009,6 +1011,9 @@ class QAPISchemaObjectTypeMember(object): all_members.append(self) seen[self.name] = self + def describe(self): + return "'%s' (member of %s)" % (self.name, self._owner) + class QAPISchemaObjectTypeVariants(object): def __init__(self, tag_name, tag_member, variants): @@ -1035,13 +1040,16 @@ class QAPISchemaObjectTypeVariants(object): class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): - def __init__(self, name, typ): - QAPISchemaObjectTypeMember.__init__(self, name, typ, False) + def __init__(self, name, typ, owner): + QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner) def check(self, schema, tag_type, seen): QAPISchemaObjectTypeMember.check(self, schema, [], seen) assert self.name in tag_type.values + def describe(self): + return "'%s' (branch of %s)" % (self.name, self._owner) + # This function exists to support ugly simple union special cases # TODO get rid of them, and drop the function def simple_union_type(self): @@ -1193,7 +1201,7 @@ class QAPISchema(object): prefix = expr.get('prefix') self._def_entity(QAPISchemaEnumType(name, info, data, prefix)) - def _make_member(self, name, typ, info): + def _make_member(self, name, typ, info, owner): optional = False if name.startswith('*'): name = name[1:] @@ -1201,10 +1209,10 @@ class QAPISchema(object): if isinstance(typ, list): assert len(typ) == 1 typ = self._make_array_type(typ[0], info) - return QAPISchemaObjectTypeMember(name, typ, optional) + return QAPISchemaObjectTypeMember(name, typ, optional, owner) - def _make_members(self, data, info): - return [self._make_member(key, value, info) + def _make_members(self, data, info, owner): + return [self._make_member(key, value, info, owner) for (key, value) in data.iteritems()] def _def_struct_type(self, expr, info): @@ -1212,25 +1220,26 @@ class QAPISchema(object): base = expr.get('base') data = expr['data'] self._def_entity(QAPISchemaObjectType(name, info, base, - self._make_members(data, info), + self._make_members(data, info, + name), None)) - def _make_variant(self, case, typ): - return QAPISchemaObjectTypeVariant(case, typ) + def _make_variant(self, case, typ, owner): + return QAPISchemaObjectTypeVariant(case, typ, owner) - def _make_simple_variant(self, case, typ, info): + def _make_simple_variant(self, case, typ, info, owner): if isinstance(typ, list): assert len(typ) == 1 typ = self._make_array_type(typ[0], info) typ = self._make_implicit_object_type(typ, info, 'wrapper', [self._make_member('data', typ, - info)]) - return QAPISchemaObjectTypeVariant(case, typ) + info, owner)]) + return QAPISchemaObjectTypeVariant(case, typ, owner) def _make_tag_enum(self, type_name, info, variants): typ = self._make_implicit_enum_type(type_name, info, [v.name for v in variants]) - return QAPISchemaObjectTypeMember('type', typ, False) + return QAPISchemaObjectTypeMember('type', typ, False, type_name) def _def_union_type(self, expr, info): name = expr['union'] @@ -1239,15 +1248,15 @@ class QAPISchema(object): tag_name = expr.get('discriminator') tag_enum = None if tag_name: - variants = [self._make_variant(key, value) + variants = [self._make_variant(key, value, name) for (key, value) in data.iteritems()] else: - variants = [self._make_simple_variant(key, value, info) + variants = [self._make_simple_variant(key, value, info, name) for (key, value) in data.iteritems()] tag_enum = self._make_tag_enum(name, info, variants) self._def_entity( QAPISchemaObjectType(name, info, base, - self._make_members(OrderedDict(), info), + self._make_members(OrderedDict(), info, name), QAPISchemaObjectTypeVariants(tag_name, tag_enum, variants))) @@ -1255,7 +1264,7 @@ class QAPISchema(object): def _def_alternate_type(self, expr, info): name = expr['alternate'] data = expr['data'] - variants = [self._make_variant(key, value) + variants = [self._make_variant(key, value, name) for (key, value) in data.iteritems()] tag_enum = self._make_tag_enum(name, info, variants) self._def_entity( @@ -1271,9 +1280,11 @@ class QAPISchema(object): gen = expr.get('gen', True) success_response = expr.get('success-response', True) if isinstance(data, OrderedDict): + owner = name + ' arguments' data = self._make_implicit_object_type(name, info, 'arg', self._make_members(data, - info)) + info, + owner)) if isinstance(rets, list): assert len(rets) == 1 rets = self._make_array_type(rets[0], info) @@ -1284,9 +1295,11 @@ class QAPISchema(object): name = expr['event'] data = expr.get('data') if isinstance(data, OrderedDict): + owner = name + ' data' data = self._make_implicit_object_type(name, info, 'arg', self._make_members(data, - info)) + info, + owner)) self._def_entity(QAPISchemaEvent(name, info, data)) def _def_exprs(self):
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)". 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 <eblake@redhat.com> --- v6: rebase on new lazy array creation and simple union 'type' motion; tweak commit message --- scripts/qapi.py | 53 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 20 deletions(-)