Re: [PATCH 16/25] qapi: Move context-sensitive checking to the proper place

[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().  The .check() assert check_exprs() did its job.
>> 
>> Time to finish the conversion job.  Move exactly the context-sensitive
>> checks to the .check().  They replace assertions there.  Context-free
>> checks stay put.
>> 
>> Fixes the misleading optional tag error demonstrated by test
>> flat-union-optional-discriminator.
>> 
>> A few other error message improve.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index f5599559ac..ac4c898e51 100644
>> --- a/scripts/qapi/common.py
>
> Thankfully, our large coverage of tests goes a long way to show that the
> conversion is correct.  I didn't notice anything obvious that might have
> been overlooked (we may still find things down the road, but I'm not
> going to hold up this patch trying to find those things).  Meanwhile,
> the conversion from assert to conditionals inside .check() looks complete.

Additional arguments supporting correctness:

* Every deleted check gets added back.

* Every added check replaces an assertion.

>> +++ b/tests/qapi-schema/args-union.err
>> @@ -1,2 +1,2 @@
>>  tests/qapi-schema/args-union.json: In command 'oops':
>> -tests/qapi-schema/args-union.json:3: 'data' for command 'oops' cannot use 
>> union type 'Uni'
>> +tests/qapi-schema/args-union.json:3: command's 'data' can take union type 
>> 'Uni' only with 'boxed': true
>
> This one is definitely nicer.
>
>> +++ b/tests/qapi-schema/flat-union-discriminator-bad-name.err
>> @@ -1,2 +1,2 @@
>>  tests/qapi-schema/flat-union-discriminator-bad-name.json: In union 
>> 'MyUnion':
>> -tests/qapi-schema/flat-union-discriminator-bad-name.json:7: discriminator 
>> of flat union 'MyUnion' uses invalid name '*switch'
>> +tests/qapi-schema/flat-union-discriminator-bad-name.json:6: discriminator 
>> '*switch' is not a member of 'base'
>> diff --git a/tests/qapi-schema/flat-union-discriminator-bad-name.json 
>> b/tests/qapi-schema/flat-union-discriminator-bad-name.json
>> index ea84b75cac..3ae8c06a89 100644
>> --- a/tests/qapi-schema/flat-union-discriminator-bad-name.json
>> +++ b/tests/qapi-schema/flat-union-discriminator-bad-name.json
>> @@ -1,5 +1,4 @@
>>  # discriminator '*switch' isn't a member of base, 'switch' is
>> -# reports "uses invalid name", which is good enough
>>  { 'enum': 'Enum', 'data': [ 'one', 'two' ] }
>>  { 'struct': 'Base',
>>    'data': { '*switch': 'Enum' } }
>
> I find this one to be borderline in quality (if we have '*switch' in the
> base, claiming that '*switch' is not a member of base is confusing until
> you realize that base actually has an optional member named 'switch') -
> but anyone that actually stumbles into this one will probably quickly
> figure out their problem, and we may be revisiting it later anyways when
> we finally include patches for a default discriminator.
>
>> +++ b/tests/qapi-schema/flat-union-optional-discriminator.err
>> @@ -1,2 +1,2 @@
>>  tests/qapi-schema/flat-union-optional-discriminator.json: In union 
>> 'MyUnion':
>> -tests/qapi-schema/flat-union-optional-discriminator.json:7: discriminator 
>> 'switch' is not a member of 'base'
>> +tests/qapi-schema/flat-union-optional-discriminator.json:6: discriminator 
>> member 'switch' of base type 'Base' must not be optional
>> diff --git a/tests/qapi-schema/flat-union-optional-discriminator.json 
>> b/tests/qapi-schema/flat-union-optional-discriminator.json
>> index 143ab23a0d..2e7f766f60 100644
>> --- a/tests/qapi-schema/flat-union-optional-discriminator.json
>> +++ b/tests/qapi-schema/flat-union-optional-discriminator.json
>> @@ -1,5 +1,4 @@
>>  # we require the discriminator to be non-optional
>> -# FIXME reports "discriminator 'switch' is not a member of base struct 
>> 'Base'"
>>  { 'enum': 'Enum', 'data': [ 'one', 'two' ] }
>>  { 'struct': 'Base',
>>    'data': { '*switch': 'Enum' } }
>
> And while the other one is borderline, I agree that this one is better.
>
>> +++ b/tests/qapi-schema/union-unknown.err
>> @@ -1,2 +1,2 @@
>>  tests/qapi-schema/union-unknown.json: In union 'Union':
>> -tests/qapi-schema/union-unknown.json:2: member 'unknown' of union 'Union' 
>> uses unknown type 'MissingType'
>> +tests/qapi-schema/union-unknown.json:2: union uses unknown type 
>> 'MissingType'
>> diff --git a/tests/qapi-schema/union-unknown.json 
>> b/tests/qapi-schema/union-unknown.json
>> index aa7e8143d8..64d3666176 100644
>> --- a/tests/qapi-schema/union-unknown.json
>> +++ b/tests/qapi-schema/union-unknown.json
>> @@ -1,3 +1,3 @@
>>  # we reject a union with unknown type in branch
>>  { 'union': 'Union',
>> -  'data': { 'unknown': 'MissingType' } }
>> +  'data': { 'unknown': ['MissingType'] } }
>> 
>
> And here you covered one more code path by going through an array type.
>
> Overall looks good.
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!