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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5025 bytes --]

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 <armbru@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.


> +++ 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 <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]