Re: [PATCH v3 5/7] 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.
>
> NB: This necessitates a change of typing for check_if() and
> check_keys(), because mypy does not believe UserDict[str, object] ⊆
> Dict[str, object]. It will, however, accept Mapping or
> MutableMapping. In this case, the immutable form is preferred as an
> input parameter because we don't actually mutate the input.
>
> Without this change, we will observe:
> qapi/expr.py:631: error: Argument 1 to "check_keys" has incompatible
> type "QAPIExpression"; expected "Dict[str, object]"
>
> NB2: Python 3.6 has an oversight for typing UserDict that makes it
> impossible to type for both runtime and analysis time. The problem is
> described in detail at https://github.com/python/typing/issues/60 - this
> workaround can be safely removed when 3.7 is our minimum version.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

Looks good to me, just one question:

[...]

> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 1b006cdc133..87b46db7fba 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -14,13 +14,14 @@
>  # This work is licensed under the terms of the GNU GPL, version 2.
>  # See the COPYING file in the top-level directory.
>  
> -from collections import OrderedDict
> +from collections import OrderedDict, UserDict
>  import os
>  import re
>  from typing import (
>      TYPE_CHECKING,
>      Dict,
>      List,
> +    Mapping,
>      Optional,
>      Set,
>      Union,
> @@ -37,15 +38,30 @@
>      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.
> +
> +
> +# 3.6 workaround: can be removed when Python 3.7+ is our required version.
> +if TYPE_CHECKING:
> +    _UserDict = UserDict[str, object]
> +else:
> +    _UserDict = UserDict
> +
> +
> +class QAPIExpression(_UserDict):

Can we subclass dict instead?

> +    def __init__(self,
> +                 initialdata: Mapping[str, object],
> +                 info: QAPISourceInfo,
> +                 doc: Optional['QAPIDoc'] = None):
> +        super().__init__(initialdata)
> +        self.info = info
> +        self.doc: Optional['QAPIDoc'] = doc
>  
>  
>  class QAPIParseError(QAPISourceError):

[...]