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

[Eduardo Habkost]

On Wed, Sep 30, 2020 at 02:32:49PM -0400, John Snow wrote:
> On 9/30/20 2:24 PM, Eduardo Habkost wrote:
> > 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.
> > 
> 
> This is a really good spot, and I indeed hadn't considered it at all when I
> did this.
> 
> (I simply made the change and observed it worked just fine!)
> 
> > 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
> > 
> 
> Hm, yes, it's a hypothetical case, but perhaps we can use an assertion to
> help guard against it if development creates that case later by accident.
> 
> That ought to be good enough for now to not waste time accommodating a
> (presently) fictional circumstance?
> 
> Thanks for the good sleuthing here.

With the current code, both
  ret += _tree_to_qlit(ifobj, level)
and
  ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
will behave exactly the same.

The former will behave weirdly if we wrap a dictionary value using
_tree_node().  We don't do that today.

The latter will behave weirdly if there's a comment or ifcond
attached in a dictionary value.  We don't do that today.

I believe the latter is less likely to be triggered by accident.

But I'd be happy with either:

  # _make_tree() shouldn't be use to wrap nodes that
  # may be printed using suppress_first_indent=True
  # (in other words, dictionary values shouldn't be wrapped using _make_tree())
  assert(not suppress_first_indent)
  ret += _tree_to_qlit(ifobj, level)

or

  # we can't add ifcond or comments to nodes that may be
  # printed using suppress_first_indent=True
  # (in other words, dictionary values can't have ifcond or comments)
  assert(not suppress_first_indent or (not comment and not ifcond))
  ret += _tree_to_qlit(ifobj, level, suppress_first_indent)


If we have time to spare later, we could do this:

  def _value_to_qlit(obj: Union[None, str, Dict[str, object], List[object], 
bool],
                     level: int = 0,
                     suppress_first_indent: bool = False) -> str:
      ...
      if obj is None:
          ...
      elif isinstance(obj, str):
          ...
      elif isinstance(obj, list):
          ...
      ...
  
  def _tree_to_qlit(obj: TreeNode, level: int = 0) -> str:
      if isinstance(obj, AnnotatedNode):
         ...
      else:
         return _value_to_qlit(obj, level)

This way, it will be impossible to set suppress_first_indent=True
on an annotated node.

-- 
Eduardo