[PATCH 0/5] qapi: Another round of minor fixes and cleanups

[PATCH 0/5] qapi: Another round of minor fixes and cleanups

[Markus Armbruster]

Markus Armbruster (5):
  qapi: Fix a botched type annotation
  qapi: Drop Indentation.__bool__()
  qapi: Bury some unused code in class Indentation
  tests/qapi-schema: Cover 'not' condition with empty argument
  qapi: Fix bogus error for 'if': { 'not': '' }

 scripts/qapi/common.py            | 19 ++++++-------------
 scripts/qapi/expr.py              | 21 +++++++++++++--------
 tests/qapi-schema/bad-if-not.err  |  2 ++
 tests/qapi-schema/bad-if-not.json |  3 +++
 tests/qapi-schema/bad-if-not.out  |  0
 tests/qapi-schema/meson.build     |  1 +
 6 files changed, 25 insertions(+), 21 deletions(-)
 create mode 100644 tests/qapi-schema/bad-if-not.err
 create mode 100644 tests/qapi-schema/bad-if-not.json
 create mode 100644 tests/qapi-schema/bad-if-not.out

-- 
2.31.1

[PATCH 1/5] qapi: Fix a botched type annotation

[Markus Armbruster]

Mypy is unhappy:

    $ mypy --config-file=scripts/qapi/mypy.ini `git-ls-files scripts/qapi/\*py`
    scripts/qapi/common.py:208: error: Function is missing a return type annotation
    scripts/qapi/common.py:227: error: Returning Any from function declared to return "str"

Messed up in commit ccea6a8637 "qapi: Factor common recursion out of
cgen_ifcond(), docgen_ifcond()".  Tidy up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/common.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 5f8f76e5b2..c4d11b9637 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -205,7 +205,8 @@ def gen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]],
                cond_fmt: str, not_fmt: str,
                all_operator: str, any_operator: str) -> str:
 
-    def do_gen(ifcond: Union[str, Dict[str, Any]], need_parens: bool):
+    def do_gen(ifcond: Union[str, Dict[str, Any]],
+               need_parens: bool) -> str:
         if isinstance(ifcond, str):
             return cond_fmt % ifcond
         assert isinstance(ifcond, dict) and len(ifcond) == 1
-- 
2.31.1

[PATCH 2/5] qapi: Drop Indentation.__bool__()

[Markus Armbruster]

Intentation.__bool__() is not worth its keep: it has just one user,
which can just as well check .__str__() instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/common.py | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index c4d11b9637..1d62c27fb7 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -142,10 +142,6 @@ def __str__(self) -> str:
         """Return the current indentation as a string of spaces."""
         return ' ' * self._level
 
-    def __bool__(self) -> bool:
-        """True when there is a non-zero indentation."""
-        return bool(self._level)
-
     def increase(self, amount: int = 4) -> None:
         """Increase the indentation level by ``amount``, default 4."""
         self._level += amount
@@ -169,8 +165,9 @@ def cgen(code: str, **kwds: object) -> str:
     Obey `indent`, and strip `EATSPACE`.
     """
     raw = code % kwds
-    if indent:
-        raw = re.sub(r'^(?!(#|$))', str(indent), raw, flags=re.MULTILINE)
+    pfx = str(indent)
+    if pfx:
+        raw = re.sub(r'^(?!(#|$))', pfx, raw, flags=re.MULTILINE)
     return re.sub(re.escape(EATSPACE) + r' *', '', raw)
 
 
-- 
2.31.1

[PATCH 3/5] qapi: Bury some unused code in class Indentation

[Markus Armbruster]

.__int__() has never been used.  Drop it.

.decrease() raises ArithmeticError when asked to decrease indentation
level below zero.  Nothing catches it.  It's a programming error.
Dumb down to assert.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/common.py | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 1d62c27fb7..489273574a 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -132,9 +132,6 @@ class Indentation:
     def __init__(self, initial: int = 0) -> None:
         self._level = initial
 
-    def __int__(self) -> int:
-        return self._level
-
     def __repr__(self) -> str:
         return "{}({:d})".format(type(self).__name__, self._level)
 
@@ -148,9 +145,7 @@ def increase(self, amount: int = 4) -> None:
 
     def decrease(self, amount: int = 4) -> None:
         """Decrease the indentation level by ``amount``, default 4."""
-        if self._level < amount:
-            raise ArithmeticError(
-                f"Can't remove {amount:d} spaces from {self!r}")
+        assert amount <= self._level
         self._level -= amount
 
 
-- 
2.31.1

[PATCH 4/5] tests/qapi-schema: Cover 'not' condition with empty argument

[Markus Armbruster]

We flag this, but the error message is bogus:

    bad-if-not.json:2: 'if' condition [] of struct is useless

The next commit will fix it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/bad-if-not.err  | 2 ++
 tests/qapi-schema/bad-if-not.json | 3 +++
 tests/qapi-schema/bad-if-not.out  | 0
 tests/qapi-schema/meson.build     | 1 +
 4 files changed, 6 insertions(+)
 create mode 100644 tests/qapi-schema/bad-if-not.err
 create mode 100644 tests/qapi-schema/bad-if-not.json
 create mode 100644 tests/qapi-schema/bad-if-not.out

diff --git a/tests/qapi-schema/bad-if-not.err b/tests/qapi-schema/bad-if-not.err
new file mode 100644
index 0000000000..b3acdd679a
--- /dev/null
+++ b/tests/qapi-schema/bad-if-not.err
@@ -0,0 +1,2 @@
+bad-if-not.json: In struct 'TestIfStruct':
+bad-if-not.json:2: 'if' condition [] of struct is useless
diff --git a/tests/qapi-schema/bad-if-not.json b/tests/qapi-schema/bad-if-not.json
new file mode 100644
index 0000000000..9fdaacc47b
--- /dev/null
+++ b/tests/qapi-schema/bad-if-not.json
@@ -0,0 +1,3 @@
+# check 'if not' with empy argument
+{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
+  'if': { 'not': '' } }
diff --git a/tests/qapi-schema/bad-if-not.out b/tests/qapi-schema/bad-if-not.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index 4697c070bc..6b2a4ce41a 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -43,6 +43,7 @@ schemas = [
   'bad-if-key.json',
   'bad-if-keys.json',
   'bad-if-list.json',
+  'bad-if-not.json',
   'bad-type-bool.json',
   'bad-type-dict.json',
   'bad-type-int.json',
-- 
2.31.1

[PATCH 5/5] qapi: Fix bogus error for 'if': { 'not': '' }

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/expr.py             | 21 +++++++++++++--------
 tests/qapi-schema/bad-if-not.err |  2 +-
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index b62f0a3640..ad3732c7f0 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -293,17 +293,22 @@ def _check_if(cond: Union[str, object]) -> None:
                 info,
                 "'if' condition of %s has conflicting keys" % source)
 
-        oper, operands = next(iter(cond.items()))
+        if 'not' in cond:
+            _check_if(cond['not'])
+        elif 'all' in cond:
+            _check_infix('all', cond['all'])
+        else:
+            _check_infix('any', cond['any'])
+
+    def _check_infix(operator: str, operands: object):
+        if not isinstance(operands, list):
+            raise QAPISemError(
+                info,
+                "'%s' condition of %s must be an array"
+                % (operator, source))
         if not operands:
             raise QAPISemError(
                 info, "'if' condition [] of %s is useless" % source)
-
-        if oper == "not":
-            _check_if(operands)
-            return
-        if oper in ("all", "any") and not isinstance(operands, list):
-            raise QAPISemError(
-                info, "'%s' condition of %s must be an array" % (oper, source))
         for operand in operands:
             _check_if(operand)
 
diff --git a/tests/qapi-schema/bad-if-not.err b/tests/qapi-schema/bad-if-not.err
index b3acdd679a..b33f5e16b8 100644
--- a/tests/qapi-schema/bad-if-not.err
+++ b/tests/qapi-schema/bad-if-not.err
@@ -1,2 +1,2 @@
 bad-if-not.json: In struct 'TestIfStruct':
-bad-if-not.json:2: 'if' condition [] of struct is useless
+bad-if-not.json:2: 'if' condition '' of struct is not a valid identifier
-- 
2.31.1

Re: [PATCH 0/5] qapi: Another round of minor fixes and cleanups

[Marc-André Lureau]

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

Hi

On Wed, Sep 8, 2021 at 8:54 AM Markus Armbruster <armbru@redhat.com> wrote:

> Markus Armbruster (5):
>   qapi: Fix a botched type annotation
>   qapi: Drop Indentation.__bool__()
>   qapi: Bury some unused code in class Indentation
>   tests/qapi-schema: Cover 'not' condition with empty argument
>   qapi: Fix bogus error for 'if': { 'not': '' }
>
>  scripts/qapi/common.py            | 19 ++++++-------------
>  scripts/qapi/expr.py              | 21 +++++++++++++--------
>  tests/qapi-schema/bad-if-not.err  |  2 ++
>  tests/qapi-schema/bad-if-not.json |  3 +++
>  tests/qapi-schema/bad-if-not.out  |  0
>  tests/qapi-schema/meson.build     |  1 +
>  6 files changed, 25 insertions(+), 21 deletions(-)
>  create mode 100644 tests/qapi-schema/bad-if-not.err
>  create mode 100644 tests/qapi-schema/bad-if-not.json
>  create mode 100644 tests/qapi-schema/bad-if-not.out
>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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

Re: [PATCH 5/5] qapi: Fix bogus error for 'if': { 'not': '' }

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi/expr.py             | 21 +++++++++++++--------
>  tests/qapi-schema/bad-if-not.err |  2 +-
>  2 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index b62f0a3640..ad3732c7f0 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -293,17 +293,22 @@ def _check_if(cond: Union[str, object]) -> None:
>                  info,
>                  "'if' condition of %s has conflicting keys" % source)
>  
> -        oper, operands = next(iter(cond.items()))
> +        if 'not' in cond:
> +            _check_if(cond['not'])
> +        elif 'all' in cond:
> +            _check_infix('all', cond['all'])
> +        else:
> +            _check_infix('any', cond['any'])
> +
> +    def _check_infix(operator: str, operands: object):
> +        if not isinstance(operands, list):
> +            raise QAPISemError(
> +                info,
> +                "'%s' condition of %s must be an array"
> +                % (operator, source))
>          if not operands:
>              raise QAPISemError(
>                  info, "'if' condition [] of %s is useless" % source)
> -
> -        if oper == "not":
> -            _check_if(operands)
> -            return
> -        if oper in ("all", "any") and not isinstance(operands, list):
> -            raise QAPISemError(
> -                info, "'%s' condition of %s must be an array" % (oper, source))
>          for operand in operands:
>              _check_if(operand)
>  
> diff --git a/tests/qapi-schema/bad-if-not.err b/tests/qapi-schema/bad-if-not.err
> index b3acdd679a..b33f5e16b8 100644
> --- a/tests/qapi-schema/bad-if-not.err
> +++ b/tests/qapi-schema/bad-if-not.err
> @@ -1,2 +1,2 @@
>  bad-if-not.json: In struct 'TestIfStruct':
> -bad-if-not.json:2: 'if' condition [] of struct is useless
> +bad-if-not.json:2: 'if' condition '' of struct is not a valid identifier

Squashing in this fixup:

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index ad3732c7f0..90bde501b0 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -300,7 +300,7 @@ def _check_if(cond: Union[str, object]) -> None:
         else:
             _check_infix('any', cond['any'])
 
-    def _check_infix(operator: str, operands: object):
+    def _check_infix(operator: str, operands: object) -> None:
         if not isinstance(operands, list):
             raise QAPISemError(
                 info,

Re: [PATCH 0/5] qapi: Another round of minor fixes and cleanups

[Markus Armbruster]

Queued.

Re: [PATCH 2/5] qapi: Drop Indentation.__bool__()

[Eric Blake]

On Wed, Sep 08, 2021 at 06:54:25AM +0200, Markus Armbruster wrote:
> Intentation.__bool__() is not worth its keep: it has just one user,

Indentation

> which can just as well check .__str__() instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi/common.py | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org