_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
-- 
2.29.2

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.

Do you need this assertion to help mypy over the hump?

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.

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

On Wed, Feb 03, 2021 at 03:42:54PM -0500, John Snow wrote:
> 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.
> > 
[...]
> 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.

Thanks for taking it into consideration.

I like having extra comments and extra asserts on cases like this
one.  It helps us see mistakes more easily, and to identify
future opportunities for cleaning up the code.

But I don't want to delay important work because of details that
don't seem to make the code objectively worse.  So I won't argue
about this.

-- 
Eduardo

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.