Re: [PATCH v4 10/19] qapi/expr.py: Add casts in a few select cases

[John Snow]

On 3/25/21 10:33 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".
> >
> > (See PEP 647 for an example of "type narrowing" that does "stick".
> >   It is available in Python 3.10, so we can't use it yet.)
> >
> > We don't need to assert that something is a str if we've already checked
> > or asserted that it is -- use a cast instead for these cases.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/expr.py | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index ca5ab7bfda..505e67bd21 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 Dict, Optional
> > +from typing import Dict, Optional, cast
> >   
> >   from .common import c_name
> >   from .error import QAPISemError
> > @@ -259,7 +259,7 @@ def check_enum(expr, info):
> >   
> >   
> >   def check_struct(expr, info):
> > -    name = expr['struct']
> > +    name = cast(str, expr['struct'])  # Asserted in check_exprs
>
> "Asserted" suggests an an assert statement.  It's actually the
> check_name_is_str() visible in the last hunk.  What about "Checked in
> check_exprs()" or "Ensured by check_exprs()"?

I missed these. "Checked" is fine.