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

[Eduardo Habkost]

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