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, > > @@ -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. > 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. --js
