Re: [PATCH v2 0/7] qapi: static typing conversion, pt5c

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Feb 8, 2023 at 11:31 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This is part five (c), and focuses on sharing strict types between
>> > parser.py and expr.py.
>> >
>> > gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5c
>> >
>> > Every commit should pass with:
>> >  - `isort -c qapi/`
>> >  - `flake8 qapi/`
>> >  - `pylint --rcfile=qapi/pylintrc qapi/`
>> >  - `mypy --config-file=qapi/mypy.ini qapi/`
>> >
>> > Some notes on this series:
>> >
>> > Patches 2 and 3 are almost entirely superseded by patch 5, but I wasn't
>> > as confident that Markus would like patch 5, so these patches aren't
>> > squashed quite as tightly as they could be -- I recommend peeking ahead
>> > at the cover letters before reviewing the actual patch diffs.
>>
>> Yes, you're taking a somewhat roundabout path there.
>
> The result of trying 10 different things and seeing what was feasible
> through trial and error, and rather less the product of an intentional
> design. In the name of just getting the ball rolling again, I sent it
> out instead of hemming and hawing over perfection. Publish early,
> Publish often! ... is what people doing the publishing say. Apologies
> to the reviewer.

The series was easy enough to review, in good part thanks to your cover
letter.

>> I think I like PATCH 5 well enough.  Do you have a tighter squash in
>> mind?
>
> Not directly. I could essentially just squash them, but that becomes a
> pretty big patch.

Worth a try.

>> > By the end of this series, the only JSON-y types we have left are:
>> >
>> > (A) QAPIExpression,
>> > (B) JSONValue,
>> > (C) _ExprValue.
>> >
>> > The argument I'm making here is that QAPIExpression and JSONValue are
>> > distinct enough to warrant having both types (for now, at least); and
>> > that _ExprValue is specialized enough to also warrant its inclusion.
>> >
>> > (Brutal honesty: my attempts at unifying this even further had even more
>> > hacks and unsatisfying conclusions, and fully unifying these types
>> > should probably wait until we're allowed to rely on some fairly modern
>> > Python versions.)
>>
>> Feels okay to me.
>
> Sorry, best I could do with reasonable effort. I will try to improve
> the situation when we bump the Python version!

Pretty low priority as far as I'm concerned.

mypy is (surprisingly, inexplicably, inexcusably, take your pick) bad at
recursive types.  We've sunk quite a bit of effort into getting the most
out of it around these fundamentally recursive data structures anyway.
I'd like to remind you that my gut feeling was "alright, let's just not
type them then."  You persuaded me to go this far.  No regrets.

The three types you mentioned are indeed distinct.

_ExprValue is the obvious stupid abstract syntax tree for the QAPI
schema language, with str and bool leaves (QAPI doesn't support
floating-point numbers), OrderedDict and list inner nodes.  It is used
for parser output.

Note that the type definition says Dict[str, object].  It's really
OrderedDict.  Plain dict should do for us since 3.6 made it ordered.

QAPIExpression augments _ExprValue, adding a QAPISourceInfo (identifying
the expression's source) and a QAPIDoc (the expressions documentation).
It is used to represent QAPI top-level expressions.  Two observations.

One, having source information only at the top-level is lazy, and leads
to sub-optimal error messages.  I'm not asking for improvement there; we
have bigger fish to fry.

Two, the fact that it wraps around _ExprValue is less than obvious.  The
type doesn't mention _ExprValue.  QAPISchemaParser._add_expr(), the
function we use turn an _ExprValue into a QAPIExpression doesn't mention
it either.  Both work on Mapping[str, object] instead.  Also not a
request for improvement.

JSONValue is an annotated JSON abstract syntax tree.  It's related to
the other two only insofar as JSON is related to the QAPI language.
Unifying plain syntax trees for these separate languages feels like a
dubious proposition to me.  Unifying *annotated* syntax trees feels
worse.