Re: [Qemu-devel] [PATCH for-4.0 v7 08/27] qapi: add a dictionary form with 'name' key for enum members

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Desugar the enum NAME form to { 'name': NAME }. This will allow to add
> new enum members, such as 'if' in the following patch.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi/common.py                        | 49 ++++++++++++++++---
>  tests/Makefile.include                        |  3 +-
>  tests/qapi-schema/enum-bad-member.err         |  1 +
>  ...-dict-member.exit => enum-bad-member.exit} |  0
>  tests/qapi-schema/enum-bad-member.json        |  2 +
>  ...um-dict-member.out => enum-bad-member.out} |  0
>  .../qapi-schema/enum-dict-member-unknown.err  |  2 +
>  .../qapi-schema/enum-dict-member-unknown.exit |  1 +
>  .../qapi-schema/enum-dict-member-unknown.json |  2 +
>  .../qapi-schema/enum-dict-member-unknown.out  |  0
>  tests/qapi-schema/enum-dict-member.err        |  1 -
>  tests/qapi-schema/enum-dict-member.json       |  2 -
>  12 files changed, 52 insertions(+), 11 deletions(-)
>  create mode 100644 tests/qapi-schema/enum-bad-member.err
>  rename tests/qapi-schema/{enum-dict-member.exit => enum-bad-member.exit} (100%)
>  create mode 100644 tests/qapi-schema/enum-bad-member.json
>  rename tests/qapi-schema/{enum-dict-member.out => enum-bad-member.out} (100%)
>  create mode 100644 tests/qapi-schema/enum-dict-member-unknown.err
>  create mode 100644 tests/qapi-schema/enum-dict-member-unknown.exit
>  create mode 100644 tests/qapi-schema/enum-dict-member-unknown.json
>  create mode 100644 tests/qapi-schema/enum-dict-member-unknown.out
>  delete mode 100644 tests/qapi-schema/enum-dict-member.err
>  delete mode 100644 tests/qapi-schema/enum-dict-member.json
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index f205805751..610840d2e5 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -740,6 +740,10 @@ def check_event(expr, info):
>                 allow_metas=meta)
>  
>  
> +def enum_get_names(expr):
> +    return [e['name'] if isinstance(e, dict) else e for e in expr['data']]
> +
> +

Slightly more complicated than v6 because it @expr isn't normalized.
We'll revisit this at [*] below.

Another way to code this would be

       members = normalized_enum_members(e['data'])
       return [m['type'] for m in members]

where

   def normalized_enum_members(members):
       [m if isinstance(m, dict) else {'name': m} for m in members]

which then gets reused in normalize_enum()

   def normalize_enum(expr)
       expr['data'] = normalized_enum_members(expr['data']

Up to you.  We could also change it on top.

>  def check_union(expr, info):
>      name = expr['union']
>      base = expr.get('base')
> @@ -799,7 +803,7 @@ def check_union(expr, info):
>          # If the discriminator names an enum type, then all members
>          # of 'data' must also be members of the enum type.
>          if enum_define:
> -            if key not in enum_define['data']:
> +            if key not in enum_get_names(enum_define):
>                  raise QAPISemError(info,
>                                     "Discriminator value '%s' is not found in "
>                                     "enum '%s'"
> @@ -831,7 +835,7 @@ def check_alternate(expr, info):
>          if qtype == 'QTYPE_QSTRING':
>              enum_expr = enum_types.get(value)
>              if enum_expr:
> -                for v in enum_expr['data']:
> +                for v in enum_get_names(enum_expr):
>                      if v in ['on', 'off']:
>                          conflicting.add('QTYPE_QBOOL')
>                      if re.match(r'[-+0-9.]', v): # lazy, could be tightened
> @@ -847,18 +851,32 @@ def check_alternate(expr, info):
>              types_seen[qt] = key
>  
>  
> +def normalize_enum(expr):
> +    members = expr['data']
> +
> +    # translate short member form to dict form
> +    expr['data'] = [m if isinstance(m, dict) else {'name': m} for m in members]
> +
> +
>  def check_enum(expr, info):
>      name = expr['enum']
> -    members = expr.get('data')
> +    members = expr['data']
>      prefix = expr.get('prefix')
>  
> -    if not isinstance(members, list):
> -        raise QAPISemError(info,
> -                           "Enum '%s' requires an array for 'data'" % name)
>      if prefix is not None and not isinstance(prefix, str):
>          raise QAPISemError(info,
>                             "Enum '%s' requires a string for 'prefix'" % name)
> +
> +    if not isinstance(members, list):
> +        raise QAPISemError(info,
> +                           "Enum '%s' requires an array for 'data'" % name)
> +

Did you move this check down intentionally?

>      for member in members:
> +        if isinstance(member, dict):
> +            source = "dictionary member of enum '%s'" % name
> +            check_known_keys(info, source, member, ['name'], [])
> +            member = member['name']
> +
>          check_name(info, "Member of enum '%s'" % name, member,
>                     enum_member=True)

Again, slightly more complicated than v6 because it now runs before
normalization.  We'll revisit this at [*] below.

>  
> @@ -1011,6 +1029,15 @@ def check_exprs(exprs):
>      return exprs
>  
>  
> +def normalize_exprs(exprs):
> +    for expr_elem in exprs:
> +        expr = expr_elem['expr']
> +        if 'enum' in expr:
> +            normalize_enum(expr)
> +
> +    return exprs
> +
> +
>  #
>  # Schema compiler frontend
>  #
> @@ -1567,6 +1594,7 @@ class QAPISchema(object):
>              f = open(fname, 'r')
>          parser = QAPISchemaParser(f)
>          exprs = check_exprs(parser.exprs)
> +        exprs = normalize_exprs(exprs)
>          self.docs = parser.docs
>          self._entity_list = []
>          self._entity_dict = {}

[*] In v6, you normalized at the beginning of check_exprs().  Now, you
normalize after check_exprs().  Works, although what I suggested in my
review of v6 was to normalize later in check_exprs(), namely right after
check_keys().  I believe that would avoid the slight complications I
noted above.

> @@ -1640,7 +1668,14 @@ class QAPISchema(object):
>                                              qtype_values, 'QTYPE'))
>  
>      def _make_enum_members(self, values):
> -        return [QAPISchemaMember(v) for v in values]
> +        enum = []
> +        for v in values:
> +            if isinstance(v, dict):
> +                name = v['name']
> +            else:
> +                name = v
> +            enum.append(QAPISchemaMember(name))
> +        return enum

Despite normalization to the long form, we still deal with both the long
and the short form here, same as in v6.  I see you addressed this in new
PATCH 11+17.  What's wrong with squashing them into this patch?

>  
>      def _make_implicit_enum_type(self, name, info, ifcond, values):
>          # See also QAPISchemaObjectTypeMember._pretty_owner()
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index fb0b449c02..2e894c1037 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -379,10 +379,11 @@ qapi-schema += double-data.json
>  qapi-schema += double-type.json
>  qapi-schema += duplicate-key.json
>  qapi-schema += empty.json
> +qapi-schema += enum-bad-member.json
>  qapi-schema += enum-bad-name.json
>  qapi-schema += enum-bad-prefix.json
>  qapi-schema += enum-clash-member.json
> -qapi-schema += enum-dict-member.json
> +qapi-schema += enum-dict-member-unknown.json
>  qapi-schema += enum-int-member.json
>  qapi-schema += enum-member-case.json
>  qapi-schema += enum-missing-data.json
> diff --git a/tests/qapi-schema/enum-bad-member.err b/tests/qapi-schema/enum-bad-member.err
> new file mode 100644
> index 0000000000..211db9e6fc
> --- /dev/null
> +++ b/tests/qapi-schema/enum-bad-member.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/enum-bad-member.json:2: Member of enum 'MyEnum' requires a string name
> diff --git a/tests/qapi-schema/enum-dict-member.exit b/tests/qapi-schema/enum-bad-member.exit
> similarity index 100%
> rename from tests/qapi-schema/enum-dict-member.exit
> rename to tests/qapi-schema/enum-bad-member.exit
> diff --git a/tests/qapi-schema/enum-bad-member.json b/tests/qapi-schema/enum-bad-member.json
> new file mode 100644
> index 0000000000..98da6828b4
> --- /dev/null
> +++ b/tests/qapi-schema/enum-bad-member.json
> @@ -0,0 +1,2 @@
> +# we reject any enum member that is not a string
> +{ 'enum': 'MyEnum', 'data': [ [ ] ] }
> diff --git a/tests/qapi-schema/enum-dict-member.out b/tests/qapi-schema/enum-bad-member.out
> similarity index 100%
> rename from tests/qapi-schema/enum-dict-member.out
> rename to tests/qapi-schema/enum-bad-member.out
> diff --git a/tests/qapi-schema/enum-dict-member-unknown.err b/tests/qapi-schema/enum-dict-member-unknown.err
> new file mode 100644
> index 0000000000..76bd0471db
> --- /dev/null
> +++ b/tests/qapi-schema/enum-dict-member-unknown.err
> @@ -0,0 +1,2 @@
> +tests/qapi-schema/enum-dict-member-unknown.json:2: Unknown key 'bad-key' in dictionary member of enum 'MyEnum'
> +Valid keys are 'name'.
> diff --git a/tests/qapi-schema/enum-dict-member-unknown.exit b/tests/qapi-schema/enum-dict-member-unknown.exit
> new file mode 100644
> index 0000000000..d00491fd7e
> --- /dev/null
> +++ b/tests/qapi-schema/enum-dict-member-unknown.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/enum-dict-member-unknown.json b/tests/qapi-schema/enum-dict-member-unknown.json
> new file mode 100644
> index 0000000000..6664c59201
> --- /dev/null
> +++ b/tests/qapi-schema/enum-dict-member-unknown.json
> @@ -0,0 +1,2 @@
> +# we reject any enum member that is not a string or a dict with 'name'
> +{ 'enum': 'MyEnum', 'data': [ { 'name': 'foo', 'bad-key': 'str' } ] }
> diff --git a/tests/qapi-schema/enum-dict-member-unknown.out b/tests/qapi-schema/enum-dict-member-unknown.out
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/qapi-schema/enum-dict-member.err b/tests/qapi-schema/enum-dict-member.err
> deleted file mode 100644
> index 8ca146ea59..0000000000
> --- a/tests/qapi-schema/enum-dict-member.err
> +++ /dev/null
> @@ -1 +0,0 @@
> -tests/qapi-schema/enum-dict-member.json:2: Member of enum 'MyEnum' requires a string name
> diff --git a/tests/qapi-schema/enum-dict-member.json b/tests/qapi-schema/enum-dict-member.json
> deleted file mode 100644
> index 79672e0f09..0000000000
> --- a/tests/qapi-schema/enum-dict-member.json
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -# we reject any enum member that is not a string
> -{ 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] }

I think we still need this one to cover "key 'name' is required in the
long form".