Re: [PATCH v7 06/10] qapi: replace if condition list with dict {'all': [...]}

[Markus Armbruster <armbru@redhat.com>]

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-all.err              |  2 +
>  tests/qapi-schema/bad-if-all.json             |  3 +
>  tests/qapi-schema/bad-if-all.out              |  0
>  tests/qapi-schema/bad-if-empty-list.json      |  2 +-
>  tests/qapi-schema/bad-if-key.err              |  3 +
>  tests/qapi-schema/bad-if-key.json             |  3 +
>  tests/qapi-schema/bad-if-key.out              |  0
>  tests/qapi-schema/bad-if-keys.err             |  2 +
>  tests/qapi-schema/bad-if-keys.json            |  3 +
>  tests/qapi-schema/bad-if-keys.out             |  0
>  tests/qapi-schema/bad-if-list.json            |  2 +-
>  tests/qapi-schema/bad-if.err                  |  2 +-
>  tests/qapi-schema/bad-if.json                 |  2 +-
>  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/meson.build                 |  3 +
>  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 +-
>  26 files changed, 138 insertions(+), 84 deletions(-)
>  create mode 100644 tests/qapi-schema/bad-if-all.err
>  create mode 100644 tests/qapi-schema/bad-if-all.json
>  create mode 100644 tests/qapi-schema/bad-if-all.out
>  create mode 100644 tests/qapi-schema/bad-if-key.err
>  create mode 100644 tests/qapi-schema/bad-if-key.json
>  create mode 100644 tests/qapi-schema/bad-if-key.out
>  create mode 100644 tests/qapi-schema/bad-if-keys.err
>  create mode 100644 tests/qapi-schema/bad-if-keys.json
>  create mode 100644 tests/qapi-schema/bad-if-keys.out
>
> 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:

Looks like you forgot to un-swap cgen_ifcond() and docgen_ifcond().  Can
do in my tree.

>      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) + ')'
>  
> -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
> +
> +    oper, operands = next(iter(ifcond.items()))
> +    oper = {'all': '&&'}[oper]
> +    operands = [cgen_ifcond(o) for o in operands]
> +    return '(' + (') ' + oper + ' (').join(operands) + ')'

I suggested a more legible version in review of v6.  Not worth a respin
by itself.

Note to self: try to get rid of redundant parenthesis here.

Note to self: cgen_ifcond() and docgen_ifcond() are almost identical.
Refactor?

>  
>  
>  def gen_if(cond: str) -> str:
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index cf98923fa6..b5187bfbca 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]``

John hasn't answered my question whether :forms: makes sensw without
:sugared:.  If it doesn't, I can drop it in my 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
>      """
>      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)

Mind if I squash in the move of the helper function to the beginning?

>  
>  
>  def normalize_members(members: object) -> None:
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index ff9c4f0a17..627735a431 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 {}

This is slightly subtle.

QAPISchemaIfCond.ifcond can look like one of

* {'all': [COND, ...]}

* {'any': [COND, ...]}          (only after PATCH 07)

* {'not': COND}                 (only after PATCH 08)

* STRING

* {}

This is just like the AST, except "absent" is now {} instead of None.
It can occur only at the root.

cgen_ifcond() and docgen_ifcond() are recursive, which means they
happily accept {} anywhere, and generate crap.

I believe the code works anyway, because it only ever creates {} in
QAPISchemaIfCond.__init__(), i.e. at the root.

Non-local correctness argument.  I'd like to try my hand at tweaking the
code so it's more obviously correct.  Not now.

>  
>      def cgen(self):
>          return cgen_ifcond(self.ifcond)
[Tests skipped for now...]