John Snow <js...@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 <js...@redhat.com> > Reviewed-by: Eduardo Habkost <ehabk...@redhat.com> > Reviewed-by: Cleber Rosa <cr...@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