| Message ID | 20231116014350.653792-7-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
On 16/11/23 02:43, John Snow wrote: > 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(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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, > @@ -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 > 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?
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, > > @@ -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
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?
On Wed, Nov 22, 2023, 7:00 AM Markus Armbruster <armbru@redhat.com> wrote: > 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
John Snow <jsnow@redhat.com> writes: > On Wed, Nov 22, 2023, 7:00 AM Markus Armbruster <armbru@redhat.com> wrote: > >> John Snow <jsnow@redhat.com> writes: >> >> > On Tue, Nov 21, 2023, 9:09 AM Markus Armbruster <armbru@redhat.com> wrote: [...] >> >> 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? Your commit messages of PATCH 3+4 show the error. Helps. Maybe qapi/schema: Adjust type narrowing for mypy's benefit 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 once we add type hints: error message goes here A simple change to use a temporary variable helps the medicine go down.
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 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,
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(-)