On 4/29/21 9:40 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Modify check_if() to build an IfPredicate tree (the schema
> documentation is updated in a following patch).
>
I'm wondering if check_if() is the right place to do this. It's
certainly convenient, but we don't build any other domain-specific types
here at all -- that all happens in schema.py.
Ok, I moved the conversion to schema.py, like other _make_foo() there
Before this patch, the return value from expr.py is conceivably
something you'd get "exactly as-is" from a JSON parser. This patch would
end that, and collapses the waveform.
I think we should build a function that turns the raw (or slightly
normalized) 'ifcond' AST into the IfPredicate object like we do for
other structures, like Members, Features, etc.
There is now a _make_if() to convert the raw json to SchemaIfCond.
I'd also like the documentation changes to eventually be squashed
directly into this patch if it changes syntax, but keeping it separate
during review makes sense.
Tentatively, I think the expanded "IF" syntax makes sense.
'if': 'COND'
'if': ['COND']
'if': { 'any': ['COND'] }
Actually, a simple list is short form for { 'all': [] }
Seems fine. I want to play around a little bit with a JsonSchema for it
though to make sure that it's something I can provide good IntelliSense
tooltips for in e.g. vscode. (A bit of a pipe-dream side project, I
admit, but if you'll humor me I'd like the chance to give it a shot.
Some constructs are simply easier to type and validate than others.)
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Tested-by: John Snow <jsnow@redhat.com>
> ---
> tests/unit/test-qmp-cmds.c | 1 +
> scripts/qapi/expr.py | 62 ++++++++++++++-----
> scripts/qapi/schema.py | 13 +---
> tests/qapi-schema/bad-if.err | 3 +-
> tests/qapi-schema/doc-good.out | 12 ++--
> tests/qapi-schema/enum-if-invalid.err | 3 +-
> tests/qapi-schema/features-if-invalid.err | 2 +-
> tests/qapi-schema/qapi-schema-test.json | 20 +++---
> tests/qapi-schema/qapi-schema-test.out | 59 ++++++++++--------
> .../qapi-schema/struct-member-if-invalid.err | 2 +-
> 10 files changed, 106 insertions(+), 71 deletions(-)
>
> diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
> index 1b0b7d99df..83efa39720 100644
> --- a/tests/unit/test-qmp-cmds.c
> +++ b/tests/unit/test-qmp-cmds.c
> @@ -51,6 +51,7 @@ FeatureStruct1 *qmp_test_features0(bool has_fs0, FeatureStruct0 *fs0,
> bool has_cfs1, CondFeatureStruct1 *cfs1,
> bool has_cfs2, CondFeatureStruct2 *cfs2,
> bool has_cfs3, CondFeatureStruct3 *cfs3,
> + bool has_cfs4, CondFeatureStruct4 *cfs4,
> Error **errp)
> {
> return g_new0(FeatureStruct1, 1);
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 496f7e0333..0a97a6f020 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -42,7 +42,14 @@
> cast,
> )
>
> -from .common import c_name
> +from .common import (
> + IfAll,
> + IfAny,
> + IfNot,
> + IfOption,
> + IfPredicate,
> + c_name,
> +)
> from .error import QAPISemError
> from .parser import QAPIDoc
> from .source import QAPISourceInfo
> @@ -261,6 +268,10 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
> """
> Normalize and validate the ``if`` member of an object.
>
> + The ``if`` field may be either a ``str``, a ``List[str]`` or a dict.
> + A ``str`` element or a ``List[str]`` will be normalized to
> + ``IfAll([str])``.
> +
> The ``if`` member may be either a ``str`` or a ``List[str]``.
> A ``str`` value will be normalized to ``List[str]``.
>
> @@ -281,25 +292,44 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
> if ifcond is None:
> return
>
> - if isinstance(ifcond, list):
> - if not ifcond:
> - raise QAPISemError(
> - info, "'if' condition [] of %s is useless" % source)
> - else:
> - # Normalize to a list
> - ifcond = expr['if'] = [ifcond]
> -
> - for elt in ifcond:
> - if not isinstance(elt, str):
> + def normalize(cond: Union[str, List[str], object]) -> IfPredicate:
> + if isinstance(cond, str):
> + if not cond.strip():
> + raise QAPISemError(
> + info,
> + "'if' condition '%s' of %s makes no sense"
> + % (cond, source))
> + return IfOption(cond)
> + if isinstance(cond, list):
> + cond = {"all": cond}
> + if not isinstance(cond, dict):
> raise QAPISemError(
> info,
> - "'if' condition of %s must be a string or a list of strings"
> - % source)
> - if not elt.strip():
> + "'if' condition of %s must be a string, "
> + "a list of strings or a dict" % source)
> + if len(cond) != 1:
> raise QAPISemError(
> info,
> - "'if' condition '%s' of %s makes no sense"
> - % (elt, source))
> + "'if' condition dict of %s must have one key: "
> + "'all', 'any' or 'not'" % source)
> + check_keys(cond, info, "'if' condition", [],
> + ["all", "any", "not"])
> + oper, operands = next(iter(cond.items()))
> + if oper == "not":
> + return IfNot(normalize(operands))
> + if not operands:
> + raise QAPISemError(
> + info, "'if' condition [] of %s is useless" % source)
> + if not isinstance(operands, list):
> + raise QAPISemError(
> + info, "'%s' condition of %s must be a list" % (oper, source))
> + operands = [normalize(o) for o in operands]
> + return IfAll(operands) if oper == "all" else IfAny(operands)
> +
> + ifcond = expr.get('if')
> + if ifcond is None:
> + return
> + expr['if'] = normalize(ifcond)
>
>
> def normalize_members(members: object) -> None:
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 366a53ab64..61664a4c5e 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -19,22 +19,15 @@
> import re
> from typing import Optional
>
> -from .common import (
> - POINTER_SUFFIX,
> - IfAll,
> - IfOption,
> - c_name,
> - mcgen,
> -)
> +from .common import POINTER_SUFFIX, c_name, mcgen
> from .error import QAPISemError, QAPISourceError
> from .expr import check_exprs
> from .parser import QAPISchemaParser
>
>
> class QAPISchemaIfCond:
> - def __init__(self, ifcond=None):
> - pred_list = [IfOption(opt) for opt in ifcond or []]
> - self.pred = IfAll(pred_list)
> + def __init__(self, pred=None):
> + self.pred = pred
>
> def gen_doc(self):
> if self.pred:
> diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.err
> index f83dee65da..454fbae387 100644
> --- a/tests/qapi-schema/bad-if.err
> +++ b/tests/qapi-schema/bad-if.err
> @@ -1,2 +1,3 @@
> bad-if.json: In struct 'TestIfStruct':
> -bad-if.json:2: 'if' condition of struct must be a string or a list of strings
> +bad-if.json:2: 'if' condition has unknown key 'value'
> +Valid keys are 'all', 'any', 'not'.
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 6bf996f539..ca7e53f3b5 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -12,15 +12,15 @@ enum QType
> module doc-good.json
> enum Enum
> member one
> - if IfAll(['defined(IFONE)'])
> + if 'defined(IFONE)'
> member two
> - if IfAll(['defined(IFCOND)'])
> + if 'defined(IFCOND)'
> feature enum-feat
> object Base
> member base1: Enum optional=False
> object Variant1
> member var1: str optional=False
> - if IfAll(['defined(IFSTR)'])
> + if 'defined(IFSTR)'
> feature member-feat
> feature variant1-feat
> object Variant2
> @@ -29,7 +29,7 @@ object Object
> tag base1
> case one: Variant1
> case two: Variant2
> - if IfAll(['IFTWO'])
> + if 'IFTWO'
> feature union-feat1
> object q_obj_Variant1-wrapper
> member data: Variant1 optional=False
> @@ -38,13 +38,13 @@ object q_obj_Variant2-wrapper
> enum SugaredUnionKind
> member one
> member two
> - if IfAll(['IFTWO'])
> + if 'IFTWO'
> object SugaredUnion
> member type: SugaredUnionKind optional=False
> tag type
> case one: q_obj_Variant1-wrapper
> case two: q_obj_Variant2-wrapper
> - if IfAll(['IFTWO'])
> + if 'IFTWO'
> feature union-feat2
> alternate Alternate
> tag type
> diff --git a/tests/qapi-schema/enum-if-invalid.err b/tests/qapi-schema/enum-if-invalid.err
> index 0556dc967b..3bb84075a9 100644
> --- a/tests/qapi-schema/enum-if-invalid.err
> +++ b/tests/qapi-schema/enum-if-invalid.err
> @@ -1,2 +1,3 @@
> enum-if-invalid.json: In enum 'TestIfEnum':
> -enum-if-invalid.json:2: 'if' condition of 'data' member 'bar' must be a string or a list of strings
> +enum-if-invalid.json:2: 'if' condition has unknown key 'val'
> +Valid keys are 'all', 'any', 'not'.
> diff --git a/tests/qapi-schema/features-if-invalid.err b/tests/qapi-schema/features-if-invalid.err
> index f63b89535e..724a810086 100644
> --- a/tests/qapi-schema/features-if-invalid.err
> +++ b/tests/qapi-schema/features-if-invalid.err
> @@ -1,2 +1,2 @@
> features-if-invalid.json: In struct 'Stru':
> -features-if-invalid.json:2: 'if' condition of 'features' member 'f' must be a string or a list of strings
> +features-if-invalid.json:2: 'if' condition of 'features' member 'f' must be a string, a list of strings or a dict
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 84b9d41f15..2d5e480b44 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -231,8 +231,8 @@
>
> { 'union': 'TestIfUnion', 'data':
> { 'foo': 'TestStruct',
> - 'bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} },
> - 'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' }
> + 'union-bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} },
> + 'if': ['defined(TEST_IF_UNION)', 'defined(TEST_IF_STRUCT)'] }
>
> { 'command': 'test-if-union-cmd',
> 'data': { 'union-cmd-arg': 'TestIfUnion' },
> @@ -241,11 +241,10 @@
> { 'alternate': 'TestIfAlternate', 'data':
> { 'foo': 'int',
> 'bar': { 'type': 'TestStruct', 'if': 'defined(TEST_IF_ALT_BAR)'} },
> - 'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
> + 'if': {'all': ['defined(TEST_IF_ALT)', 'defined(TEST_IF_STRUCT)'] } }
>
> -{ 'command': 'test-if-alternate-cmd',
> - 'data': { 'alt-cmd-arg': 'TestIfAlternate' },
> - 'if': 'defined(TEST_IF_ALT)' }
> +{ 'command': 'test-if-alternate-cmd', 'data': { 'alt-cmd-arg': 'TestIfAlternate' },
> + 'if': {'all': ['defined(TEST_IF_ALT)', {'not': 'defined(TEST_IF_NOT_ALT)'}] } }
>
> { 'command': 'test-if-cmd',
> 'data': {
> @@ -259,7 +258,7 @@
> { 'event': 'TEST_IF_EVENT', 'data':
> { 'foo': 'TestIfStruct',
> 'bar': { 'type': ['TestIfEnum'], 'if': 'defined(TEST_IF_EVT_BAR)' } },
> - 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
> + 'if': ['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'] }
>
> # test 'features'
>
> @@ -290,6 +289,10 @@
> 'data': { 'foo': 'int' },
> 'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)',
> 'defined(TEST_IF_COND_2)'] } ] }
> +{ 'struct': 'CondFeatureStruct4',
> + 'data': { 'foo': 'int' },
> + 'features': [ { 'name': 'feature1', 'if': {'any': ['defined(TEST_IF_COND_1)',
> + 'defined(TEST_IF_COND_2)'] } } ] }
>
> { 'enum': 'FeatureEnum1',
> 'data': [ 'eins', 'zwei', 'drei' ],
> @@ -313,7 +316,8 @@
> '*fs4': 'FeatureStruct4',
> '*cfs1': 'CondFeatureStruct1',
> '*cfs2': 'CondFeatureStruct2',
> - '*cfs3': 'CondFeatureStruct3' },
> + '*cfs3': 'CondFeatureStruct3',
> + '*cfs4': 'CondFeatureStruct4' },
> 'returns': 'FeatureStruct1',
> 'features': [] }
>
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index c2d303aa18..f859bf648d 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -298,49 +298,49 @@ command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Unio
> object TestIfStruct
> member foo: int optional=False
> member bar: int optional=False
> - if IfAll(['defined(TEST_IF_STRUCT_BAR)'])
> - if IfAll(['defined(TEST_IF_STRUCT)'])
> + if 'defined(TEST_IF_STRUCT_BAR)'
> + if 'defined(TEST_IF_STRUCT)'
> enum TestIfEnum
> member foo
> member bar
> - if IfAll(['defined(TEST_IF_ENUM_BAR)'])
> - if IfAll(['defined(TEST_IF_ENUM)'])
> + if 'defined(TEST_IF_ENUM_BAR)'
> + if 'defined(TEST_IF_ENUM)'
> object q_obj_TestStruct-wrapper
> member data: TestStruct optional=False
> enum TestIfUnionKind
> member foo
> - member bar
> - if IfAll(['defined(TEST_IF_UNION_BAR)'])
> - if IfAll(['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)'])
> + member union-bar
> + if 'defined(TEST_IF_UNION_BAR)'
> + if IfAll(['defined(TEST_IF_UNION)', 'defined(TEST_IF_STRUCT)'])
> object TestIfUnion
> member type: TestIfUnionKind optional=False
> tag type
> case foo: q_obj_TestStruct-wrapper
> - case bar: q_obj_str-wrapper
> - if IfAll(['defined(TEST_IF_UNION_BAR)'])
> - if IfAll(['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)'])
> + case union-bar: q_obj_str-wrapper
> + if 'defined(TEST_IF_UNION_BAR)'
> + if IfAll(['defined(TEST_IF_UNION)', 'defined(TEST_IF_STRUCT)'])
> object q_obj_test-if-union-cmd-arg
> member union-cmd-arg: TestIfUnion optional=False
> - if IfAll(['defined(TEST_IF_UNION)'])
> + if 'defined(TEST_IF_UNION)'
> command test-if-union-cmd q_obj_test-if-union-cmd-arg -> None
> gen=True success_response=True boxed=False oob=False preconfig=False
> - if IfAll(['defined(TEST_IF_UNION)'])
> + if 'defined(TEST_IF_UNION)'
> alternate TestIfAlternate
> tag type
> case foo: int
> case bar: TestStruct
> - if IfAll(['defined(TEST_IF_ALT_BAR)'])
> - if IfAll(['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)'])
> + if 'defined(TEST_IF_ALT_BAR)'
> + if IfAll(['defined(TEST_IF_ALT)', 'defined(TEST_IF_STRUCT)'])
> object q_obj_test-if-alternate-cmd-arg
> member alt-cmd-arg: TestIfAlternate optional=False
> - if IfAll(['defined(TEST_IF_ALT)'])
> + if IfAll(['defined(TEST_IF_ALT)', IfNot('defined(TEST_IF_NOT_ALT)')])
> command test-if-alternate-cmd q_obj_test-if-alternate-cmd-arg -> None
> gen=True success_response=True boxed=False oob=False preconfig=False
> - if IfAll(['defined(TEST_IF_ALT)'])
> + if IfAll(['defined(TEST_IF_ALT)', IfNot('defined(TEST_IF_NOT_ALT)')])
> object q_obj_test-if-cmd-arg
> member foo: TestIfStruct optional=False
> member bar: TestIfEnum optional=False
> - if IfAll(['defined(TEST_IF_CMD_BAR)'])
> + if 'defined(TEST_IF_CMD_BAR)'
> if IfAll(['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'])
> command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree
> gen=True success_response=True boxed=False oob=False preconfig=False
> @@ -348,15 +348,15 @@ command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree
> command test-cmd-return-def-three None -> UserDefThree
> gen=True success_response=True boxed=False oob=False preconfig=False
> array TestIfEnumList TestIfEnum
> - if IfAll(['defined(TEST_IF_ENUM)'])
> + if 'defined(TEST_IF_ENUM)'
> object q_obj_TEST_IF_EVENT-arg
> member foo: TestIfStruct optional=False
> member bar: TestIfEnumList optional=False
> - if IfAll(['defined(TEST_IF_EVT_BAR)'])
> - if IfAll(['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)'])
> + if 'defined(TEST_IF_EVT_BAR)'
> + if IfAll(['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'])
> event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
> boxed=False
> - if IfAll(['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)'])
> + if IfAll(['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'])
> object FeatureStruct0
> member foo: int optional=False
> object FeatureStruct1
> @@ -379,17 +379,21 @@ object FeatureStruct4
> object CondFeatureStruct1
> member foo: int optional=False
> feature feature1
> - if IfAll(['defined(TEST_IF_FEATURE_1)'])
> + if 'defined(TEST_IF_FEATURE_1)'
> object CondFeatureStruct2
> member foo: int optional=False
> feature feature1
> - if IfAll(['defined(TEST_IF_FEATURE_1)'])
> + if 'defined(TEST_IF_FEATURE_1)'
> feature feature2
> - if IfAll(['defined(TEST_IF_FEATURE_2)'])
> + if 'defined(TEST_IF_FEATURE_2)'
> object CondFeatureStruct3
> member foo: int optional=False
> feature feature1
> if IfAll(['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'])
> +object CondFeatureStruct4
> + member foo: int optional=False
> + feature feature1
> + if IfAny(['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'])
> enum FeatureEnum1
> member eins
> member zwei
> @@ -417,6 +421,7 @@ object q_obj_test-features0-arg
> member cfs1: CondFeatureStruct1 optional=True
> member cfs2: CondFeatureStruct2 optional=True
> member cfs3: CondFeatureStruct3 optional=True
> + member cfs4: CondFeatureStruct4 optional=True
> command test-features0 q_obj_test-features0-arg -> FeatureStruct1
> gen=True success_response=True boxed=False oob=False preconfig=False
> command test-command-features1 None -> None
> @@ -429,13 +434,13 @@ command test-command-features3 None -> None
> command test-command-cond-features1 None -> None
> gen=True success_response=True boxed=False oob=False preconfig=False
> feature feature1
> - if IfAll(['defined(TEST_IF_FEATURE_1)'])
> + if 'defined(TEST_IF_FEATURE_1)'
> command test-command-cond-features2 None -> None
> gen=True success_response=True boxed=False oob=False preconfig=False
> feature feature1
> - if IfAll(['defined(TEST_IF_FEATURE_1)'])
> + if 'defined(TEST_IF_FEATURE_1)'
> feature feature2
> - if IfAll(['defined(TEST_IF_FEATURE_2)'])
> + if 'defined(TEST_IF_FEATURE_2)'
> command test-command-cond-features3 None -> None
> gen=True success_response=True boxed=False oob=False preconfig=False
> feature feature1
> diff --git a/tests/qapi-schema/struct-member-if-invalid.err b/tests/qapi-schema/struct-member-if-invalid.err
> index 42e7fdae3c..c18157c1f9 100644
> --- a/tests/qapi-schema/struct-member-if-invalid.err
> +++ b/tests/qapi-schema/struct-member-if-invalid.err
> @@ -1,2 +1,2 @@
> struct-member-if-invalid.json: In struct 'Stru':
> -struct-member-if-invalid.json:2: 'if' condition of 'data' member 'member' must be a string or a list of strings
> +struct-member-if-invalid.json:2: 'if' condition of 'data' member 'member' must be a string, a list of strings or a dict
>