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

[John Snow]

On 9/24/20 8:05 PM, Cleber Rosa wrote:
> On Wed, Sep 23, 2020 at 03:42:24PM -0400, Eduardo Habkost wrote:
> > On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
> > > 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 | 25 ++++++++++++++++++++++---
> > >   1 file changed, 22 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > > index 1872a8a3cc..f6b55a87c1 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]
> > >   
> > >   
> > >   # Names must be letters, numbers, -, and _.  They must start with letter,
> > > @@ -280,9 +288,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)
> > > +        expr: Expression = expr_elem['expr']
> > > +        for key in expr.keys():
> > > +            assert isinstance(key, str)
> >
> > mypy doesn't seem to require the key type asserts, on my testing.
>
> Do you mean that mypy actually takes notice of the type assert?  And
> includes that as source of information for the type check or am I
> misinterpreting you?
>
> BTW, what I understood from this assert is that a more specific
> type than the MutableMapping is desirable here.  Did I get that
> right John?

Yes, we do want a more specific type. We'll get one somewhere in part 5 
when parser.py gets a workout.

> - Cleber.

mypy takes notice of assert isinstance(x, FooType) because below this 
line, it is not possible for x to be anything other than a FooType.

You can use this to "downcast" types.

you can use cast() too, but those are "unsafe", in that they don't 
actually check. assert *will* check.

You can also constrain types by doing a simple:

if isinstance(x, FooType):
    x.FooMethod()