Re: [PATCH 01/25] qapi: Tighten QAPISchemaFOO.check() assertions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 9/24/19 8:28 AM, Markus Armbruster wrote:
>> When we introduced the QAPISchema intermediate representation (commit
>> ac88219a6c7), we took a shortcut: we left check_exprs() & friends
>> alone instead of moving semantic checks into the
>> QAPISchemaFOO.check().  check_exprs() still checks and reports errors,
>> and the .check() assert check_exprs() did the job.  There are a few
>> gaps, though.
>> 
>> QAPISchemaArrayType.check() neglects to assert the element type is not
>> an array.  Add the assertion.
>> 
>> QAPISchemaObjectTypeVariants.check() neglects to assert the tag member
>> is not optional.  Add the assertion.
>> 
>> It neglects to assert the tag member is not conditional.  Add the
>> assertion.
>> 
>> It neglects to assert we actually have variants.  Add the assertion.
>> 
>> It asserts the variants are object types, but neglects to assert they
>> don't have variants.  Tighten the assertion.
>> 
>> QAPISchemaObjectTypeVariants.check_clash() has the same issue.
>> However, it can run only after .check().  Delete the assertion instead
>> of tightening it.
>> 
>> QAPISchemaAlternateType.check() neglects to assert the branch types
>> don't conflict.  Fixing that isn't trivial, so add just a TODO comment
>> for now.  It'll be resolved later in this series.
>
> I'm guessing you found these by deleting check_exprs() and seeing what
> failed due to inadequate .check()

The first two or three I found by staring at the code.  The remainder I
found when I moved checks from check_exprs() to .check() [PATCH 16]: any
check that doesn't replace an assertion means the assertion was missing.

>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  scripts/qapi/common.py | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!