Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com>
>> ---
>
>> 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 <ebl...@redhat.com>
Thanks!
