[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 10/19] qapi/expr.py: Add casts in a few select cases
From: |
Markus Armbruster |
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 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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)
- [PATCH v4 05/19] qapi/expr.py: constrain incoming expression types, (continued)
- [PATCH v4 05/19] qapi/expr.py: constrain incoming expression types, John Snow, 2021/03/25
- [PATCH v4 06/19] qapi/expr.py: Add assertion for union type 'check_dict', John Snow, 2021/03/25
- [PATCH v4 08/19] qapi: add tests for invalid 'data' field type, John Snow, 2021/03/25
- [PATCH v4 09/19] qapi/expr.py: Check type of 'data' member, John Snow, 2021/03/25
- [PATCH v4 10/19] qapi/expr.py: Add casts in a few select cases, John Snow, 2021/03/25
- Re: [PATCH v4 10/19] qapi/expr.py: Add casts in a few select cases,
Markus Armbruster <=
- [PATCH v4 12/19] qapi/expr.py: add type hint annotations, John Snow, 2021/03/25
- [PATCH v4 13/19] qapi/expr.py: Consolidate check_if_str calls in check_if, John Snow, 2021/03/25
- [PATCH v4 15/19] qapi/expr.py: enable pylint checks, John Snow, 2021/03/25
- [PATCH v4 17/19] qapi/expr.py: Use tuples instead of lists for static data, John Snow, 2021/03/25
- [PATCH v4 16/19] qapi/expr.py: Add docstrings, John Snow, 2021/03/25
- [PATCH v4 18/19] qapi/expr.py: move related checks inside check_xxx functions, John Snow, 2021/03/25