Re: [PATCH 03/19] qapi/schema: name QAPISchemaInclude entities

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Nov 21, 2023, 8:33 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > It simplifies typing to mandate that entities will always have a name;
>> > to achieve this we can occasionally assign an internal name. This
>> > alleviates errors such as:
>> >
>> > qapi/schema.py:287: error: Argument 1 to "__init__" of
>> > "QAPISchemaEntity" has incompatible type "None"; expected "str"
>> > [arg-type]
>> >
>> > Trying to fix it the other way by allowing entities to only have
>> > optional names opens up a nightmare portal of whackamole to try and
>> > audit that every other pathway doesn't actually pass a None name when we
>> > expect it to; this is the simpler direction of consitifying the typing.
>>
>> Arguably, that nightmare is compile-time proof of "we are not mistaking
>> QAPISchemaInclude for a named entity".
>>
>> When I added the include directive, I shoehorned it into the existing
>> representation of the QAPI schema as "list of QAPISchemaEntity" by
>> making it a subtype of QAPISchemaEntity.  That was a somewhat lazy hack.
>>
>> Note that qapi-code-gen.rst distinguishes between definitions and
>> directives.
>>
>> The places where mypy gripes that .name isn't 'str' generally want
>> something with a name (what qapi-code-gen.rst calls a definition).  If
>> we somehow pass them an include directive, they'll use None for a name,
>> which is no good.  mypy is pointing out this problem.
>>
>> What to do about it?
>>
>> 1. Paper it over: give include directives some made-up name (this
>> patch).  Now the places use the made-up name instead of None, and mypy
>> can't see the problem anymore.
>>
>> 2. Assert .name is not None until mypy is happy.  I guess that's what
>> you called opening "a nightmare portal of whackamole".
>>
>
> Yep.
>
>
>> 3. Clean up the typing: have a type for top-level expression (has no
>> name), and a subtype for definition (has a name).  Rough sketch
>> appended.  Thoughts?
>>
>
> Oh, that'll work. I tried to keep to "minimal SLOC" but where you want to
> see deeper fixes, I'm happy to deviate. I'll give it a shot.

I do appreciate the minimal fix!  I *like* exploring "minimal" first.
In this case, the exploration led me to not like my lazy hack anymore :)

[...]

> I'll try the refactor out in a patch at the end of my series and see how
> feasible it is.
>
> (I haven't reviewed it deeply yet, so if there's an obvious problem I'll
> find it when I go to implement this. conceptually it seems fine.)

Thanks!