Re: [PATCH v3 08/20] qapi/schema: add type narrowing to lookup_type()

[John Snow]

On Tue, Feb 20, 2024 at 5:39 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@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 <jsnow@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...