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