Re: [PATCH v5 7/9] qapi: normalize 'if' condition to IfPredicate tree

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 18572 bytes --]

Hi

On Tue, Jun 15, 2021 at 3:34 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Modify check_if() to normalize the condition tree.
>
> How is it normalized?  Let me rephrase my question: how does the IR
> change?  If the generated code changes, how?
>

Not anymore, since we dropped the sugar form. Updated in v6.


> > Add _make_if() to build a QAPISchemaIfCond tree.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Tested-by: John Snow <jsnow@redhat.com>
> > ---
> >  tests/unit/test-qmp-cmds.c                    |  1 +
> >  scripts/qapi/expr.py                          | 51 +++++++++------
> >  scripts/qapi/schema.py                        | 62 +++++++++++++------
> >  tests/qapi-schema/bad-if-empty-list.json      |  2 +-
> >  tests/qapi-schema/bad-if-list.json            |  2 +-
> >  tests/qapi-schema/bad-if.err                  |  3 +-
> >  tests/qapi-schema/doc-good.out                | 12 ++--
> >  tests/qapi-schema/enum-if-invalid.err         |  3 +-
> >  tests/qapi-schema/features-if-invalid.err     |  2 +-
> >  tests/qapi-schema/qapi-schema-test.json       | 32 ++++++----
> >  tests/qapi-schema/qapi-schema-test.out        | 59 ++++++++++--------
> >  .../qapi-schema/struct-member-if-invalid.err  |  2 +-
> >  .../qapi-schema/union-branch-if-invalid.json  |  2 +-
> >  13 files changed, 143 insertions(+), 90 deletions(-)
> >
> > diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
> > index 1b0b7d99df..83efa39720 100644
> > --- a/tests/unit/test-qmp-cmds.c
> > +++ b/tests/unit/test-qmp-cmds.c
> > @@ -51,6 +51,7 @@ FeatureStruct1 *qmp_test_features0(bool has_fs0,
> FeatureStruct0 *fs0,
> >                                     bool has_cfs1, CondFeatureStruct1
> *cfs1,
> >                                     bool has_cfs2, CondFeatureStruct2
> *cfs2,
> >                                     bool has_cfs3, CondFeatureStruct3
> *cfs3,
> > +                                   bool has_cfs4, CondFeatureStruct4
> *cfs4,
> >                                     Error **errp)
> >  {
> >      return g_new0(FeatureStruct1, 1);
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index 496f7e0333..60ffe9ef03 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -261,12 +261,12 @@ def check_if(expr: _JSONObject, info:
> QAPISourceInfo, source: str) -> None:
> >      """
> >      Normalize and validate the ``if`` member of an object.
> >
> > -    The ``if`` member may be either a ``str`` or a ``List[str]``.
> > -    A ``str`` value will be normalized to ``List[str]``.
>
> Double-checking: is this doc comment correct before this patch?
>

I think it was


> > +    The ``if`` field may be either a ``str`` or a dict.
> > +    A ``str`` element will be normalized to ``{'all': List[str]}``.
> >
> >      :forms:
> > -      :sugared: ``Union[str, List[str]]``
> > -      :canonical: ``List[str]``
> > +      :sugared: ``Union[str, dict]``
> > +      :canonical: ``Union[str, dict]``
> >
> >      :param expr: The expression containing the ``if`` member to
> validate.
> >      :param info: QAPI schema source file information.
> > @@ -281,25 +281,38 @@ def check_if(expr: _JSONObject, info:
> QAPISourceInfo, source: str) -> None:
> >      if ifcond is None:
> >          return
> >
> > -    if isinstance(ifcond, list):
> > -        if not ifcond:
> > -            raise QAPISemError(
> > -                info, "'if' condition [] of %s is useless" % source)
> > -    else:
> > -        # Normalize to a list
> > -        ifcond = expr['if'] = [ifcond]
> > -
> > -    for elt in ifcond:
> > -        if not isinstance(elt, str):
> > +    def normalize(cond: Union[str, object]) -> Union[str, object]:
>
> This definition is in the middle of check_if()'s body.  Intentional?
>

why not


> > +        if isinstance(cond, str):
> > +            if not cond.strip():
> > +                raise QAPISemError(
> > +                    info,
> > +                    "'if' condition '%s' of %s makes no sense"
> > +                    % (cond, source))
> > +            return cond
> > +        if not isinstance(cond, dict):
> >              raise QAPISemError(
> >                  info,
> > -                "'if' condition of %s must be a string or a list of
> strings"
> > -                % source)
> > -        if not elt.strip():
> > +                "'if' condition of %s must be a string or a dict" %
> source)
> > +        if len(cond) != 1:
> >              raise QAPISemError(
> >                  info,
> > -                "'if' condition '%s' of %s makes no sense"
> > -                % (elt, source))
> > +                "'if' condition dict of %s must have one key: "
>
> Exactly one key, to be precise.
>
> > +                "'all', 'any' or 'not'" % source)
> > +        check_keys(cond, info, "'if' condition", [],
> > +                   ["all", "any", "not"])
>
> Hmmm.  Getting members of @cond before check_keys() would be wrong, but
> you don't do that.  Still, I like to call check_keys() early, just to
> reduce the chance for accidents.
>
> If we check_keys() first, we're left with just two possible errors:
> empty dict (len(cond)==0), and conflicting keys (len(cond)>1).  We could
> choose to diagnose them separately, but it's probably not worth the
> bother.
>
> > +        oper, operands = next(iter(cond.items()))
> > +        if not operands:
> > +            raise QAPISemError(
> > +                info, "'if' condition [] of %s is useless" % source)
> > +        if oper == "not":
> > +            return {oper: normalize(operands)}
> > +        if oper in ("all", "any") and not isinstance(operands, list):
> > +            raise QAPISemError(
> > +                info, "'%s' condition of %s must be a list" % (oper,
> source))
> > +        operands = [normalize(o) for o in operands]
> > +        return {oper: operands}
>
> I guess making this a function enables writing
>
>                return NE
>
> instead of
>
>                expr['if] = NE
>                return
>
> which is slightly more compact, and factors out the assignment's left
> hand side.  Feels like a wash, but up to you.
>
>
gone in v6

> +
> > +    expr['if'] = normalize(ifcond)
> >
> >
> >  def normalize_members(members: object) -> None:
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index f52caaeecc..9864e49c54 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -22,6 +22,8 @@
> >  from .common import (
> >      POINTER_SUFFIX,
> >      IfAll,
> > +    IfAny,
> > +    IfNot,
> >      IfOption,
> >      c_name,
> >  )
> > @@ -31,15 +33,14 @@
> >
> >
> >  class QAPISchemaIfCond:
> > -    def __init__(self, ifcond=None):
> > -        self.ifcond = ifcond or []
> > -        self.pred = IfAll([IfOption(opt) for opt in self.ifcond])
> > +    def __init__(self, pred=None):
> > +        self.pred = pred
> >
> >      def docgen(self):
> > -        return self.pred.docgen()
> > +        return self and self.pred.docgen()
>
> The more code relying on your __bool__() methods I see, the less I like
> them.
>
> Can we do self.pred and self.pred.docgen()?
>
> >
> >      def cgen(self):
> > -        return self.pred.cgen()
> > +        return self and self.pred.cgen()
>
> Likewise.
>
> >
> >      # Returns true if the condition is not void
> >      def __bool__(self):
> > @@ -991,16 +992,41 @@ def _def_predefineds(self):
> >          self._def_entity(QAPISchemaEnumType('QType', None, None, None,
> None,
> >                                              qtype_values, 'QTYPE'))
> >
> > +    def _make_if(self, cond):
> > +        if isinstance(cond, QAPISchemaIfCond):
> > +            return cond
> > +        if cond is None:
> > +            return QAPISchemaIfCond()
> > +
> > +        def make_pred(node):
> > +            if isinstance(node, str):
> > +                return IfOption(node)
> > +            oper, operands = next(iter(node.items()))
> > +            op2cls = {
> > +                'all': IfAll,
> > +                'any': IfAny,
> > +                'not': IfNot,
> > +            }
> > +
> > +            if isinstance(operands, list):
> > +                operands = [make_pred(o) for o in operands]
> > +            else:
> > +                operands = make_pred(operands)
> > +
> > +            return op2cls[oper](operands)
> > +
> > +        return QAPISchemaIfCond(make_pred(cond))
>
> Maybe it's the weather, but I find this pretty impenetrable right now.
>
> make_if() appears to accept either QAPISchemaIfCond, None, or a tree
> whose inner nodes are {'any': List[tree]}, {'all': List[tree]}, {'not':
> tree}, or str.  Quite the omnivore.
>
> None of the callers I can see pass QAPISchemaIfCond.  Am I confused?
>
> make_pred() appears to accept the tree.  The part dealing with str is
> obvious.
>
> next(iter(node.items())) appears to map a dict {key: val} to a tuple
> (key, val).  Too clever by half.
>
> val, and thus @operands then is either a list of trees (all, any), or a
> tree (not).
>
> The tree(s) in @operands get recursively processed.  Now @operands is
> either a List[IfPredicate], or an IfPredicate.
>
> IfAll and IfAny take the former, IfNot takes the latter.  Works out
> (*quack*), but I'm not sure mypy will be happy with it.
>

I removed the IfCond AST altogether in v6.


> > +
> >      def _make_features(self, features, info):
> >          if features is None:
> >              return []
> >          return [QAPISchemaFeature(f['name'], info,
> > -                                  QAPISchemaIfCond(f.get('if')))
> > +                                  self._make_if(f.get('if')))
> >                  for f in features]
> >
> >      def _make_enum_members(self, values, info):
> >          return [QAPISchemaEnumMember(v['name'], info,
> > -                                     QAPISchemaIfCond(v.get('if')))
> > +                                     self._make_if(v.get('if')))
> >                  for v in values]
> >
> >      def _make_implicit_enum_type(self, name, info, ifcond, values):
> > @@ -1046,7 +1072,7 @@ def _def_enum_type(self, expr, info, doc):
> >          name = expr['enum']
> >          data = expr['data']
> >          prefix = expr.get('prefix')
> > -        ifcond = QAPISchemaIfCond(expr.get('if'))
> > +        ifcond = self._make_if(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          self._def_entity(QAPISchemaEnumType(
> >              name, info, doc, ifcond, features,
> > @@ -1065,7 +1091,7 @@ def _make_member(self, name, typ, ifcond,
> features, info):
> >
> >      def _make_members(self, data, info):
> >          return [self._make_member(key, value['type'],
> > -                                  QAPISchemaIfCond(value.get('if')),
> > +                                  self._make_if(value.get('if')),
> >                                    value.get('features'), info)
> >                  for (key, value) in data.items()]
> >
> > @@ -1073,7 +1099,7 @@ def _def_struct_type(self, expr, info, doc):
> >          name = expr['struct']
> >          base = expr.get('base')
> >          data = expr['data']
> > -        ifcond = QAPISchemaIfCond(expr.get('if'))
> > +        ifcond = self._make_if(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          self._def_entity(QAPISchemaObjectType(
> >              name, info, doc, ifcond, features, base,
> > @@ -1096,7 +1122,7 @@ def _def_union_type(self, expr, info, doc):
> >          name = expr['union']
> >          data = expr['data']
> >          base = expr.get('base')
> > -        ifcond = QAPISchemaIfCond(expr.get('if'))
> > +        ifcond = self._make_if(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          tag_name = expr.get('discriminator')
> >          tag_member = None
> > @@ -1107,7 +1133,7 @@ def _def_union_type(self, expr, info, doc):
> >          if tag_name:
> >              variants = [
> >                  self._make_variant(key, value['type'],
> > -                                   QAPISchemaIfCond(value.get('if')),
> > +                                   self._make_if(value.get('if')),
> >                                     info)
> >                  for (key, value) in data.items()
> >              ]
> > @@ -1115,11 +1141,11 @@ def _def_union_type(self, expr, info, doc):
> >          else:
> >              variants = [
> >                  self._make_simple_variant(key, value['type'],
> > -
> QAPISchemaIfCond(value.get('if')),
> > +
> self._make_if(value.get('if')),
> >                                            info)
> >                  for (key, value) in data.items()
> >              ]
> > -            enum = [{'name': v.name, 'if': v.ifcond.ifcond} for v in
> variants]
> > +            enum = [{'name': v.name, 'if': v.ifcond} for v in variants]
>
> Another riddle for me to solve?
>

See [PATCH v6 04/11] qapi: _make_enum_members() to work with pre-built
QAPISchemaIfCond


> >              typ = self._make_implicit_enum_type(name, info, ifcond,
> enum)
> >              tag_member = QAPISchemaObjectTypeMember('type', info, typ,
> False)
> >              members = [tag_member]
> > @@ -1132,11 +1158,11 @@ def _def_union_type(self, expr, info, doc):
> >      def _def_alternate_type(self, expr, info, doc):
> >          name = expr['alternate']
> >          data = expr['data']
> > -        ifcond = QAPISchemaIfCond(expr.get('if'))
> > +        ifcond = self._make_if(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          variants = [
> >              self._make_variant(key, value['type'],
> > -                               QAPISchemaIfCond(value.get('if')),
> > +                               self._make_if(value.get('if')),
> >                                 info)
> >              for (key, value) in data.items()
> >          ]
> > @@ -1156,7 +1182,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 = QAPISchemaIfCond(expr.get('if'))
> > +        ifcond = self._make_if(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          if isinstance(data, OrderedDict):
> >              data = self._make_implicit_object_type(
> > @@ -1175,7 +1201,7 @@ def _def_event(self, expr, info, doc):
> >          name = expr['event']
> >          data = expr.get('data')
> >          boxed = expr.get('boxed', False)
> > -        ifcond = QAPISchemaIfCond(expr.get('if'))
> > +        ifcond = self._make_if(expr.get('if'))
> >          features = self._make_features(expr.get('features'), info)
> >          if isinstance(data, OrderedDict):
> >              data = self._make_implicit_object_type(
> > diff --git a/tests/qapi-schema/bad-if-empty-list.json
> b/tests/qapi-schema/bad-if-empty-list.json
> > index 94f2eb8670..b62b5671df 100644
> > --- a/tests/qapi-schema/bad-if-empty-list.json
> > +++ b/tests/qapi-schema/bad-if-empty-list.json
> > @@ -1,3 +1,3 @@
> >  # check empty 'if' list
> >  { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> > -  'if': [] }
> > +  'if': { 'all': [] } }
> > diff --git a/tests/qapi-schema/bad-if-list.json
> b/tests/qapi-schema/bad-if-list.json
> > index ea3d95bb6b..1fefef16a7 100644
> > --- a/tests/qapi-schema/bad-if-list.json
> > +++ b/tests/qapi-schema/bad-if-list.json
> > @@ -1,3 +1,3 @@
> >  # check invalid 'if' content
> >  { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> > -  'if': ['foo', ' '] }
> > +  'if': { 'all': ['foo', ' '] } }
> > diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.err
> > index f83dee65da..454fbae387 100644
> > --- a/tests/qapi-schema/bad-if.err
> > +++ b/tests/qapi-schema/bad-if.err
> > @@ -1,2 +1,3 @@
> >  bad-if.json: In struct 'TestIfStruct':
> > -bad-if.json:2: 'if' condition of struct must be a string or a list of
> strings
> > +bad-if.json:2: 'if' condition has unknown key 'value'
> > +Valid keys are 'all', 'any', 'not'.
> > diff --git a/tests/qapi-schema/doc-good.out
> b/tests/qapi-schema/doc-good.out
> > index db1d23c6bf..4d951f97ee 100644
> > --- a/tests/qapi-schema/doc-good.out
> > +++ b/tests/qapi-schema/doc-good.out
> > @@ -12,15 +12,15 @@ enum QType
> >  module doc-good.json
> >  enum Enum
> >      member one
> > -        if IfAll([IfOption('defined(IFONE)')])
> > +        if IfOption('defined(IFONE)')
> >      member two
> > -    if IfAll([IfOption('defined(IFCOND)')])
> > +    if IfOption('defined(IFCOND)')
> >      feature enum-feat
> >  object Base
> >      member base1: Enum optional=False
> >  object Variant1
> >      member var1: str optional=False
> > -        if IfAll([IfOption('defined(IFSTR)')])
> > +        if IfOption('defined(IFSTR)')
> >          feature member-feat
> >      feature variant1-feat
> >  object Variant2
> > @@ -29,7 +29,7 @@ object Object
> >      tag base1
> >      case one: Variant1
> >      case two: Variant2
> > -        if IfAll([IfOption('IFTWO')])
> > +        if IfOption('IFTWO')
> >      feature union-feat1
> >  object q_obj_Variant1-wrapper
> >      member data: Variant1 optional=False
> > @@ -38,13 +38,13 @@ object q_obj_Variant2-wrapper
> >  enum SugaredUnionKind
> >      member one
> >      member two
> > -        if IfAll([IfOption('IFTWO')])
> > +        if IfOption('IFTWO')
> >  object SugaredUnion
> >      member type: SugaredUnionKind optional=False
> >      tag type
> >      case one: q_obj_Variant1-wrapper
> >      case two: q_obj_Variant2-wrapper
> > -        if IfAll([IfOption('IFTWO')])
> > +        if IfOption('IFTWO')
> >      feature union-feat2
> >  alternate Alternate
> >      tag type
> [...]
>
> Skipping the tests for now because I'm running out of brain-juice.
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 25251 bytes --]