Re: [PATCH 06/19] qapi/schema: adjust type narrowing for mypy's benefit

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Nov 21, 2023, 9:09 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > We already take care to perform some type narrowing for arg_type and
>> > ret_type, but not in a way where mypy can utilize the result. A simple
>> > change to use a temporary variable helps the medicine go down.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  scripts/qapi/schema.py | 17 +++++++++--------
>> >  1 file changed, 9 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index 4600a566005..a1094283828 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -825,13 +825,14 @@ def __init__(self, name, info, doc, ifcond, features,
>> >      def check(self, schema):
>> >          super().check(schema)
>> >          if self._arg_type_name:
>> > -            self.arg_type = schema.resolve_type(
>> > +            arg_type = schema.resolve_type(
>> >                  self._arg_type_name, self.info, "command's 'data'")
>> > -            if not isinstance(self.arg_type, QAPISchemaObjectType):
>> > +            if not isinstance(arg_type, QAPISchemaObjectType):
>> >                  raise QAPISemError(
>> >                      self.info,
>> >                      "command's 'data' cannot take %s"
>> > -                    % self.arg_type.describe())
>> > +                    % arg_type.describe())
>> > +            self.arg_type = arg_type
>> >              if self.arg_type.variants and not self.boxed:
>> >                  raise QAPISemError(
>> >                      self.info,

Same story as for QAPISchemaEvent.check() below.  Correct?

>> > @@ -848,8 +849,7 @@ def check(self, schema):
>> >              if self.name not in 
>> > self.info.pragma.command_returns_exceptions:
>> >                  typ = self.ret_type
>> >                  if isinstance(typ, QAPISchemaArrayType):
>> > -                    typ = self.ret_type.element_type
>> > -                    assert typ
>> > +                    typ = typ.element_type
>>
>
> In this case, we've narrowed typ but not self.ret_type and mypy is not sure
> they're synonymous here (lack of power in mypy's model, maybe?). Work in
> terms of the temporary type we've already narrowed so mypy knows we have an
> element_type field.

The conditional ensures @typ is QAPISchemaArrayType.

In mypy's view, @typ is QAPISchemaArrayType, but self.ret_type is only
Optional[QAPISchemaType].

Therefore, it chokes on self.ret_type.element_type, but is happy with
typ.element_type.

Correct?

Why delete the assertion?  Oh!  Hmm, should the deletion go into PATCH
10?

>>                  if not isinstance(typ, QAPISchemaObjectType):
>> >                      raise QAPISemError(
>> >                          self.info,
>> > @@ -885,13 +885,14 @@ def __init__(self, name, info, doc, ifcond, 
>> > features, arg_type, boxed):
>> >      def check(self, schema):
>> >          super().check(schema)
>> >          if self._arg_type_name:
>> > -            self.arg_type = schema.resolve_type(
>> > +            typ = schema.resolve_type(
>> >                  self._arg_type_name, self.info, "event's 'data'")
>> > -            if not isinstance(self.arg_type, QAPISchemaObjectType):
>> > +            if not isinstance(typ, QAPISchemaObjectType):
>> >                  raise QAPISemError(
>> >                      self.info,
>> >                      "event's 'data' cannot take %s"
>> > -                    % self.arg_type.describe())
>> > +                    % typ.describe())
>> > +            self.arg_type = typ
>> >              if self.arg_type.variants and not self.boxed:
>> >                  raise QAPISemError(
>> >                      self.info,
>>
>> Harmless enough.  I can't quite see the mypy problem, though.  Care to
>> elaborate a bit?
>>
>
> self.arg_type has a narrower type- or, it WILL at the end of this series -
> so we need to narrow a temporary variable first before assigning it to the
> object state.
>
> We already perform the necessary check/narrowing, so this is really just
> pointing out that it's a bad idea to assign the state before the type
> check. Now we type check before assigning state.

After PATCH 16, .resolve_type() will return QAPISchemaType, and
self.arg_type will be Optional[QAPISchemaObjectType].  Correct?