On Tue, Nov 21, 2023, 8:33 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@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 <js...@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.)
>
