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

[John Snow <jsnow@redhat.com>]

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.

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.)

> 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.

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.

(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