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.
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):
--
2.41.0John 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".
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 <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?
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:
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.)
>
John Snow <jsnow@redhat.com> writes: > 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. I do appreciate the minimal fix! I *like* exploring "minimal" first. In this case, the exploration led me to not like my lazy hack anymore :) [...] > 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.) Thanks!
On Wed, Nov 22, 2023 at 4:38 AM Markus Armbruster <armbru@redhat.com> wrote: > > John Snow <jsnow@redhat.com> writes: > > > 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. > > I do appreciate the minimal fix! I *like* exploring "minimal" first. > In this case, the exploration led me to not like my lazy hack anymore :) > > [...] > > > 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.) > > Thanks! > Got this working with minor changes to sphinx's qapidox extension, which isn't typechecked so it escaped notice. I'm going to continue working through your feedback as-is, then when I get to the end of the series, I'll attempt to float this patch forward to replace this patch under discussion. Thanks! --js
© 2016 - 2024 Red Hat, Inc.