diff mbox series

[v2,07/16] qapi/expr.py: Add casts in a few select cases

Message ID 20201026213637.47087-8-jsnow@redhat.com
State New
Headers show
Series qapi: static typing conversion, pt3 | expand

Commit Message

John Snow Oct. 26, 2020, 9:36 p.m. UTC
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(-)

Comments

Markus Armbruster Nov. 18, 2020, 3:41 p.m. UTC | #1
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 2654a72e8333..b8720b723377 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

Suggest # check_exprs() ensures this

>      members = expr['data']
>  
>      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

Likewise.

>      base = expr.get('base')
>      discriminator = expr.get('discriminator')
>      members = expr['data']
> @@ -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:
>          check_name_is_str(name, info, "'%s'" % meta)

Uh, why do we need to cast then?

It's not actually "asserted".  check_name_is_str() ensures str by
raising an error if not str.

>          info.set_defn(meta, name)
>          check_defn_name_str(name, info, meta)
diff mbox series

Patch

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 2654a72e8333..b8720b723377 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']
 
     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']
@@ -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:
         check_name_is_str(name, info, "'%s'" % meta)
         info.set_defn(meta, name)
         check_defn_name_str(name, info, meta)