[PATCH v4 0/6] qapi: static typing conversion, pt5c

[PATCH v4 0/6] qapi: static typing conversion, pt5c

[John Snow]

This is part five (c), and focuses on sharing strict types between
parser.py and expr.py.

gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5c

Every commit should pass with:
 - `isort -c qapi/`
 - `flake8 qapi/`
 - `pylint --rcfile=qapi/pylintrc qapi/`
 - `mypy --config-file=qapi/mypy.ini qapi/`

V4:
 - Dropped the "split check_exprs" patch
 - Based the QAPIExpression class on Dict[str, object] instead
 - Removed the type workaround patch, which is no longer needed.
 - Added a new patch to fix a problem with Python 3.6 and pylint

V3:
 - Squashed a bunch of patches into the QAPIExpression patch
 - Added a few 'info' locals whenever there were >= 3 usages to help
   minimize some churn
 - Removed some type redundancy from docstrings
 - Brought along the two patches from pt0 that I want merged.
 - Removed 'pexpr' entirely, there's no intermediate state where it's
   needed now.
 - Minor style issues.

John Snow (6):
  qapi: Update flake8 config
  qapi: update pylint configuration
  qapi: Add minor typing workaround for 3.6
  qapi/parser: add QAPIExpression type
  qapi: remove _JSONObject
  qapi: remove JSON value FIXME

 scripts/qapi/.flake8   |   3 +-
 scripts/qapi/expr.py   | 101 ++++++++++++++++-------------------------
 scripts/qapi/parser.py |  41 +++++++++--------
 scripts/qapi/pylintrc  |   1 +
 scripts/qapi/schema.py |  72 +++++++++++++++--------------
 5 files changed, 104 insertions(+), 114 deletions(-)

-- 
2.39.0

[PATCH v4 1/6] qapi: Update flake8 config

[John Snow]

New versions of flake8 don't like same-line comments. (It's a version
newer than what fc37 ships, but it still makes my life easier to fix it
now.)

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

diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
index 6b158c68b84..a873ff67309 100644
--- a/scripts/qapi/.flake8
+++ b/scripts/qapi/.flake8
@@ -1,2 +1,3 @@
 [flake8]
-extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
+# Prefer pylint's bare-except checks to flake8's
+extend-ignore = E722
-- 
2.39.0

[PATCH v4 2/6] qapi: update pylint configuration

[John Snow]

Newer versions of pylint disable the "no-self-use" message by
default. Older versions don't, though. If we leave the suppressions in,
pylint yelps about useless options. Just tell pylint to shush.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/pylintrc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index a7246282030..90546df5345 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -23,6 +23,7 @@ disable=fixme,
         too-many-statements,
         too-many-instance-attributes,
         consider-using-f-string,
+        useless-option-value,
 
 [REPORTS]
 
-- 
2.39.0

[PATCH v4 3/6] qapi: Add minor typing workaround for 3.6

[John Snow]

Pylint under 3.6 does not believe that Collection is subscriptable at
runtime. It is, making this a Pylint
bug. https://github.com/PyCQA/pylint/issues/2377

They closed it as fixed, but that doesn't seem to be true as of Pylint
2.13.9, the latest version you can install under Python 3.6. 2.13.9 was
released 2022-05-13, about seven months after the bug was closed.

The least-annoying fix here is to just use the more specific type
Sequence, only because it seems to work in 3.6.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 5a1782b57ea..8701351fdfc 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -33,11 +33,11 @@
 
 import re
 from typing import (
-    Collection,
     Dict,
     Iterable,
     List,
     Optional,
+    Sequence,
     Union,
     cast,
 )
@@ -195,8 +195,8 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
 def check_keys(value: _JSONObject,
                info: QAPISourceInfo,
                source: str,
-               required: Collection[str],
-               optional: Collection[str]) -> None:
+               required: Sequence[str],
+               optional: Sequence[str]) -> None:
     """
     Ensure that a dict has a specific set of keys.
 
-- 
2.39.0

Re: [PATCH v4 3/6] qapi: Add minor typing workaround for 3.6

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Pylint under 3.6 does not believe that Collection is subscriptable at
> runtime. It is, making this a Pylint
> bug. https://github.com/PyCQA/pylint/issues/2377
>
> They closed it as fixed, but that doesn't seem to be true as of Pylint
> 2.13.9, the latest version you can install under Python 3.6. 2.13.9 was
> released 2022-05-13, about seven months after the bug was closed.
>
> The least-annoying fix here is to just use the more specific type
> Sequence, only because it seems to work in 3.6.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 5a1782b57ea..8701351fdfc 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -33,11 +33,11 @@
>  
>  import re
>  from typing import (
> -    Collection,
>      Dict,
>      Iterable,
>      List,
>      Optional,
> +    Sequence,
>      Union,
>      cast,
>  )
> @@ -195,8 +195,8 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
>  def check_keys(value: _JSONObject,
>                 info: QAPISourceInfo,
>                 source: str,
> -               required: Collection[str],
> -               optional: Collection[str]) -> None:
> +               required: Sequence[str],
> +               optional: Sequence[str]) -> None:
>      """
>      Ensure that a dict has a specific set of keys.

The actual arguments are always List[str].  You actually used that until
v3 of the patch, and switched to the maximally general Collection[str]
in v4, with rationale that ended up in commit 538cd41065a:

    qapi/expr.py: Modify check_keys to accept any Collection
    
    This is a minor adjustment that lets parameters @required and
    @optional take tuple arguments, in particular ().  Later patches will
    make use of that.

No later patch ever did.

I'd prefer maximally stupid List[str], but it's no big deal either way.

Re: [PATCH v4 3/6] qapi: Add minor typing workaround for 3.6

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> John Snow <jsnow@redhat.com> writes:
>
>> Pylint under 3.6 does not believe that Collection is subscriptable at
>> runtime. It is, making this a Pylint
>> bug. https://github.com/PyCQA/pylint/issues/2377
>>
>> They closed it as fixed, but that doesn't seem to be true as of Pylint
>> 2.13.9, the latest version you can install under Python 3.6. 2.13.9 was
>> released 2022-05-13, about seven months after the bug was closed.
>>
>> The least-annoying fix here is to just use the more specific type
>> Sequence, only because it seems to work in 3.6.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  scripts/qapi/expr.py | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 5a1782b57ea..8701351fdfc 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -33,11 +33,11 @@
>>  
>>  import re
>>  from typing import (
>> -    Collection,
>>      Dict,
>>      Iterable,
>>      List,
>>      Optional,
>> +    Sequence,
>>      Union,
>>      cast,
>>  )
>> @@ -195,8 +195,8 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
>>  def check_keys(value: _JSONObject,
>>                 info: QAPISourceInfo,
>>                 source: str,
>> -               required: Collection[str],
>> -               optional: Collection[str]) -> None:
>> +               required: Sequence[str],
>> +               optional: Sequence[str]) -> None:
>>      """
>>      Ensure that a dict has a specific set of keys.
>
> The actual arguments are always List[str].  You actually used that until
> v3 of the patch, and switched to the maximally general Collection[str]
> in v4, with rationale that ended up in commit 538cd41065a:
>
>     qapi/expr.py: Modify check_keys to accept any Collection
>     
>     This is a minor adjustment that lets parameters @required and
>     @optional take tuple arguments, in particular ().  Later patches will
>     make use of that.
>
> No later patch ever did.
>
> I'd prefer maximally stupid List[str], but it's no big deal either way.

Regardless,
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v4 3/6] qapi: Add minor typing workaround for 3.6

[John Snow]

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

On Wed, Feb 15, 2023, 8:36 AM Markus Armbruster <armbru@redhat.com> wrote:

> Markus Armbruster <armbru@redhat.com> writes:
>
> > John Snow <jsnow@redhat.com> writes:
> >
> >> Pylint under 3.6 does not believe that Collection is subscriptable at
> >> runtime. It is, making this a Pylint
> >> bug. https://github.com/PyCQA/pylint/issues/2377
> >>
> >> They closed it as fixed, but that doesn't seem to be true as of Pylint
> >> 2.13.9, the latest version you can install under Python 3.6. 2.13.9 was
> >> released 2022-05-13, about seven months after the bug was closed.
> >>
> >> The least-annoying fix here is to just use the more specific type
> >> Sequence, only because it seems to work in 3.6.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >>  scripts/qapi/expr.py | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> >> index 5a1782b57ea..8701351fdfc 100644
> >> --- a/scripts/qapi/expr.py
> >> +++ b/scripts/qapi/expr.py
> >> @@ -33,11 +33,11 @@
> >>
> >>  import re
> >>  from typing import (
> >> -    Collection,
> >>      Dict,
> >>      Iterable,
> >>      List,
> >>      Optional,
> >> +    Sequence,
> >>      Union,
> >>      cast,
> >>  )
> >> @@ -195,8 +195,8 @@ def check_defn_name_str(name: str, info:
> QAPISourceInfo, meta: str) -> None:
> >>  def check_keys(value: _JSONObject,
> >>                 info: QAPISourceInfo,
> >>                 source: str,
> >> -               required: Collection[str],
> >> -               optional: Collection[str]) -> None:
> >> +               required: Sequence[str],
> >> +               optional: Sequence[str]) -> None:
> >>      """
> >>      Ensure that a dict has a specific set of keys.
> >
> > The actual arguments are always List[str].  You actually used that until
> > v3 of the patch, and switched to the maximally general Collection[str]
> > in v4, with rationale that ended up in commit 538cd41065a:
> >
> >     qapi/expr.py: Modify check_keys to accept any Collection
> >
> >     This is a minor adjustment that lets parameters @required and
> >     @optional take tuple arguments, in particular ().  Later patches will
> >     make use of that.
> >
> > No later patch ever did.
>

I have some in "pt5d" that do - but this is the set of patches that do some
optional cleanups that you've dropped from earlier sets.

>
> > I'd prefer maximally stupid List[str], but it's no big deal either way.
>
> Regardless,
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>

Thanks- everything else look OK enough for now?

>

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

[PATCH v4 4/6] qapi/parser: add QAPIExpression type

[John Snow]

This patch creates a new type, QAPIExpression, which represents a parsed
expression complete with QAPIDoc and QAPISourceInfo.

This patch turns parser.exprs into a list of QAPIExpression instead,
and adjusts expr.py to match.

This allows the types we specify in parser.py to be "remembered" all the
way through expr.py and into schema.py. Several assertions around
packing and unpacking this data can be removed as a result.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py   | 82 +++++++++++++++++-------------------------
 scripts/qapi/parser.py | 46 ++++++++++++++----------
 scripts/qapi/schema.py | 72 ++++++++++++++++++++-----------------
 3 files changed, 100 insertions(+), 100 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 8701351fdfc..52153a2eec5 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -44,7 +44,7 @@
 
 from .common import c_name
 from .error import QAPISemError
-from .parser import QAPIDoc
+from .parser import QAPIExpression
 from .source import QAPISourceInfo
 
 
@@ -229,12 +229,11 @@ def pprint(elems: Iterable[str]) -> str:
                pprint(unknown), pprint(allowed)))
 
 
-def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_flags(expr: QAPIExpression) -> None:
     """
     Ensure flag members (if present) have valid values.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError:
         When certain flags have an invalid value, or when
@@ -243,18 +242,18 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
     for key in ('gen', 'success-response'):
         if key in expr and expr[key] is not False:
             raise QAPISemError(
-                info, "flag '%s' may only use false value" % key)
+                expr.info, "flag '%s' may only use false value" % key)
     for key in ('boxed', 'allow-oob', 'allow-preconfig', 'coroutine'):
         if key in expr and expr[key] is not True:
             raise QAPISemError(
-                info, "flag '%s' may only use true value" % key)
+                expr.info, "flag '%s' may only use true value" % key)
     if 'allow-oob' in expr and 'coroutine' in expr:
         # This is not necessarily a fundamental incompatibility, but
         # we don't have a use case and the desired semantics isn't
         # obvious.  The simplest solution is to forbid it until we get
         # a use case for it.
-        raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
-                                 "are incompatible")
+        raise QAPISemError(
+            expr.info, "flags 'allow-oob' and 'coroutine' are incompatible")
 
 
 def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
@@ -447,12 +446,11 @@ def check_features(features: Optional[object],
         check_if(feat, info, source)
 
 
-def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_enum(expr: QAPIExpression) -> None:
     """
     Normalize and validate this expression as an ``enum`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: When ``expr`` is not a valid ``enum``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -460,6 +458,7 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None:
     name = expr['enum']
     members = expr['data']
     prefix = expr.get('prefix')
+    info = expr.info
 
     if not isinstance(members, list):
         raise QAPISemError(info, "'data' must be an array")
@@ -486,12 +485,11 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None:
         check_features(member.get('features'), info)
 
 
-def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_struct(expr: QAPIExpression) -> None:
     """
     Normalize and validate this expression as a ``struct`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: When ``expr`` is not a valid ``struct``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -499,16 +497,15 @@ def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None:
     name = cast(str, expr['struct'])  # Checked in check_exprs
     members = expr['data']
 
-    check_type(members, info, "'data'", allow_dict=name)
-    check_type(expr.get('base'), info, "'base'")
+    check_type(members, expr.info, "'data'", allow_dict=name)
+    check_type(expr.get('base'), expr.info, "'base'")
 
 
-def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_union(expr: QAPIExpression) -> None:
     """
     Normalize and validate this expression as a ``union`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: when ``expr`` is not a valid ``union``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -517,6 +514,7 @@ def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None:
     base = expr['base']
     discriminator = expr['discriminator']
     members = expr['data']
+    info = expr.info
 
     check_type(base, info, "'base'", allow_dict=name)
     check_name_is_str(discriminator, info, "'discriminator'")
@@ -531,17 +529,17 @@ def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None:
         check_type(value['type'], info, source, allow_array=not base)
 
 
-def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_alternate(expr: QAPIExpression) -> None:
     """
     Normalize and validate this expression as an ``alternate`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: When ``expr`` is not a valid ``alternate``.
     :return: None, ``expr`` is normalized in-place as needed.
     """
     members = expr['data']
+    info = expr.info
 
     if not members:
         raise QAPISemError(info, "'data' must not be empty")
@@ -557,12 +555,11 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None:
         check_type(value['type'], info, source, allow_array=True)
 
 
-def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_command(expr: QAPIExpression) -> None:
     """
     Normalize and validate this expression as a ``command`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: When ``expr`` is not a valid ``command``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -572,17 +569,16 @@ def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:
     boxed = expr.get('boxed', False)
 
     if boxed and args is None:
-        raise QAPISemError(info, "'boxed': true requires 'data'")
-    check_type(args, info, "'data'", allow_dict=not boxed)
-    check_type(rets, info, "'returns'", allow_array=True)
+        raise QAPISemError(expr.info, "'boxed': true requires 'data'")
+    check_type(args, expr.info, "'data'", allow_dict=not boxed)
+    check_type(rets, expr.info, "'returns'", allow_array=True)
 
 
-def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_event(expr: QAPIExpression) -> None:
     """
     Normalize and validate this expression as an ``event`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: When ``expr`` is not a valid ``event``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -591,11 +587,11 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None:
     boxed = expr.get('boxed', False)
 
     if boxed and args is None:
-        raise QAPISemError(info, "'boxed': true requires 'data'")
-    check_type(args, info, "'data'", allow_dict=not boxed)
+        raise QAPISemError(expr.info, "'boxed': true requires 'data'")
+    check_type(args, expr.info, "'data'", allow_dict=not boxed)
 
 
-def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
+def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
     """
     Validate and normalize a list of parsed QAPI schema expressions.
 
@@ -607,21 +603,9 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
     :raise QAPISemError: When any expression fails validation.
     :return: The same list of expressions (now modified).
     """
-    for expr_elem in exprs:
-        # Expression
-        assert isinstance(expr_elem['expr'], dict)
-        for key in expr_elem['expr'].keys():
-            assert isinstance(key, str)
-        expr: _JSONObject = expr_elem['expr']
-
-        # QAPISourceInfo
-        assert isinstance(expr_elem['info'], QAPISourceInfo)
-        info: QAPISourceInfo = expr_elem['info']
-
-        # Optional[QAPIDoc]
-        tmp = expr_elem.get('doc')
-        assert tmp is None or isinstance(tmp, QAPIDoc)
-        doc: Optional[QAPIDoc] = tmp
+    for expr in exprs:
+        info = expr.info
+        doc = expr.doc
 
         if 'include' in expr:
             continue
@@ -653,24 +637,24 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
         if meta == 'enum':
             check_keys(expr, info, meta,
                        ['enum', 'data'], ['if', 'features', 'prefix'])
-            check_enum(expr, info)
+            check_enum(expr)
         elif meta == 'union':
             check_keys(expr, info, meta,
                        ['union', 'base', 'discriminator', 'data'],
                        ['if', 'features'])
             normalize_members(expr.get('base'))
             normalize_members(expr['data'])
-            check_union(expr, info)
+            check_union(expr)
         elif meta == 'alternate':
             check_keys(expr, info, meta,
                        ['alternate', 'data'], ['if', 'features'])
             normalize_members(expr['data'])
-            check_alternate(expr, info)
+            check_alternate(expr)
         elif meta == 'struct':
             check_keys(expr, info, meta,
                        ['struct', 'data'], ['base', 'if', 'features'])
             normalize_members(expr['data'])
-            check_struct(expr, info)
+            check_struct(expr)
         elif meta == 'command':
             check_keys(expr, info, meta,
                        ['command'],
@@ -678,17 +662,17 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
                         'gen', 'success-response', 'allow-oob',
                         'allow-preconfig', 'coroutine'])
             normalize_members(expr.get('data'))
-            check_command(expr, info)
+            check_command(expr)
         elif meta == 'event':
             check_keys(expr, info, meta,
                        ['event'], ['data', 'boxed', 'if', 'features'])
             normalize_members(expr.get('data'))
-            check_event(expr, info)
+            check_event(expr)
         else:
             assert False, 'unexpected meta type'
 
         check_if(expr, info, meta)
         check_features(expr.get('features'), info)
-        check_flags(expr, info)
+        check_flags(expr)
 
     return exprs
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 1b006cdc133..50906e27d49 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -21,6 +21,7 @@
     TYPE_CHECKING,
     Dict,
     List,
+    Mapping,
     Optional,
     Set,
     Union,
@@ -37,15 +38,24 @@
     from .schema import QAPISchemaFeature, QAPISchemaMember
 
 
-#: Represents a single Top Level QAPI schema expression.
-TopLevelExpr = Dict[str, object]
-
 # Return value alias for get_expr().
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
-# FIXME: Consolidate and centralize definitions for TopLevelExpr,
-# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
-# several modules.
+
+# FIXME: Consolidate and centralize definitions for _ExprValue,
+# JSONValue, and _JSONObject; currently scattered across several
+# modules.
+
+
+class QAPIExpression(Dict[str, object]):
+    # pylint: disable=too-few-public-methods
+    def __init__(self,
+                 data: Mapping[str, object],
+                 info: QAPISourceInfo,
+                 doc: Optional['QAPIDoc'] = None):
+        super().__init__(data)
+        self.info = info
+        self.doc: Optional['QAPIDoc'] = doc
 
 
 class QAPIParseError(QAPISourceError):
@@ -100,7 +110,7 @@ def __init__(self,
         self.line_pos = 0
 
         # Parser output:
-        self.exprs: List[Dict[str, object]] = []
+        self.exprs: List[QAPIExpression] = []
         self.docs: List[QAPIDoc] = []
 
         # Showtime!
@@ -147,8 +157,7 @@ def _parse(self) -> None:
                                        "value of 'include' must be a string")
                 incl_fname = os.path.join(os.path.dirname(self._fname),
                                           include)
-                self.exprs.append({'expr': {'include': incl_fname},
-                                   'info': info})
+                self._add_expr(OrderedDict({'include': incl_fname}), info)
                 exprs_include = self._include(include, info, incl_fname,
                                               self._included)
                 if exprs_include:
@@ -165,17 +174,18 @@ def _parse(self) -> None:
                 for name, value in pragma.items():
                     self._pragma(name, value, info)
             else:
-                expr_elem = {'expr': expr,
-                             'info': info}
-                if cur_doc:
-                    if not cur_doc.symbol:
-                        raise QAPISemError(
-                            cur_doc.info, "definition documentation required")
-                    expr_elem['doc'] = cur_doc
-                self.exprs.append(expr_elem)
+                if cur_doc and not cur_doc.symbol:
+                    raise QAPISemError(
+                        cur_doc.info, "definition documentation required")
+                self._add_expr(expr, info, cur_doc)
             cur_doc = None
         self.reject_expr_doc(cur_doc)
 
+    def _add_expr(self, expr: Mapping[str, object],
+                  info: QAPISourceInfo,
+                  doc: Optional['QAPIDoc'] = None) -> None:
+        self.exprs.append(QAPIExpression(expr, info, doc))
+
     @staticmethod
     def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
         if doc and doc.symbol:
@@ -784,7 +794,7 @@ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
                                % feature.name)
         self.features[feature.name].connect(feature)
 
-    def check_expr(self, expr: TopLevelExpr) -> None:
+    def check_expr(self, expr: QAPIExpression) -> None:
         if self.has_section('Returns') and 'command' not in expr:
             raise QAPISemError(self.info,
                                "'Returns:' is only valid for commands")
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index cd8661125cd..207e4d71f39 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -17,7 +17,7 @@
 from collections import OrderedDict
 import os
 import re
-from typing import Optional
+from typing import List, Optional
 
 from .common import (
     POINTER_SUFFIX,
@@ -29,7 +29,7 @@
 )
 from .error import QAPIError, QAPISemError, QAPISourceError
 from .expr import check_exprs
-from .parser import QAPISchemaParser
+from .parser import QAPIExpression, QAPISchemaParser
 
 
 class QAPISchemaIfCond:
@@ -964,10 +964,11 @@ def module_by_fname(self, fname):
         name = self._module_name(fname)
         return self._module_dict[name]
 
-    def _def_include(self, expr, info, doc):
+    def _def_include(self, expr: QAPIExpression):
         include = expr['include']
-        assert doc is None
-        self._def_entity(QAPISchemaInclude(self._make_module(include), info))
+        assert expr.doc is None
+        self._def_entity(
+            QAPISchemaInclude(self._make_module(include), expr.info))
 
     def _def_builtin_type(self, name, json_type, c_type):
         self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
@@ -1045,14 +1046,15 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
                 name, info, None, ifcond, None, None, members, None))
         return name
 
-    def _def_enum_type(self, expr, info, doc):
+    def _def_enum_type(self, expr: QAPIExpression):
         name = expr['enum']
         data = expr['data']
         prefix = expr.get('prefix')
         ifcond = QAPISchemaIfCond(expr.get('if'))
+        info = expr.info
         features = self._make_features(expr.get('features'), info)
         self._def_entity(QAPISchemaEnumType(
-            name, info, doc, ifcond, features,
+            name, info, expr.doc, ifcond, features,
             self._make_enum_members(data, info), prefix))
 
     def _make_member(self, name, typ, ifcond, features, info):
@@ -1072,14 +1074,15 @@ def _make_members(self, data, info):
                                   value.get('features'), info)
                 for (key, value) in data.items()]
 
-    def _def_struct_type(self, expr, info, doc):
+    def _def_struct_type(self, expr: QAPIExpression):
         name = expr['struct']
         base = expr.get('base')
         data = expr['data']
+        info = expr.info
         ifcond = QAPISchemaIfCond(expr.get('if'))
         features = self._make_features(expr.get('features'), info)
         self._def_entity(QAPISchemaObjectType(
-            name, info, doc, ifcond, features, base,
+            name, info, expr.doc, ifcond, features, base,
             self._make_members(data, info),
             None))
 
@@ -1089,11 +1092,13 @@ def _make_variant(self, case, typ, ifcond, info):
             typ = self._make_array_type(typ[0], info)
         return QAPISchemaVariant(case, info, typ, ifcond)
 
-    def _def_union_type(self, expr, info, doc):
+    def _def_union_type(self, expr: QAPIExpression):
         name = expr['union']
         base = expr['base']
         tag_name = expr['discriminator']
         data = expr['data']
+        assert isinstance(data, dict)
+        info = expr.info
         ifcond = QAPISchemaIfCond(expr.get('if'))
         features = self._make_features(expr.get('features'), info)
         if isinstance(base, dict):
@@ -1105,17 +1110,19 @@ def _def_union_type(self, expr, info, doc):
                                QAPISchemaIfCond(value.get('if')),
                                info)
             for (key, value) in data.items()]
-        members = []
+        members: List[QAPISchemaObjectTypeMember] = []
         self._def_entity(
-            QAPISchemaObjectType(name, info, doc, ifcond, features,
+            QAPISchemaObjectType(name, info, expr.doc, ifcond, features,
                                  base, members,
                                  QAPISchemaVariants(
                                      tag_name, info, None, variants)))
 
-    def _def_alternate_type(self, expr, info, doc):
+    def _def_alternate_type(self, expr: QAPIExpression):
         name = expr['alternate']
         data = expr['data']
+        assert isinstance(data, dict)
         ifcond = QAPISchemaIfCond(expr.get('if'))
+        info = expr.info
         features = self._make_features(expr.get('features'), info)
         variants = [
             self._make_variant(key, value['type'],
@@ -1124,11 +1131,11 @@ def _def_alternate_type(self, expr, info, doc):
             for (key, value) in data.items()]
         tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
         self._def_entity(
-            QAPISchemaAlternateType(name, info, doc, ifcond, features,
-                                    QAPISchemaVariants(
-                                        None, info, tag_member, variants)))
+            QAPISchemaAlternateType(
+                name, info, expr.doc, ifcond, features,
+                QAPISchemaVariants(None, info, tag_member, variants)))
 
-    def _def_command(self, expr, info, doc):
+    def _def_command(self, expr: QAPIExpression):
         name = expr['command']
         data = expr.get('data')
         rets = expr.get('returns')
@@ -1139,6 +1146,7 @@ def _def_command(self, expr, info, doc):
         allow_preconfig = expr.get('allow-preconfig', False)
         coroutine = expr.get('coroutine', False)
         ifcond = QAPISchemaIfCond(expr.get('if'))
+        info = expr.info
         features = self._make_features(expr.get('features'), info)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
@@ -1147,44 +1155,42 @@ def _def_command(self, expr, info, doc):
         if isinstance(rets, list):
             assert len(rets) == 1
             rets = self._make_array_type(rets[0], info)
-        self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, features,
-                                           data, rets,
+        self._def_entity(QAPISchemaCommand(name, info, expr.doc, ifcond,
+                                           features, data, rets,
                                            gen, success_response,
                                            boxed, allow_oob, allow_preconfig,
                                            coroutine))
 
-    def _def_event(self, expr, info, doc):
+    def _def_event(self, expr: QAPIExpression):
         name = expr['event']
         data = expr.get('data')
         boxed = expr.get('boxed', False)
         ifcond = QAPISchemaIfCond(expr.get('if'))
+        info = expr.info
         features = self._make_features(expr.get('features'), info)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
                 name, info, ifcond,
                 'arg', self._make_members(data, info))
-        self._def_entity(QAPISchemaEvent(name, info, doc, ifcond, features,
-                                         data, boxed))
+        self._def_entity(QAPISchemaEvent(name, info, expr.doc, ifcond,
+                                         features, data, boxed))
 
     def _def_exprs(self, exprs):
-        for expr_elem in exprs:
-            expr = expr_elem['expr']
-            info = expr_elem['info']
-            doc = expr_elem.get('doc')
+        for expr in exprs:
             if 'enum' in expr:
-                self._def_enum_type(expr, info, doc)
+                self._def_enum_type(expr)
             elif 'struct' in expr:
-                self._def_struct_type(expr, info, doc)
+                self._def_struct_type(expr)
             elif 'union' in expr:
-                self._def_union_type(expr, info, doc)
+                self._def_union_type(expr)
             elif 'alternate' in expr:
-                self._def_alternate_type(expr, info, doc)
+                self._def_alternate_type(expr)
             elif 'command' in expr:
-                self._def_command(expr, info, doc)
+                self._def_command(expr)
             elif 'event' in expr:
-                self._def_event(expr, info, doc)
+                self._def_event(expr)
             elif 'include' in expr:
-                self._def_include(expr, info, doc)
+                self._def_include(expr)
             else:
                 assert False
 
-- 
2.39.0

Re: [PATCH v4 4/6] qapi/parser: add QAPIExpression type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This patch creates a new type, QAPIExpression, which represents a parsed
> expression complete with QAPIDoc and QAPISourceInfo.
>
> This patch turns parser.exprs into a list of QAPIExpression instead,
> and adjusts expr.py to match.
>
> This allows the types we specify in parser.py to be "remembered" all the
> way through expr.py and into schema.py. Several assertions around
> packing and unpacking this data can be removed as a result.

Suggest to add:

  It also corrects a harmless typing error.  Before the patch,
  check_exprs() allegedly takes a List[_JSONObject].  It actually takes
  a list of dicts of the form

      {'expr': E, 'info': I, 'doc': D}

  where E is of type _ExprValue, I is of type QAPISourceInfo, and D is
  of type QAPIDoc.  Key 'doc' is optional.  This is not a _JSONObject!
  Passes type checking anyway, because _JSONObject is Dict[str, object].

> Signed-off-by: John Snow <jsnow@redhat.com>

[...]

> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 1b006cdc133..50906e27d49 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -21,6 +21,7 @@
>      TYPE_CHECKING,
>      Dict,
>      List,
> +    Mapping,
>      Optional,
>      Set,
>      Union,
> @@ -37,15 +38,24 @@
>      from .schema import QAPISchemaFeature, QAPISchemaMember
>  
>  
> -#: Represents a single Top Level QAPI schema expression.
> -TopLevelExpr = Dict[str, object]
> -
>  # Return value alias for get_expr().
>  _ExprValue = Union[List[object], Dict[str, object], str, bool]
>  
> -# FIXME: Consolidate and centralize definitions for TopLevelExpr,
> -# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
> -# several modules.
> +
> +# FIXME: Consolidate and centralize definitions for _ExprValue,
> +# JSONValue, and _JSONObject; currently scattered across several
> +# modules.
> +
> +
> +class QAPIExpression(Dict[str, object]):
> +    # pylint: disable=too-few-public-methods
> +    def __init__(self,
> +                 data: Mapping[str, object],

Would @expr be a better name?

> +                 info: QAPISourceInfo,
> +                 doc: Optional['QAPIDoc'] = None):
> +        super().__init__(data)
> +        self.info = info
> +        self.doc: Optional['QAPIDoc'] = doc
>  
>  
>  class QAPIParseError(QAPISourceError):

[...]

Regardless,
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v4 4/6] qapi/parser: add QAPIExpression type

[John Snow]

On Wed, Feb 15, 2023 at 4:43 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > This patch creates a new type, QAPIExpression, which represents a parsed
> > expression complete with QAPIDoc and QAPISourceInfo.
> >
> > This patch turns parser.exprs into a list of QAPIExpression instead,
> > and adjusts expr.py to match.
> >
> > This allows the types we specify in parser.py to be "remembered" all the
> > way through expr.py and into schema.py. Several assertions around
> > packing and unpacking this data can be removed as a result.
>
> Suggest to add:
>
>   It also corrects a harmless typing error.  Before the patch,
>   check_exprs() allegedly takes a List[_JSONObject].  It actually takes
>   a list of dicts of the form
>
>       {'expr': E, 'info': I, 'doc': D}
>
>   where E is of type _ExprValue, I is of type QAPISourceInfo, and D is
>   of type QAPIDoc.  Key 'doc' is optional.  This is not a _JSONObject!
>   Passes type checking anyway, because _JSONObject is Dict[str, object].
>
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> [...]
>
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 1b006cdc133..50906e27d49 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -21,6 +21,7 @@
> >      TYPE_CHECKING,
> >      Dict,
> >      List,
> > +    Mapping,
> >      Optional,
> >      Set,
> >      Union,
> > @@ -37,15 +38,24 @@
> >      from .schema import QAPISchemaFeature, QAPISchemaMember
> >
> >
> > -#: Represents a single Top Level QAPI schema expression.
> > -TopLevelExpr = Dict[str, object]
> > -
> >  # Return value alias for get_expr().
> >  _ExprValue = Union[List[object], Dict[str, object], str, bool]
> >
> > -# FIXME: Consolidate and centralize definitions for TopLevelExpr,
> > -# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
> > -# several modules.
> > +
> > +# FIXME: Consolidate and centralize definitions for _ExprValue,
> > +# JSONValue, and _JSONObject; currently scattered across several
> > +# modules.
> > +
> > +
> > +class QAPIExpression(Dict[str, object]):
> > +    # pylint: disable=too-few-public-methods
> > +    def __init__(self,
> > +                 data: Mapping[str, object],
>
> Would @expr be a better name?
>

Linters don't seem to mind the new parameter name. Feel free to make
the substitution if you don't mind "expr" sometimes referring to a
QAPIExpression and sometimes referring to the JSON-y data inside of
it. I am less particular about consistency in my local variable names
than you are, so it's a matter of taste for you specifically.

> > +                 info: QAPISourceInfo,
> > +                 doc: Optional['QAPIDoc'] = None):
> > +        super().__init__(data)
> > +        self.info = info
> > +        self.doc: Optional['QAPIDoc'] = doc
> >
> >
> >  class QAPIParseError(QAPISourceError):
>
> [...]
>
> Regardless,
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>

[PATCH v4 5/6] qapi: remove _JSONObject

[John Snow]

We can remove this alias as it only has two usages now, and no longer
pays for the confusion of "yet another type".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py   | 13 +++----------
 scripts/qapi/parser.py |  5 ++---
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 52153a2eec5..fc4defecc8e 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -48,14 +48,6 @@
 from .source import QAPISourceInfo
 
 
-# Deserialized JSON objects as returned by the parser.
-# The values of this mapping are not necessary to exhaustively type
-# here (and also not practical as long as mypy lacks recursive
-# types), because the purpose of this module is to interrogate that
-# type.
-_JSONObject = Dict[str, object]
-
-
 # See check_name_str(), below.
 valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
                         r'(x-)?'
@@ -192,7 +184,7 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
                 info, "%s name should not end in 'List'" % meta)
 
 
-def check_keys(value: _JSONObject,
+def check_keys(value: Dict[str, object],
                info: QAPISourceInfo,
                source: str,
                required: Sequence[str],
@@ -256,7 +248,8 @@ def check_flags(expr: QAPIExpression) -> None:
             expr.info, "flags 'allow-oob' and 'coroutine' are incompatible")
 
 
-def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
+def check_if(expr: Dict[str, object],
+             info: QAPISourceInfo, source: str) -> None:
     """
     Validate the ``if`` member of an object.
 
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 50906e27d49..d570086e1a9 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -42,9 +42,8 @@
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
 
-# FIXME: Consolidate and centralize definitions for _ExprValue,
-# JSONValue, and _JSONObject; currently scattered across several
-# modules.
+# FIXME: Consolidate and centralize definitions for _ExprValue and
+# JSONValue; currently scattered across several modules.
 
 
 class QAPIExpression(Dict[str, object]):
-- 
2.39.0

Re: [PATCH v4 5/6] qapi: remove _JSONObject

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> We can remove this alias as it only has two usages now, and no longer
> pays for the confusion of "yet another type".
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v4 6/6] qapi: remove JSON value FIXME

[John Snow]

With the two major JSON-ish type hierarchies clarified for distinct
purposes; QAPIExpression for parsed expressions and JSONValue for
introspection data, remove this FIXME as no longer an action item.

A third JSON-y data type, _ExprValue, is not meant to represent JSON in
the abstract but rather only the possible legal return values from a
single function, get_expr(). It isn't appropriate to attempt to merge it
with either of the above two types.

In theory, it may be possible to define a completely agnostic
one-size-fits-all JSON type hierarchy that any other user could borrow -
in practice, it's tough to wrangle the differences between invariant,
covariant and contravariant types: input and output parameters demand
different properties of such a structure.

However, QAPIExpression serves to authoritatively type user input to the
QAPI parser, while JSONValue serves to authoritatively type qapi
generator *output* to be served back to client users at runtime via
QMP. The AST for these two types are different and cannot be wholly
merged into a unified syntax.

They could, in theory, share some JSON primitive definitions. In
practice, this is currently more trouble than it's worth with mypy's
current expressive power. As such, declare this "done enough for now".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index d570086e1a9..878f90b4583 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -42,10 +42,6 @@
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
 
-# FIXME: Consolidate and centralize definitions for _ExprValue and
-# JSONValue; currently scattered across several modules.
-
-
 class QAPIExpression(Dict[str, object]):
     # pylint: disable=too-few-public-methods
     def __init__(self,
-- 
2.39.0

Re: [PATCH v4 6/6] qapi: remove JSON value FIXME

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> With the two major JSON-ish type hierarchies clarified for distinct
> purposes; QAPIExpression for parsed expressions and JSONValue for
> introspection data, remove this FIXME as no longer an action item.
>
> A third JSON-y data type, _ExprValue, is not meant to represent JSON in
> the abstract but rather only the possible legal return values from a
> single function, get_expr(). It isn't appropriate to attempt to merge it
> with either of the above two types.
>
> In theory, it may be possible to define a completely agnostic
> one-size-fits-all JSON type hierarchy that any other user could borrow -
> in practice, it's tough to wrangle the differences between invariant,
> covariant and contravariant types: input and output parameters demand
> different properties of such a structure.
>
> However, QAPIExpression serves to authoritatively type user input to the
> QAPI parser, while JSONValue serves to authoritatively type qapi
> generator *output* to be served back to client users at runtime via
> QMP. The AST for these two types are different and cannot be wholly
> merged into a unified syntax.
>
> They could, in theory, share some JSON primitive definitions. In
> practice, this is currently more trouble than it's worth with mypy's
> current expressive power. As such, declare this "done enough for now".
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v4 0/6] qapi: static typing conversion, pt5c

[Markus Armbruster]

I had a few suggestions, but none of them requires a respin.  Let's
discuss them, and then I merge.

Re: [PATCH v4 0/6] qapi: static typing conversion, pt5c

[John Snow]

On Wed, Feb 15, 2023 at 8:39 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> I had a few suggestions, but none of them requires a respin.  Let's
> discuss them, and then I merge.

Hiya, I lost track of things a little due to the other Python
discussion. Who is waiting for whom?

--js

Re: [PATCH v4 0/6] qapi: static typing conversion, pt5c

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Feb 15, 2023 at 8:39 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> I had a few suggestions, but none of them requires a respin.  Let's
>> discuss them, and then I merge.
>
> Hiya, I lost track of things a little due to the other Python
> discussion. Who is waiting for whom?

Just two questions remain:

* PATCH 3: Dumb down check_keys() argument all the way to List[str]?

* PATCH 4: Suggested commit message addition okay?

We settle them, and then I'll take it from there.

Re: [PATCH v4 0/6] qapi: static typing conversion, pt5c

[John Snow]

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

On Tue, Feb 21, 2023, 1:42 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Wed, Feb 15, 2023 at 8:39 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >>
> >> I had a few suggestions, but none of them requires a respin.  Let's
> >> discuss them, and then I merge.
> >
> > Hiya, I lost track of things a little due to the other Python
> > discussion. Who is waiting for whom?
>
> Just two questions remain:
>
> * PATCH 3: Dumb down check_keys() argument all the way to List[str]?
>

Kinda prefer not to, but maybe I'm being too precious. (I have some more
exploratory patches that do use tuples here instead, but admit it's not
crucial.)

From a pure typing perspective, I wish I could leave it as it is now,
because I prefer to type input types as loosely as possible: claim only the
minimum power we need, instead of enforcing the specificity we happen to
have.

With the bug for 3.6 that is forcing me to use a more specific type anyway,
maybe you're right and I should just use List[] until I'm allowed to have
my proper abstraction.

OK, before I go further, lemme say that you can change it to List[] if you
want. I won't be too precious about it. (You can rewrite the patch in
question if you don't want to wait 24h.)

But, a question about typing strategy:

As a python tooling maintainer, should I push people to type as flexible as
possible or as *specific* as possible in general?

Flexible: (e.g. Sequence or Iterable)
- More likely to get along with other code
- More "pythonic", abstractly
- Less useful as documentation; if a function always happens to get a list,
is it annoying to pretend it's merely a sequence?

Specific: (e.g. List)
- Most useful as documentation
- Can assert greater knowledge of all callers
- More power afforded to function ("room to grow"?)
- More likely to require non-local edits when changing functionality or
refactoring
- More likely to require "casts" at callsites to convert data types

I think I lean towards the flexible/broad typing strategy in general, but
lament it cannot be applied appropriately here today.


> * PATCH 4: Suggested commit message addition okay?
>

Yes, ACK.


> We settle them, and then I'll take it from there.
>
>

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

Re: [PATCH v4 0/6] qapi: static typing conversion, pt5c

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Feb 21, 2023, 1:42 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > On Wed, Feb 15, 2023 at 8:39 AM Markus Armbruster <armbru@redhat.com>
>> wrote:
>> >>
>> >> I had a few suggestions, but none of them requires a respin.  Let's
>> >> discuss them, and then I merge.
>> >
>> > Hiya, I lost track of things a little due to the other Python
>> > discussion. Who is waiting for whom?
>>
>> Just two questions remain:
>>
>> * PATCH 3: Dumb down check_keys() argument all the way to List[str]?
>>
>
> Kinda prefer not to, but maybe I'm being too precious. (I have some more
> exploratory patches that do use tuples here instead, but admit it's not
> crucial.)
>
> From a pure typing perspective, I wish I could leave it as it is now,
> because I prefer to type input types as loosely as possible: claim only the
> minimum power we need, instead of enforcing the specificity we happen to
> have.
>
> With the bug for 3.6 that is forcing me to use a more specific type anyway,
> maybe you're right and I should just use List[] until I'm allowed to have
> my proper abstraction.
>
> OK, before I go further, lemme say that you can change it to List[] if you
> want. I won't be too precious about it. (You can rewrite the patch in
> question if you don't want to wait 24h.)
>
> But, a question about typing strategy:

A good one!

> As a python tooling maintainer, should I push people to type as flexible as
> possible or as *specific* as possible in general?
>
> Flexible: (e.g. Sequence or Iterable)
> - More likely to get along with other code
> - More "pythonic", abstractly
> - Less useful as documentation; if a function always happens to get a list,
> is it annoying to pretend it's merely a sequence?
>
> Specific: (e.g. List)
> - Most useful as documentation
> - Can assert greater knowledge of all callers
> - More power afforded to function ("room to grow"?)
> - More likely to require non-local edits when changing functionality or
> refactoring
> - More likely to require "casts" at callsites to convert data types
>
> I think I lean towards the flexible/broad typing strategy in general, but
> lament it cannot be applied appropriately here today.

Adjusting strategy to context can make some sense, I think.

When we're exposing a library to unknown users, we want to ask for the
weakest possible preconditions, i.e. use maximally abstract types for
input.

That's one end of a spectrum, I think.  Here's an example for the other
end: say I write a helper function for just one caller, which passes
arguments of just one type.  I'd use exactly that type.  KISS.

Much of the time, we're somewhere in between, i.e. in the realm of
judgement calls and taste.

Sticking to weakest possible preconditions ("flexible") everywhere saves
you the trouble of making the judgement calls.  Defensible point of
view.  Not mine, though.

The more abstract the type is, the more flexibility we shift from callee
to callers.  I want to be able to put the flexibility where I expect it
to be needed more.

Moreover, I really, really like types as documentation.  When I see a
fully concrete type List[str], I know all there is to know about the
argument's type.  When I see Collection[str], I know less.  When I need
to know more, I get to chase callers, just as if there was no type at
all.

And most of all, I habitually eschew abstraction unless I see concrete
benefits.  Don't get me wrong, abstraction is awesome!  Probably the
most essential tool in the toolbox.  But like any powerful tool, it
should be used with care.

Back to the QAPI code generator.  So far, there are no unknown users,
simply because all users are right in the tree.  However, the thing has
become large enough to be difficult to keep in your brain all at once.
We don't *want* to know all users anymore.  We want some abstraction.

But where do we want it, and how much?  Where to put abstract
interfaces?

Not within .py files, I'd say.  Between them.  Or maybe just some of
them.  Ah, those pesky judgement calls!

Do I make sense?  Comments?  Objections?

>> * PATCH 4: Suggested commit message addition okay?
>>
>
> Yes, ACK.
>
>
>> We settle them, and then I'll take it from there.

No need for a respin.