| Message ID | 1455778109-6278-4-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > The whole point of an alternate is to allow some type-safety while > still accepting more than one JSON type. Meanwhile, the 'any' > type exists to bypass type-safety altogether. The two are > incompatible: you can't accept every type, and still tell which > branch of the alternate to use for the parse; fix this to give a > sane error instead of a Python stack trace. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v11: new patch > --- > scripts/qapi.py | 5 ++++- > tests/Makefile | 1 + > tests/qapi-schema/alternate-any.err | 1 + > tests/qapi-schema/alternate-any.exit | 1 + > tests/qapi-schema/alternate-any.json | 4 ++++ > tests/qapi-schema/alternate-any.out | 0 > 6 files changed, 11 insertions(+), 1 deletion(-) > create mode 100644 tests/qapi-schema/alternate-any.err > create mode 100644 tests/qapi-schema/alternate-any.exit > create mode 100644 tests/qapi-schema/alternate-any.json > create mode 100644 tests/qapi-schema/alternate-any.out > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index f97236f..17bf633 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -629,7 +629,10 @@ def check_alternate(expr, expr_info): > value, > allow_metas=['built-in', 'union', 'struct', 'enum']) > qtype = find_alternate_member_qtype(value) > - assert qtype > + if not qtype: > + raise QAPIExprError(expr_info, > + "Alternate '%s' member '%s' cannot use " > + "type '%s'" % (name, key, value)) > if qtype in types_seen: > raise QAPIExprError(expr_info, > "Alternate '%s' member '%s' can't " Interestingly, find_alternate_member_qtype(T) can return None in two ways: when builtin_types[T] is None (only for T == 'any'), and when T is neither built-in, struct, enum or union (it must be alternate then). This leads to the question whether this patch catches exactly 'any', as the commit message claims, or alternate as well. > diff --git a/tests/Makefile b/tests/Makefile > index c1c605f..7c66d16 100644 > --- a/tests/Makefile > +++ b/tests/Makefile > @@ -241,6 +241,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y) > > check-qtest-generic-y += tests/qom-test$(EXESUF) > > +qapi-schema += alternate-any.json > qapi-schema += alternate-array.json > qapi-schema += alternate-base.json > qapi-schema += alternate-clash.json > diff --git a/tests/qapi-schema/alternate-any.err b/tests/qapi-schema/alternate-any.err > new file mode 100644 > index 0000000..aaa0154 > --- /dev/null > +++ b/tests/qapi-schema/alternate-any.err > @@ -0,0 +1 @@ > +tests/qapi-schema/alternate-any.json:2: Alternate 'Alt' member 'one' cannot use type 'any' > diff --git a/tests/qapi-schema/alternate-any.exit b/tests/qapi-schema/alternate-any.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/alternate-any.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/alternate-any.json b/tests/qapi-schema/alternate-any.json > new file mode 100644 > index 0000000..e47a73a > --- /dev/null > +++ b/tests/qapi-schema/alternate-any.json > @@ -0,0 +1,4 @@ > +# we do not allow the 'any' type as an alternate branch > +{ 'alternate': 'Alt', > + 'data': { 'one': 'any', > + 'two': 'int' } } > diff --git a/tests/qapi-schema/alternate-any.out b/tests/qapi-schema/alternate-any.out > new file mode 100644 > index 0000000..e69de29 Could use a test for alternate member of alternate type.
On 02/18/2016 05:05 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> The whole point of an alternate is to allow some type-safety while >> still accepting more than one JSON type. Meanwhile, the 'any' >> type exists to bypass type-safety altogether. The two are >> incompatible: you can't accept every type, and still tell which >> branch of the alternate to use for the parse; fix this to give a >> sane error instead of a Python stack trace. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> > > Interestingly, find_alternate_member_qtype(T) can return None in two > ways: when builtin_types[T] is None (only for T == 'any'), and when T is > neither built-in, struct, enum or union (it must be alternate then). > > This leads to the question whether this patch catches exactly 'any', as > the commit message claims, or alternate as well. > >> >> @@ -629,7 +629,10 @@ def check_alternate(expr, expr_info): >> value, >> allow_metas=['built-in', 'union', 'struct', 'enum']) >> qtype = find_alternate_member_qtype(value) > > > Could use a test for alternate member of alternate type. One step ahead of you: commit 3d0c4829 added the test alternate-nested.json, and commits 44bd1276 and dd883c6f fixed the parser to reject it (first by a hard-coded check, then via allow_metas[] excluding alternates). 'any' is the only value that could sneak through, because it is a subset of 'built-in' which allow_metas[] whitelisted. If you want to squash any of this information into the commit message, though, I don't mind.
Eric Blake <eblake@redhat.com> writes: > On 02/18/2016 05:05 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> The whole point of an alternate is to allow some type-safety while >>> still accepting more than one JSON type. Meanwhile, the 'any' >>> type exists to bypass type-safety altogether. The two are >>> incompatible: you can't accept every type, and still tell which >>> branch of the alternate to use for the parse; fix this to give a >>> sane error instead of a Python stack trace. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> > >> >> Interestingly, find_alternate_member_qtype(T) can return None in two >> ways: when builtin_types[T] is None (only for T == 'any'), and when T is >> neither built-in, struct, enum or union (it must be alternate then). >> >> This leads to the question whether this patch catches exactly 'any', as >> the commit message claims, or alternate as well. >> > >>> >>> @@ -629,7 +629,10 @@ def check_alternate(expr, expr_info): >>> value, >>> allow_metas=['built-in', 'union', 'struct', 'enum']) >>> qtype = find_alternate_member_qtype(value) >> > >> >> Could use a test for alternate member of alternate type. > > One step ahead of you: commit 3d0c4829 added the test > alternate-nested.json, and commits 44bd1276 and dd883c6f fixed the > parser to reject it (first by a hard-coded check, then via allow_metas[] > excluding alternates). 'any' is the only value that could sneak > through, because it is a subset of 'built-in' which allow_metas[] > whitelisted. Then find_alternate_member_qtype()'s final return None is unreachable, correct? > If you want to squash any of this information into the commit message, > though, I don't mind. I'll consider it when I apply.
diff --git a/scripts/qapi.py b/scripts/qapi.py index f97236f..17bf633 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -629,7 +629,10 @@ def check_alternate(expr, expr_info): value, allow_metas=['built-in', 'union', 'struct', 'enum']) qtype = find_alternate_member_qtype(value) - assert qtype + if not qtype: + raise QAPIExprError(expr_info, + "Alternate '%s' member '%s' cannot use " + "type '%s'" % (name, key, value)) if qtype in types_seen: raise QAPIExprError(expr_info, "Alternate '%s' member '%s' can't " diff --git a/tests/Makefile b/tests/Makefile index c1c605f..7c66d16 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -241,6 +241,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y) check-qtest-generic-y += tests/qom-test$(EXESUF) +qapi-schema += alternate-any.json qapi-schema += alternate-array.json qapi-schema += alternate-base.json qapi-schema += alternate-clash.json diff --git a/tests/qapi-schema/alternate-any.err b/tests/qapi-schema/alternate-any.err new file mode 100644 index 0000000..aaa0154 --- /dev/null +++ b/tests/qapi-schema/alternate-any.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-any.json:2: Alternate 'Alt' member 'one' cannot use type 'any' diff --git a/tests/qapi-schema/alternate-any.exit b/tests/qapi-schema/alternate-any.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/alternate-any.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/alternate-any.json b/tests/qapi-schema/alternate-any.json new file mode 100644 index 0000000..e47a73a --- /dev/null +++ b/tests/qapi-schema/alternate-any.json @@ -0,0 +1,4 @@ +# we do not allow the 'any' type as an alternate branch +{ 'alternate': 'Alt', + 'data': { 'one': 'any', + 'two': 'int' } }
The whole point of an alternate is to allow some type-safety while still accepting more than one JSON type. Meanwhile, the 'any' type exists to bypass type-safety altogether. The two are incompatible: you can't accept every type, and still tell which branch of the alternate to use for the parse; fix this to give a sane error instead of a Python stack trace. Signed-off-by: Eric Blake <eblake@redhat.com> --- v11: new patch --- scripts/qapi.py | 5 ++++- tests/Makefile | 1 + tests/qapi-schema/alternate-any.err | 1 + tests/qapi-schema/alternate-any.exit | 1 + tests/qapi-schema/alternate-any.json | 4 ++++ tests/qapi-schema/alternate-any.out | 0 6 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 tests/qapi-schema/alternate-any.err create mode 100644 tests/qapi-schema/alternate-any.exit create mode 100644 tests/qapi-schema/alternate-any.json create mode 100644 tests/qapi-schema/alternate-any.out diff --git a/tests/qapi-schema/alternate-any.out b/tests/qapi-schema/alternate-any.out new file mode 100644 index 0000000..e69de29