Re: [PATCH v3 03/16] qapi/expr.py: constrain incoming expression types

[Markus Armbruster]

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>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> ---
>  scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 5694c501fa3..783282b53ce 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -15,9 +15,17 @@
>  # See the COPYING file in the top-level directory.
>  
>  import re
> +from typing import MutableMapping, Optional
>  
>  from .common import c_name
>  from .error import QAPISemError
> +from .parser import QAPIDoc
> +from .source import QAPISourceInfo
> +
> +
> +# Expressions in their raw form are JSON-like structures with arbitrary 
> forms.
> +# Minimally, their top-level form must be a mapping of strings to values.
> +Expression = MutableMapping[str, object]

MutableMapping, fancy.  It's only ever dict.  Why abstract from that?

The use of object is again owed to mypy's inability to do recursive
types.  What we really have here is something like

   Expression = Union[bool, str, dict[str, Expression], list[Expression]]

with the root further constrained to the Union's dict branch.  Spell
that out in a bit more detail, like you did in introspect.py?

Hmm, there you used Any, not object.  I guess that's because mypy lets
you get away with object here, but not there.  Correct?

Also, PEP 8 comment line length.

>  
>  
>  # Names must be letters, numbers, -, and _.  They must start with letter,
> @@ -287,9 +295,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)
> +        for key in expr_elem['expr'].keys():
> +            assert isinstance(key, str)
> +        expr: Expression = expr_elem['expr']

You're fine with repeating exp_elem['expr'] here, and ...

> +
> +        # QAPISourceInfo
> +        assert isinstance(expr_elem['info'], QAPISourceInfo)
> +        info: QAPISourceInfo = expr_elem['info']

... expr_elem['info'] here, but ...

> +
> +        # Optional[QAPIDoc]
> +        tmp = expr_elem.get('doc')
> +        assert tmp is None or isinstance(tmp, QAPIDoc)
> +        doc: Optional[QAPIDoc] = tmp

... you avoid repetition of expr_elem.get('doc') here.  Any particular
reason?

>  
>          if 'include' in expr:
>              continue