Re: [PATCH v4 04/14] qapi/introspect.py: guard against ifcond/comment misuse

[Markus Armbruster <armbru@redhat.com>]

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.