1. Patchew
  2. QEMU
  3. [PATCH 00/19] qapi: statically type schema.py
  4. View patch

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

John Snow posted 19 patches 1 year ago
There is a newer version of this series
[PATCH 08/19] qapi/schema: add static typing and assertions to lookup_type()
Posted by John Snow 1 year ago 
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]:
+        typ = self.lookup_entity(name, QAPISchemaType)
+        if typ is None:
+            return None
+        assert isinstance(typ, QAPISchemaType)
+        return typ
 
     def resolve_type(self, name, info, what):
         typ = self.lookup_type(name)
-- 
2.41.0
Re: [PATCH 08/19] qapi/schema: add static typing and assertions to lookup_type()
Posted by Markus Armbruster 1 year ago 
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?

> +        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?

>  
>      def resolve_type(self, name, info, what):
>          typ = self.lookup_type(name)
Re: [PATCH 08/19] qapi/schema: add static typing and assertions to lookup_type()
Posted by John Snow 1 year ago 
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.


> > +        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. 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)
>
>
Re: [PATCH 08/19] qapi/schema: add static typing and assertions to lookup_type()
Posted by Markus Armbruster 1 year ago 
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)
>>
>>
Re: [PATCH 08/19] qapi/schema: add static typing and assertions to lookup_type()
Posted by John Snow 1 year ago 
On Wed, Nov 22, 2023 at 7:09 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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
>     >>>

Huh. I ... huh!

Well, this apparently only works in Python 3.10!+

TypeError: Subscripted generics cannot be used with class and instance checks

>
> >                                                         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)
> >>
> >>
>
Re: [PATCH 08/19] qapi/schema: add static typing and assertions to lookup_type()
Posted by Markus Armbruster 1 year ago 
John Snow <jsnow@redhat.com> writes:

> On Wed, Nov 22, 2023 at 7:09 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> 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]:

[...]

>> >> > +        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
>>     >>>
>
> Huh. I ... huh!
>
> Well, this apparently only works in Python 3.10!+
>
> TypeError: Subscripted generics cannot be used with class and instance checks

We should be able to use it "soon" after 3.9 reaches EOL, approximately
October 2025.

*Sigh*

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.