Re: [PATCH 08/19] qapi/schema: add static typing and assertions to lookup_type()

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Nov 21, 2023, 9:21 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 | 8 ++++++--
>> >  1 file changed, 6 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index a1094283828..3308f334872 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -968,8 +968,12 @@ def lookup_entity(self, name, typ=None):
>> >              return None
>> >          return ent
>> >
>> > -    def lookup_type(self, name):
>> > -        return self.lookup_entity(name, QAPISchemaType)
>> > +    def lookup_type(self, name: str) -> Optional[QAPISchemaType]:
>>
>> Any particular reason not to delay the type hints until PATCH 16?
>>
>
> I forget. In some cases I did things a little differently so that the type
> checking would pass for each patch in the series, which sometimes required
> some concessions.
>
> Is this one of those cases? Uh, I forget.
>
> If it isn't, its almost certainly the case that I just figured I'd type
> this one function in one place instead of splitting it apart into two
> patches.
>
> I can try to shift the typing later and see what happens if you prefer it
> that way.

Well, you structured the series as "minor code changes to prepare for
type hints", followed by "add type hints".  So that's what I expect.
When patches deviate from what I expect, I go "am I missing something?"

Adding type hints along the way could work, too.  But let's try to stick
to the plan, and add them all in PATCH 16.

>> > +        typ = self.lookup_entity(name, QAPISchemaType)
>> > +        if typ is None:
>> > +            return None
>> > +        assert isinstance(typ, QAPISchemaType)
>> > +        return typ
>>
>> Would
>>
>>            typ = self.lookup_entity(name, QAPISchemaType)
>>            assert isinstance(typ, Optional[QAPISchemaType])
>>            return typ
>>
>> work?
>>
>
> I don't *think* so, Optional isn't a runtime construct.

Let me try...

    $ python
    Python 3.11.5 (main, Aug 28 2023, 00:00:00) [GCC 12.3.1 20230508 (Red Hat 
12.3.1-1)] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from typing import Optional
    >>> x=None
    >>> isinstance(x, Optional[str])
    True
    >>> 

>                                                         We can combine it
> into "assert x is None or isinstance(x, foo)" though - I believe that's
> used elsewhere in the qapi generator.

>> >
>> >      def resolve_type(self, name, info, what):
>> >          typ = self.lookup_type(name)
>>
>>