On Tue, Feb 20, 2024 at 5:39 AM Markus Armbruster <arm...@redhat.com> wrote: > > John Snow <js...@redhat.com> writes: > > > This function is a bit hard to type as-is; mypy needs some assertions to > > assist with the type narrowing. > > > > Signed-off-by: John Snow <js...@redhat.com> > > --- > > scripts/qapi/schema.py | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index 043ee7556e6..e617abb03af 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > @@ -997,7 +997,9 @@ def lookup_entity(self, name, typ=None): > def lookup_entity(self, name, typ=None): > ent = self._entity_dict.get(name) > if typ and not isinstance(ent, typ): > return None > > return ent > > > > def lookup_type(self, name): > > - return self.lookup_entity(name, QAPISchemaType) > > + typ = self.lookup_entity(name, QAPISchemaType) > > + assert typ is None or isinstance(typ, QAPISchemaType) > > + return typ > > > > def resolve_type(self, name, info, what): > > typ = self.lookup_type(name) > > I figure the real trouble-maker is .lookup_entity(). > > When not passed an optional type argument, it returns QAPISchemaEntity. > > When passed an optional type argument, it returns that type or None. > > Too cute for type hints to express, I guess. > > What if we drop .lookup_entity()'s optional argument? There are just > three callers: > > 1. .lookup_type(), visible above. > > def lookup_type(self, name): > ent = self.lookup_entity(name) > if isinstance(ent, QAPISchemaType): > return ent > return None > > This should permit typing it as -> Optional[QAPISchemaType] without > further ado. > > 2. ._make_implicit_object_type() below > > Uses .lookup_type() to check whether the implicit object type already > exists. We figure we could > > typ = self.lookup_entity(name) > if typ: > assert(isinstance(typ, QAPISchemaObjectType)) > # The implicit object type has multiple users. This can > > 3. QAPIDocDirective.run() doesn't pass a type argument, so no change. > > Thoughts? > > If you'd prefer not to rock the boat for this series, could it still > make sense as a followup?
It makes sense as a follow-up, I think. I had other patches in the past that attempted to un-cuten these functions and make them more statically solid, but the shifting sands kept making it easier to put off until later. Lemme see if I can just tack this on to the end of the series and see how it behaves...