Re: [PATCH v4 39/46] qapi/introspect.py: Unify return type of _make_tree()

[Eduardo Habkost]

On Wed, Sep 30, 2020 at 12:31:43AM -0400, John Snow wrote:
> Returning a *something* or a Tuple of *something* is hard to accurately
> type. Let's just always return a tuple for structural consistency.
> 
> Instances of the 'TreeNode' type can be replaced with the slightly more
> specific 'AnnotatedNode' type.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

So, the only place where this seems to make a difference is
_tree_to_qlit().

We just need to prove that
  _tree_to_qlit(o, ...)
will have exactly the same result as
  _tree_to_qlit((o, None), ...).

For reference, this is the beginning of _tree_to_qlit():

| def _tree_to_qlit(obj: TreeNode,
|                   level: int = 0,
|                   suppress_first_indent: bool = False) -> str:
| 
|     def indent(level: int) -> str:
|         return level * 4 * ' '
| 
|     if isinstance(obj, tuple):
|         ifobj, extra = obj

`obj` is the return value of _make_tree()

`ifobj` is the original `obj` argument to _make_tree().

|         ifcond = extra.get('if')

ifcond will be None.

|         comment = extra.get('comment')

comment will be None

|         ret = ''
|         if comment:
|             ret += indent(level) + '/* %s */\n' % comment

nop

|         if ifcond:
|             ret += gen_if(ifcond)

nop

|         ret += _tree_to_qlit(ifobj, level)

ret will be '', so this is equivalent to:

  ret = _tree_to_qlit(ifobj, level)

which is almost good.

The only difference seems to that suppress_first_indent=True will
be ignored.  We should pass suppress_first_indent as argument in
the recursive call above, just in case.

The existing code will behave weirdly if there are comments or
conditions and suppress_first_indent=True, but I suggest we try
to address this issue later.

|         if ifcond:
|             ret += '\n' + gen_endif(ifcond)

nop

|         return ret


> ---
>  scripts/qapi/introspect.py | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 5cbdc7414bd..1c3ba41f1dc 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -53,14 +53,12 @@
>  
>  def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>                 extra: Optional[Extra] = None
> -               ) -> Union[TreeNode, AnnotatedNode]:
> +               ) -> AnnotatedNode:
>      if extra is None:
>          extra = {}
>      if ifcond:
>          extra['if'] = ifcond
> -    if extra:
> -        return (obj, extra)
> -    return obj
> +    return (obj, extra)
>  
>  
>  def _tree_to_qlit(obj: TreeNode,
> @@ -128,7 +126,7 @@ def __init__(self, prefix: str, unmask: bool):
>              ' * QAPI/QMP schema introspection', __doc__)
>          self._unmask = unmask
>          self._schema: Optional[QAPISchema] = None
> -        self._trees: List[TreeNode] = []
> +        self._trees: List[AnnotatedNode] = []
>          self._used_types: List[QAPISchemaType] = []
>          self._name_map: Dict[str, str] = {}
>          self._genc.add(mcgen('''
> @@ -195,7 +193,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>  
>      @classmethod
>      def _gen_features(cls,
> -                      features: List[QAPISchemaFeature]) -> List[TreeNode]:
> +                      features: List[QAPISchemaFeature]
> +                      ) -> List[AnnotatedNode]:
>          return [_make_tree(f.name, f.ifcond) for f in features]
>  
>      def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> @@ -215,7 +214,7 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>          self._trees.append(_make_tree(obj, ifcond, extra))
>  
>      def _gen_member(self,
> -                    member: QAPISchemaObjectTypeMember) -> TreeNode:
> +                    member: QAPISchemaObjectTypeMember) -> AnnotatedNode:
>          obj: _DObject = {
>              'name': member.name,
>              'type': self._use_type(member.type)
> @@ -231,7 +230,7 @@ def _gen_variants(self, tag_name: str,
>          return {'tag': tag_name,
>                  'variants': [self._gen_variant(v) for v in variants]}
>  
> -    def _gen_variant(self, variant: QAPISchemaVariant) -> TreeNode:
> +    def _gen_variant(self, variant: QAPISchemaVariant) -> AnnotatedNode:
>          obj: _DObject = {
>              'case': variant.name,
>              'type': self._use_type(variant.type)
> -- 
> 2.26.2
> 

-- 
Eduardo