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.