Re: [Qemu-devel] [PATCH v7 08/14] qapi: Track location that created an implicit type

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> A future patch will move some error checking from the parser
> to the various QAPISchema*.check() methods, which run only
> after parsing completes.  It will thus be possible to create
> a python instance representing an implicit QAPI type that
> parses fine but will fail validation during check().  Since
> all errors have to have an associated 'info' location, we
> need a location to be associated with those implicit types.
> The intuitive info to use is the location of the enclosing
> entity that caused the creation of the implicit type.

Would you like to mention you already added info to the implicit array
types a couple of patches ago?

>
> Note that we do not anticipate builtin types being used in
> an error message (as they are not part of the user's QAPI
> input, the user can't cause a semantic error in their
> behavior), so we exempt those types from requiring info, by
> setting a flag to track the completion of _def_predefineds(),
> and tracking that flag in _def_entity().
>
> No change to the generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: better commit message and comments, fix info assertion to
> use instance flag rather than ugly leaky abstraction static flag
> v6: improve commit message, track implicit enum info, rebase
> on new lazy array handling
> ---
>  scripts/qapi.py | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index f5040da..e49f72b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -795,9 +795,9 @@ class QAPISchemaEntity(object):
>          self.name = name
>          # For explicitly defined entities, info points to the (explicit)
>          # definition.  For builtins (and their arrays), info is None.
> -        # TODO For other implicitly defined entities, it should point to
> -        # a place that triggers implicit definition; there may be more
> -        # than one such place.
> +        # For other implicitly defined entities, it points to a place
> +        # that triggers implicit definition; there may be more than one
> +        # such place.
>          self.info = info
>
>      def c_name(self):
> @@ -1153,7 +1153,9 @@ class QAPISchema(object):
>          try:
>              self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
>              self._entity_dict = {}
> +            self._predefined_done = False
>              self._def_predefineds()
> +            self._predefined_done = True
>              self._def_exprs()
>              self.check()
>          except (QAPISchemaError, QAPIExprError), err:
> @@ -1161,6 +1163,9 @@ class QAPISchema(object):
>              exit(1)
>
>      def _def_entity(self, ent):
> +        if self._predefined_done:
> +            # Only the predefined types are allowed to not have info
> +            assert ent.info

Or possibly

        assert ent.info or not self._predefined_done

With self._predefined_done replaced by self._predefining, we'd get

        assert ent.info or self._predefining

Pick the bikeshed color you like best :)

>          assert ent.name not in self._entity_dict
>          self._entity_dict[ent.name] = ent
>
> @@ -1203,9 +1208,9 @@ class QAPISchema(object):
>                                                            [], None)
>          self._def_entity(self.the_empty_object_type)
>
> -    def _make_implicit_enum_type(self, name, values):
> +    def _make_implicit_enum_type(self, name, info, values):
>          name = name + 'Kind'
> -        self._def_entity(QAPISchemaEnumType(name, None, values, None))
> +        self._def_entity(QAPISchemaEnumType(name, info, values, None))
>          return name
>
>      def _make_array_type(self, element_type, info):
> @@ -1214,12 +1219,12 @@ class QAPISchema(object):
>              self._def_entity(QAPISchemaArrayType(name, info, element_type))
>          return name
>
> -    def _make_implicit_object_type(self, name, role, members):
> +    def _make_implicit_object_type(self, name, info, role, members):
>          if not members:
>              return None
>          name = ':obj-%s-%s' % (name, role)
>          if not self.lookup_entity(name, QAPISchemaObjectType):
> -            self._def_entity(QAPISchemaObjectType(name, None, None,
> +            self._def_entity(QAPISchemaObjectType(name, info, None,
>                                                    members, None))
>          return name
>
> @@ -1259,11 +1264,11 @@ class QAPISchema(object):
>              assert len(typ) == 1
>              typ = self._make_array_type(typ[0], info)
>          typ = self._make_implicit_object_type(
> -            typ, 'wrapper', [self._make_member('data', typ, info)])
> +            typ, info, 'wrapper', [self._make_member('data', typ, info)])
>          return QAPISchemaObjectTypeVariant(case, typ)
>
> -    def _make_implicit_tag(self, type_name, variants):
> -        typ = self._make_implicit_enum_type(type_name,
> +    def _make_implicit_tag(self, type_name, info, variants):
> +        typ = self._make_implicit_enum_type(type_name, info,
>                                              [v.name for v in variants])
>          return QAPISchemaObjectTypeUnionTag('type', typ, False)
>
> @@ -1279,7 +1284,7 @@ class QAPISchema(object):
>          else:
>              variants = [self._make_simple_variant(key, value, info)
>                          for (key, value) in data.iteritems()]
> -            tag_enum = self._make_implicit_tag(name, variants)
> +            tag_enum = self._make_implicit_tag(name, info, variants)
>          self._def_entity(
>              QAPISchemaObjectType(name, info, base,
>                                   self._make_members(OrderedDict(), info),
> @@ -1292,7 +1297,7 @@ class QAPISchema(object):
>          data = expr['data']
>          variants = [self._make_variant(key, value)
>                      for (key, value) in data.iteritems()]
> -        tag_enum = self._make_implicit_tag(name, variants)
> +        tag_enum = self._make_implicit_tag(name, info, variants)
>          self._def_entity(
>              QAPISchemaAlternateType(name, info,
>                                      QAPISchemaObjectTypeVariants(None,
> @@ -1307,7 +1312,7 @@ class QAPISchema(object):
>          success_response = expr.get('success-response', True)
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
> -                name, 'arg', self._make_members(data, info))
> +                name, info, 'arg', self._make_members(data, info))
>          if isinstance(rets, list):
>              assert len(rets) == 1
>              rets = self._make_array_type(rets[0], info)
> @@ -1319,7 +1324,7 @@ class QAPISchema(object):
>          data = expr.get('data')
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
> -                name, 'arg', self._make_members(data, info))
> +                name, info, 'arg', self._make_members(data, info))
>          self._def_entity(QAPISchemaEvent(name, info, data))
>
>      def _def_exprs(self):

Tedious plumbing work, mostly :)