Re: [PATCH 07/19] qapi/introspect: assert schema.lookup_type did not fail

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Nov 21, 2023, 9:17 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > lookup_type() is capable of returning None, but introspect.py isn't
>> > prepared for that. (And rightly so, if these built-in types are absent,
>> > something has gone hugely wrong.)
>> >
>> > RFC: This is slightly cumbersome as-is, but a patch at the end of this
>> series
>> > tries to address it with some slightly slicker lookup functions that
>> > don't need as much hand-holding.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  scripts/qapi/introspect.py | 8 ++++++--
>> >  1 file changed, 6 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> > index 67c7d89aae0..42981bce163 100644
>> > --- a/scripts/qapi/introspect.py
>> > +++ b/scripts/qapi/introspect.py
>> > @@ -227,10 +227,14 @@ 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')
>> > +            tmp = self._schema.lookup_type('int')
>> > +            assert tmp is not None
>>
>> More laconic: assert tmp
>>
>
> *looks up "laconic"*
>
> hey, "terse" is even fewer letters!

Touché!

> (but, you're right. I think I adopted the "is not none" out of a habit for
> distinguishing false-y values from the None value, but in this case we

It's a good habit.

> really wouldn't want to have either, so the shorter form is fine, though
> for mypy's sake we only care about guarding against None here.)

Right.

>> > +            typ = tmp
>> >          elif (isinstance(typ, QAPISchemaArrayType) and
>> >                typ.element_type.json_type() == 'int'):
>> > -            typ = self._schema.lookup_type('intList')
>> > +            tmp = self._schema.lookup_type('intList')
>> > +            assert tmp is not None
>> > +            typ = tmp
>> >          # Add type to work queue if new
>> >          if typ not in self._used_types:
>> >              self._used_types.append(typ)
>>
>> Not fond of naming things @tmp, but I don't have a better name to offer.
>>
>> We could avoid the lookup by having _def_predefineds() set suitable
>> attributes, like it serts .the_empty_object_type.  Matter of taste.  Not
>> now unless you want to.
>>
>
>  Check the end of the series for different lookup methods, too. We can
> discuss your preferred solution then, perhaps?

Works for me.