Re: [PATCH v4 05/19] qapi/expr.py: constrain incoming expression types

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> mypy does not know the types of values stored in Dicts that masquerade
> as objects. Help the type checker out by constraining the type.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index b4bbcd54c0..b75c85c160 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -15,9 +15,18 @@
>  # See the COPYING file in the top-level directory.
>  
>  import re
> +from typing import Dict, Optional
>  
>  from .common import c_name
>  from .error import QAPISemError
> +from .parser import QAPIDoc
> +from .source import QAPISourceInfo
> +
> +
> +# Deserialized JSON objects as returned by the parser;
> +# The values of this mapping are not necessary to exhaustively type

Not necessary and also not practical with current mypy.  Correct?

> +# here, because the purpose of this module is to interrogate that type.
> +_JSONObject = Dict[str, object]
>  
>  
>  # Names consist of letters, digits, -, and _, starting with a letter.
> @@ -315,9 +324,20 @@ def check_event(expr, info):
>  
>  def check_exprs(exprs):
>      for expr_elem in exprs:
> -        expr = expr_elem['expr']
> -        info = expr_elem['info']
> -        doc = expr_elem.get('doc')
> +        # Expression
> +        assert isinstance(expr_elem['expr'], dict)

I dislike relaxing OrderedDict to dict here.  I'm going to accept it
anyway, because the difference between the two is going away in 3.7, and
because so far order actually matters only in certain sub-expressions,
not top-level expressions.

> +        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
>  
>          if 'include' in expr:
>              continue

I see you didn't bite on the idea to do less checking here.  Okay.