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

[John Snow <jsnow@redhat.com>]

On Wed, Nov 22, 2023 at 4:38 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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!
>

Got this working with minor changes to sphinx's qapidox extension,
which isn't typechecked so it escaped notice. I'm going to continue
working through your feedback as-is, then when I get to the end of the
series, I'll attempt to float this patch forward to replace this patch
under discussion.

Thanks!

--js