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

[Markus Armbruster]

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?