Re: [PATCH v3 07/16] qapi/expr.py: Add casts in a few select cases

[John Snow]

On 2/24/21 7:32 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > Casts are instructions to the type checker only, they aren't "safe" and
> > should probably be avoided in general. In this case, when we perform
> > type checking on a nested structure, the type of each field does not
> > "stick".
> >
> > We don't need to assert that something is a str if we've already checked
> > that it is -- use a cast instead for these cases.
> >
> > 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 | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index afa6bd07769..f45d6be1f4c 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -15,7 +15,7 @@
> >   # See the COPYING file in the top-level directory.
> >   
> >   import re
> > -from typing import MutableMapping, Optional
> > +from typing import MutableMapping, Optional, cast
> >   
> >   from .common import c_name
> >   from .error import QAPISemError
> > @@ -232,7 +232,7 @@ def check_enum(expr, info):
> >   
> >   
> >   def check_struct(expr, info):
> > -    name = expr['struct']
> > +    name = cast(str, expr['struct'])  # Asserted in check_exprs
> >       members = expr['data']
> >   
>
> I believe you need this only so you can declare check_type()'s
> allow_dict to be Optional[str].  More busy-work around allow_dict...
>
> I'm tempted to ask for allow_dict: Any and call it a day.

You're right, this is because of the signature for the allow_dict 
argument. Ultimately, the pragma is a collection of strings and we need 
to prove we are looking up a string somewhere or other.

(Or tell the type checker to leave us alone.)

Unfortunately, the "check" in check_exprs falls off almost immediately. 
Working with dictly-typed objects is a little annoying for this reason.

This works for now; but finding a better way to do the pragma checks is 
probably the more correct thing. (And not something I really want to try 
and get through review until we're done typing, I think.)

> >       check_type(members, info, "'data'", allow_dict=name)
> > @@ -240,7 +240,7 @@ def check_struct(expr, info):
> >   
> >   
> >   def check_union(expr, info):
> > -    name = expr['union']
> > +    name = cast(str, expr['union'])  # Asserted in check_exprs
> >       base = expr.get('base')
> >       discriminator = expr.get('discriminator')
> >       members = expr['data']
>
> Likwewise.
>
> > @@ -337,7 +337,7 @@ def check_exprs(exprs):
> >           else:
> >               raise QAPISemError(info, "expression is missing metatype")
> >   
> > -        name = expr[meta]
> > +        name = cast(str, expr[meta])  # Asserted right below:
>
> "Checked", not "asserted".

Oh, yes.

> >           check_name_is_str(name, info, "'%s'" % meta)
> >           info.set_defn(meta, name)
> >           check_defn_name_str(name, info, meta)
>
>
> Cast before check gives me a queasy feeling.  It's lying to the type
> checker.
>
> Hamfisted way to avoid lying:
>
>             check_name_is_str(expr[meta], ...)
>             name = cast(str, expr[meta])
>
> Something like
>
>             name = check_name_is_str(name, ...)
>
> might be neater, but then we'd have to find a better function name.

OK, I'll look into re-ordering this.