Re: [PATCH 04/14] qapi: Split up check_type()

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On Thu, Mar 16, 2023 at 08:13:15AM +0100, Markus Armbruster wrote:
>> check_type() can check type names, arrays, and implicit struct types.
>> Callers pass flags to select from this menu.  This makes the function
>> somewhat hard to read.  Moreover, a few minor bugs are hiding in
>> there, as we'll see shortly.
>> 
>> Split it into check_type_name(), check_type_name_or_implicit().  Each
>
> You omitted check_type_name_or_array() in this summary

Oops!

>> of them is a copy of the original specialized to a certain set of
>> flags.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi/expr.py | 116 +++++++++++++++++++++++++------------------
>>  1 file changed, 67 insertions(+), 49 deletions(-)
>
>> 
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 59bdd86024..bc04bf34c2 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -333,62 +333,74 @@ def normalize_members(members: object) -> None:
>>              members[key] = {'type': arg}
>>  
>>  
>> -def check_type(value: Optional[object],
>> -               info: QAPISourceInfo,
>> -               source: str,
>> -               allow_array: bool = False,
>> -               allow_dict: Union[bool, str] = False) -> None:
>
> There are few enough callers to see that they do indeed have exactly
> one of (nearly) three call patterns.
>
>> -    """
>> -    Normalize and validate the QAPI type of ``value``.
>> -
>> -    Python types of ``str`` or ``None`` are always allowed.
>> -
>> -    :param value: The value to check.
>> -    :param info: QAPI schema source file information.
>> -    :param source: Error string describing this ``value``.
>> -    :param allow_array:
>> -        Allow a ``List[str]`` of length 1, which indicates an array of
>> -        the type named by the list element.
>> -    :param allow_dict:
>> -        Allow a dict.  Its members can be struct type members or union
>> -        branches.  When the value of ``allow_dict`` is in pragma
>> -        ``member-name-exceptions``, the dict's keys may violate the
>> -        member naming rules.  The dict members are normalized in place.
>> -
>> -    :raise QAPISemError: When ``value`` fails validation.
>> -    :return: None, ``value`` is normalized in-place as needed.
>> -    """
>> +def check_type_name(value: Optional[object],
>> +                    info: QAPISourceInfo, source: str) -> None:
>
> check_type_name() replaces callers that relied on the default for
> allow_array and allow_dict

Yes.

>> +    if value is None:
>
> Loses out on the documentation.  Not sure how much that matters to
> you?

You mean the doc string?

I could copy and specialize it along with the code, but the new function
is so simple...  not sure it's worth explaining.

>> +        return
>> +
>> +    if isinstance(value, str):
>> +        return
>> +
>> +    if isinstance(value, list):
>> +        raise QAPISemError(info, "%s cannot be an array" % source)
>> +
>> +    raise QAPISemError(info, "%s should be a type name" % source)
>> +
>> +
>> +def check_type_name_or_array(value: Optional[object],
>> +                             info: QAPISourceInfo, source: str) -> None:
>
> check_type_name_or_array() replaces all callers that passed
> allow_array=True.

Yes.

>>      if value is None:
>
> Another copy without documentation.

Same doubts.

>>          return
>>  
>> -    # Type name
>>      if isinstance(value, str):
>>          return
>>  
>> -    # Array type
>>      if isinstance(value, list):
>> -        if not allow_array:
>> -            raise QAPISemError(info, "%s cannot be an array" % source)
>>          if len(value) != 1 or not isinstance(value[0], str):
>>              raise QAPISemError(info,
>>                                 "%s: array type must contain single type name" %
>>                                 source)
>>          return
>>  
>> -    # Anonymous type
>> +    raise QAPISemError(info,
>> +                       "%s should be a type name" % source)
>>  
>> -    if not allow_dict:
>> -        raise QAPISemError(info, "%s should be a type name" % source)
>> +
>> +def check_type_name_or_implicit(value: Optional[object],
>> +                                info: QAPISourceInfo, source: str,
>> +                                parent_name: Optional[str]) -> None:
>
> And check_type_name_or_implicit replaces all callers that passed
> allow_dict=str, where str is now the parent_name.

Yes.

>                                                    (Wow, that was an
> odd overload of the parameter name - I like the split version better).

It was less bad than what it replaced :)

Commit 638c4af9310 (qapi: Clean up member name case checking)

> ...
>> @@ -560,10 +572,13 @@ def check_command(expr: QAPIExpression) -> None:
>>      rets = expr.get('returns')
>>      boxed = expr.get('boxed', False)
>>  
>> -    if boxed and args is None:
>> -        raise QAPISemError(expr.info, "'boxed': true requires 'data'")
>> -    check_type(args, expr.info, "'data'", allow_dict=not boxed)
>> -    check_type(rets, expr.info, "'returns'", allow_array=True)
>> +    if boxed:
>> +        if args is None:
>> +            raise QAPISemError(expr.info, "'boxed': true requires 'data'")
>> +        check_type_name(args, expr.info, "'data'")
>> +    else:
>> +        check_type_name_or_implicit(args, expr.info, "'data'", None)
>
> And this use of allow_dict was the weirdest, where it really does fit
> better as calls into two separate functions.
>
> With the fixed commit message, and with or without more function docs,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!