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

From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: Michael Roth <michael.roth@amd.com>,
	Cleber Rosa <crosa@redhat.com>,
	qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>
Subject: Re: [PATCH v4 10/19] qapi/expr.py: Add casts in a few select cases
Date: Thu, 25 Mar 2021 15:33:39 +0100	[thread overview]
Message-ID: <87mturs3p8.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20210325060356.4040114-11-jsnow@redhat.com> (John Snow's message of "Thu, 25 Mar 2021 02:03:47 -0400")

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()"?

>      members = expr['data']
>  
>      check_type(members, info, "'data'", allow_dict=name)
> @@ -267,7 +267,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']
> @@ -366,8 +366,8 @@ def check_exprs(exprs):
>          else:
>              raise QAPISemError(info, "expression is missing metatype")
>  
> -        name = expr[meta]
> -        check_name_is_str(name, info, "'%s'" % meta)
> +        check_name_is_str(expr[meta], info, "'%s'" % meta)
> +        name = cast(str, expr[meta])
>          info.set_defn(meta, name)
>          check_defn_name_str(name, info, meta)