[v4] qapi: static typing conversion, pt3

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

Posted by John Snow 4 years, 10 months ago

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
     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)
 
-- 
2.30.2

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

Posted by Markus Armbruster 4 years, 10 months ago

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)

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

Posted by John Snow 4 years, 10 months ago

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.