John Snow <js...@redhat.com> writes:
> On 2/24/21 7:32 AM, Markus Armbruster wrote:
>> John Snow <js...@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 <js...@redhat.com>
>>> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
>>> Reviewed-by: Cleber Rosa <cr...@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 afa6bd07769..f45d6be1f4c 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']
>>>
>>
>> I believe you need this only so you can declare check_type()'s
>> allow_dict to be Optional[str]. More busy-work around allow_dict...
>>
>> I'm tempted to ask for allow_dict: Any and call it a day.
>>
>
> You're right, this is because of the signature for the allow_dict
> argument. Ultimately, the pragma is a collection of strings and we need
> to prove we are looking up a string somewhere or other.
>
> (Or tell the type checker to leave us alone.)
>
> Unfortunately, the "check" in check_exprs falls off almost immediately.
What do you mean by "falls off"?
> Working with dictly-typed objects is a little annoying for this reason.
>
> This works for now; but finding a better way to do the pragma checks is
> probably the more correct thing. (And not something I really want to try
> and get through review until we're done typing, I think.)
Yes, there's probably a neater way to do the name case checking (with
pragma-directed exceptions), and yes, we should refrain from looking for
it right now.
>>> 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']
>>
>> Likwewise.
>>
>>> @@ -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:
>>
>> "Checked", not "asserted".
>>
>
> Oh, yes.
>
>>> check_name_is_str(name, info, "'%s'" % meta)
>>> info.set_defn(meta, name)
>>> check_defn_name_str(name, info, meta)
>>
>>
>> Cast before check gives me a queasy feeling. It's lying to the type
>> checker.
>>
>> Hamfisted way to avoid lying:
>>
>> check_name_is_str(expr[meta], ...)
>> name = cast(str, expr[meta])
>>
>> Something like
>>
>> name = check_name_is_str(name, ...)
>>
>> might be neater, but then we'd have to find a better function name.
>>
>
> OK, I'll look into re-ordering this.
Thanks! To avoid misunderstandings: ham-fisted would do for now.
