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

[John Snow]

On 2/24/21 5:01 AM, Markus Armbruster wrote:
> 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?

I don't know! I referenced this in the cover letter. I cannot remember 
the reason anymore. It had R-Bs on it so I left it alone.

There are some differences, but I no longer remember why I thought they 
applied. Maybe some of my more exploratory work wanted it. Dunno.

> 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?

Terminology problem?

I am using "Expression" to mean very specifically a top-level object as 
returned from parser.py, which *must* be an Object, so it *must* be a 
mapping of str => yaddayadda.

The type as I intended it is Expression = Dict[str, yaddayadda]

where yaddayadda is
Union[int, str, bool, List[yaddayadda], Dict[str, yaddayadda]]
expr.py is what validates the yaddayadda, so there's no point in trying 
to type it further, I think.

Probably worth a better comment.

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

Yep. I can get away with the stricter type here because of how we use 
it, so I did. That's an artifact of it not being recursive and how 
expr.py's entire raison d'etre is using isinstance() checks to 
effectively downcast for us everywhere already.

> Also, PEP 8 comment line length.

Augh.

Is there a way to set emacs mode highlighting in Python such that it 
highlights when I run past the 72-col margin, but only for comments?

I have the general-purpose highlighter on for the 80-col margin.

I'm not familiar with any setting like this for any of the linters or 
pycharm right away either.

> >   
> >   
> >   # 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?

Because this looks like garbage written by a drunkard:

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

> >   
> >           if 'include' in expr:
> >               continue