On Tue, Jan 16, 2024 at 6:09 AM Markus Armbruster <arm...@redhat.com> wrote: > > John Snow <js...@redhat.com> writes: > > > allow resolve_type to be used for both built-in and user-specified > > type definitions. In the event that the type cannot be resolved, assert > > that 'info' and 'what' were both provided in order to create a usable > > QAPISemError. > > > > In practice, 'info' will only be None for built-in definitions, which > > *should not fail* type lookup. > > > > As a convenience, allow the 'what' and 'info' parameters to be elided > > entirely so that it can be used as a can-not-fail version of > > lookup_type. > > The convenience remains unused until the next patch. It should be added > there.
Okie-ducky. > > > Note: there are only three callsites to resolve_type at present where > > "info" is perceived to be possibly None: > > > > 1) QAPISchemaArrayType.check() > > 2) QAPISchemaObjectTypeMember.check() > > 3) QAPISchemaEvent.check() > > > > Of those three, only the first actually ever passes None; > > Yes. More below. Scary... > > > the other two > > are limited by their base class initializers which accept info=None, but > > They do? > In the case of QAPISchemaObjectTypeMember, the parent class QAPISchemaMember allows initialization with info=None. I can't fully trace all of the callsites, but one of them at least is in types.py: > enum_members = members + [QAPISchemaEnumMember('_MAX', None)] which necessitates, for now, info-less QAPISchemaEnumMember, which necessitates info-less QAPISchemaMember. There are others, etc. > > neither actually use it in practice. > > > > Signed-off-by: John Snow <js...@redhat.com> > > Hmm. Scary. > > We look up types by name in two ways: > > 1. Failure is a semantic error > > Use .resolve_type(), passing real @info and @what. > > Users: > > * QAPISchemaArrayType.check() resolving the element type > > Fine print: when the array type is built-in, we pass None @info and > @what. The built-in array type's element type must exist for > .resolve_type() to work. This commit changes .resolve_type() to > assert it does. > > * QAPISchemaObjectType.check() resolving the base type > > * QAPISchemaObjectTypeMember.check() resolving the member type > > * QAPISchemaCommand.check() resolving argument type (if named) and > return type (which is always named). > > * QAPISchemaEvent.check() resolving argument type (if named). > > Note all users are in .check() methods. That's where type named get > resolved. > > 2. Handle failure > > Use .lookup_type(), which returns None when the named type doesn't > exist. > > Users: > > * QAPISchemaVariants.check(), to look up the base type containing the > tag member for error reporting purposes. Failure would be a > programming error. > > * .resolve_type(), which handles failure as semantic error > > * ._make_array_type(), which uses it as "type exists already" > predicate. > > * QAPISchemaGenIntrospectVisitor._use_type(), to look up certain > built-in types. Failure would be a programming error. > > The next commit switches the uses where failure would be a programming > error from .lookup_type() to .resolve_type() without @info and @what, so > failure trips its assertion. I don't like it, because it overloads > .resolve_type() to serve two rather different use cases: > > 1. Failure is a semantic error; pass @info and @what > > 2. Failure is a programming error; don't pass @info and what > > The odd one out is of course QAPISchemaArrayType.check(), which wants to > use 1. for the user's types and 2. for built-in types. Let's ignore it > for a second. "Let's ignore what motivated this patch" aww... > > I prefer to do 2. like typ = .lookup_type(); assert typ. We can factor > this out into its own helper if that helps (pardon the pun). > > Back to QAPISchemaArrayType.check(). Its need to resolve built-in > element types, which have no info, necessitates .resolve_type() taking > Optional[QAPISourceInfo]. This might bother you. It doesn't bother me, > unless it leads to mypy complications I can't see. Well, with this patch I allowed it to take Optional[QAPISourceInfo] - just keep in mind that QAPISemError *requires* an info object, even though the typing there is also Optional[QAPISourceInfo] ... It will assert that info is present in __str__. Actually, I'd love to change that too - and make it fully required - but since built-in types have no info, there's too many places I'd need to change to enforce this as a static type. Still. > > We can simply leave it as is. Adding the assertion to .resolve_type() > is fine. > > Ot we complicate QAPISchemaArrayType.check() to simplify > .resolve_type()'s typing, roughly like this: > > if self.info: > self.element_type = schema.resolve_type( > self._element_type_name, > self.info, self.info.defn_meta) > else: # built-in type > self.element_type = schema.lookup_type( > self._element_type_name) > assert self.element_type > > Not sure it's worth the trouble. Thoughts? I suppose it's your call, ultimately. This patch exists primarily to help in two places: (A) QAPISchemaArrayType.check(), as you've noticed, because it uses the same path for both built-in and user-defined types. This is the only place in the code where this occurs *at the moment*, but I can't predict the future. (B) Calls to lookup_type in introspect.py which look up built-in types and must-not-fail. It was cumbersome in the old patchset, but this one makes it simpler. I suppose at the moment, having the assert directly in resolve_type just means we get to use the same helper/pathway for both user-defined and built-in types, which matches the infrastructure we already have, which doesn't differentiate between the two. (By which I mean, all of the Schema classes are not split into built-in and user-defined types, so it is invisible to the type system.) I could add conditional logic to the array check, and leave the lookup_type calls in introspect.py being a little cumbersome - my main concern with that solution is that I might be leaving a nasty booby-trap in the future if someone wants to add a new built-in type or something gets refactored to share more code pathways. Maybe that's not fully rational, but it's why I went the way I did. (P.S. I still violently want to create an info object that represents built-in definitions so I can just get rid of all the Optional[QAPISourceInfo] types from everywhere. I know I tried to do it before and you vetoed it, but the desire lives on in my heart.) > > > --- > > scripts/qapi/schema.py | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index 66a78f28fd4..a77b51d1b96 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > @@ -1001,9 +1001,10 @@ def lookup_type(self, name): > > assert typ is None or isinstance(typ, QAPISchemaType) > > return typ > > > > - def resolve_type(self, name, info, what): > > + def resolve_type(self, name, info=None, what=None): > > typ = self.lookup_type(name) > > if not typ: > > + assert info and what # built-in types must not fail lookup > > if callable(what): > > what = what(info) > > raise QAPISemError( >