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

[John Snow]

On Mon, Mar 11, 2024 at 2:14 PM John Snow <jsnow@redhat.com> wrote:
>
> 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...

Oh, I see what you're doing. Well, I think it's fine if you want to,
but it's also fine to keep this "stricter" method. There's also ways
to type it using mypy's @overload which I've monkey'd with in the
past. Dealer's choice, honestly, but I think I'm eager to just get to
the "fully typed" baseline and then worry about changing more stuff.