Re: [PATCH v3 10/20] qapi: use schema.resolve_type instead of schema.lookup_type

[John Snow]

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