On Tue, Mar 19, 2024, 12:02 PM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@redhat.com> writes: > > > On Fri, Mar 15, 2024, 11:23 AM Markus Armbruster <arm...@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 <arm...@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 <js...@redhat.com> > > Thanks! > >
