| Message ID | 20240201224246.39480-11-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
John Snow <jsnow@redhat.com> writes: > the function lookup_type() is capable of returning None, but some > callers aren't prepared for that and assume it will always succeed. For > static type analysis purposes, this creates problems at those callsites. > > Modify resolve_type() - which already cannot ever return None - to allow > 'info' and 'what' parameters to be elided, which infers that the type > lookup is a built-in type lookup. > > Change several callsites to use the new form of resolve_type to avoid > the more laborious strictly-typed error-checking scaffolding: > > ret = lookup_type("foo") > assert ret is not None > typ: QAPISchemaType = ret > > which can be replaced with the simpler: > > typ = resolve_type("foo") > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/introspect.py | 4 ++-- > scripts/qapi/schema.py | 5 ++--- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index 67c7d89aae0..c38df61a6d5 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -227,10 +227,10 @@ def _use_type(self, typ: QAPISchemaType) -> str: > > # Map the various integer types to plain int > if typ.json_type() == 'int': > - typ = self._schema.lookup_type('int') > + typ = self._schema.resolve_type('int') > elif (isinstance(typ, QAPISchemaArrayType) and > typ.element_type.json_type() == 'int'): > - typ = self._schema.lookup_type('intList') > + typ = self._schema.resolve_type('intList') > # Add type to work queue if new > if typ not in self._used_types: > self._used_types.append(typ) > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index 573be7275a6..074897ee4fb 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -650,8 +650,7 @@ def check(self, schema, seen): > "discriminator '%s' is not a member of %s" > % (self._tag_name, base)) > # Here we do: > - base_type = schema.lookup_type(self.tag_member.defined_in) > - assert base_type > + base_type = schema.resolve_type(self.tag_member.defined_in) > if not base_type.is_implicit(): > base = "base type '%s'" % self.tag_member.defined_in > if not isinstance(self.tag_member.type, QAPISchemaEnumType): > @@ -1001,7 +1000,7 @@ def lookup_type(self, name): > assert typ is None or isinstance(typ, QAPISchemaType) > return typ > > - def resolve_type(self, name, info, what): > + def resolve_type(self, name, info=None, what=None): > typ = self.lookup_type(name) > if not typ: > assert info and what # built-in types must not fail lookup I still dislike this, but I don't think discussing this more will help. I can either accept it as the price of all your other work, or come up with my own solution. Let's focus on the remainder of the series.
On Tue, Feb 20, 2024 at 10:18 AM Markus Armbruster <armbru@redhat.com> wrote: > > John Snow <jsnow@redhat.com> writes: > > > the function lookup_type() is capable of returning None, but some > > callers aren't prepared for that and assume it will always succeed. For > > static type analysis purposes, this creates problems at those callsites. > > > > Modify resolve_type() - which already cannot ever return None - to allow > > 'info' and 'what' parameters to be elided, which infers that the type > > lookup is a built-in type lookup. > > > > Change several callsites to use the new form of resolve_type to avoid > > the more laborious strictly-typed error-checking scaffolding: > > > > ret = lookup_type("foo") > > assert ret is not None > > typ: QAPISchemaType = ret > > > > which can be replaced with the simpler: > > > > typ = resolve_type("foo") > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > scripts/qapi/introspect.py | 4 ++-- > > scripts/qapi/schema.py | 5 ++--- > > 2 files changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > > index 67c7d89aae0..c38df61a6d5 100644 > > --- a/scripts/qapi/introspect.py > > +++ b/scripts/qapi/introspect.py > > @@ -227,10 +227,10 @@ def _use_type(self, typ: QAPISchemaType) -> str: > > > > # Map the various integer types to plain int > > if typ.json_type() == 'int': > > - typ = self._schema.lookup_type('int') > > + typ = self._schema.resolve_type('int') > > elif (isinstance(typ, QAPISchemaArrayType) and > > typ.element_type.json_type() == 'int'): > > - typ = self._schema.lookup_type('intList') > > + typ = self._schema.resolve_type('intList') > > # Add type to work queue if new > > if typ not in self._used_types: > > self._used_types.append(typ) > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index 573be7275a6..074897ee4fb 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > @@ -650,8 +650,7 @@ def check(self, schema, seen): > > "discriminator '%s' is not a member of %s" > > % (self._tag_name, base)) > > # Here we do: > > - base_type = schema.lookup_type(self.tag_member.defined_in) > > - assert base_type > > + base_type = schema.resolve_type(self.tag_member.defined_in) > > if not base_type.is_implicit(): > > base = "base type '%s'" % self.tag_member.defined_in > > if not isinstance(self.tag_member.type, QAPISchemaEnumType): > > @@ -1001,7 +1000,7 @@ def lookup_type(self, name): > > assert typ is None or isinstance(typ, QAPISchemaType) > > return typ > > > > - def resolve_type(self, name, info, what): > > + def resolve_type(self, name, info=None, what=None): > > typ = self.lookup_type(name) > > if not typ: > > assert info and what # built-in types must not fail lookup > > I still dislike this, but I don't think discussing this more will help. > I can either accept it as the price of all your other work, or come up > with my own solution. > > Let's focus on the remainder of the series. You're absolutely more than welcome to take the series and fork it to help bring it home - as long as it passes type checking at the end, I won't really mind what happens to it in the interim :) Sorry if that comes across as being stubborn, it's more the case that I genuinely don't think I see your point of view, and feel unable to target it. (Sorry.) --js
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 67c7d89aae0..c38df61a6d5 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -227,10 +227,10 @@ def _use_type(self, typ: QAPISchemaType) -> str: # Map the various integer types to plain int if typ.json_type() == 'int': - typ = self._schema.lookup_type('int') + typ = self._schema.resolve_type('int') elif (isinstance(typ, QAPISchemaArrayType) and typ.element_type.json_type() == 'int'): - typ = self._schema.lookup_type('intList') + typ = self._schema.resolve_type('intList') # Add type to work queue if new if typ not in self._used_types: self._used_types.append(typ) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 573be7275a6..074897ee4fb 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -650,8 +650,7 @@ def check(self, schema, seen): "discriminator '%s' is not a member of %s" % (self._tag_name, base)) # Here we do: - base_type = schema.lookup_type(self.tag_member.defined_in) - assert base_type + base_type = schema.resolve_type(self.tag_member.defined_in) if not base_type.is_implicit(): base = "base type '%s'" % self.tag_member.defined_in if not isinstance(self.tag_member.type, QAPISchemaEnumType): @@ -1001,7 +1000,7 @@ def lookup_type(self, name): assert typ is None or isinstance(typ, QAPISchemaType) return typ - def resolve_type(self, name, info, what): + def resolve_type(self, name, info=None, what=None): typ = self.lookup_type(name) if not typ: assert info and what # built-in types must not fail lookup
the function lookup_type() is capable of returning None, but some callers aren't prepared for that and assume it will always succeed. For static type analysis purposes, this creates problems at those callsites. Modify resolve_type() - which already cannot ever return None - to allow 'info' and 'what' parameters to be elided, which infers that the type lookup is a built-in type lookup. Change several callsites to use the new form of resolve_type to avoid the more laborious strictly-typed error-checking scaffolding: ret = lookup_type("foo") assert ret is not None typ: QAPISchemaType = ret which can be replaced with the simpler: typ = resolve_type("foo") Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/introspect.py | 4 ++-- scripts/qapi/schema.py | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-)