Re: [PATCH 03/19] qapi/schema: name QAPISchemaInclude entities

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 16214 bytes --]

On Tue, Nov 21, 2023, 8:33 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > It simplifies typing to mandate that entities will always have a name;
> > to achieve this we can occasionally assign an internal name. This
> > alleviates errors such as:
> >
> > qapi/schema.py:287: error: Argument 1 to "__init__" of
> > "QAPISchemaEntity" has incompatible type "None"; expected "str"
> > [arg-type]
> >
> > Trying to fix it the other way by allowing entities to only have
> > optional names opens up a nightmare portal of whackamole to try and
> > audit that every other pathway doesn't actually pass a None name when we
> > expect it to; this is the simpler direction of consitifying the typing.
>
> Arguably, that nightmare is compile-time proof of "we are not mistaking
> QAPISchemaInclude for a named entity".
>
> When I added the include directive, I shoehorned it into the existing
> representation of the QAPI schema as "list of QAPISchemaEntity" by
> making it a subtype of QAPISchemaEntity.  That was a somewhat lazy hack.
>
> Note that qapi-code-gen.rst distinguishes between definitions and
> directives.
>
> The places where mypy gripes that .name isn't 'str' generally want
> something with a name (what qapi-code-gen.rst calls a definition).  If
> we somehow pass them an include directive, they'll use None for a name,
> which is no good.  mypy is pointing out this problem.
>
> What to do about it?
>
> 1. Paper it over: give include directives some made-up name (this
> patch).  Now the places use the made-up name instead of None, and mypy
> can't see the problem anymore.
>
> 2. Assert .name is not None until mypy is happy.  I guess that's what
> you called opening "a nightmare portal of whackamole".
>

Yep.


> 3. Clean up the typing: have a type for top-level expression (has no
> name), and a subtype for definition (has a name).  Rough sketch
> appended.  Thoughts?
>

Oh, that'll work. I tried to keep to "minimal SLOC" but where you want to
see deeper fixes, I'm happy to deviate. I'll give it a shot.


> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/schema.py | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 153e703e0ef..0fb44452dd5 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -220,7 +220,9 @@ def visit(self, visitor):
> >
> >  class QAPISchemaInclude(QAPISchemaEntity):
> >      def __init__(self, sub_module, info):
> > -        super().__init__(None, info, None)
> > +        # Includes are internal entity objects; and may occur multiple
> times
> > +        name = f"q_include_{info.fname}:{info.line}"
> > +        super().__init__(name, info, None)
> >          self._sub_module = sub_module
> >
> >      def visit(self, visitor):
>
> There are two instances of .name is None:
>
>     def __repr__(self) -> str:
>         if self.name is None:
>             return "<%s at 0x%x>" % (type(self).__name__, id(self))
>         return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
>                                     id(self))
>
> and
>
>     def _def_entity(self, ent: QAPISchemaEntity) -> None:
>         # Only the predefined types are allowed to not have info
>         assert ent.info or self._predefining
>         self._entity_list.append(ent)
>         if ent.name is None:
>             return
>         [...]
>
> Don't they need to be updated?
>

Oh, yes. I remove some assertions later in the series but I very likely
missed these if cases.


>
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 24ad166d52..eaff1df534 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -70,6 +70,45 @@ def is_present(self) -> bool:
>
>
>  class QAPISchemaEntity:
> +    def __init__(self, info: Optional[QAPISourceInfo]):
> +        # For explicitly defined entities, info points to the (explicit)
> +        # definition.  For builtins (and their arrays), info is None.
> +        # For implicitly defined entities, info points to a place that
> +        # triggered the implicit definition (there may be more than one
> +        # such place).
> +        self.info = info
> +        self._module: Optional[QAPISchemaModule] = None
> +        self._checked = False
> +
> +    def __repr__(self) -> str:
> +        return "<%s at 0x%x>" % (type(self).__name__, id(self))
> +
> +    def check(self, schema: QAPISchema) -> None:
> +        self._checked = True
> +
> +    def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
> +        pass
> +
> +    def check_doc(self) -> None:
> +        pass
> +
> +    def _set_module(
> +        self, schema: QAPISchema, info: Optional[QAPISourceInfo]
> +    ) -> None:
> +        assert self._checked
> +        fname = info.fname if info else
> QAPISchemaModule.BUILTIN_MODULE_NAME
> +        self._module = schema.module_by_fname(fname)
> +        self._module.add_entity(self)
> +
> +    def set_module(self, schema: QAPISchema) -> None:
> +        self._set_module(schema, self.info)
> +
> +    def visit(self, visitor: QAPISchemaVisitor) -> None:
> +        # pylint: disable=unused-argument
> +        assert self._checked
> +
> +
> +class QAPISchemaDefinition(QAPISchemaEntity):
>      meta: str
>
>      def __init__(
> @@ -80,24 +119,16 @@ def __init__(
>          ifcond: Optional[QAPISchemaIfCond] = None,
>          features: Optional[List[QAPISchemaFeature]] = None,
>      ):
> +        super().__init__(info)
>          for f in features or []:
>              f.set_defined_in(name)
>          self.name = name
> -        self._module: Optional[QAPISchemaModule] = None
> -        # For explicitly defined entities, info points to the (explicit)
> -        # definition.  For builtins (and their arrays), info is None.
> -        # For implicitly defined entities, info points to a place that
> -        # triggered the implicit definition (there may be more than one
> -        # such place).
> -        self.info = info
>          self.doc = doc
>          self._ifcond = ifcond or QAPISchemaIfCond()
>          self.features = features or []
>          self._checked = False
> -
> +
>      def __repr__(self) -> str:
> -        if self.name is None:
> -            return "<%s at 0x%x>" % (type(self).__name__, id(self))
>          return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
>                                      id(self))
>
> @@ -122,17 +153,6 @@ def check_doc(self) -> None:
>          if self.doc:
>              self.doc.check()
>
> -    def _set_module(
> -        self, schema: QAPISchema, info: Optional[QAPISourceInfo]
> -    ) -> None:
> -        assert self._checked
> -        fname = info.fname if info else
> QAPISchemaModule.BUILTIN_MODULE_NAME
> -        self._module = schema.module_by_fname(fname)
> -        self._module.add_entity(self)
> -
> -    def set_module(self, schema: QAPISchema) -> None:
> -        self._set_module(schema, self.info)
> -
>      @property
>      def ifcond(self) -> QAPISchemaIfCond:
>          assert self._checked
> @@ -141,10 +161,6 @@ def ifcond(self) -> QAPISchemaIfCond:
>      def is_implicit(self) -> bool:
>          return not self.info
>
> -    def visit(self, visitor: QAPISchemaVisitor) -> None:
> -        # pylint: disable=unused-argument
> -        assert self._checked
> -
>      def describe(self) -> str:
>          return "%s '%s'" % (self.meta, self.name)
>
> @@ -301,9 +317,7 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
>
>  class QAPISchemaInclude(QAPISchemaEntity):
>      def __init__(self, sub_module: QAPISchemaModule, info:
> QAPISourceInfo):
> -        # Includes are internal entity objects; and may occur multiple
> times
> -        name = f"q_include_{info.fname}:{info.line}"
> -        super().__init__(name, info, None)
> +        super().__init__(info)
>          self._sub_module = sub_module
>
>      def visit(self, visitor: QAPISchemaVisitor) -> None:
> @@ -311,7 +325,7 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
>          visitor.visit_include(self._sub_module.name, self.info)
>
>
> -class QAPISchemaType(QAPISchemaEntity):
> +class QAPISchemaType(QAPISchemaDefinition):
>      # Return the C type for common use.
>      # For the types we commonly box, this is a pointer type.
>      def c_type(self) -> str:
> @@ -977,7 +991,7 @@ def __init__(
>          super().__init__(name, info, typ, False, ifcond)
>
>
> -class QAPISchemaCommand(QAPISchemaEntity):
> +class QAPISchemaCommand(QAPISchemaDefinition):
>      meta = 'command'
>
>      def __init__(
> @@ -1059,7 +1073,7 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
>              self.coroutine)
>
>
> -class QAPISchemaEvent(QAPISchemaEntity):
> +class QAPISchemaEvent(QAPISchemaDefinition):
>      meta = 'event'
>
>      def __init__(
> @@ -1133,7 +1147,7 @@ def __init__(self, fname: str):
>          exprs = check_exprs(parser.exprs)
>          self.docs = parser.docs
>          self._entity_list: List[QAPISchemaEntity] = []
> -        self._entity_dict: Dict[str, QAPISchemaEntity] = {}
> +        self._entity_dict: Dict[str, QAPISchemaDefinition] = {}
>          self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
>          self._schema_dir = os.path.dirname(fname)
>          self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
> @@ -1145,9 +1159,12 @@ def __init__(self, fname: str):
>          self.check()
>
>      def _def_entity(self, ent: QAPISchemaEntity) -> None:
> +        self._entity_list.append(ent)
> +
> +    def _def_definition(self, ent: QAPISchemaDefinition) -> None:
>          # Only the predefined types are allowed to not have info
>          assert ent.info or self._predefining
> -        self._entity_list.append(ent)
> +        self._def_entity(ent)
>          if ent.name is None:
>              return
>          # TODO reject names that differ only in '_' vs. '.'  vs. '-',
> @@ -1163,7 +1180,7 @@ def _def_entity(self, ent: QAPISchemaEntity) -> None:
>                  ent.info, "%s is already defined" % other_ent.describe())
>          self._entity_dict[ent.name] = ent
>
> -    def get_entity(self, name: str) -> Optional[QAPISchemaEntity]:
> +    def get_definition(self, name: str) -> Optional[QAPISchemaDefinition]:
>          return self._entity_dict.get(name)
>
>      def get_typed_entity(
> @@ -1171,7 +1188,7 @@ def get_typed_entity(
>          name: str,
>          typ: Type[_EntityType]
>      ) -> Optional[_EntityType]:
> -        ent = self.get_entity(name)
> +        ent = self.get_definition(name)
>          if ent is not None and not isinstance(ent, typ):
>              etype = type(ent).__name__
>              ttype = typ.__name__
> @@ -1225,7 +1242,7 @@ def _def_include(self, expr: QAPIExpression) -> None:
>      def _def_builtin_type(
>          self, name: str, json_type: str, c_type: str
>      ) -> None:
> -        self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
> +        self._def_definition(QAPISchemaBuiltinType(name, json_type,
> c_type))
>          # Instantiating only the arrays that are actually used would
>          # be nice, but we can't as long as their generated code
>          # (qapi-builtin-types.[ch]) may be shared by some other
> @@ -1251,14 +1268,14 @@ def _def_predefineds(self) -> None:
>              self._def_builtin_type(*t)
>          self.the_empty_object_type = QAPISchemaObjectType(
>              'q_empty', None, None, None, None, None, [], None)
> -        self._def_entity(self.the_empty_object_type)
> +        self._def_definition(self.the_empty_object_type)
>
>          qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
>                    'qbool']
>          qtype_values = self._make_enum_members(
>              [{'name': n} for n in qtypes], None)
>
> -        self._def_entity(QAPISchemaEnumType('QType', None, None, None,
> None,
> +        self._def_definition(QAPISchemaEnumType('QType', None, None,
> None, None,
>                                              qtype_values, 'QTYPE'))
>
>      def _make_features(
> @@ -1294,8 +1311,8 @@ def _make_array_type(
>          self, element_type: str, info: Optional[QAPISourceInfo]
>      ) -> str:
>          name = element_type + 'List'    # reserved by
> check_defn_name_str()
> -        if not self.get_entity(name):
> -            self._def_entity(QAPISchemaArrayType(name, info,
> element_type))
> +        if not self.get_definition(name):
> +            self._def_definition(QAPISchemaArrayType(name, info,
> element_type))
>          return name
>
>      def _make_implicit_object_type(
> @@ -1317,7 +1334,7 @@ def _make_implicit_object_type(
>              # later.
>              pass
>          else:
> -            self._def_entity(QAPISchemaObjectType(
> +            self._def_definition(QAPISchemaObjectType(
>                  name, info, None, ifcond, None, None, members, None))
>          return name
>
> @@ -1328,7 +1345,7 @@ def _def_enum_type(self, expr: QAPIExpression) ->
> None:
>          ifcond = QAPISchemaIfCond(expr.get('if'))
>          info = expr.info
>          features = self._make_features(expr.get('features'), info)
> -        self._def_entity(QAPISchemaEnumType(
> +        self._def_definition(QAPISchemaEnumType(
>              name, info, expr.doc, ifcond, features,
>              self._make_enum_members(data, info), prefix))
>
> @@ -1367,7 +1384,7 @@ def _def_struct_type(self, expr: QAPIExpression) ->
> None:
>          info = expr.info
>          ifcond = QAPISchemaIfCond(expr.get('if'))
>          features = self._make_features(expr.get('features'), info)
> -        self._def_entity(QAPISchemaObjectType(
> +        self._def_definition(QAPISchemaObjectType(
>              name, info, expr.doc, ifcond, features, base,
>              self._make_members(data, info),
>              None))
> @@ -1403,7 +1420,7 @@ def _def_union_type(self, expr: QAPIExpression) ->
> None:
>                                 info)
>              for (key, value) in data.items()]
>          members: List[QAPISchemaObjectTypeMember] = []
> -        self._def_entity(
> +        self._def_definition(
>              QAPISchemaObjectType(name, info, expr.doc, ifcond, features,
>                                   base, members,
>                                   QAPISchemaVariants(
> @@ -1422,7 +1439,7 @@ def _def_alternate_type(self, expr: QAPIExpression)
> -> None:
>                                 info)
>              for (key, value) in data.items()]
>          tag_member = QAPISchemaObjectTypeMember('type', info, 'QType',
> False)
> -        self._def_entity(
> +        self._def_definition(
>              QAPISchemaAlternateType(
>                  name, info, expr.doc, ifcond, features,
>                  QAPISchemaVariants(None, info, tag_member, variants)))
> @@ -1447,7 +1464,7 @@ def _def_command(self, expr: QAPIExpression) -> None:
>          if isinstance(rets, list):
>              assert len(rets) == 1
>              rets = self._make_array_type(rets[0], info)
> -        self._def_entity(QAPISchemaCommand(name, info, expr.doc, ifcond,
> +        self._def_definition(QAPISchemaCommand(name, info, expr.doc,
> ifcond,
>                                             features, data, rets,
>                                             gen, success_response,
>                                             boxed, allow_oob,
> allow_preconfig,
> @@ -1464,7 +1481,7 @@ def _def_event(self, expr: QAPIExpression) -> None:
>              data = self._make_implicit_object_type(
>                  name, info, ifcond,
>                  'arg', self._make_members(data, info))
> -        self._def_entity(QAPISchemaEvent(name, info, expr.doc, ifcond,
> +        self._def_definition(QAPISchemaEvent(name, info, expr.doc, ifcond,
>                                           features, data, boxed))
>
>      def _def_exprs(self, exprs: List[QAPIExpression]) -> None:
>

I'll try the refactor out in a patch at the end of my series and see how
feasible it is.

(I haven't reviewed it deeply yet, so if there's an obvious problem I'll
find it when I go to implement this. conceptually it seems fine.)

>

[-- Attachment #2: Type: text/html, Size: 22220 bytes --]