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?
Yep.
>> > @@ -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?
I think so, yes. In this conditional block, we need to work in terms of typ, which has been narrowed. The broader type doesn't have .element_type.
Why delete the assertion? Oh! Hmm, should the deletion go into PATCH
10?
Yeah, just a patch-splitting goof. I'll repair that.
>> 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?
Sounds right. Sometimes it's a little hard to see what the error is before the rest of the types go in, a hazard of needing all patches to bisect without regression.
Do you want a more elaborate commit message?
--js