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

[Eduardo Habkost]

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.

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

  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>


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

-- 
Eduardo