Re: [PATCH v2 2/7] qapi/parser.py: add ParsedExpression type

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> This is an immutable, named, typed tuple. It's arguably nicer than
> arbitrary dicts for passing data around when using strict typing.
>
> This patch turns parser.exprs into a list of ParsedExpressions 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   | 29 +++++++++--------------------
>  scripts/qapi/parser.py | 29 ++++++++++++++++++-----------
>  scripts/qapi/schema.py |  6 +++---
>  3 files changed, 30 insertions(+), 34 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index d01543006d8..293f830fe9d 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 ParsedExpression
>  from .source import QAPISourceInfo
>  
>  
> @@ -595,29 +595,17 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None:
>      check_type(args, info, "'data'", allow_dict=not boxed)
>  
>  
> -def check_expr(expr_elem: _JSONObject) -> None:
> +def check_expr(pexpr: ParsedExpression) -> None:
>      """
> -    Validate and normalize a parsed QAPI schema expression.
> +    Validate and normalize a `ParsedExpression`.

Doc comment style question: what's our attitude towards repeating (parts
of) the function signature in its doc comment?

In general, I dislike comments that merely rephrase code as prose.

Untyped Python builds a habit of mentioning the types in the doc
comment.  That's additional information.  In typed Python, it's
rephrased information.

Keeping the old line would be just fine with me.

>  
> -    :param expr_elem: The parsed expression to normalize and validate.
> +    :param pexpr: The parsed expression to normalize and validate.
>  
>      :raise QAPISemError: When this expression fails validation.
> -    :return: None, ``expr`` is normalized in-place as needed.
> +    :return: None, ``pexpr`` is normalized in-place as needed.
>      """
> -    # 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
> +    expr = pexpr.expr
> +    info = pexpr.info
>  
>      if 'include' in expr:
>          return
> @@ -637,6 +625,7 @@ def check_expr(expr_elem: _JSONObject) -> None:
>      info.set_defn(meta, name)
>      check_defn_name_str(name, info, meta)
>  
> +    doc = pexpr.doc
>      if doc:
>          if doc.symbol != name:
>              raise QAPISemError(
> @@ -688,7 +677,7 @@ def check_expr(expr_elem: _JSONObject) -> None:
>      check_flags(expr, info)
>  
>  
> -def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
> +def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
>      """
>      Validate and normalize a list of parsed QAPI schema expressions.
>  
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 1b006cdc133..f897dffbfd4 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -21,6 +21,7 @@
>      TYPE_CHECKING,
>      Dict,
>      List,
> +    NamedTuple,
>      Optional,
>      Set,
>      Union,
> @@ -48,6 +49,12 @@
>  # several modules.
>  
>  
> +class ParsedExpression(NamedTuple):
> +    expr: TopLevelExpr
> +    info: QAPISourceInfo
> +    doc: Optional['QAPIDoc']
> +
> +
>  class QAPIParseError(QAPISourceError):
>      """Error class for all QAPI schema parsing errors."""
>      def __init__(self, parser: 'QAPISchemaParser', msg: str):
> @@ -100,7 +107,7 @@ def __init__(self,
>          self.line_pos = 0
>  
>          # Parser output:
> -        self.exprs: List[Dict[str, object]] = []
> +        self.exprs: List[ParsedExpression] = []
>          self.docs: List[QAPIDoc] = []
>  
>          # Showtime!
> @@ -147,8 +154,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 +171,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: TopLevelExpr,
> +                  info: QAPISourceInfo,
> +                  doc: Optional['QAPIDoc'] = None) -> None:
> +        self.exprs.append(ParsedExpression(expr, info, doc))
> +
>      @staticmethod
>      def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
>          if doc and doc.symbol:
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index cd8661125cd..89f8e60db36 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -1168,9 +1168,9 @@ def _def_event(self, expr, info, doc):
>  
>      def _def_exprs(self, exprs):
>          for expr_elem in exprs:

Rename @expr_elem to @pexpr for consistency with check_expr()?

> -            expr = expr_elem['expr']
> -            info = expr_elem['info']
> -            doc = expr_elem.get('doc')
> +            expr = expr_elem.expr
> +            info = expr_elem.info
> +            doc = expr_elem.doc
>              if 'enum' in expr:
>                  self._def_enum_type(expr, info, doc)
>              elif 'struct' in expr: