From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: Michael Roth <michael.roth@amd.com>,
qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>,
Cleber Rosa <crosa@redhat.com>
Subject: Re: [PATCH v4 04/14] qapi/introspect.py: guard against ifcond/comment misuse
Date: Thu, 04 Feb 2021 16:06:44 +0100 [thread overview]
Message-ID: <87lfc3dg8r.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <fc2f7fd7-b212-ca3a-ef87-a05c4cd297cf@redhat.com> (John Snow's message of "Wed, 3 Feb 2021 15:42:54 -0500")
John Snow <jsnow@redhat.com> writes:
> On 2/3/21 9:08 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> _tree_to_qlit is called recursively on dict values alone; at such a
>>> point in generating output it is too late to apply an ifcond. Similarly,
>>> comments do not necessarily have a "tidy" place they can be printed in
>>> such a circumstance.
>>>
>>> Forbid this usage.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/introspect.py | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index 4749f65ea3c..ccdf4f1c0d0 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -43,6 +43,12 @@ def indent(level):
>>> ifobj, extra = obj
>>> ifcond = extra.get('if')
>>> comment = extra.get('comment')
>>> +
>>> + # NB: _tree_to_qlit is called recursively on the values of a key:value
>>> + # pair; those values can't be decorated with comments or conditionals.
>>> + msg = "dict values cannot have attached comments or if-conditionals."
>>> + assert not suppress_first_indent, msg
>>> +
>>> ret = ''
>>> if comment:
>>> ret += indent(level) + '/* %s */\n' % comment
>> This uses @suppress_first_indent as a proxy for "@obj is a value in
>> a
>> dict". Works, because we pass suppress_first_indent=True exactly
>> there. Took me a minute to see, though.
>>
>
> Yes, the link is a little tenuous; in truth, the field could be
> renamed "dict_value: bool" or so to make it more clear, at the expense
> of making the inner workings of _tree_to_qlit more opaque.
>
> So we happen to know that only dict values want to suppress the
> indent; and the error message explains what went wrong in language
> (subjectively, again) more directly helpful to the theoretical hapless user.
>
> (Tentatively: I'll amend the parameter name to make the relationship
> more concrete, but I expect you'll have more to say.)
>
>> Do you need this assertion to help mypy over the hump?
>>
>
> It was added as a result of an observation by Eduardo that by always
> generating annotation data (To make the return type from _make_tree
> not conditional) that there were cases where you could conceivably
> call _tree_to_qlit that didn't make sense; or at least we couldn't
> prove easily that it wouldn't happen.
>
> (Of course, in practice, it does not.)
>
> I added the assertion to call attention to the limitations of this
> existing code: passing annotations alongside dict values made no
> sense.
>
> (Or maybe made no sense.)
>
> Conceptually it makes sense that some keys of a dict might be
> conditionally compiled out, but in terms of the actual data structures
> we use to convey this information, we don't actually use dicts to
> represent keys like that ... we use a list, actually.
>
> (See visit_object_type_flat)
>
> Anyway, this was an attempt to clear up that misunderstanding for
> reviewers less familiar with these structures, and to guard against
> future code in particular that may misunderstand it.
I doubt the guard is needed for code. This stuff hasn't changed in a
long time. JSON is set in stone. If we ever need extras beyond ifcond
and comment (unlikely), we can stuff them in just like ifcond and
comment.
I accept readers and reviewers may find it useful.
> It isn't (to my recollection) necessary for mypy. If you want to
> remove it, I think I'd like Eduardo to sign off on that matter.
>
> (We both found this code very, very confusing to read and modify. For
> whatever reason, I still find it fairly hard to reason about clearly.)
Sorry about that. If I had known how much trouble the cheap and
somewhat hacky extension of the QLit-generating code for ifcond (commit
d626b6c1ae7) would give you[*], I would've nacked it.
>> Perhaps we'd be better off with two functions, one that takes possibly
>> annotated @obj, and one that takes only plain @obj. "Yes, but not now"
>> woule be one acceptable answer to that.
>>
>
> Yes, I tried to prototype this a few times but found that it quickly
> touched too many things and I was losing appetite for re-spins. Recent
> reviews urged a focus on "typing what we have, where possible" before
> making alterations. The debate between "fix-then-type" or
> "type-then-fix" is valid, but largely intractable.
Yes, we need to focus, and resist mission creep.
When review uncovers improvement opportunities, we need to decide
whether to pursue within the series, in a follow-up series, or
"later"[**].
This should *not* stop us from pointing out improvement opportunities!
With the benefit of hindsight: I wish we had kicked this one down the
road some. Sunk cost, though.
> Since my only immediate goal was "Get everything typed", the
> "type-then-fix" approach has the side-effect of dropping improvements
> that aren't strictly needed whenever possible.
>
> LONG STORY SHORT: Yes, I think that design would be better overall,
> but I think it should wait for later. In particular, if you embark
> upon your more radical rewrite of introspection, it could just be
> handled at that time.
Got it.
> (My careful separation of scalars vs non-scalars in the typing comment
> later in this series is an artifact of the time spent playing around
> with splitting this function out into two mutually recursive
> functions, but found it was too noisy in an already long-challenged
> series.)
>
> --js
[*] And then indirectly me, to be honest.
[**] Which may well mean "never" in practice.
next prev parent reply other threads:[~2021-02-04 15:19 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-02 17:46 [PATCH v4 00/14] qapi: static typing conversion, pt2 John Snow
2021-02-02 17:46 ` [PATCH v4 01/14] qapi/introspect.py: assert schema is not None John Snow
2021-02-02 17:46 ` [PATCH v4 02/14] qapi/introspect.py: use _make_tree for features nodes John Snow
2021-02-03 13:49 ` Markus Armbruster
2021-02-02 17:46 ` [PATCH v4 03/14] qapi/introspect.py: add _gen_features helper John Snow
2021-02-02 17:46 ` [PATCH v4 04/14] qapi/introspect.py: guard against ifcond/comment misuse John Snow
2021-02-03 14:08 ` Markus Armbruster
2021-02-03 20:42 ` John Snow
2021-02-03 21:18 ` Eduardo Habkost
2021-02-04 15:06 ` Markus Armbruster [this message]
2021-02-02 17:46 ` [PATCH v4 05/14] qapi/introspect.py: Unify return type of _make_tree() John Snow
2021-02-02 17:46 ` [PATCH v4 06/14] qapi/introspect.py: replace 'extra' dict with 'comment' argument John Snow
2021-02-03 14:23 ` Markus Armbruster
2021-02-03 21:21 ` John Snow
2021-02-04 8:37 ` Markus Armbruster
2021-02-02 17:46 ` [PATCH v4 07/14] qapi/introspect.py: Introduce preliminary tree typing John Snow
2021-02-03 14:30 ` Markus Armbruster
2021-02-03 21:40 ` John Snow
2021-02-02 17:46 ` [PATCH v4 08/14] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
2021-02-03 14:47 ` Markus Armbruster
2021-02-03 21:50 ` Eduardo Habkost
2021-02-04 15:37 ` Markus Armbruster
2021-02-04 16:20 ` John Snow
2021-02-04 16:28 ` Eduardo Habkost
2021-02-05 8:45 ` Markus Armbruster
2021-02-03 23:12 ` John Snow
2021-02-05 9:10 ` Markus Armbruster
2021-02-02 17:46 ` [PATCH v4 09/14] qapi/introspect.py: improve _tree_to_qlit error message John Snow
2021-02-02 17:46 ` [PATCH v4 10/14] qapi/introspect.py: improve readability of _tree_to_qlit John Snow
2021-02-02 17:46 ` [PATCH v4 11/14] qapi/introspect.py: add type hint annotations John Snow
2021-02-03 15:15 ` Markus Armbruster
2021-02-03 23:27 ` John Snow
2021-02-05 13:42 ` Markus Armbruster
2021-02-08 21:39 ` John Snow
2021-02-08 21:53 ` John Snow
2021-02-09 9:06 ` Markus Armbruster
2021-02-10 17:31 ` John Snow
2021-02-02 17:46 ` [PATCH v4 12/14] qapi/introspect.py: add introspect.json dummy types John Snow
2021-02-02 17:46 ` [PATCH v4 13/14] qapi/introspect.py: Add docstring to _tree_to_qlit John Snow
2021-02-02 17:46 ` [PATCH v4 14/14] qapi/introspect.py: Update copyright and authors list John Snow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87lfc3dg8r.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=crosa@redhat.com \
--cc=ehabkost@redhat.com \
--cc=jsnow@redhat.com \
--cc=michael.roth@amd.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).