John Snow <js...@redhat.com> writes:
> On 2/3/21 9:08 AM, Markus Armbruster wrote:
>> John Snow <js...@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 <js...@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.
