John Snow <js...@redhat.com> writes: > On Tue, Nov 21, 2023, 9:09 AM Markus Armbruster <arm...@redhat.com> wrote: > >> John Snow <js...@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 <js...@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?
