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".
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?
> 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?
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:
