Re: [PATCH v5 24/25] qapi: Tighten check whether implicit object type already exists

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2142 bytes --]

On Tue, Mar 19, 2024, 12:02 PM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Fri, Mar 15, 2024, 11:23 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> Entities with names starting with q_obj_ are implicit object types.
> >> Therefore, QAPISchema._make_implicit_object_type()'s .lookup_entity()
> >> can only return a QAPISchemaObjectType.  Assert that.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  scripts/qapi/schema.py | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> index e52930a48a..a6180f93c6 100644
> >> --- a/scripts/qapi/schema.py
> >> +++ b/scripts/qapi/schema.py
> >> @@ -1297,8 +1297,9 @@ def _make_implicit_object_type(
> >>              return None
> >>          # See also QAPISchemaObjectTypeMember.describe()
> >>          name = 'q_obj_%s-%s' % (name, role)
> >> -        typ = self.lookup_entity(name, QAPISchemaObjectType)
> >> +        typ = self.lookup_entity(name)
> >>          if typ:
> >> +            assert(isinstance(typ, QAPISchemaObjectType))
> >>              # The implicit object type has multiple users.  This can
> >>              # only be a duplicate definition, which will be flagged
> >>              # later.
> >> --
> >> 2.44.0
> >>
> >
> > Seems obviously fine, though I don't suppose this narrowing will be
> > "remembered" by the type system. Do we care?
>
> mypy passes without it.  It's for catching programming errors and
> helping the reader along.  The former are unlikely, and the latter is
> debatable, but when in doubt, assert.
>

mmhmm. I was just wondering if we could tighten the typing of typ itself,
or if it conflicted with legitimate broader uses. it happens a lot in qapi
that we're regulated by broader base types in parent classes etc.

If we CAN tighten the variable/field (without runtime code changes), we
should do so. If we can't, this patch is 100% totally fine as is.


> > Reviewed-by: John Snow <jsnow@redhat.com>
>
> Thanks!
>
>

[-- Attachment #2: Type: text/html, Size: 3444 bytes --]