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

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> 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.

That's okay.  However, a good part of the typing exercise's benefit is
the pinpointing of needlessly cute code, i.e. code that could be just as
well be less cute.  To actually reap the benefit, we need to make it
less cute.  If we put it off, we risk to forget.  Acceptable if we take
appropriate steps not to forget.