Re: [PATCH v6 07/11] qapi: replace if condition list with dict {'all': [...]}

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

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

Hi

On Tue, Aug 3, 2021 at 5:05 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Replace the simple list sugar form with a recursive structure that will
> > accept other operators in the following commits (all, any or not).
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  scripts/qapi/common.py                        | 23 +++++--
> >  scripts/qapi/expr.py                          | 52 ++++++++++------
> >  scripts/qapi/schema.py                        |  2 +-
> >  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.json               |  3 +-
> >  tests/qapi-schema/doc-good.out                | 13 ++--
> >  tests/qapi-schema/doc-good.txt                |  6 ++
> >  tests/qapi-schema/enum-if-invalid.err         |  3 +-
> >  tests/qapi-schema/features-if-invalid.err     |  2 +-
> >  tests/qapi-schema/qapi-schema-test.json       | 25 ++++----
> >  tests/qapi-schema/qapi-schema-test.out        | 62 +++++++++----------
> >  .../qapi-schema/struct-member-if-invalid.err  |  2 +-
> >  .../qapi-schema/union-branch-if-invalid.json  |  2 +-
> >  15 files changed, 119 insertions(+), 83 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 5181a0f167..51463510c9 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -13,7 +13,8 @@
> >
> >  import re
> >  from typing import (
> > -    List,
> > +    Any,
> > +    Dict,
> >      Match,
> >      Optional,
> >      Union,
> > @@ -199,16 +200,28 @@ def guardend(name: str) -> str:
> >                   name=c_fname(name).upper())
> >
> >
> > -def cgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> > +def docgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
>
> Uh, why do you swap cgen_ifcond() and docgen_ifcond()?  Accident?
>

Yes, fixed


> >      if not ifcond:
> >          return ''
> > -    return '(' + ') && ('.join(ifcond) + ')'
> > +    if isinstance(ifcond, str):
> > +        return ifcond
> >
> > +    oper, operands = next(iter(ifcond.items()))
> > +    oper = {'all': ' and '}[oper]
> > +    operands = [docgen_ifcond(o) for o in operands]
> > +    return '(' + oper.join(operands) + ')'
>
> What a nice review speedbump you buried here...
>
> The whole block boils down to the much less exciting
>
>        operands = [docgen_ifcond(o) for o in ifcond['all']]
>        return '(' + ' and '.join(operands) + ')'
>
> Peeking ahead, I understand that you did it this way here so you can
> extend it trivially there.  Matter of taste; what counts is the final
> result and minimizing reviewer WTFs/minute along the way.
>
> Since the WTFs/minute is a done deed now, what remains is the final
> result, which I expect to review shortly.  But please try a bit harder
> to be boring next time ;)
>

Ah sorry, I didn't realize while splitting the patches


> >
> > -def docgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> > +
> > +def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
> >      if not ifcond:
> >          return ''
> > -    return ' and '.join(ifcond)
> > +    if isinstance(ifcond, str):
> > +        return ifcond
>
> This is what gets rid of the redundant parenthesises in the common case
> "single condition string".
>
> > +
> > +    oper, operands = next(iter(ifcond.items()))
> > +    oper = {'all': '&&'}[oper]
> > +    operands = [cgen_ifcond(o) for o in operands]
> > +    return '(' + (') ' + oper + ' (').join(operands) + ')'
>
> This line is hard to read.  Easier, I think:
>
>        oper = {'all': ' && '}[oper]
>        operands = ['(' + cgen_ifcond(o) + ')' for o in operands]
>        return oper.join(operands)
>
> Neither your version nor mine gets rid of the redundant parenthesises in
> the (uncommon) case "complex condition expression".
> tests/test-qapi-introspect.c still has
>
>     #if (defined(TEST_IF_COND_1)) && (defined(TEST_IF_COND_2))
>                 QLIT_QSTR("feature1"),
>     #endif /* (defined(TEST_IF_COND_1)) && (defined(TEST_IF_COND_2)) */
>
> Mildly annoying.  I'm willing to leave this for later.
>

I see


> Code smell: cgen_ifcond() and docgen_ifcond() are almost identical.  Can
> also be left for later.
>

ok


> >
> >
> >  def gen_if(cond: str) -> str:
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index 496f7e0333..3ee66c5f62 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -259,14 +259,12 @@ def check_flags(expr: _JSONObject, info:
> QAPISourceInfo) -> None:
> >
> >  def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) ->
> None:
> >      """
> > -    Normalize and validate the ``if`` member of an object.
> > +    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]``.
> > +    The ``if`` member may be either a ``str`` or a dict.
> >
> >      :forms:
> > -      :sugared: ``Union[str, List[str]]``
> > -      :canonical: ``List[str]``
> > +      :canonical: ``Union[str, dict]``
>
> Does this :forms: thing make sense without any :sugared:?  John, you
> added (invented?) it in commit a48653638fa, but no explanation made it
> into the tree.
>
> >
> >      :param expr: The expression containing the ``if`` member to
> validate.
> >      :param info: QAPI schema source file information.
> > @@ -275,31 +273,45 @@ def check_if(expr: _JSONObject, info:
> QAPISourceInfo, source: str) -> None:
> >      :raise QAPISemError:
> >          When the "if" member fails validation, or when there are no
> >          non-empty conditions.
> > -    :return: None, ``expr`` is normalized in-place as needed.
> > +    :return: None
> >      """
>
> Looks like there's a bit more going on than the commit message made me
> expect.
>
> >      ifcond = expr.get('if')
> >      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]
> > +    def _check_if(cond: Union[str, object]) -> None:
> > +        if isinstance(cond, str):
> > +            if not cond.strip():
> > +                raise QAPISemError(
> > +                    info,
> > +                    "'if' condition '%s' of %s makes no sense"
> > +                    % (cond, source))
> > +            return
> >
> > -    for elt in ifcond:
> > -        if not isinstance(elt, str):
> > +        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: "
> > +                "'all'" % source)
> > +        check_keys(cond, info, "'if' condition", [],
> > +                   ["all"])
> > +
> > +        oper, operands = next(iter(cond.items()))
> > +        if not operands:
> > +            raise QAPISemError(
> > +                info, "'if' condition [] of %s is useless" % source)
> > +
> > +        if oper in ("all") and not isinstance(operands, list):
> > +            raise QAPISemError(
> > +                info, "'%s' condition of %s must be a list" % (oper,
> source))
> > +        for operand in operands:
> > +            _check_if(operand)
> > +
> > +    _check_if(ifcond)
>
> Putting the function's helper in the middle of the function reminds me
> of Mark Twain's "The Awful German Language":
>
>     "The trunks being now ready, he DE- after kissing his mother and
>     sisters, and once more pressing to his bosom his adored Gretchen,
>     who, dressed in simple white muslin, with a single tuberose in the
>     ample folds of her rich brown hair, had tottered feebly down the
>     stairs, still pale from the terror and excitement of the past
>     evening, but longing to lay her poor aching head yet once again upon
>     the breast of him whom she loved more dearly than life itself,
>     PARTED."
>
> I find it hard to read.
>
>
Matter of taste, I guess. I'll let you fix it up to your preference as
follow up if you don't mind.


> >
> >
> >  def normalize_members(members: object) -> None:
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 30d6a01ad1..d2fbdbe583 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -32,7 +32,7 @@
> >
> >  class QAPISchemaIfCond:
> >      def __init__(self, ifcond=None):
> > -        self.ifcond = ifcond or []
> > +        self.ifcond = ifcond or {}
> >
> >      def cgen(self):
> >          return cgen_ifcond(self.ifcond)
> > 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..8278c49368 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'.
>
> "keys are" is awkward when there's just one.  Okay since there soon will
> be more.
>
>
right

Test case bad-if.json is meant to cover "value of key 'if' has an
> invalid JSON type".  Before the patch, str and list are valid, and the
> test uses (invalid) dict.  Afterwards, str and dict are, and the test
> still uses (now valid) dict.  In other words, it now tests something
> else entirely.
>
> I think this test should be updated to something like
>
>     'if': [ 'defined(TEST_IF_STRUCT)' ]
>
> and a new test added to cover invalid dict key.
>

ok, and I added some tests to cover new error paths.


> > diff --git a/tests/qapi-schema/doc-good.json
> b/tests/qapi-schema/doc-good.json
> > index 423ea23e07..25b1053e8a 100644
> > --- a/tests/qapi-schema/doc-good.json
> > +++ b/tests/qapi-schema/doc-good.json
> > @@ -70,7 +70,8 @@
> >  # @base1:
> >  # the first member
> >  ##
> > -{ 'struct': 'Base', 'data': { 'base1': 'Enum' } }
> > +{ 'struct': 'Base', 'data': { 'base1': 'Enum' },
> > +  'if': { 'all': ['IFALL1', 'IFALL2'] } }
>
> We lack cover for this before your patch.  Thanks for fixing it.
>
> >
> >  ##
> >  # @Variant1:
> > diff --git a/tests/qapi-schema/doc-good.out
> b/tests/qapi-schema/doc-good.out
> > index 8f54ceff2e..689d084f3a 100644
> > --- a/tests/qapi-schema/doc-good.out
> > +++ b/tests/qapi-schema/doc-good.out
> > @@ -12,15 +12,16 @@ enum QType
> >  module doc-good.json
> >  enum Enum
> >      member one
> > -        if ['defined(IFONE)']
> > +        if defined(IFONE)
> >      member two
> > -    if ['defined(IFCOND)']
> > +    if defined(IFCOND)
> >      feature enum-feat
> >  object Base
> >      member base1: Enum optional=False
> > +    if OrderedDict([('all', ['IFALL1', 'IFALL2'])])
> >  object Variant1
> >      member var1: str optional=False
> > -        if ['defined(IFSTR)']
> > +        if defined(IFSTR)
> >          feature member-feat
> >      feature variant1-feat
> >  object Variant2
> > @@ -29,7 +30,7 @@ object Object
> >      tag base1
> >      case one: Variant1
> >      case two: Variant2
> > -        if ['IFTWO']
> > +        if IFTWO
> >      feature union-feat1
> >  object q_obj_Variant1-wrapper
> >      member data: Variant1 optional=False
> > @@ -38,13 +39,13 @@ object q_obj_Variant2-wrapper
> >  enum SugaredUnionKind
> >      member one
> >      member two
> > -        if ['IFTWO']
> > +        if IFTWO
> >  object SugaredUnion
> >      member type: SugaredUnionKind optional=False
> >      tag type
> >      case one: q_obj_Variant1-wrapper
> >      case two: q_obj_Variant2-wrapper
> > -        if ['IFTWO']
> > +        if IFTWO
> >      feature union-feat2
> >  alternate Alternate
> >      tag type
> > diff --git a/tests/qapi-schema/doc-good.txt
> b/tests/qapi-schema/doc-good.txt
> > index 726727af74..4490108cb7 100644
> > --- a/tests/qapi-schema/doc-good.txt
> > +++ b/tests/qapi-schema/doc-good.txt
> > @@ -76,6 +76,12 @@ Members
> >     the first member
> >
> >
> > +If
> > +~~
> > +
> > +"(IFALL1 and IFALL2)"
> > +
> > +
>
> The documentation generated for conditionals is poor before and after
> your work.  Observation, not demand.
>
> >  "Variant1" (Object)
> >  -------------------
> >
>
> [...]
>
>

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