[v5,2/9] qapi: replace List[str] by QAPISchemaIfCond

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Wrap the 'if' condition in a higher-level object. Not only does this
provide more type safety but it also enables further refactoring without
too much churn.

The following patches will change the syntax of the schema 'if'
conditions to be predicate expressions, and will generate code for
different target languages (C, and Rust in another series).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py         |  2 +-
 scripts/qapi/commands.py       |  4 +-
 scripts/qapi/events.py         |  5 ++-
 scripts/qapi/gen.py            | 14 +++----
 scripts/qapi/introspect.py     | 26 ++++++------
 scripts/qapi/schema.py         | 74 +++++++++++++++++++++++-----------
 scripts/qapi/types.py          | 33 +++++++--------
 scripts/qapi/visit.py          | 23 ++++++-----
 tests/qapi-schema/test-qapi.py |  2 +-
 9 files changed, 106 insertions(+), 77 deletions(-)

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Wrap the 'if' condition in a higher-level object. Not only does this

I can see "wrap in an object".  I'm afraid don't get what makes it
"higher-level".

> provide more type safety but it also enables further refactoring without
> too much churn.

I figure by "more type safety" you mean "can't accidentally confuse some
other list of strings with a conditional", which is true, but isn't
really about *type* safety.

Maybe:

  Wrap the 'if' condition in an object.  Not only is this a bit safer,
  it also enables further refactoring without too much churn.

> The following patches will change the syntax of the schema 'if'
> conditions to be predicate expressions, and will generate code for
> different target languages (C, and Rust in another series).

Since different target languages aren't actually generated in this
series, I'd say "and will enable generating code for different target
languages, such as Rust."

>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Tested-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py         |  2 +-
>  scripts/qapi/commands.py       |  4 +-
>  scripts/qapi/events.py         |  5 ++-
>  scripts/qapi/gen.py            | 14 +++----
>  scripts/qapi/introspect.py     | 26 ++++++------
>  scripts/qapi/schema.py         | 74 +++++++++++++++++++++++-----------
>  scripts/qapi/types.py          | 33 +++++++--------
>  scripts/qapi/visit.py          | 23 ++++++-----
>  tests/qapi-schema/test-qapi.py |  2 +-
>  9 files changed, 106 insertions(+), 77 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 87c67ab23f..b737949007 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -116,7 +116,7 @@ def _nodes_for_ifcond(self, ifcond, with_if=True):
>          the conditions are in literal-text and the commas are not.
>          If with_if is False, we don't return the "(If: " and ")".
>          """
> -        condlist = intersperse([nodes.literal('', c) for c in ifcond],
> +        condlist = intersperse([nodes.literal('', c) for c in ifcond.ifcond],
>                                 nodes.Text(', '))
>          if not with_if:
>              return condlist

Note for later: many hunks replace ifcond (the unwrapped Sequence[str])
by ifcond.ifcond (the wrapped one, with the wrapper peeled off).
Mechanical.

> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 0e13d51054..3654825968 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -17,7 +17,6 @@
>      Dict,
>      List,
>      Optional,
> -    Sequence,
>      Set,
>  )
>  
> @@ -31,6 +30,7 @@
>  from .schema import (
>      QAPISchema,
>      QAPISchemaFeature,
> +    QAPISchemaIfCond,
>      QAPISchemaObjectType,
>      QAPISchemaType,
>  )
> @@ -301,7 +301,7 @@ def visit_end(self) -> None:
>      def visit_command(self,
>                        name: str,
>                        info: Optional[QAPISourceInfo],
> -                      ifcond: Sequence[str],
> +                      ifcond: QAPISchemaIfCond,
>                        features: List[QAPISchemaFeature],
>                        arg_type: Optional[QAPISchemaObjectType],
>                        ret_type: Optional[QAPISchemaType],

Note for later: many hunks replace ifcond: Sequence[str] or
Iterable[str] by ifcond: QAPISchemaIfCond.  Mechanical.

> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index fee8c671e7..82475e84ec 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -12,7 +12,7 @@
>  See the COPYING file in the top-level directory.
>  """
>  
> -from typing import List, Optional, Sequence
> +from typing import List, Optional
>  
>  from .common import c_enum_const, c_name, mcgen
>  from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
> @@ -20,6 +20,7 @@
>      QAPISchema,
>      QAPISchemaEnumMember,
>      QAPISchemaFeature,
> +    QAPISchemaIfCond,
>      QAPISchemaObjectType,
>  )
>  from .source import QAPISourceInfo
> @@ -227,7 +228,7 @@ def visit_end(self) -> None:
>      def visit_event(self,
>                      name: str,
>                      info: Optional[QAPISourceInfo],
> -                    ifcond: Sequence[str],
> +                    ifcond: QAPISchemaIfCond,
>                      features: List[QAPISchemaFeature],
>                      arg_type: Optional[QAPISchemaObjectType],
>                      boxed: bool) -> None:
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 1fa503bdbd..1c5b190276 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -18,7 +18,6 @@
>      Dict,
>      Iterator,
>      Optional,
> -    Sequence,
>      Tuple,
>  )
>  
> @@ -32,6 +31,7 @@
>      mcgen,
>  )
>  from .schema import (
> +    QAPISchemaIfCond,
>      QAPISchemaModule,
>      QAPISchemaObjectType,
>      QAPISchemaVisitor,
> @@ -85,7 +85,7 @@ def write(self, output_dir: str) -> None:
>                  fp.write(text)
>  
>  
> -def _wrap_ifcond(ifcond: Sequence[str], before: str, after: str) -> str:
> +def _wrap_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) -> str:
>      if before == after:
>          return after   # suppress empty #if ... #endif
>  
> @@ -95,9 +95,9 @@ def _wrap_ifcond(ifcond: Sequence[str], before: str, after: str) -> str:
>      if added[0] == '\n':
>          out += '\n'
>          added = added[1:]
> -    out += gen_if(ifcond)
> +    out += gen_if(ifcond.ifcond)
>      out += added
> -    out += gen_endif(ifcond)
> +    out += gen_endif(ifcond.ifcond)
>      return out
>  
>  
> @@ -127,9 +127,9 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
>  class QAPIGenCCode(QAPIGen):
>      def __init__(self, fname: str):
>          super().__init__(fname)
> -        self._start_if: Optional[Tuple[Sequence[str], str, str]] = None
> +        self._start_if: Optional[Tuple[QAPISchemaIfCond, str, str]] = None
>  
> -    def start_if(self, ifcond: Sequence[str]) -> None:
> +    def start_if(self, ifcond: QAPISchemaIfCond) -> None:
>          assert self._start_if is None
>          self._start_if = (ifcond, self._body, self._preamble)
>  
> @@ -187,7 +187,7 @@ def _bottom(self) -> str:
>  
>  
>  @contextmanager
> -def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]:
> +def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
>      """
>      A with-statement context manager that wraps with `start_if()` / `end_if()`.
>  
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 9a348ca2e5..77a8c33ad4 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -15,11 +15,9 @@
>      Any,
>      Dict,
>      Generic,
> -    Iterable,
>      List,
>      Optional,
>      Sequence,
> -    Tuple,
>      TypeVar,
>      Union,
>  )
> @@ -38,6 +36,7 @@
>      QAPISchemaEntity,
>      QAPISchemaEnumMember,
>      QAPISchemaFeature,
> +    QAPISchemaIfCond,
>      QAPISchemaObjectType,
>      QAPISchemaObjectTypeMember,
>      QAPISchemaType,
> @@ -91,11 +90,11 @@ class Annotated(Generic[_ValueT]):
>      """
>      # TODO: Remove after Python 3.7 adds @dataclass:
>      # pylint: disable=too-few-public-methods
> -    def __init__(self, value: _ValueT, ifcond: Iterable[str],
> +    def __init__(self, value: _ValueT, ifcond: QAPISchemaIfCond,
>                   comment: Optional[str] = None):
>          self.value = value
>          self.comment: Optional[str] = comment
> -        self.ifcond: Tuple[str, ...] = tuple(ifcond)
> +        self.ifcond = ifcond
>  
>  
>  def _tree_to_qlit(obj: JSONValue,
> @@ -125,10 +124,10 @@ def indent(level: int) -> str:
>          if obj.comment:
>              ret += indent(level) + f"/* {obj.comment} */\n"
>          if obj.ifcond:
> -            ret += gen_if(obj.ifcond)
> +            ret += gen_if(obj.ifcond.ifcond)
>          ret += _tree_to_qlit(obj.value, level)
>          if obj.ifcond:
> -            ret += '\n' + gen_endif(obj.ifcond)
> +            ret += '\n' + gen_endif(obj.ifcond.ifcond)
>          return ret
>  
>      ret = ''
> @@ -254,7 +253,7 @@ def _gen_features(features: Sequence[QAPISchemaFeature]
>          return [Annotated(f.name, f.ifcond) for f in features]
>  
>      def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
> -                  ifcond: Sequence[str] = (),
> +                  ifcond: QAPISchemaIfCond = QAPISchemaIfCond(),

The same QAPISchemaIfCond object gets reused every time we don't pass an
@ifcond argument.  Looks a bit scary, but works, because we don't ever
mutate it.

Elsewhere, we None as default, though: QAPISchemaEntity.__init__(),
QAPISchemaMember.__init__().

>                    features: Sequence[QAPISchemaFeature] = ()) -> None:
>          """
>          Build and append a SchemaInfo object to self._trees.
> @@ -305,7 +304,7 @@ def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>          self._gen_tree(name, 'builtin', {'json-type': json_type})
>  
>      def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
> -                        ifcond: Sequence[str],
> +                        ifcond: QAPISchemaIfCond,
>                          features: List[QAPISchemaFeature],
>                          members: List[QAPISchemaEnumMember],
>                          prefix: Optional[str]) -> None:
> @@ -316,14 +315,14 @@ def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>          )
>  
>      def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
> -                         ifcond: Sequence[str],
> +                         ifcond: QAPISchemaIfCond,
>                           element_type: QAPISchemaType) -> None:
>          element = self._use_type(element_type)
>          self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>                         ifcond)
>  
>      def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
> -                               ifcond: Sequence[str],
> +                               ifcond: QAPISchemaIfCond,
>                                 features: List[QAPISchemaFeature],
>                                 members: List[QAPISchemaObjectTypeMember],
>                                 variants: Optional[QAPISchemaVariants]) -> None:
> @@ -336,7 +335,7 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>          self._gen_tree(name, 'object', obj, ifcond, features)
>  
>      def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
> -                             ifcond: Sequence[str],
> +                             ifcond: QAPISchemaIfCond,
>                               features: List[QAPISchemaFeature],
>                               variants: QAPISchemaVariants) -> None:
>          self._gen_tree(
> @@ -348,7 +347,7 @@ def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>          )
>  
>      def visit_command(self, name: str, info: Optional[QAPISourceInfo],
> -                      ifcond: Sequence[str],
> +                      ifcond: QAPISchemaIfCond,
>                        features: List[QAPISchemaFeature],
>                        arg_type: Optional[QAPISchemaObjectType],
>                        ret_type: Optional[QAPISchemaType], gen: bool,
> @@ -367,7 +366,8 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
>          self._gen_tree(name, 'command', obj, ifcond, features)
>  
>      def visit_event(self, name: str, info: Optional[QAPISourceInfo],
> -                    ifcond: Sequence[str], features: List[QAPISchemaFeature],
> +                    ifcond: QAPISchemaIfCond,
> +                    features: List[QAPISchemaFeature],
>                      arg_type: Optional[QAPISchemaObjectType],
>                      boxed: bool) -> None:
>          assert self._schema is not None
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index d1d27ff7ee..bc357ebbfa 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -25,6 +25,20 @@
>  from .parser import QAPISchemaParser
>  
>  
> +class QAPISchemaIfCond:
> +    def __init__(self, ifcond=None):
> +        self.ifcond = ifcond or []
> +
> +    # Returns true if the condition is not void
> +    def __bool__(self):
> +        return bool(self.ifcond)

I'm not sure I like this one.

In the two places where we default parameter icond=None, we use

    ifcond or QAPISchemaIfCond()

to flip to the default value we actually want.  Works as intended for
None and for non-empty QAPISchemaIfCond.  For empty QAPISchemaIfCond, it
throws away the value and creates a new, equivalent one.  Works, but
kind of by accident.

This is an instance of a more general problem: when I see an
Optional[ObjectType], I expect it to be falsy if and only if it's None.
Perhaps I shouldn't.  Doesn't mean we should press __bool__() into
service for checking "is there a condition".  A boring non-dunder method
might be clearer.

I understand what you mean by "condition is void", but it sounds a bit
odd to me.  How do you like "Is a condition present?"

> +
> +    def __eq__(self, other):
> +        if not isinstance(other, QAPISchemaIfCond):
> +            return NotImplemented
> +        return self.ifcond == other.ifcond

Stupid question: why do we need to override __eq__()?

Hmm, probably for _make_implicit_object_type().

Why raise on type mismatch?  Feels rather un-pythonic to me.

> +
> +
>  class QAPISchemaEntity:
>      meta: Optional[str] = None
>  
> @@ -42,7 +56,7 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None):
>          # such place).
>          self.info = info
>          self.doc = doc
> -        self._ifcond = ifcond or []
> +        self._ifcond = ifcond or QAPISchemaIfCond()
>          self.features = features or []
>          self._checked = False
>  
> @@ -77,7 +91,7 @@ def set_module(self, schema):
>  
>      @property
>      def ifcond(self):
> -        assert self._checked
> +        assert self._checked and isinstance(self._ifcond, QAPISchemaIfCond)
>          return self._ifcond
>  
>      def is_implicit(self):
> @@ -601,7 +615,7 @@ def check(self, schema, seen):
>          else:                   # simple union
>              assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>              assert not self.tag_member.optional
> -            assert self.tag_member.ifcond == []
> +            assert not self.tag_member.ifcond
>          if self._tag_name:    # flat union
>              # branches that are not explicitly covered get an empty type
>              cases = {v.name for v in self.variants}
> @@ -646,7 +660,7 @@ def __init__(self, name, info, ifcond=None):
>          assert isinstance(name, str)
>          self.name = name
>          self.info = info
> -        self.ifcond = ifcond or []
> +        self.ifcond = ifcond or QAPISchemaIfCond()
>          self.defined_in = None
>  
>      def set_defined_in(self, name):
> @@ -968,11 +982,13 @@ def _def_predefineds(self):
>      def _make_features(self, features, info):
>          if features is None:
>              return []
> -        return [QAPISchemaFeature(f['name'], info, f.get('if'))
> +        return [QAPISchemaFeature(f['name'], info,
> +                                  QAPISchemaIfCond(f.get('if')))
>                  for f in features]
>  
>      def _make_enum_members(self, values, info):
> -        return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
> +        return [QAPISchemaEnumMember(v['name'], info,
> +                                     QAPISchemaIfCond(v.get('if')))
>                  for v in values]
>  

Note for later: several hunks wrap condition expressions in an object
like this.  Mechanical.

>      def _make_implicit_enum_type(self, name, info, ifcond, values):
> @@ -1008,7 +1024,7 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
>              # TODO kill simple unions or implement the disjunction
>  
>              # pylint: disable=protected-access
> -            assert (ifcond or []) == typ._ifcond
> +            assert ifcond == typ._ifcond

I'm not sure I understand this part.  Leaving for later.

>          else:
>              self._def_entity(QAPISchemaObjectType(
>                  name, info, None, ifcond, None, None, members, None))
> @@ -1018,7 +1034,7 @@ def _def_enum_type(self, expr, info, doc):
>          name = expr['enum']
>          data = expr['data']
>          prefix = expr.get('prefix')
> -        ifcond = expr.get('if')
> +        ifcond = QAPISchemaIfCond(expr.get('if'))
>          features = self._make_features(expr.get('features'), info)
>          self._def_entity(QAPISchemaEnumType(
>              name, info, doc, ifcond, features,
> @@ -1036,7 +1052,8 @@ def _make_member(self, name, typ, ifcond, features, info):
>                                            self._make_features(features, info))
>  
>      def _make_members(self, data, info):
> -        return [self._make_member(key, value['type'], value.get('if'),
> +        return [self._make_member(key, value['type'],
> +                                  QAPISchemaIfCond(value.get('if')),
>                                    value.get('features'), info)
>                  for (key, value) in data.items()]
>  
> @@ -1044,7 +1061,7 @@ def _def_struct_type(self, expr, info, doc):
>          name = expr['struct']
>          base = expr.get('base')
>          data = expr['data']
> -        ifcond = expr.get('if')
> +        ifcond = QAPISchemaIfCond(expr.get('if'))
>          features = self._make_features(expr.get('features'), info)
>          self._def_entity(QAPISchemaObjectType(
>              name, info, doc, ifcond, features, base,
> @@ -1067,7 +1084,7 @@ def _def_union_type(self, expr, info, doc):
>          name = expr['union']
>          data = expr['data']
>          base = expr.get('base')
> -        ifcond = expr.get('if')
> +        ifcond = QAPISchemaIfCond(expr.get('if'))
>          features = self._make_features(expr.get('features'), info)
>          tag_name = expr.get('discriminator')
>          tag_member = None
> @@ -1076,15 +1093,21 @@ def _def_union_type(self, expr, info, doc):
>                  name, info, ifcond,
>                  'base', self._make_members(base, info))
>          if tag_name:
> -            variants = [self._make_variant(key, value['type'],
> -                                           value.get('if'), info)
> -                        for (key, value) in data.items()]
> +            variants = [
> +                self._make_variant(key, value['type'],
> +                                   QAPISchemaIfCond(value.get('if')),
> +                                   info)
> +                for (key, value) in data.items()
> +            ]

I think we more usually put the closing parenthesis like this:

               variants = [
                  ...
                  for (key, value) in data.items()]

More of the same below.

>              members = []
>          else:
> -            variants = [self._make_simple_variant(key, value['type'],
> -                                                  value.get('if'), info)
> -                        for (key, value) in data.items()]
> -            enum = [{'name': v.name, 'if': v.ifcond} for v in variants]
> +            variants = [
> +                self._make_simple_variant(key, value['type'],
> +                                          QAPISchemaIfCond(value.get('if')),
> +                                          info)
> +                for (key, value) in data.items()
> +            ]
> +            enum = [{'name': v.name, 'if': v.ifcond.ifcond} for v in variants]
>              typ = self._make_implicit_enum_type(name, info, ifcond, enum)
>              tag_member = QAPISchemaObjectTypeMember('type', info, typ, False)
>              members = [tag_member]
> @@ -1097,11 +1120,14 @@ def _def_union_type(self, expr, info, doc):
>      def _def_alternate_type(self, expr, info, doc):
>          name = expr['alternate']
>          data = expr['data']
> -        ifcond = expr.get('if')
> +        ifcond = QAPISchemaIfCond(expr.get('if'))
>          features = self._make_features(expr.get('features'), info)
> -        variants = [self._make_variant(key, value['type'], value.get('if'),
> -                                       info)
> -                    for (key, value) in data.items()]
> +        variants = [
> +            self._make_variant(key, value['type'],
> +                               QAPISchemaIfCond(value.get('if')),
> +                               info)
> +            for (key, value) in data.items()
> +        ]
>          tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
>          self._def_entity(
>              QAPISchemaAlternateType(name, info, doc, ifcond, features,
> @@ -1118,7 +1144,7 @@ def _def_command(self, expr, info, doc):
>          allow_oob = expr.get('allow-oob', False)
>          allow_preconfig = expr.get('allow-preconfig', False)
>          coroutine = expr.get('coroutine', False)
> -        ifcond = expr.get('if')
> +        ifcond = QAPISchemaIfCond(expr.get('if'))
>          features = self._make_features(expr.get('features'), info)
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
> @@ -1137,7 +1163,7 @@ def _def_event(self, expr, info, doc):
>          name = expr['event']
>          data = expr.get('data')
>          boxed = expr.get('boxed', False)
> -        ifcond = expr.get('if')
> +        ifcond = QAPISchemaIfCond(expr.get('if'))
>          features = self._make_features(expr.get('features'), info)
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 20d572a23a..3673cf0f49 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -13,7 +13,7 @@
>  # See the COPYING file in the top-level directory.
>  """
>  
> -from typing import List, Optional, Sequence
> +from typing import List, Optional
>  
>  from .common import (
>      c_enum_const,
> @@ -27,6 +27,7 @@
>      QAPISchema,
>      QAPISchemaEnumMember,
>      QAPISchemaFeature,
> +    QAPISchemaIfCond,
>      QAPISchemaObjectType,
>      QAPISchemaObjectTypeMember,
>      QAPISchemaType,
> @@ -50,13 +51,13 @@ def gen_enum_lookup(name: str,
>  ''',
>                  c_name=c_name(name))
>      for memb in members:
> -        ret += gen_if(memb.ifcond)
> +        ret += gen_if(memb.ifcond.ifcond)
>          index = c_enum_const(name, memb.name, prefix)
>          ret += mcgen('''
>          [%(index)s] = "%(name)s",
>  ''',
>                       index=index, name=memb.name)
> -        ret += gen_endif(memb.ifcond)
> +        ret += gen_endif(memb.ifcond.ifcond)
>  
>      ret += mcgen('''
>      },
> @@ -80,12 +81,12 @@ def gen_enum(name: str,
>                  c_name=c_name(name))
>  
>      for memb in enum_members:
> -        ret += gen_if(memb.ifcond)
> +        ret += gen_if(memb.ifcond.ifcond)
>          ret += mcgen('''
>      %(c_enum)s,
>  ''',
>                       c_enum=c_enum_const(name, memb.name, prefix))
> -        ret += gen_endif(memb.ifcond)
> +        ret += gen_endif(memb.ifcond.ifcond)
>  
>      ret += mcgen('''
>  } %(c_name)s;
> @@ -125,7 +126,7 @@ def gen_array(name: str, element_type: QAPISchemaType) -> str:
>  def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str:
>      ret = ''
>      for memb in members:
> -        ret += gen_if(memb.ifcond)
> +        ret += gen_if(memb.ifcond.ifcond)
>          if memb.optional:
>              ret += mcgen('''
>      bool has_%(c_name)s;
> @@ -135,11 +136,11 @@ def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str:
>      %(c_type)s %(c_name)s;
>  ''',
>                       c_type=memb.type.c_type(), c_name=c_name(memb.name))
> -        ret += gen_endif(memb.ifcond)
> +        ret += gen_endif(memb.ifcond.ifcond)
>      return ret
>  
>  
> -def gen_object(name: str, ifcond: Sequence[str],
> +def gen_object(name: str, ifcond: QAPISchemaIfCond,
>                 base: Optional[QAPISchemaObjectType],
>                 members: List[QAPISchemaObjectTypeMember],
>                 variants: Optional[QAPISchemaVariants]) -> str:
> @@ -158,7 +159,7 @@ def gen_object(name: str, ifcond: Sequence[str],
>      ret += mcgen('''
>  
>  ''')
> -    ret += gen_if(ifcond)
> +    ret += gen_if(ifcond.ifcond)
>      ret += mcgen('''
>  struct %(c_name)s {
>  ''',
> @@ -192,7 +193,7 @@ def gen_object(name: str, ifcond: Sequence[str],
>      ret += mcgen('''
>  };
>  ''')
> -    ret += gen_endif(ifcond)
> +    ret += gen_endif(ifcond.ifcond)
>  
>      return ret
>  
> @@ -219,13 +220,13 @@ def gen_variants(variants: QAPISchemaVariants) -> str:
>      for var in variants.variants:
>          if var.type.name == 'q_empty':
>              continue
> -        ret += gen_if(var.ifcond)
> +        ret += gen_if(var.ifcond.ifcond)
>          ret += mcgen('''
>          %(c_type)s %(c_name)s;
>  ''',
>                       c_type=var.type.c_unboxed_type(),
>                       c_name=c_name(var.name))
> -        ret += gen_endif(var.ifcond)
> +        ret += gen_endif(var.ifcond.ifcond)
>  
>      ret += mcgen('''
>      } u;
> @@ -307,7 +308,7 @@ def _gen_type_cleanup(self, name: str) -> None:
>      def visit_enum_type(self,
>                          name: str,
>                          info: Optional[QAPISourceInfo],
> -                        ifcond: Sequence[str],
> +                        ifcond: QAPISchemaIfCond,
>                          features: List[QAPISchemaFeature],
>                          members: List[QAPISchemaEnumMember],
>                          prefix: Optional[str]) -> None:
> @@ -318,7 +319,7 @@ def visit_enum_type(self,
>      def visit_array_type(self,
>                           name: str,
>                           info: Optional[QAPISourceInfo],
> -                         ifcond: Sequence[str],
> +                         ifcond: QAPISchemaIfCond,
>                           element_type: QAPISchemaType) -> None:
>          with ifcontext(ifcond, self._genh, self._genc):
>              self._genh.preamble_add(gen_fwd_object_or_array(name))
> @@ -328,7 +329,7 @@ def visit_array_type(self,
>      def visit_object_type(self,
>                            name: str,
>                            info: Optional[QAPISourceInfo],
> -                          ifcond: Sequence[str],
> +                          ifcond: QAPISchemaIfCond,
>                            features: List[QAPISchemaFeature],
>                            base: Optional[QAPISchemaObjectType],
>                            members: List[QAPISchemaObjectTypeMember],
> @@ -351,7 +352,7 @@ def visit_object_type(self,
>      def visit_alternate_type(self,
>                               name: str,
>                               info: Optional[QAPISourceInfo],
> -                             ifcond: Sequence[str],
> +                             ifcond: QAPISchemaIfCond,
>                               features: List[QAPISchemaFeature],
>                               variants: QAPISchemaVariants) -> None:
>          with ifcontext(ifcond, self._genh):
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 9e96f3c566..67721b2470 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -13,7 +13,7 @@
>  See the COPYING file in the top-level directory.
>  """
>  
> -from typing import List, Optional, Sequence
> +from typing import List, Optional
>  
>  from .common import (
>      c_enum_const,
> @@ -29,6 +29,7 @@
>      QAPISchemaEnumMember,
>      QAPISchemaEnumType,
>      QAPISchemaFeature,
> +    QAPISchemaIfCond,
>      QAPISchemaObjectType,
>      QAPISchemaObjectTypeMember,
>      QAPISchemaType,
> @@ -78,7 +79,7 @@ def gen_visit_object_members(name: str,
>  
>      for memb in members:
>          deprecated = 'deprecated' in [f.name for f in memb.features]
> -        ret += gen_if(memb.ifcond)
> +        ret += gen_if(memb.ifcond.ifcond)
>          if memb.optional:
>              ret += mcgen('''
>      if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
> @@ -111,7 +112,7 @@ def gen_visit_object_members(name: str,
>              ret += mcgen('''
>      }
>  ''')
> -        ret += gen_endif(memb.ifcond)
> +        ret += gen_endif(memb.ifcond.ifcond)
>  
>      if variants:
>          tag_member = variants.tag_member
> @@ -125,7 +126,7 @@ def gen_visit_object_members(name: str,
>          for var in variants.variants:
>              case_str = c_enum_const(tag_member.type.name, var.name,
>                                      tag_member.type.prefix)
> -            ret += gen_if(var.ifcond)
> +            ret += gen_if(var.ifcond.ifcond)
>              if var.type.name == 'q_empty':
>                  # valid variant and nothing to do
>                  ret += mcgen('''
> @@ -141,7 +142,7 @@ def gen_visit_object_members(name: str,
>                               case=case_str,
>                               c_type=var.type.c_name(), c_name=c_name(var.name))
>  
> -            ret += gen_endif(var.ifcond)
> +            ret += gen_endif(var.ifcond.ifcond)
>          ret += mcgen('''
>      default:
>          abort();
> @@ -227,7 +228,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
>                  c_name=c_name(name))
>  
>      for var in variants.variants:
> -        ret += gen_if(var.ifcond)
> +        ret += gen_if(var.ifcond.ifcond)
>          ret += mcgen('''
>      case %(case)s:
>  ''',
> @@ -253,7 +254,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
>          ret += mcgen('''
>          break;
>  ''')
> -        ret += gen_endif(var.ifcond)
> +        ret += gen_endif(var.ifcond.ifcond)
>  
>      ret += mcgen('''
>      case QTYPE_NONE:
> @@ -352,7 +353,7 @@ def _begin_user_module(self, name: str) -> None:
>      def visit_enum_type(self,
>                          name: str,
>                          info: Optional[QAPISourceInfo],
> -                        ifcond: Sequence[str],
> +                        ifcond: QAPISchemaIfCond,
>                          features: List[QAPISchemaFeature],
>                          members: List[QAPISchemaEnumMember],
>                          prefix: Optional[str]) -> None:
> @@ -363,7 +364,7 @@ def visit_enum_type(self,
>      def visit_array_type(self,
>                           name: str,
>                           info: Optional[QAPISourceInfo],
> -                         ifcond: Sequence[str],
> +                         ifcond: QAPISchemaIfCond,
>                           element_type: QAPISchemaType) -> None:
>          with ifcontext(ifcond, self._genh, self._genc):
>              self._genh.add(gen_visit_decl(name))
> @@ -372,7 +373,7 @@ def visit_array_type(self,
>      def visit_object_type(self,
>                            name: str,
>                            info: Optional[QAPISourceInfo],
> -                          ifcond: Sequence[str],
> +                          ifcond: QAPISchemaIfCond,
>                            features: List[QAPISchemaFeature],
>                            base: Optional[QAPISchemaObjectType],
>                            members: List[QAPISchemaObjectTypeMember],
> @@ -394,7 +395,7 @@ def visit_object_type(self,
>      def visit_alternate_type(self,
>                               name: str,
>                               info: Optional[QAPISourceInfo],
> -                             ifcond: Sequence[str],
> +                             ifcond: QAPISchemaIfCond,
>                               features: List[QAPISchemaFeature],
>                               variants: QAPISchemaVariants) -> None:
>          with ifcontext(ifcond, self._genh, self._genc):
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index f1c4deb9a5..2ec328b22e 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -95,7 +95,7 @@ def _print_variants(variants):
>      @staticmethod
>      def _print_if(ifcond, indent=4):
>          if ifcond:
> -            print('%sif %s' % (' ' * indent, ifcond))
> +            print('%sif %s' % (' ' * indent, ifcond.ifcond))
>  
>      @classmethod
>      def _print_features(cls, features, indent=4):

If feel this is a bit harder to review than necessary, because you take
two steps at once:

1. Wrap Sequence[str] in an object

2. Add methods to the object to clean up the resulting mess some

Step 1. by itself should be pretty much mechanical: adjust the type
hints, create the wrapper object on write, peel it off on read.  The
result will be slightly ugly in places.

I'd expect step 2 to be much smaller, and easier to understand.  It
could perhaps be split into one patch per method, but I hope it's
reviewable just fine even without.

Hi

On Mon, Jun 14, 2021 at 4:20 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Wrap the 'if' condition in a higher-level object. Not only does this
>
> I can see "wrap in an object".  I'm afraid don't get what makes it
> "higher-level".
>

ok


> > provide more type safety but it also enables further refactoring without
> > too much churn.
>
> I figure by "more type safety" you mean "can't accidentally confuse some
> other list of strings with a conditional", which is true, but isn't
> really about *type* safety.
>
> Maybe:
>
>   Wrap the 'if' condition in an object.  Not only is this a bit safer,
>   it also enables further refactoring without too much churn.
>
>
ok

> The following patches will change the syntax of the schema 'if'
> > conditions to be predicate expressions, and will generate code for
> > different target languages (C, and Rust in another series).
>
> Since different target languages aren't actually generated in this
> series, I'd say "and will enable generating code for different target
> languages, such as Rust."
>

ok


> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Tested-by: John Snow <jsnow@redhat.com>
> > ---
> >  docs/sphinx/qapidoc.py         |  2 +-
> >  scripts/qapi/commands.py       |  4 +-
> >  scripts/qapi/events.py         |  5 ++-
> >  scripts/qapi/gen.py            | 14 +++----
> >  scripts/qapi/introspect.py     | 26 ++++++------
> >  scripts/qapi/schema.py         | 74 +++++++++++++++++++++++-----------
> >  scripts/qapi/types.py          | 33 +++++++--------
> >  scripts/qapi/visit.py          | 23 ++++++-----
> >  tests/qapi-schema/test-qapi.py |  2 +-
> >  9 files changed, 106 insertions(+), 77 deletions(-)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index 87c67ab23f..b737949007 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -116,7 +116,7 @@ def _nodes_for_ifcond(self, ifcond, with_if=True):
> >          the conditions are in literal-text and the commas are not.
> >          If with_if is False, we don't return the "(If: " and ")".
> >          """
> > -        condlist = intersperse([nodes.literal('', c) for c in ifcond],
> > +        condlist = intersperse([nodes.literal('', c) for c in
> ifcond.ifcond],
> >                                 nodes.Text(', '))
> >          if not with_if:
> >              return condlist
>
> Note for later: many hunks replace ifcond (the unwrapped Sequence[str])
> by ifcond.ifcond (the wrapped one, with the wrapper peeled off).
> Mechanical.
>
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index 0e13d51054..3654825968 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -17,7 +17,6 @@
> >      Dict,
> >      List,
> >      Optional,
> > -    Sequence,
> >      Set,
> >  )
> >
> > @@ -31,6 +30,7 @@
> >  from .schema import (
> >      QAPISchema,
> >      QAPISchemaFeature,
> > +    QAPISchemaIfCond,
> >      QAPISchemaObjectType,
> >      QAPISchemaType,
> >  )
> > @@ -301,7 +301,7 @@ def visit_end(self) -> None:
> >      def visit_command(self,
> >                        name: str,
> >                        info: Optional[QAPISourceInfo],
> > -                      ifcond: Sequence[str],
> > +                      ifcond: QAPISchemaIfCond,
> >                        features: List[QAPISchemaFeature],
> >                        arg_type: Optional[QAPISchemaObjectType],
> >                        ret_type: Optional[QAPISchemaType],
>
> Note for later: many hunks replace ifcond: Sequence[str] or
> Iterable[str] by ifcond: QAPISchemaIfCond.  Mechanical.
>
> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > index fee8c671e7..82475e84ec 100644
> > --- a/scripts/qapi/events.py
> > +++ b/scripts/qapi/events.py
> > @@ -12,7 +12,7 @@
> >  See the COPYING file in the top-level directory.
> >  """
> >
> > -from typing import List, Optional, Sequence
> > +from typing import List, Optional
> >
> >  from .common import c_enum_const, c_name, mcgen
> >  from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
> > @@ -20,6 +20,7 @@
> >      QAPISchema,
> >      QAPISchemaEnumMember,
> >      QAPISchemaFeature,
> > +    QAPISchemaIfCond,
> >      QAPISchemaObjectType,
> >  )
> >  from .source import QAPISourceInfo
> > @@ -227,7 +228,7 @@ def visit_end(self) -> None:
> >      def visit_event(self,
> >                      name: str,
> >                      info: Optional[QAPISourceInfo],
> > -                    ifcond: Sequence[str],
> > +                    ifcond: QAPISchemaIfCond,
> >                      features: List[QAPISchemaFeature],
> >                      arg_type: Optional[QAPISchemaObjectType],
> >                      boxed: bool) -> None:
> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > index 1fa503bdbd..1c5b190276 100644
> > --- a/scripts/qapi/gen.py
> > +++ b/scripts/qapi/gen.py
> > @@ -18,7 +18,6 @@
> >      Dict,
> >      Iterator,
> >      Optional,
> > -    Sequence,
> >      Tuple,
> >  )
> >
> > @@ -32,6 +31,7 @@
> >      mcgen,
> >  )
> >  from .schema import (
> > +    QAPISchemaIfCond,
> >      QAPISchemaModule,
> >      QAPISchemaObjectType,
> >      QAPISchemaVisitor,
> > @@ -85,7 +85,7 @@ def write(self, output_dir: str) -> None:
> >                  fp.write(text)
> >
> >
> > -def _wrap_ifcond(ifcond: Sequence[str], before: str, after: str) -> str:
> > +def _wrap_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) ->
> str:
> >      if before == after:
> >          return after   # suppress empty #if ... #endif
> >
> > @@ -95,9 +95,9 @@ def _wrap_ifcond(ifcond: Sequence[str], before: str,
> after: str) -> str:
> >      if added[0] == '\n':
> >          out += '\n'
> >          added = added[1:]
> > -    out += gen_if(ifcond)
> > +    out += gen_if(ifcond.ifcond)
> >      out += added
> > -    out += gen_endif(ifcond)
> > +    out += gen_endif(ifcond.ifcond)
> >      return out
> >
> >
> > @@ -127,9 +127,9 @@ def build_params(arg_type:
> Optional[QAPISchemaObjectType],
> >  class QAPIGenCCode(QAPIGen):
> >      def __init__(self, fname: str):
> >          super().__init__(fname)
> > -        self._start_if: Optional[Tuple[Sequence[str], str, str]] = None
> > +        self._start_if: Optional[Tuple[QAPISchemaIfCond, str, str]] =
> None
> >
> > -    def start_if(self, ifcond: Sequence[str]) -> None:
> > +    def start_if(self, ifcond: QAPISchemaIfCond) -> None:
> >          assert self._start_if is None
> >          self._start_if = (ifcond, self._body, self._preamble)
> >
> > @@ -187,7 +187,7 @@ def _bottom(self) -> str:
> >
> >
> >  @contextmanager
> > -def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) ->
> Iterator[None]:
> > +def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) ->
> Iterator[None]:
> >      """
> >      A with-statement context manager that wraps with `start_if()` /
> `end_if()`.
> >
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index 9a348ca2e5..77a8c33ad4 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -15,11 +15,9 @@
> >      Any,
> >      Dict,
> >      Generic,
> > -    Iterable,
> >      List,
> >      Optional,
> >      Sequence,
> > -    Tuple,
> >      TypeVar,
> >      Union,
> >  )
> > @@ -38,6 +36,7 @@
> >      QAPISchemaEntity,
> >      QAPISchemaEnumMember,
> >      QAPISchemaFeature,
> > +    QAPISchemaIfCond,
> >      QAPISchemaObjectType,
> >      QAPISchemaObjectTypeMember,
> >      QAPISchemaType,
> > @@ -91,11 +90,11 @@ class Annotated(Generic[_ValueT]):
> >      """
> >      # TODO: Remove after Python 3.7 adds @dataclass:
> >      # pylint: disable=too-few-public-methods
> > -    def __init__(self, value: _ValueT, ifcond: Iterable[str],
> > +    def __init__(self, value: _ValueT, ifcond: QAPISchemaIfCond,
> >                   comment: Optional[str] = None):
> >          self.value = value
> >          self.comment: Optional[str] = comment
> > -        self.ifcond: Tuple[str, ...] = tuple(ifcond)
> > +        self.ifcond = ifcond
> >
> >
> >  def _tree_to_qlit(obj: JSONValue,
> > @@ -125,10 +124,10 @@ def indent(level: int) -> str:
> >          if obj.comment:
> >              ret += indent(level) + f"/* {obj.comment} */\n"
> >          if obj.ifcond:
> > -            ret += gen_if(obj.ifcond)
> > +            ret += gen_if(obj.ifcond.ifcond)
> >          ret += _tree_to_qlit(obj.value, level)
> >          if obj.ifcond:
> > -            ret += '\n' + gen_endif(obj.ifcond)
> > +            ret += '\n' + gen_endif(obj.ifcond.ifcond)
> >          return ret
> >
> >      ret = ''
> > @@ -254,7 +253,7 @@ def _gen_features(features:
> Sequence[QAPISchemaFeature]
> >          return [Annotated(f.name, f.ifcond) for f in features]
> >
> >      def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
> > -                  ifcond: Sequence[str] = (),
> > +                  ifcond: QAPISchemaIfCond = QAPISchemaIfCond(),
>
> The same QAPISchemaIfCond object gets reused every time we don't pass an
> @ifcond argument.  Looks a bit scary, but works, because we don't ever
> mutate it.
>
> Elsewhere, we None as default, though: QAPISchemaEntity.__init__(),
> QAPISchemaMember.__init__().
>

A mechanical change, isn't it?


> >                    features: Sequence[QAPISchemaFeature] = ()) -> None:
> >          """
> >          Build and append a SchemaInfo object to self._trees.
> > @@ -305,7 +304,7 @@ def visit_builtin_type(self, name: str, info:
> Optional[QAPISourceInfo],
> >          self._gen_tree(name, 'builtin', {'json-type': json_type})
> >
> >      def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
> > -                        ifcond: Sequence[str],
> > +                        ifcond: QAPISchemaIfCond,
> >                          features: List[QAPISchemaFeature],
> >                          members: List[QAPISchemaEnumMember],
> >                          prefix: Optional[str]) -> None:
> > @@ -316,14 +315,14 @@ def visit_enum_type(self, name: str, info:
> Optional[QAPISourceInfo],
> >          )
> >
> >      def visit_array_type(self, name: str, info:
> Optional[QAPISourceInfo],
> > -                         ifcond: Sequence[str],
> > +                         ifcond: QAPISchemaIfCond,
> >                           element_type: QAPISchemaType) -> None:
> >          element = self._use_type(element_type)
> >          self._gen_tree('[' + element + ']', 'array', {'element-type':
> element},
> >                         ifcond)
> >
> >      def visit_object_type_flat(self, name: str, info:
> Optional[QAPISourceInfo],
> > -                               ifcond: Sequence[str],
> > +                               ifcond: QAPISchemaIfCond,
> >                                 features: List[QAPISchemaFeature],
> >                                 members:
> List[QAPISchemaObjectTypeMember],
> >                                 variants: Optional[QAPISchemaVariants])
> -> None:
> > @@ -336,7 +335,7 @@ def visit_object_type_flat(self, name: str, info:
> Optional[QAPISourceInfo],
> >          self._gen_tree(name, 'object', obj, ifcond, features)
> >
> >      def visit_alternate_type(self, name: str, info:
> Optional[QAPISourceInfo],
> > -                             ifcond: Sequence[str],
> > +                             ifcond: QAPISchemaIfCond,
> >                               features: List[QAPISchemaFeature],
> >                               variants: QAPISchemaVariants) -> None:
> >          self._gen_tree(
> > @@ -348,7 +347,7 @@ def visit_alternate_type(self, name: str, info:
> Optional[QAPISourceInfo],
> >          )
> >
> >      def visit_command(self, name: str, info: Optional[QAPISourceInfo],
> > -                      ifcond: Sequence[str],
> > +                      ifcond: QAPISchemaIfCond,
> >                        features: List[QAPISchemaFeature],
> >                        arg_type: Optional[QAPISchemaObjectType],
> >                        ret_type: Optional[QAPISchemaType], gen: bool,
> > @@ -367,7 +366,8 @@ def visit_command(self, name: str, info:
> Optional[QAPISourceInfo],
> >          self._gen_tree(name, 'command', obj, ifcond, features)
> >
> >      def visit_event(self, name: str, info: Optional[QAPISourceInfo],
> > -                    ifcond: Sequence[str], features:
> List[QAPISchemaFeature],
> > +                    ifcond: QAPISchemaIfCond,
> > +                    features: List[QAPISchemaFeature],
> >                      arg_type: Optional[QAPISchemaObjectType],
> >                      boxed: bool) -> None:
> >          assert self._schema is not None
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index d1d27ff7ee..bc357ebbfa 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -25,6 +25,20 @@
> >  from .parser import QAPISchemaParser
> >
> >
> > +class QAPISchemaIfCond:
> > +    def __init__(self, ifcond=None):
> > +        self.ifcond = ifcond or []
> > +
> > +    # Returns true if the condition is not void
> > +    def __bool__(self):
> > +        return bool(self.ifcond)
>
> I'm not sure I like this one.
>
> In the two places where we default parameter icond=None, we use
>
>     ifcond or QAPISchemaIfCond()
>
> to flip to the default value we actually want.  Works as intended for
> None and for non-empty QAPISchemaIfCond.  For empty QAPISchemaIfCond, it
> throws away the value and creates a new, equivalent one.  Works, but
> kind of by accident.
>
> This is an instance of a more general problem: when I see an
> Optional[ObjectType], I expect it to be falsy if and only if it's None.
> Perhaps I shouldn't.  Doesn't mean we should press __bool__() into
> service for checking "is there a condition".  A boring non-dunder method
> might be clearer.
>
> I understand what you mean by "condition is void", but it sounds a bit
> odd to me.  How do you like "Is a condition present?"
>

The current code uses falsy values for ifcond (whether it is [], (), None
whatever). Implementing __bool__() allowed to keep the existing condition
code (ie: if ifcond).

After the wrapping object is introduced, we have "if ifcond.ifcond", which
is quite ugly.

If you think "if ifcond" isn't clear enough (with __bool__()), we can have
"if ifcond.is_present()". I don't have a preference.


> > +
> > +    def __eq__(self, other):
> > +        if not isinstance(other, QAPISchemaIfCond):
> > +            return NotImplemented
> > +        return self.ifcond == other.ifcond
>
> Stupid question: why do we need to override __eq__()?
>
> Hmm, probably for _make_implicit_object_type().
>
>
Yes, the code works with schema objects and ifcond. I'll special case the
assertion for now, and remove that method.


> Why raise on type mismatch?  Feels rather un-pythonic to me.
>

What else should it do? Could probably be removed for now.


> > +
> > +
> >  class QAPISchemaEntity:
> >      meta: Optional[str] = None
> >
> > @@ -42,7 +56,7 @@ def __init__(self, name: str, info, doc, ifcond=None,
> features=None):
> >          # such place).
> >          self.info = info
> >          self.doc = doc
> > -        self._ifcond = ifcond or []
> > +        self._ifcond = ifcond or QAPISchemaIfCond()
> >          self.features = features or []
> >          self._checked = False
> >
> > @@ -77,7 +91,7 @@ def set_module(self, schema):
> >
> >      @property
> >      def ifcond(self):
> > -        assert self._checked
> > +        assert self._checked and isinstance(self._ifcond,
> QAPISchemaIfCond)
> >          return self._ifcond
> >
> >      def is_implicit(self):
> > @@ -601,7 +615,7 @@ def check(self, schema, seen):
> >          else:                   # simple union
> >              assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> >              assert not self.tag_member.optional
> > -            assert self.tag_member.ifcond == []
> > +            assert not self.tag_member.ifcond
> >          if self._tag_name:    # flat union
> >              # branches that are not explicitly covered get an empty type
> >              cases = {v.name for v in self.variants}
> > @@ -646,7 +660,7 @@ def __init__(self, name, info, ifcond=None):
> >          assert isinstance(name, str)
> >          self.name = name
> >          self.info = info
> > -        self.ifcond = ifcond or []
> > +        self.ifcond = ifcond or QAPISchemaIfCond()
> >          self.defined_in = None
> >
> >      def set_defined_in(self, name):
> > @@ -968,11 +982,13 @@ def _def_predefineds(self):
> >      def _make_features(self, features, info):
> >          if features is None:
> >              return []
> > -        return [QAPISchemaFeature(f['name'], info, f.get('if'))
> > +        return [QAPISchemaFeature(f['name'], info,
> > +                                  QAPISchemaIfCond(f.get('if')))
> >                  for f in features]
> >
> >      def _make_enum_members(self, values, info):
> > -        return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
> > +        return [QAPISchemaEnumMember(v['name'], info,
> > +                                     QAPISchemaIfCond(v.get('if')))
> >                  for v in values]
> >
>
> Note for later: several hunks wrap condition expressions in an object
> like this.  Mechanical.
>
> >      def _make_implicit_enum_type(self, name, info, ifcond, values):
> > @@ -1008,7 +1024,7 @@ def _make_implicit_object_type(self, name, info,
> ifcond, role, members):
> >              # TODO kill simple unions or implement the disjunction
> >
> >              # pylint: disable=protected-access
> > -            assert (ifcond or []) == typ._ifcond
> > +            assert ifcond == typ._ifcond
>
> I'm not sure I understand this part.  Leaving for later.
>
> >          else:
> >              self._def_entity(QAPISchemaObjectType(
> >                  name, info, None, ifcond, None, None, members, None))
> > @@ -1018,7 +1034,7 @@ def _def_enum_type(self, expr, info, doc):
> >          name = expr['enum']
> >          data = expr['data']
> >          prefix = expr.get('prefix')
> > -        ifcond = expr.get('if')
> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          self._def_entity(QAPISchemaEnumType(
> >              name, info, doc, ifcond, features,
> > @@ -1036,7 +1052,8 @@ def _make_member(self, name, typ, ifcond,
> features, info):
> >                                            self._make_features(features,
> info))
> >
> >      def _make_members(self, data, info):
> > -        return [self._make_member(key, value['type'], value.get('if'),
> > +        return [self._make_member(key, value['type'],
> > +                                  QAPISchemaIfCond(value.get('if')),
> >                                    value.get('features'), info)
> >                  for (key, value) in data.items()]
> >
> > @@ -1044,7 +1061,7 @@ def _def_struct_type(self, expr, info, doc):
> >          name = expr['struct']
> >          base = expr.get('base')
> >          data = expr['data']
> > -        ifcond = expr.get('if')
> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          self._def_entity(QAPISchemaObjectType(
> >              name, info, doc, ifcond, features, base,
> > @@ -1067,7 +1084,7 @@ def _def_union_type(self, expr, info, doc):
> >          name = expr['union']
> >          data = expr['data']
> >          base = expr.get('base')
> > -        ifcond = expr.get('if')
> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          tag_name = expr.get('discriminator')
> >          tag_member = None
> > @@ -1076,15 +1093,21 @@ def _def_union_type(self, expr, info, doc):
> >                  name, info, ifcond,
> >                  'base', self._make_members(base, info))
> >          if tag_name:
> > -            variants = [self._make_variant(key, value['type'],
> > -                                           value.get('if'), info)
> > -                        for (key, value) in data.items()]
> > +            variants = [
> > +                self._make_variant(key, value['type'],
> > +                                   QAPISchemaIfCond(value.get('if')),
> > +                                   info)
> > +                for (key, value) in data.items()
> > +            ]
>
> I think we more usually put the closing parenthesis like this:
>
>                variants = [
>                   ...
>                   for (key, value) in data.items()]
>
> More of the same below.
>

Works for me.


> >              members = []
> >          else:
> > -            variants = [self._make_simple_variant(key, value['type'],
> > -                                                  value.get('if'), info)
> > -                        for (key, value) in data.items()]
> > -            enum = [{'name': v.name, 'if': v.ifcond} for v in variants]
> > +            variants = [
> > +                self._make_simple_variant(key, value['type'],
> > +
> QAPISchemaIfCond(value.get('if')),
> > +                                          info)
> > +                for (key, value) in data.items()
> > +            ]
> > +            enum = [{'name': v.name, 'if': v.ifcond.ifcond} for v in
> variants]
> >              typ = self._make_implicit_enum_type(name, info, ifcond,
> enum)
> >              tag_member = QAPISchemaObjectTypeMember('type', info, typ,
> False)
> >              members = [tag_member]
> > @@ -1097,11 +1120,14 @@ def _def_union_type(self, expr, info, doc):
> >      def _def_alternate_type(self, expr, info, doc):
> >          name = expr['alternate']
> >          data = expr['data']
> > -        ifcond = expr.get('if')
> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> > -        variants = [self._make_variant(key, value['type'],
> value.get('if'),
> > -                                       info)
> > -                    for (key, value) in data.items()]
> > +        variants = [
> > +            self._make_variant(key, value['type'],
> > +                               QAPISchemaIfCond(value.get('if')),
> > +                               info)
> > +            for (key, value) in data.items()
> > +        ]
> >          tag_member = QAPISchemaObjectTypeMember('type', info, 'QType',
> False)
> >          self._def_entity(
> >              QAPISchemaAlternateType(name, info, doc, ifcond, features,
> > @@ -1118,7 +1144,7 @@ def _def_command(self, expr, info, doc):
> >          allow_oob = expr.get('allow-oob', False)
> >          allow_preconfig = expr.get('allow-preconfig', False)
> >          coroutine = expr.get('coroutine', False)
> > -        ifcond = expr.get('if')
> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          if isinstance(data, OrderedDict):
> >              data = self._make_implicit_object_type(
> > @@ -1137,7 +1163,7 @@ def _def_event(self, expr, info, doc):
> >          name = expr['event']
> >          data = expr.get('data')
> >          boxed = expr.get('boxed', False)
> > -        ifcond = expr.get('if')
> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          if isinstance(data, OrderedDict):
> >              data = self._make_implicit_object_type(
> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> > index 20d572a23a..3673cf0f49 100644
> > --- a/scripts/qapi/types.py
> > +++ b/scripts/qapi/types.py
> > @@ -13,7 +13,7 @@
> >  # See the COPYING file in the top-level directory.
> >  """
> >
> > -from typing import List, Optional, Sequence
> > +from typing import List, Optional
> >
> >  from .common import (
> >      c_enum_const,
> > @@ -27,6 +27,7 @@
> >      QAPISchema,
> >      QAPISchemaEnumMember,
> >      QAPISchemaFeature,
> > +    QAPISchemaIfCond,
> >      QAPISchemaObjectType,
> >      QAPISchemaObjectTypeMember,
> >      QAPISchemaType,
> > @@ -50,13 +51,13 @@ def gen_enum_lookup(name: str,
> >  ''',
> >                  c_name=c_name(name))
> >      for memb in members:
> > -        ret += gen_if(memb.ifcond)
> > +        ret += gen_if(memb.ifcond.ifcond)
> >          index = c_enum_const(name, memb.name, prefix)
> >          ret += mcgen('''
> >          [%(index)s] = "%(name)s",
> >  ''',
> >                       index=index, name=memb.name)
> > -        ret += gen_endif(memb.ifcond)
> > +        ret += gen_endif(memb.ifcond.ifcond)
> >
> >      ret += mcgen('''
> >      },
> > @@ -80,12 +81,12 @@ def gen_enum(name: str,
> >                  c_name=c_name(name))
> >
> >      for memb in enum_members:
> > -        ret += gen_if(memb.ifcond)
> > +        ret += gen_if(memb.ifcond.ifcond)
> >          ret += mcgen('''
> >      %(c_enum)s,
> >  ''',
> >                       c_enum=c_enum_const(name, memb.name, prefix))
> > -        ret += gen_endif(memb.ifcond)
> > +        ret += gen_endif(memb.ifcond.ifcond)
> >
> >      ret += mcgen('''
> >  } %(c_name)s;
> > @@ -125,7 +126,7 @@ def gen_array(name: str, element_type:
> QAPISchemaType) -> str:
> >  def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) ->
> str:
> >      ret = ''
> >      for memb in members:
> > -        ret += gen_if(memb.ifcond)
> > +        ret += gen_if(memb.ifcond.ifcond)
> >          if memb.optional:
> >              ret += mcgen('''
> >      bool has_%(c_name)s;
> > @@ -135,11 +136,11 @@ def gen_struct_members(members:
> List[QAPISchemaObjectTypeMember]) -> str:
> >      %(c_type)s %(c_name)s;
> >  ''',
> >                       c_type=memb.type.c_type(), c_name=c_name(memb.name
> ))
> > -        ret += gen_endif(memb.ifcond)
> > +        ret += gen_endif(memb.ifcond.ifcond)
> >      return ret
> >
> >
> > -def gen_object(name: str, ifcond: Sequence[str],
> > +def gen_object(name: str, ifcond: QAPISchemaIfCond,
> >                 base: Optional[QAPISchemaObjectType],
> >                 members: List[QAPISchemaObjectTypeMember],
> >                 variants: Optional[QAPISchemaVariants]) -> str:
> > @@ -158,7 +159,7 @@ def gen_object(name: str, ifcond: Sequence[str],
> >      ret += mcgen('''
> >
> >  ''')
> > -    ret += gen_if(ifcond)
> > +    ret += gen_if(ifcond.ifcond)
> >      ret += mcgen('''
> >  struct %(c_name)s {
> >  ''',
> > @@ -192,7 +193,7 @@ def gen_object(name: str, ifcond: Sequence[str],
> >      ret += mcgen('''
> >  };
> >  ''')
> > -    ret += gen_endif(ifcond)
> > +    ret += gen_endif(ifcond.ifcond)
> >
> >      return ret
> >
> > @@ -219,13 +220,13 @@ def gen_variants(variants: QAPISchemaVariants) ->
> str:
> >      for var in variants.variants:
> >          if var.type.name == 'q_empty':
> >              continue
> > -        ret += gen_if(var.ifcond)
> > +        ret += gen_if(var.ifcond.ifcond)
> >          ret += mcgen('''
> >          %(c_type)s %(c_name)s;
> >  ''',
> >                       c_type=var.type.c_unboxed_type(),
> >                       c_name=c_name(var.name))
> > -        ret += gen_endif(var.ifcond)
> > +        ret += gen_endif(var.ifcond.ifcond)
> >
> >      ret += mcgen('''
> >      } u;
> > @@ -307,7 +308,7 @@ def _gen_type_cleanup(self, name: str) -> None:
> >      def visit_enum_type(self,
> >                          name: str,
> >                          info: Optional[QAPISourceInfo],
> > -                        ifcond: Sequence[str],
> > +                        ifcond: QAPISchemaIfCond,
> >                          features: List[QAPISchemaFeature],
> >                          members: List[QAPISchemaEnumMember],
> >                          prefix: Optional[str]) -> None:
> > @@ -318,7 +319,7 @@ def visit_enum_type(self,
> >      def visit_array_type(self,
> >                           name: str,
> >                           info: Optional[QAPISourceInfo],
> > -                         ifcond: Sequence[str],
> > +                         ifcond: QAPISchemaIfCond,
> >                           element_type: QAPISchemaType) -> None:
> >          with ifcontext(ifcond, self._genh, self._genc):
> >              self._genh.preamble_add(gen_fwd_object_or_array(name))
> > @@ -328,7 +329,7 @@ def visit_array_type(self,
> >      def visit_object_type(self,
> >                            name: str,
> >                            info: Optional[QAPISourceInfo],
> > -                          ifcond: Sequence[str],
> > +                          ifcond: QAPISchemaIfCond,
> >                            features: List[QAPISchemaFeature],
> >                            base: Optional[QAPISchemaObjectType],
> >                            members: List[QAPISchemaObjectTypeMember],
> > @@ -351,7 +352,7 @@ def visit_object_type(self,
> >      def visit_alternate_type(self,
> >                               name: str,
> >                               info: Optional[QAPISourceInfo],
> > -                             ifcond: Sequence[str],
> > +                             ifcond: QAPISchemaIfCond,
> >                               features: List[QAPISchemaFeature],
> >                               variants: QAPISchemaVariants) -> None:
> >          with ifcontext(ifcond, self._genh):
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 9e96f3c566..67721b2470 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -13,7 +13,7 @@
> >  See the COPYING file in the top-level directory.
> >  """
> >
> > -from typing import List, Optional, Sequence
> > +from typing import List, Optional
> >
> >  from .common import (
> >      c_enum_const,
> > @@ -29,6 +29,7 @@
> >      QAPISchemaEnumMember,
> >      QAPISchemaEnumType,
> >      QAPISchemaFeature,
> > +    QAPISchemaIfCond,
> >      QAPISchemaObjectType,
> >      QAPISchemaObjectTypeMember,
> >      QAPISchemaType,
> > @@ -78,7 +79,7 @@ def gen_visit_object_members(name: str,
> >
> >      for memb in members:
> >          deprecated = 'deprecated' in [f.name for f in memb.features]
> > -        ret += gen_if(memb.ifcond)
> > +        ret += gen_if(memb.ifcond.ifcond)
> >          if memb.optional:
> >              ret += mcgen('''
> >      if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
> > @@ -111,7 +112,7 @@ def gen_visit_object_members(name: str,
> >              ret += mcgen('''
> >      }
> >  ''')
> > -        ret += gen_endif(memb.ifcond)
> > +        ret += gen_endif(memb.ifcond.ifcond)
> >
> >      if variants:
> >          tag_member = variants.tag_member
> > @@ -125,7 +126,7 @@ def gen_visit_object_members(name: str,
> >          for var in variants.variants:
> >              case_str = c_enum_const(tag_member.type.name, var.name,
> >                                      tag_member.type.prefix)
> > -            ret += gen_if(var.ifcond)
> > +            ret += gen_if(var.ifcond.ifcond)
> >              if var.type.name == 'q_empty':
> >                  # valid variant and nothing to do
> >                  ret += mcgen('''
> > @@ -141,7 +142,7 @@ def gen_visit_object_members(name: str,
> >                               case=case_str,
> >                               c_type=var.type.c_name(), c_name=c_name(
> var.name))
> >
> > -            ret += gen_endif(var.ifcond)
> > +            ret += gen_endif(var.ifcond.ifcond)
> >          ret += mcgen('''
> >      default:
> >          abort();
> > @@ -227,7 +228,7 @@ def gen_visit_alternate(name: str, variants:
> QAPISchemaVariants) -> str:
> >                  c_name=c_name(name))
> >
> >      for var in variants.variants:
> > -        ret += gen_if(var.ifcond)
> > +        ret += gen_if(var.ifcond.ifcond)
> >          ret += mcgen('''
> >      case %(case)s:
> >  ''',
> > @@ -253,7 +254,7 @@ def gen_visit_alternate(name: str, variants:
> QAPISchemaVariants) -> str:
> >          ret += mcgen('''
> >          break;
> >  ''')
> > -        ret += gen_endif(var.ifcond)
> > +        ret += gen_endif(var.ifcond.ifcond)
> >
> >      ret += mcgen('''
> >      case QTYPE_NONE:
> > @@ -352,7 +353,7 @@ def _begin_user_module(self, name: str) -> None:
> >      def visit_enum_type(self,
> >                          name: str,
> >                          info: Optional[QAPISourceInfo],
> > -                        ifcond: Sequence[str],
> > +                        ifcond: QAPISchemaIfCond,
> >                          features: List[QAPISchemaFeature],
> >                          members: List[QAPISchemaEnumMember],
> >                          prefix: Optional[str]) -> None:
> > @@ -363,7 +364,7 @@ def visit_enum_type(self,
> >      def visit_array_type(self,
> >                           name: str,
> >                           info: Optional[QAPISourceInfo],
> > -                         ifcond: Sequence[str],
> > +                         ifcond: QAPISchemaIfCond,
> >                           element_type: QAPISchemaType) -> None:
> >          with ifcontext(ifcond, self._genh, self._genc):
> >              self._genh.add(gen_visit_decl(name))
> > @@ -372,7 +373,7 @@ def visit_array_type(self,
> >      def visit_object_type(self,
> >                            name: str,
> >                            info: Optional[QAPISourceInfo],
> > -                          ifcond: Sequence[str],
> > +                          ifcond: QAPISchemaIfCond,
> >                            features: List[QAPISchemaFeature],
> >                            base: Optional[QAPISchemaObjectType],
> >                            members: List[QAPISchemaObjectTypeMember],
> > @@ -394,7 +395,7 @@ def visit_object_type(self,
> >      def visit_alternate_type(self,
> >                               name: str,
> >                               info: Optional[QAPISourceInfo],
> > -                             ifcond: Sequence[str],
> > +                             ifcond: QAPISchemaIfCond,
> >                               features: List[QAPISchemaFeature],
> >                               variants: QAPISchemaVariants) -> None:
> >          with ifcontext(ifcond, self._genh, self._genc):
> > diff --git a/tests/qapi-schema/test-qapi.py
> b/tests/qapi-schema/test-qapi.py
> > index f1c4deb9a5..2ec328b22e 100755
> > --- a/tests/qapi-schema/test-qapi.py
> > +++ b/tests/qapi-schema/test-qapi.py
> > @@ -95,7 +95,7 @@ def _print_variants(variants):
> >      @staticmethod
> >      def _print_if(ifcond, indent=4):
> >          if ifcond:
> > -            print('%sif %s' % (' ' * indent, ifcond))
> > +            print('%sif %s' % (' ' * indent, ifcond.ifcond))
> >
> >      @classmethod
> >      def _print_features(cls, features, indent=4):
>
> If feel this is a bit harder to review than necessary, because you take
> two steps at once:
>
> 1. Wrap Sequence[str] in an object
>
> 2. Add methods to the object to clean up the resulting mess some
>
> Step 1. by itself should be pretty much mechanical: adjust the type
> hints, create the wrapper object on write, peel it off on read.  The
> result will be slightly ugly in places.
>
> I'd expect step 2 to be much smaller, and easier to understand.  It
> could perhaps be split into one patch per method, but I hope it's
> reviewable just fine even without.
>
>
>

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Mon, Jun 14, 2021 at 4:20 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Wrap the 'if' condition in a higher-level object. Not only does this
>>
>> I can see "wrap in an object".  I'm afraid don't get what makes it
>> "higher-level".
>>
>
> ok
>
>
>> > provide more type safety but it also enables further refactoring without
>> > too much churn.
>>
>> I figure by "more type safety" you mean "can't accidentally confuse some
>> other list of strings with a conditional", which is true, but isn't
>> really about *type* safety.
>>
>> Maybe:
>>
>>   Wrap the 'if' condition in an object.  Not only is this a bit safer,
>>   it also enables further refactoring without too much churn.
>>
>>
> ok
>
>> > The following patches will change the syntax of the schema 'if'
>> > conditions to be predicate expressions, and will generate code for
>> > different target languages (C, and Rust in another series).
>>
>> Since different target languages aren't actually generated in this
>> series, I'd say "and will enable generating code for different target
>> languages, such as Rust."
>>
>
> ok
>
>
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > Tested-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  docs/sphinx/qapidoc.py         |  2 +-
>> >  scripts/qapi/commands.py       |  4 +-
>> >  scripts/qapi/events.py         |  5 ++-
>> >  scripts/qapi/gen.py            | 14 +++----
>> >  scripts/qapi/introspect.py     | 26 ++++++------
>> >  scripts/qapi/schema.py         | 74 +++++++++++++++++++++++-----------
>> >  scripts/qapi/types.py          | 33 +++++++--------
>> >  scripts/qapi/visit.py          | 23 ++++++-----
>> >  tests/qapi-schema/test-qapi.py |  2 +-
>> >  9 files changed, 106 insertions(+), 77 deletions(-)
>> >
>> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> > index 87c67ab23f..b737949007 100644
>> > --- a/docs/sphinx/qapidoc.py
>> > +++ b/docs/sphinx/qapidoc.py
>> > @@ -116,7 +116,7 @@ def _nodes_for_ifcond(self, ifcond, with_if=True):
>> >          the conditions are in literal-text and the commas are not.
>> >          If with_if is False, we don't return the "(If: " and ")".
>> >          """
>> > -        condlist = intersperse([nodes.literal('', c) for c in ifcond],
>> > +        condlist = intersperse([nodes.literal('', c) for c in ifcond.ifcond],
>> >                                 nodes.Text(', '))
>> >          if not with_if:
>> >              return condlist
>>
>> Note for later: many hunks replace ifcond (the unwrapped Sequence[str])
>> by ifcond.ifcond (the wrapped one, with the wrapper peeled off).
>> Mechanical.
>>
>> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> > index 0e13d51054..3654825968 100644
>> > --- a/scripts/qapi/commands.py
>> > +++ b/scripts/qapi/commands.py
>> > @@ -17,7 +17,6 @@
>> >      Dict,
>> >      List,
>> >      Optional,
>> > -    Sequence,
>> >      Set,
>> >  )
>> >
>> > @@ -31,6 +30,7 @@
>> >  from .schema import (
>> >      QAPISchema,
>> >      QAPISchemaFeature,
>> > +    QAPISchemaIfCond,
>> >      QAPISchemaObjectType,
>> >      QAPISchemaType,
>> >  )
>> > @@ -301,7 +301,7 @@ def visit_end(self) -> None:
>> >      def visit_command(self,
>> >                        name: str,
>> >                        info: Optional[QAPISourceInfo],
>> > -                      ifcond: Sequence[str],
>> > +                      ifcond: QAPISchemaIfCond,
>> >                        features: List[QAPISchemaFeature],
>> >                        arg_type: Optional[QAPISchemaObjectType],
>> >                        ret_type: Optional[QAPISchemaType],
>>
>> Note for later: many hunks replace ifcond: Sequence[str] or
>> Iterable[str] by ifcond: QAPISchemaIfCond.  Mechanical.
>>
>> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> > index fee8c671e7..82475e84ec 100644
>> > --- a/scripts/qapi/events.py
>> > +++ b/scripts/qapi/events.py
>> > @@ -12,7 +12,7 @@
>> >  See the COPYING file in the top-level directory.
>> >  """
>> >
>> > -from typing import List, Optional, Sequence
>> > +from typing import List, Optional
>> >
>> >  from .common import c_enum_const, c_name, mcgen
>> >  from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
>> > @@ -20,6 +20,7 @@
>> >      QAPISchema,
>> >      QAPISchemaEnumMember,
>> >      QAPISchemaFeature,
>> > +    QAPISchemaIfCond,
>> >      QAPISchemaObjectType,
>> >  )
>> >  from .source import QAPISourceInfo
>> > @@ -227,7 +228,7 @@ def visit_end(self) -> None:
>> >      def visit_event(self,
>> >                      name: str,
>> >                      info: Optional[QAPISourceInfo],
>> > -                    ifcond: Sequence[str],
>> > +                    ifcond: QAPISchemaIfCond,
>> >                      features: List[QAPISchemaFeature],
>> >                      arg_type: Optional[QAPISchemaObjectType],
>> >                      boxed: bool) -> None:
>> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> > index 1fa503bdbd..1c5b190276 100644
>> > --- a/scripts/qapi/gen.py
>> > +++ b/scripts/qapi/gen.py
>> > @@ -18,7 +18,6 @@
>> >      Dict,
>> >      Iterator,
>> >      Optional,
>> > -    Sequence,
>> >      Tuple,
>> >  )
>> >
>> > @@ -32,6 +31,7 @@
>> >      mcgen,
>> >  )
>> >  from .schema import (
>> > +    QAPISchemaIfCond,
>> >      QAPISchemaModule,
>> >      QAPISchemaObjectType,
>> >      QAPISchemaVisitor,
>> > @@ -85,7 +85,7 @@ def write(self, output_dir: str) -> None:
>> >                  fp.write(text)
>> >
>> >
>> > -def _wrap_ifcond(ifcond: Sequence[str], before: str, after: str) -> str:
>> > +def _wrap_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) -> str:
>> >      if before == after:
>> >          return after   # suppress empty #if ... #endif
>> >
>> > @@ -95,9 +95,9 @@ def _wrap_ifcond(ifcond: Sequence[str], before: str, after: str) -> str:
>> >      if added[0] == '\n':
>> >          out += '\n'
>> >          added = added[1:]
>> > -    out += gen_if(ifcond)
>> > +    out += gen_if(ifcond.ifcond)
>> >      out += added
>> > -    out += gen_endif(ifcond)
>> > +    out += gen_endif(ifcond.ifcond)
>> >      return out
>> >
>> >
>> > @@ -127,9 +127,9 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
>> >  class QAPIGenCCode(QAPIGen):
>> >      def __init__(self, fname: str):
>> >          super().__init__(fname)
>> > -        self._start_if: Optional[Tuple[Sequence[str], str, str]] = None
>> > +        self._start_if: Optional[Tuple[QAPISchemaIfCond, str, str]] = None
>> >
>> > -    def start_if(self, ifcond: Sequence[str]) -> None:
>> > +    def start_if(self, ifcond: QAPISchemaIfCond) -> None:
>> >          assert self._start_if is None
>> >          self._start_if = (ifcond, self._body, self._preamble)
>> >
>> > @@ -187,7 +187,7 @@ def _bottom(self) -> str:
>> >
>> >
>> >  @contextmanager
>> > -def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]:
>> > +def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
>> >      """
>> >      A with-statement context manager that wraps with `start_if()` / `end_if()`.
>> >
>> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> > index 9a348ca2e5..77a8c33ad4 100644
>> > --- a/scripts/qapi/introspect.py
>> > +++ b/scripts/qapi/introspect.py
>> > @@ -15,11 +15,9 @@
>> >      Any,
>> >      Dict,
>> >      Generic,
>> > -    Iterable,
>> >      List,
>> >      Optional,
>> >      Sequence,
>> > -    Tuple,
>> >      TypeVar,
>> >      Union,
>> >  )
>> > @@ -38,6 +36,7 @@
>> >      QAPISchemaEntity,
>> >      QAPISchemaEnumMember,
>> >      QAPISchemaFeature,
>> > +    QAPISchemaIfCond,
>> >      QAPISchemaObjectType,
>> >      QAPISchemaObjectTypeMember,
>> >      QAPISchemaType,
>> > @@ -91,11 +90,11 @@ class Annotated(Generic[_ValueT]):
>> >      """
>> >      # TODO: Remove after Python 3.7 adds @dataclass:
>> >      # pylint: disable=too-few-public-methods
>> > -    def __init__(self, value: _ValueT, ifcond: Iterable[str],
>> > +    def __init__(self, value: _ValueT, ifcond: QAPISchemaIfCond,
>> >                   comment: Optional[str] = None):
>> >          self.value = value
>> >          self.comment: Optional[str] = comment
>> > -        self.ifcond: Tuple[str, ...] = tuple(ifcond)
>> > +        self.ifcond = ifcond
>> >
>> >
>> >  def _tree_to_qlit(obj: JSONValue,
>> > @@ -125,10 +124,10 @@ def indent(level: int) -> str:
>> >          if obj.comment:
>> >              ret += indent(level) + f"/* {obj.comment} */\n"
>> >          if obj.ifcond:
>> > -            ret += gen_if(obj.ifcond)
>> > +            ret += gen_if(obj.ifcond.ifcond)
>> >          ret += _tree_to_qlit(obj.value, level)
>> >          if obj.ifcond:
>> > -            ret += '\n' + gen_endif(obj.ifcond)
>> > +            ret += '\n' + gen_endif(obj.ifcond.ifcond)
>> >          return ret
>> >
>> >      ret = ''
>> > @@ -254,7 +253,7 @@ def _gen_features(features: Sequence[QAPISchemaFeature]
>> >          return [Annotated(f.name, f.ifcond) for f in features]
>> >
>> >      def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>> > -                  ifcond: Sequence[str] = (),
>> > +                  ifcond: QAPISchemaIfCond = QAPISchemaIfCond(),
>>
>> The same QAPISchemaIfCond object gets reused every time we don't pass an
>> @ifcond argument.  Looks a bit scary, but works, because we don't ever
>> mutate it.
>>
>> Elsewhere, we None as default, though: QAPISchemaEntity.__init__(),
>> QAPISchemaMember.__init__().
>>
>
> A mechanical change, isn't it?

We do the same thing in two different ways.

One: default parameter ifcond to QAPISchemaIfCond(), done.

Two: default it to None, use it like ifcond or QAPISchemaIfCond().

The first way is less verbose, but looks like a Python rookie mistake at
a glance.

Can we agree on one way to do this?

>> >                    features: Sequence[QAPISchemaFeature] = ()) -> None:
>> >          """
>> >          Build and append a SchemaInfo object to self._trees.
>> > @@ -305,7 +304,7 @@ def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>> >          self._gen_tree(name, 'builtin', {'json-type': json_type})
>> >
>> >      def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>> > -                        ifcond: Sequence[str],
>> > +                        ifcond: QAPISchemaIfCond,
>> >                          features: List[QAPISchemaFeature],
>> >                          members: List[QAPISchemaEnumMember],
>> >                          prefix: Optional[str]) -> None:
>> > @@ -316,14 +315,14 @@ def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>> >          )
>> >
>> >      def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
>> > -                         ifcond: Sequence[str],
>> > +                         ifcond: QAPISchemaIfCond,
>> >                           element_type: QAPISchemaType) -> None:
>> >          element = self._use_type(element_type)
>> >          self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>> >                         ifcond)
>> >
>> >      def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>> > -                               ifcond: Sequence[str],
>> > +                               ifcond: QAPISchemaIfCond,
>> >                                 features: List[QAPISchemaFeature],
>> >                                 members: List[QAPISchemaObjectTypeMember],
>> >                                 variants: Optional[QAPISchemaVariants]) -> None:
>> > @@ -336,7 +335,7 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>> >          self._gen_tree(name, 'object', obj, ifcond, features)
>> >
>> >      def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>> > -                             ifcond: Sequence[str],
>> > +                             ifcond: QAPISchemaIfCond,
>> >                               features: List[QAPISchemaFeature],
>> >                               variants: QAPISchemaVariants) -> None:
>> >          self._gen_tree(
>> > @@ -348,7 +347,7 @@ def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>> >          )
>> >
>> >      def visit_command(self, name: str, info: Optional[QAPISourceInfo],
>> > -                      ifcond: Sequence[str],
>> > +                      ifcond: QAPISchemaIfCond,
>> >                        features: List[QAPISchemaFeature],
>> >                        arg_type: Optional[QAPISchemaObjectType],
>> >                        ret_type: Optional[QAPISchemaType], gen: bool,
>> > @@ -367,7 +366,8 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
>> >          self._gen_tree(name, 'command', obj, ifcond, features)
>> >
>> >      def visit_event(self, name: str, info: Optional[QAPISourceInfo],
>> > -                    ifcond: Sequence[str], features: List[QAPISchemaFeature],
>> > +                    ifcond: QAPISchemaIfCond,
>> > +                    features: List[QAPISchemaFeature],
>> >                      arg_type: Optional[QAPISchemaObjectType],
>> >                      boxed: bool) -> None:
>> >          assert self._schema is not None
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index d1d27ff7ee..bc357ebbfa 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -25,6 +25,20 @@
>> >  from .parser import QAPISchemaParser
>> >
>> >
>> > +class QAPISchemaIfCond:
>> > +    def __init__(self, ifcond=None):
>> > +        self.ifcond = ifcond or []
>> > +
>> > +    # Returns true if the condition is not void
>> > +    def __bool__(self):
>> > +        return bool(self.ifcond)
>>
>> I'm not sure I like this one.
>>
>> In the two places where we default parameter icond=None, we use
>>
>>     ifcond or QAPISchemaIfCond()
>>
>> to flip to the default value we actually want.  Works as intended for
>> None and for non-empty QAPISchemaIfCond.  For empty QAPISchemaIfCond, it
>> throws away the value and creates a new, equivalent one.  Works, but
>> kind of by accident.
>>
>> This is an instance of a more general problem: when I see an
>> Optional[ObjectType], I expect it to be falsy if and only if it's None.
>> Perhaps I shouldn't.  Doesn't mean we should press __bool__() into
>> service for checking "is there a condition".  A boring non-dunder method
>> might be clearer.
>>
>> I understand what you mean by "condition is void", but it sounds a bit
>> odd to me.  How do you like "Is a condition present?"
>>
>
> The current code uses falsy values for ifcond (whether it is [], (), None
> whatever). Implementing __bool__() allowed to keep the existing condition
> code (ie: if ifcond).
>
> After the wrapping object is introduced, we have "if ifcond.ifcond", which
> is quite ugly.

It is.

> If you think "if ifcond" isn't clear enough (with __bool__()), we can have
> "if ifcond.is_present()". I don't have a preference.

I think I'd prefer .is_present().  I'm kind of wary of overriding
dunders when it can make innocent-looking code behave in possibly
surprising ways.  Perhaps I just haven't been steeped in the Python
sauce long enough.

John, what do you think?

>> > +
>> > +    def __eq__(self, other):
>> > +        if not isinstance(other, QAPISchemaIfCond):
>> > +            return NotImplemented
>> > +        return self.ifcond == other.ifcond
>>
>> Stupid question: why do we need to override __eq__()?
>>
>> Hmm, probably for _make_implicit_object_type().
>>
>>
> Yes, the code works with schema objects and ifcond. I'll special case the
> assertion for now, and remove that method.
>
>
>> Why raise on type mismatch?  Feels rather un-pythonic to me.
>>
>
> What else should it do? Could probably be removed for now.

Python lets me compare arbitrary objects regardless of type.  Different
objects are unqual, unless __eq__ makes them equal.

You want to make different QAPISchemaIfCond wrapping the equal .ifcond
equal.  Makes sense to me.  But I don't see a need for also changing the
case "@other is not a QAPISchemaIfCond" from False to NotImplemented.

>> > +
>> > +
>> >  class QAPISchemaEntity:
>> >      meta: Optional[str] = None
>> >
>> > @@ -42,7 +56,7 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None):
>> >          # such place).
>> >          self.info = info
>> >          self.doc = doc
>> > -        self._ifcond = ifcond or []
>> > +        self._ifcond = ifcond or QAPISchemaIfCond()
>> >          self.features = features or []
>> >          self._checked = False
>> >
>> > @@ -77,7 +91,7 @@ def set_module(self, schema):
>> >
>> >      @property
>> >      def ifcond(self):
>> > -        assert self._checked
>> > +        assert self._checked and isinstance(self._ifcond, QAPISchemaIfCond)
>> >          return self._ifcond
>> >
>> >      def is_implicit(self):
>> > @@ -601,7 +615,7 @@ def check(self, schema, seen):
>> >          else:                   # simple union
>> >              assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>> >              assert not self.tag_member.optional
>> > -            assert self.tag_member.ifcond == []
>> > +            assert not self.tag_member.ifcond
>> >          if self._tag_name:    # flat union
>> >              # branches that are not explicitly covered get an empty type
>> >              cases = {v.name for v in self.variants}
>> > @@ -646,7 +660,7 @@ def __init__(self, name, info, ifcond=None):
>> >          assert isinstance(name, str)
>> >          self.name = name
>> >          self.info = info
>> > -        self.ifcond = ifcond or []
>> > +        self.ifcond = ifcond or QAPISchemaIfCond()
>> >          self.defined_in = None
>> >
>> >      def set_defined_in(self, name):
>> > @@ -968,11 +982,13 @@ def _def_predefineds(self):
>> >      def _make_features(self, features, info):
>> >          if features is None:
>> >              return []
>> > -        return [QAPISchemaFeature(f['name'], info, f.get('if'))
>> > +        return [QAPISchemaFeature(f['name'], info,
>> > +                                  QAPISchemaIfCond(f.get('if')))
>> >                  for f in features]
>> >
>> >      def _make_enum_members(self, values, info):
>> > -        return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
>> > +        return [QAPISchemaEnumMember(v['name'], info,
>> > +                                     QAPISchemaIfCond(v.get('if')))
>> >                  for v in values]
>> >
>>
>> Note for later: several hunks wrap condition expressions in an object
>> like this.  Mechanical.
>>
>> >      def _make_implicit_enum_type(self, name, info, ifcond, values):
>> > @@ -1008,7 +1024,7 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
>> >              # TODO kill simple unions or implement the disjunction
>> >
>> >              # pylint: disable=protected-access
>> > -            assert (ifcond or []) == typ._ifcond
>> > +            assert ifcond == typ._ifcond
>>
>> I'm not sure I understand this part.  Leaving for later.
>>
>> >          else:
>> >              self._def_entity(QAPISchemaObjectType(
>> >                  name, info, None, ifcond, None, None, members, None))
>> > @@ -1018,7 +1034,7 @@ def _def_enum_type(self, expr, info, doc):
>> >          name = expr['enum']
>> >          data = expr['data']
>> >          prefix = expr.get('prefix')
>> > -        ifcond = expr.get('if')
>> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
>> >          features = self._make_features(expr.get('features'), info)
>> >          self._def_entity(QAPISchemaEnumType(
>> >              name, info, doc, ifcond, features,
>> > @@ -1036,7 +1052,8 @@ def _make_member(self, name, typ, ifcond, features, info):
>> >                                            self._make_features(features, info))
>> >
>> >      def _make_members(self, data, info):
>> > -        return [self._make_member(key, value['type'], value.get('if'),
>> > +        return [self._make_member(key, value['type'],
>> > +                                  QAPISchemaIfCond(value.get('if')),
>> >                                    value.get('features'), info)
>> >                  for (key, value) in data.items()]
>> >
>> > @@ -1044,7 +1061,7 @@ def _def_struct_type(self, expr, info, doc):
>> >          name = expr['struct']
>> >          base = expr.get('base')
>> >          data = expr['data']
>> > -        ifcond = expr.get('if')
>> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
>> >          features = self._make_features(expr.get('features'), info)
>> >          self._def_entity(QAPISchemaObjectType(
>> >              name, info, doc, ifcond, features, base,
>> > @@ -1067,7 +1084,7 @@ def _def_union_type(self, expr, info, doc):
>> >          name = expr['union']
>> >          data = expr['data']
>> >          base = expr.get('base')
>> > -        ifcond = expr.get('if')
>> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
>> >          features = self._make_features(expr.get('features'), info)
>> >          tag_name = expr.get('discriminator')
>> >          tag_member = None
>> > @@ -1076,15 +1093,21 @@ def _def_union_type(self, expr, info, doc):
>> >                  name, info, ifcond,
>> >                  'base', self._make_members(base, info))
>> >          if tag_name:
>> > -            variants = [self._make_variant(key, value['type'],
>> > -                                           value.get('if'), info)
>> > -                        for (key, value) in data.items()]
>> > +            variants = [
>> > +                self._make_variant(key, value['type'],
>> > +                                   QAPISchemaIfCond(value.get('if')),
>> > +                                   info)
>> > +                for (key, value) in data.items()
>> > +            ]
>>
>> I think we more usually put the closing parenthesis like this:
>>
>>                variants = [
>>                   ...
>>                   for (key, value) in data.items()]
>>
>> More of the same below.
>>
>
> Works for me.
>
>
>> >              members = []
>> >          else:
>> > -            variants = [self._make_simple_variant(key, value['type'],
>> > -                                                  value.get('if'), info)
>> > -                        for (key, value) in data.items()]
>> > -            enum = [{'name': v.name, 'if': v.ifcond} for v in variants]
>> > +            variants = [
>> > +                self._make_simple_variant(key, value['type'],
>> > +                                          QAPISchemaIfCond(value.get('if')),
>> > +                                          info)
>> > +                for (key, value) in data.items()
>> > +            ]
>> > +            enum = [{'name': v.name, 'if': v.ifcond.ifcond} for v in variants]
>> >              typ = self._make_implicit_enum_type(name, info, ifcond, enum)
>> >              tag_member = QAPISchemaObjectTypeMember('type', info, typ, False)
>> >              members = [tag_member]
>> > @@ -1097,11 +1120,14 @@ def _def_union_type(self, expr, info, doc):
>> >      def _def_alternate_type(self, expr, info, doc):
>> >          name = expr['alternate']
>> >          data = expr['data']
>> > -        ifcond = expr.get('if')
>> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
>> >          features = self._make_features(expr.get('features'), info)
>> > -        variants = [self._make_variant(key, value['type'], value.get('if'),
>> > -                                       info)
>> > -                    for (key, value) in data.items()]
>> > +        variants = [
>> > +            self._make_variant(key, value['type'],
>> > +                               QAPISchemaIfCond(value.get('if')),
>> > +                               info)
>> > +            for (key, value) in data.items()
>> > +        ]
>> >          tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
>> >          self._def_entity(
>> >              QAPISchemaAlternateType(name, info, doc, ifcond, features,
>> > @@ -1118,7 +1144,7 @@ def _def_command(self, expr, info, doc):
>> >          allow_oob = expr.get('allow-oob', False)
>> >          allow_preconfig = expr.get('allow-preconfig', False)
>> >          coroutine = expr.get('coroutine', False)
>> > -        ifcond = expr.get('if')
>> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
>> >          features = self._make_features(expr.get('features'), info)
>> >          if isinstance(data, OrderedDict):
>> >              data = self._make_implicit_object_type(
>> > @@ -1137,7 +1163,7 @@ def _def_event(self, expr, info, doc):
>> >          name = expr['event']
>> >          data = expr.get('data')
>> >          boxed = expr.get('boxed', False)
>> > -        ifcond = expr.get('if')
>> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
>> >          features = self._make_features(expr.get('features'), info)
>> >          if isinstance(data, OrderedDict):
>> >              data = self._make_implicit_object_type(
>> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
>> > index 20d572a23a..3673cf0f49 100644
>> > --- a/scripts/qapi/types.py
>> > +++ b/scripts/qapi/types.py
>> > @@ -13,7 +13,7 @@
>> >  # See the COPYING file in the top-level directory.
>> >  """
>> >
>> > -from typing import List, Optional, Sequence
>> > +from typing import List, Optional
>> >
>> >  from .common import (
>> >      c_enum_const,
>> > @@ -27,6 +27,7 @@
>> >      QAPISchema,
>> >      QAPISchemaEnumMember,
>> >      QAPISchemaFeature,
>> > +    QAPISchemaIfCond,
>> >      QAPISchemaObjectType,
>> >      QAPISchemaObjectTypeMember,
>> >      QAPISchemaType,
>> > @@ -50,13 +51,13 @@ def gen_enum_lookup(name: str,
>> >  ''',
>> >                  c_name=c_name(name))
>> >      for memb in members:
>> > -        ret += gen_if(memb.ifcond)
>> > +        ret += gen_if(memb.ifcond.ifcond)
>> >          index = c_enum_const(name, memb.name, prefix)
>> >          ret += mcgen('''
>> >          [%(index)s] = "%(name)s",
>> >  ''',
>> >                       index=index, name=memb.name)
>> > -        ret += gen_endif(memb.ifcond)
>> > +        ret += gen_endif(memb.ifcond.ifcond)
>> >
>> >      ret += mcgen('''
>> >      },
>> > @@ -80,12 +81,12 @@ def gen_enum(name: str,
>> >                  c_name=c_name(name))
>> >
>> >      for memb in enum_members:
>> > -        ret += gen_if(memb.ifcond)
>> > +        ret += gen_if(memb.ifcond.ifcond)
>> >          ret += mcgen('''
>> >      %(c_enum)s,
>> >  ''',
>> >                       c_enum=c_enum_const(name, memb.name, prefix))
>> > -        ret += gen_endif(memb.ifcond)
>> > +        ret += gen_endif(memb.ifcond.ifcond)
>> >
>> >      ret += mcgen('''
>> >  } %(c_name)s;
>> > @@ -125,7 +126,7 @@ def gen_array(name: str, element_type: QAPISchemaType) -> str:
>> >  def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str:
>> >      ret = ''
>> >      for memb in members:
>> > -        ret += gen_if(memb.ifcond)
>> > +        ret += gen_if(memb.ifcond.ifcond)
>> >          if memb.optional:
>> >              ret += mcgen('''
>> >      bool has_%(c_name)s;
>> > @@ -135,11 +136,11 @@ def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str:
>> >      %(c_type)s %(c_name)s;
>> >  ''',
>> >                       c_type=memb.type.c_type(), c_name=c_name(memb.name))
>> > -        ret += gen_endif(memb.ifcond)
>> > +        ret += gen_endif(memb.ifcond.ifcond)
>> >      return ret
>> >
>> >
>> > -def gen_object(name: str, ifcond: Sequence[str],
>> > +def gen_object(name: str, ifcond: QAPISchemaIfCond,
>> >                 base: Optional[QAPISchemaObjectType],
>> >                 members: List[QAPISchemaObjectTypeMember],
>> >                 variants: Optional[QAPISchemaVariants]) -> str:
>> > @@ -158,7 +159,7 @@ def gen_object(name: str, ifcond: Sequence[str],
>> >      ret += mcgen('''
>> >
>> >  ''')
>> > -    ret += gen_if(ifcond)
>> > +    ret += gen_if(ifcond.ifcond)
>> >      ret += mcgen('''
>> >  struct %(c_name)s {
>> >  ''',
>> > @@ -192,7 +193,7 @@ def gen_object(name: str, ifcond: Sequence[str],
>> >      ret += mcgen('''
>> >  };
>> >  ''')
>> > -    ret += gen_endif(ifcond)
>> > +    ret += gen_endif(ifcond.ifcond)
>> >
>> >      return ret
>> >
>> > @@ -219,13 +220,13 @@ def gen_variants(variants: QAPISchemaVariants) -> str:
>> >      for var in variants.variants:
>> >          if var.type.name == 'q_empty':
>> >              continue
>> > -        ret += gen_if(var.ifcond)
>> > +        ret += gen_if(var.ifcond.ifcond)
>> >          ret += mcgen('''
>> >          %(c_type)s %(c_name)s;
>> >  ''',
>> >                       c_type=var.type.c_unboxed_type(),
>> >                       c_name=c_name(var.name))
>> > -        ret += gen_endif(var.ifcond)
>> > +        ret += gen_endif(var.ifcond.ifcond)
>> >
>> >      ret += mcgen('''
>> >      } u;
>> > @@ -307,7 +308,7 @@ def _gen_type_cleanup(self, name: str) -> None:
>> >      def visit_enum_type(self,
>> >                          name: str,
>> >                          info: Optional[QAPISourceInfo],
>> > -                        ifcond: Sequence[str],
>> > +                        ifcond: QAPISchemaIfCond,
>> >                          features: List[QAPISchemaFeature],
>> >                          members: List[QAPISchemaEnumMember],
>> >                          prefix: Optional[str]) -> None:
>> > @@ -318,7 +319,7 @@ def visit_enum_type(self,
>> >      def visit_array_type(self,
>> >                           name: str,
>> >                           info: Optional[QAPISourceInfo],
>> > -                         ifcond: Sequence[str],
>> > +                         ifcond: QAPISchemaIfCond,
>> >                           element_type: QAPISchemaType) -> None:
>> >          with ifcontext(ifcond, self._genh, self._genc):
>> >              self._genh.preamble_add(gen_fwd_object_or_array(name))
>> > @@ -328,7 +329,7 @@ def visit_array_type(self,
>> >      def visit_object_type(self,
>> >                            name: str,
>> >                            info: Optional[QAPISourceInfo],
>> > -                          ifcond: Sequence[str],
>> > +                          ifcond: QAPISchemaIfCond,
>> >                            features: List[QAPISchemaFeature],
>> >                            base: Optional[QAPISchemaObjectType],
>> >                            members: List[QAPISchemaObjectTypeMember],
>> > @@ -351,7 +352,7 @@ def visit_object_type(self,
>> >      def visit_alternate_type(self,
>> >                               name: str,
>> >                               info: Optional[QAPISourceInfo],
>> > -                             ifcond: Sequence[str],
>> > +                             ifcond: QAPISchemaIfCond,
>> >                               features: List[QAPISchemaFeature],
>> >                               variants: QAPISchemaVariants) -> None:
>> >          with ifcontext(ifcond, self._genh):
>> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>> > index 9e96f3c566..67721b2470 100644
>> > --- a/scripts/qapi/visit.py
>> > +++ b/scripts/qapi/visit.py
>> > @@ -13,7 +13,7 @@
>> >  See the COPYING file in the top-level directory.
>> >  """
>> >
>> > -from typing import List, Optional, Sequence
>> > +from typing import List, Optional
>> >
>> >  from .common import (
>> >      c_enum_const,
>> > @@ -29,6 +29,7 @@
>> >      QAPISchemaEnumMember,
>> >      QAPISchemaEnumType,
>> >      QAPISchemaFeature,
>> > +    QAPISchemaIfCond,
>> >      QAPISchemaObjectType,
>> >      QAPISchemaObjectTypeMember,
>> >      QAPISchemaType,
>> > @@ -78,7 +79,7 @@ def gen_visit_object_members(name: str,
>> >
>> >      for memb in members:
>> >          deprecated = 'deprecated' in [f.name for f in memb.features]
>> > -        ret += gen_if(memb.ifcond)
>> > +        ret += gen_if(memb.ifcond.ifcond)
>> >          if memb.optional:
>> >              ret += mcgen('''
>> >      if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
>> > @@ -111,7 +112,7 @@ def gen_visit_object_members(name: str,
>> >              ret += mcgen('''
>> >      }
>> >  ''')
>> > -        ret += gen_endif(memb.ifcond)
>> > +        ret += gen_endif(memb.ifcond.ifcond)
>> >
>> >      if variants:
>> >          tag_member = variants.tag_member
>> > @@ -125,7 +126,7 @@ def gen_visit_object_members(name: str,
>> >          for var in variants.variants:
>> >              case_str = c_enum_const(tag_member.type.name, var.name,
>> >                                      tag_member.type.prefix)
>> > -            ret += gen_if(var.ifcond)
>> > +            ret += gen_if(var.ifcond.ifcond)
>> >              if var.type.name == 'q_empty':
>> >                  # valid variant and nothing to do
>> >                  ret += mcgen('''
>> > @@ -141,7 +142,7 @@ def gen_visit_object_members(name: str,
>> >                               case=case_str,
>> >                               c_type=var.type.c_name(), c_name=c_name(var.name))
>> >
>> > -            ret += gen_endif(var.ifcond)
>> > +            ret += gen_endif(var.ifcond.ifcond)
>> >          ret += mcgen('''
>> >      default:
>> >          abort();
>> > @@ -227,7 +228,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
>> >                  c_name=c_name(name))
>> >
>> >      for var in variants.variants:
>> > -        ret += gen_if(var.ifcond)
>> > +        ret += gen_if(var.ifcond.ifcond)
>> >          ret += mcgen('''
>> >      case %(case)s:
>> >  ''',
>> > @@ -253,7 +254,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
>> >          ret += mcgen('''
>> >          break;
>> >  ''')
>> > -        ret += gen_endif(var.ifcond)
>> > +        ret += gen_endif(var.ifcond.ifcond)
>> >
>> >      ret += mcgen('''
>> >      case QTYPE_NONE:
>> > @@ -352,7 +353,7 @@ def _begin_user_module(self, name: str) -> None:
>> >      def visit_enum_type(self,
>> >                          name: str,
>> >                          info: Optional[QAPISourceInfo],
>> > -                        ifcond: Sequence[str],
>> > +                        ifcond: QAPISchemaIfCond,
>> >                          features: List[QAPISchemaFeature],
>> >                          members: List[QAPISchemaEnumMember],
>> >                          prefix: Optional[str]) -> None:
>> > @@ -363,7 +364,7 @@ def visit_enum_type(self,
>> >      def visit_array_type(self,
>> >                           name: str,
>> >                           info: Optional[QAPISourceInfo],
>> > -                         ifcond: Sequence[str],
>> > +                         ifcond: QAPISchemaIfCond,
>> >                           element_type: QAPISchemaType) -> None:
>> >          with ifcontext(ifcond, self._genh, self._genc):
>> >              self._genh.add(gen_visit_decl(name))
>> > @@ -372,7 +373,7 @@ def visit_array_type(self,
>> >      def visit_object_type(self,
>> >                            name: str,
>> >                            info: Optional[QAPISourceInfo],
>> > -                          ifcond: Sequence[str],
>> > +                          ifcond: QAPISchemaIfCond,
>> >                            features: List[QAPISchemaFeature],
>> >                            base: Optional[QAPISchemaObjectType],
>> >                            members: List[QAPISchemaObjectTypeMember],
>> > @@ -394,7 +395,7 @@ def visit_object_type(self,
>> >      def visit_alternate_type(self,
>> >                               name: str,
>> >                               info: Optional[QAPISourceInfo],
>> > -                             ifcond: Sequence[str],
>> > +                             ifcond: QAPISchemaIfCond,
>> >                               features: List[QAPISchemaFeature],
>> >                               variants: QAPISchemaVariants) -> None:
>> >          with ifcontext(ifcond, self._genh, self._genc):
>> > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
>> > index f1c4deb9a5..2ec328b22e 100755
>> > --- a/tests/qapi-schema/test-qapi.py
>> > +++ b/tests/qapi-schema/test-qapi.py
>> > @@ -95,7 +95,7 @@ def _print_variants(variants):
>> >      @staticmethod
>> >      def _print_if(ifcond, indent=4):
>> >          if ifcond:
>> > -            print('%sif %s' % (' ' * indent, ifcond))
>> > +            print('%sif %s' % (' ' * indent, ifcond.ifcond))
>> >
>> >      @classmethod
>> >      def _print_features(cls, features, indent=4):
>>
>> If feel this is a bit harder to review than necessary, because you take
>> two steps at once:
>>
>> 1. Wrap Sequence[str] in an object
>>
>> 2. Add methods to the object to clean up the resulting mess some
>>
>> Step 1. by itself should be pretty much mechanical: adjust the type
>> hints, create the wrapper object on write, peel it off on read.  The
>> result will be slightly ugly in places.
>>
>> I'd expect step 2 to be much smaller, and easier to understand.  It
>> could perhaps be split into one patch per method, but I hope it's
>> reviewable just fine even without.

Hope you didn't miss this remark.