Re: [PATCH v2 30/38] qapi/introspect.py: Add a typed 'extra' structure

[John Snow]

On 9/23/20 12:13 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:00:53PM -0400, John Snow wrote:
> > Typing arbitrarily shaped dicts with mypy is difficult prior to Python
> > 3.8; using explicit structures is nicer.
> >
> > Since we always define an Extra type now, the return type of _make_tree
> > simplifies and always returns the tuple.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/introspect.py | 31 +++++++++++++++++++------------
> >   1 file changed, 19 insertions(+), 12 deletions(-)
>
> Here I'm confused by both the original code and the new code.
>
> I will try to review as a refactoring of existing code, but I'll
> have suggestions for follow ups:
>
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index b036fcf9ce..41ca8afc67 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -10,6 +10,8 @@
> >   See the COPYING file in the top-level directory.
> >   """
> >   
> > +from typing import (NamedTuple, Optional, Sequence)
> > +
> >   from .common import (
> >       c_name,
> >       gen_endif,
> > @@ -21,16 +23,21 @@
> >                        QAPISchemaType)
> >   
> >   
> > -def _make_tree(obj, ifcond, features, extra=None):
> > -    if extra is None:
> > -        extra = {}
> > -    if ifcond:
> > -        extra['if'] = ifcond
> > +class Extra(NamedTuple):
> > +    """
> > +    Extra contains data that isn't intended for output by introspection.
> > +    """
> > +    comment: Optional[str] = None
> > +    ifcond: Sequence[str] = tuple()
> > +
> > +
> > +def _make_tree(obj, ifcond, features,
> > +               extra: Optional[Extra] = None):
> > +    comment = extra.comment if extra else None
> > +    extra = Extra(comment, ifcond)
>
> Here we have one big difference: now `extra` is being recreated,
> and all fields except `extra.comment` are being ignored.  On the
> original version, all fields in `extra` were being kept.  This
> makes the existence of the `extra` argument pointless.

Yup, oops.

> If you are going through the trouble of changing the type of the
> 4rd argument to _make_tree(), this seems more obvious:

Yes, agree. I came up with something similar after talking to you this 
morning.

>    diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>    index 41ca8afc672..c62af94c9ad 100644
>    --- a/scripts/qapi/introspect.py
>    +++ b/scripts/qapi/introspect.py
>    @@ -32,8 +32,7 @@ class Extra(NamedTuple):
>     
>     
>     def _make_tree(obj, ifcond, features,
>    -               extra: Optional[Extra] = None):
>    -    comment = extra.comment if extra else None
>    +               comment: Optional[str] = None):
>         extra = Extra(comment, ifcond)
>         if features:
>             obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in 
> features]
>    @@ -170,16 +169,16 @@ const QLitObject %(c_name)s = %(c_string)s;
>             return self._name(typ.name)
>     
>         def _gen_tree(self, name, mtype, obj, ifcond, features):
>    -        extra = None
>    +        comment = None
>             if mtype not in ('command', 'event', 'builtin', 'array'):
>                 if not self._unmask:
>                     # Output a comment to make it easy to map masked names
>                     # back to the source when reading the generated output.
>    -                extra = Extra(comment=f'"{self._name(name)}" = {name}')
>    +                comment = f'"{self._name(name)}" = {name}'
>                 name = self._name(name)
>             obj['name'] = name
>             obj['meta-type'] = mtype
>    -        self._trees.append(_make_tree(obj, ifcond, features, extra))
>    +        self._trees.append(_make_tree(obj, ifcond, features, comment))
>     
>         def _gen_member(self, member):
>             obj = {'name': member.name, 'type': self._use_type(member.type)}
>
> I understand you're trying to just make minimal refactoring, and I
> don't think this should block your cleanup series.  So:
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I appreciate the benefit-of-the-doubt, but I think this change is worth 
making while we're here.

> >       if features:
> > -        obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
> > -    if extra:
> > -        return (obj, extra)
> > -    return obj
> > +        obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in features]
> > +    return (obj, extra)
> >   
> >   
> >   def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
> > @@ -40,8 +47,8 @@ def indent(level):
> >   
> >       if isinstance(obj, tuple):
> >           ifobj, extra = obj
> > -        ifcond = extra.get('if')
> > -        comment = extra.get('comment')
> > +        ifcond = extra.ifcond
> > +        comment = extra.comment
> >           ret = ''
> >           if comment:
> >               ret += indent(level) + '/* %s */\n' % comment
> > @@ -168,7 +175,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
> >               if not self._unmask:
> >                   # Output a comment to make it easy to map masked names
> >                   # back to the source when reading the generated output.
> > -                extra = {'comment': '"%s" = %s' % (self._name(name), name)}
> > +                extra = Extra(comment=f'"{self._name(name)}" = {name}')
> >               name = self._name(name)
> >           obj['name'] = name
> >           obj['meta-type'] = mtype
> > --
> > 2.26.2