On Sat, Feb 11, 2023, 1:49 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@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 <js...@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? > ...apparently yes? class QAPIExpression(Dict[str, object]): ... Only question I have is what the init signature ought to be, though keeping it as what I already have here apparently passes the type checker. I think we lose the ability to do kwargs init in that case, but mypy doesn't give us a lyskov warning, so maybe it's fine? I'll have to play around with it. dict is implemented at the cpython level, so I'm not as sure of how subclassing it works. They document three distinct signatures for its initializer, which is also a python built-in. I can rename that initialdata parameter too, pending what I discover above. > > + 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): > > [...] > >
