On Wed, Nov 22, 2023, 7:00 AM Markus Armbruster <arm...@redhat.com> wrote:
> 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? > 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
