John Snow <js...@redhat.com> writes:
> The typing of _make_tree and friends is a bit involved, but it can be
> done with some stubbed out types and a bit of elbow grease. The
> forthcoming patches attempt to make some simplifications, but having the
> type hints in advance may aid in review of subsequent patches.
>
>
> Some notes on the abstract types used at this point, and what they
> represent:
>
> - TreeValue represents any object in the type tree. _make_tree is an
> optional call -- not every node in the final type tree will have been
> passed to _make_tree, so this type encompasses not only what is passed
> to _make_tree (dicts, strings) or returned from it (dicts, strings, a
> 2-tuple), but any recursive value for any of the dicts passed to
> _make_tree -- which includes lists, strings, integers, null constants,
> and so on.
>
> - _DObject is a type alias I use to mean "A JSON-style object,
> represented as a Python dict." There is no "JSON" type in Python, they
> are converted natively to recursively nested dicts and lists, with
> leaf values of str, int, float, None, True/False and so on. This type
> structure is not possible to accurately portray in mypy yet, so a
> placeholder is used.
>
> In this case, _DObject is being used to refer to SchemaInfo-like
> structures as defined in qapi/introspect.json, OR any sub-object
> values they may reference. We don't have strong typing available for
> those, so a generic alternative is used.
>
> - Extra refers explicitly to the dict containing "extra" information
> about a node in the tree. mypy does not offer per-key typing for dicts
> in Python 3.6, so this is the best we can do here.
>
> - Annotated refers to (one of) the return types of _make_tree:
> It represents a 2-tuple of (TreeValue, Extra).
>
>
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> Signed-off-by: John Snow <js...@redhat.com>
> ---
> scripts/qapi/introspect.py | 157 ++++++++++++++++++++++++++++---------
> scripts/qapi/mypy.ini | 5 --
> scripts/qapi/schema.py | 2 +-
> 3 files changed, 121 insertions(+), 43 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 63f721ebfb6..803288a64e7 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -10,7 +10,16 @@
> See the COPYING file in the top-level directory.
> """
>
> -from typing import Optional, Sequence, cast
> +from typing import (
> + Any,
> + Dict,
> + List,
> + Optional,
> + Sequence,
> + Tuple,
> + Union,
> + cast,
> +)
>
> from .common import (
> c_name,
> @@ -20,13 +29,56 @@
> )
> from .gen import QAPISchemaMonolithicCVisitor
> from .schema import (
> + QAPISchema,
> QAPISchemaArrayType,
> QAPISchemaBuiltinType,
> + QAPISchemaEntity,
> + QAPISchemaEnumMember,
> + QAPISchemaFeature,
> + QAPISchemaObjectType,
> + QAPISchemaObjectTypeMember,
> QAPISchemaType,
> + QAPISchemaVariant,
> + QAPISchemaVariants,
> )
> +from .source import QAPISourceInfo
>
>
> -def _make_tree(obj, ifcond, features, extra=None):
> +# This module constructs a tree-like data structure that is used to
"Tree-like" suggests it's not a tree, it just looks like one if you
squint. Drop "-like"?
> +# generate the introspection information for QEMU. It behaves similarly
> +# to a JSON value.
> +#
> +# A complexity over JSON is that our values may or may not be annotated.
It's the obvious abstract syntax tree for JSON, hacked up^W^Wextended to
support certain annotations.
Let me add a bit of context and history.
The module's job is generating qapi-introspect.[ch] for a QAPISchema.
The purpose of qapi-introspect.[ch] is providing the information
query-qmp-schema needs, i.e. (a suitable C representation of) a JSON
value conforming to [SchemaInfo]. Details of this C representation are
not interesting right now.
We first go from QAPISchema to a suitable Python representation of
[SchemaInfo], then from there to the C source code, neatly separating
concerns.
Stupidest solution Python representation that could possibly work: the
obvious abstract syntax tree for JSON (that's also how Python's json
module works).
Parts corresponding to QAPISchema parts guarded by 'if' conditionals
need to be guarded by #if conditionals.
We want to prefix parts corresponding to certain QAPISchema parts with a
comment.
These two requirements came later, and were hacked into the existing
stupidest solution: any tree node can be a tuple (json, extra), where
json is the "stupidest" node, and extra is a dict of annotations. In
other words, to annotate an unannotated node N with dict D, replace N by
(N, D).
Possible annotations:
'comment': str
'if': Sequence[str]
They say there are just three answers a Marine may give to an officer's
questions: "Yes, sir!", "No, sir!", "No excuse, sir!". Let me put that
to use here:
Is this an elegant design? No, sir!
Is the code easy to read? No excuse, sir!
Was it cheap to make? Yes, sir!
> +#
> +# Un-annotated values may be:
> +# Scalar: str, bool, None.
> +# Non-scalar: List, Dict
> +# _Value = Union[str, bool, None, Dict[str, Value], List[Value]]
> +#
> +# With optional annotations, the type of all values is:
> +# TreeValue = Union[_Value, Annotated[_Value]]
> +#
> +# Sadly, mypy does not support recursive types, so we must approximate this.
> +_stub = Any
> +_scalar = Union[str, bool, None]
> +_nonscalar = Union[Dict[str, _stub], List[_stub]]
> +_value = Union[_scalar, _nonscalar]
> +TreeValue = Union[_value, 'Annotated']
Are there naming conventions for this kind of variables? I'm asking
because you capitalize some, but not all, and I can't see a pattern.
Ignorant question: only 'Annotated' has quotes; why?
There is "_Value", "Value" and "_value". Suggest to add "value"
somewhere, for completeness ;-P
I find the names _value and TreeValue a bit unfortunate: the difference
between the two isn't Tree. I'll come back to this below.
> +
> +# This is just an alias for an object in the structure described above:
> +_DObject = Dict[str, object]
I'm confused. Which structure, and why do we want to alias it?
> +
> +# Represents the annotations themselves:
> +Annotations = Dict[str, object]
Losely typed. I have no idea whether that's bad :)
> +
> +# Represents an annotated node (of some kind).
> +Annotated = Tuple[_value, Annotations]
So, _value seems to represent a JSON value, Annotated an annotated JSON
value, and TreeValue their union, i.e. a possibly annotated JSON value.
Naming is hard... BareJsonValue, AnnotatedJsonValue, JsonValue?
> +
> +
> +def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
I'd expect obj: _value, i.e. "unannotated value".
> + features: List[QAPISchemaFeature],
> + extra: Optional[Annotations] = None
> + ) -> TreeValue:
> if extra is None:
> extra = {}
> if ifcond:
> @@ -39,9 +91,11 @@ def _make_tree(obj, ifcond, features, extra=None):
> return obj
>
>
> -def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
> +def _tree_to_qlit(obj: TreeValue,
> + level: int = 0,
> + suppress_first_indent: bool = False) -> str:
>
> - def indent(level):
> + def indent(level: int) -> str:
> return level * 4 * ' '
>
> if isinstance(obj, tuple):
> @@ -91,21 +145,20 @@ def indent(level):
> return ret
>
>
> -def to_c_string(string):
> +def to_c_string(string: str) -> str:
> return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
>
>
> class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
> -
Intentional?
> - def __init__(self, prefix, unmask):
> + def __init__(self, prefix: str, unmask: bool):
> super().__init__(
> prefix, 'qapi-introspect',
> ' * QAPI/QMP schema introspection', __doc__)
> self._unmask = unmask
> - self._schema = None
> - self._trees = []
> - self._used_types = []
> - self._name_map = {}
> + self._schema: Optional[QAPISchema] = None
> + self._trees: List[TreeValue] = []
> + self._used_types: List[QAPISchemaType] = []
> + self._name_map: Dict[str, str] = {}
> self._genc.add(mcgen('''
> #include "qemu/osdep.h"
> #include "%(prefix)sqapi-introspect.h"
> @@ -113,10 +166,10 @@ def __init__(self, prefix, unmask):
> ''',
> prefix=prefix))
>
> - def visit_begin(self, schema):
> + def visit_begin(self, schema: QAPISchema) -> None:
> self._schema = schema
>
> - def visit_end(self):
> + def visit_end(self) -> None:
> # visit the types that are actually used
> for typ in self._used_types:
> typ.visit(self)
> @@ -138,18 +191,18 @@ def visit_end(self):
> self._used_types = []
> self._name_map = {}
>
> - def visit_needed(self, entity):
> + def visit_needed(self, entity: QAPISchemaEntity) -> bool:
> # Ignore types on first pass; visit_end() will pick up used types
> return not isinstance(entity, QAPISchemaType)
>
> - def _name(self, name):
> + def _name(self, name: str) -> str:
> if self._unmask:
> return name
> if name not in self._name_map:
> self._name_map[name] = '%d' % len(self._name_map)
> return self._name_map[name]
>
> - def _use_type(self, typ):
> + def _use_type(self, typ: QAPISchemaType) -> str:
> # Map the various integer types to plain int
> if typ.json_type() == 'int':
> typ = self._schema.lookup_type('int')
> @@ -168,8 +221,10 @@ def _use_type(self, typ):
> return '[' + self._use_type(typ.element_type) + ']'
> return self._name(typ.name)
>
> - def _gen_tree(self, name, mtype, obj, ifcond, features):
> - extra = None
> + def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> + ifcond: List[str],
> + features: Optional[List[QAPISchemaFeature]]) -> None:
_gen_tree() builds a complete tree (i.e. one SchemaInfo), and adds it to
._trees.
The SchemaInfo's common parts are name, meta-type and features.
_gen_tree() takes them as arguments @name, @mtype, @features.
It takes SchemaInfo's variant parts as a dict @obj.
It completes @obj into an unannotated tree node by the common parts into
@obj.
It also takes a QAPI conditional argument @ifcond.
Now let me review the type annotations:
* name: str matches SchemaInfo, good.
* mtype: str approximates SchemaInfo's enum SchemaMetaType (it's not
Python Enum because those were off limits when this code was written).
* obj: _DObject ... I'd expect "unannotated JSON value".
* ifcond: List[str] should work, but gen_if(), gen_endif() use
Sequence[str]. Suggest to pick one (please explain why), and stick to
it.
More instances of ifcond: List[str] elsewhere; I'm not flagging them.
* features: Optional[List[QAPISchemaFeature]] is correct. "No features"
has two representations: None and []. I guess we could eliminate
None, trading a tiny bit of efficiency for simpler typing. Not a
demand.
> + extra: Optional[Annotations] = None
"No annotations" is represented as None here, not {}. I guess we could
use {} for simpler typing. Not a demand.
> if mtype not in ('command', 'event', 'builtin', 'array'):
> if not self._unmask:
> # Output a comment to make it easy to map masked names
> @@ -180,44 +235,64 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
> obj['meta-type'] = mtype
> self._trees.append(_make_tree(obj, ifcond, features, extra))
>
> - def _gen_member(self, member):
> - obj = {'name': member.name, 'type': self._use_type(member.type)}
> + def _gen_member(self,
> + member: QAPISchemaObjectTypeMember) -> TreeValue:
> + obj: _DObject = {
I'd expect "unannotated value". More of the same below.
> + 'name': member.name,
> + 'type': self._use_type(member.type)
> + }
> if member.optional:
> obj['default'] = None
> return _make_tree(obj, member.ifcond, member.features)
>
> - def _gen_variants(self, tag_name, variants):
> + def _gen_variants(self, tag_name: str,
> + variants: List[QAPISchemaVariant]) -> _DObject:
> return {'tag': tag_name,
> 'variants': [self._gen_variant(v) for v in variants]}
>
> - def _gen_variant(self, variant):
> - obj = {'case': variant.name, 'type': self._use_type(variant.type)}
> + def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
> + obj: _DObject = {
> + 'case': variant.name,
> + 'type': self._use_type(variant.type)
> + }
> return _make_tree(obj, variant.ifcond, None)
>
> - def visit_builtin_type(self, name, info, json_type):
> + def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
A built-in's type info is always None. Perhaps we should drop the
parameter.
> + json_type: str) -> None:
> self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>
> - def visit_enum_type(self, name, info, ifcond, features, members, prefix):
> + def visit_enum_type(self, name: str, info: QAPISourceInfo,
> + ifcond: List[str], features: List[QAPISchemaFeature],
> + members: List[QAPISchemaEnumMember],
> + prefix: Optional[str]) -> None:
> self._gen_tree(name, 'enum',
> {'values': [_make_tree(m.name, m.ifcond, None)
> for m in members]},
> ifcond, features)
>
> - def visit_array_type(self, name, info, ifcond, element_type):
> + def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
Here, @info is indeed optional: it's None when @element_type is a
built-in type.
> + ifcond: List[str],
> + element_type: QAPISchemaType) -> None:
> element = self._use_type(element_type)
> self._gen_tree('[' + element + ']', 'array', {'element-type':
> element},
> ifcond, None)
>
> - def visit_object_type_flat(self, name, info, ifcond, features,
> - members, variants):
> - obj = {'members': [self._gen_member(m) for m in members]}
> + def visit_object_type_flat(self, name: str, info:
> Optional[QAPISourceInfo],
And here it is optional due to the internal object type 'q_empty'.
> + ifcond: List[str],
> + features: List[QAPISchemaFeature],
> + members: Sequence[QAPISchemaObjectTypeMember],
> + variants: Optional[QAPISchemaVariants]) ->
> None:
We represent "no variants" as None, not as []. I guess we could
eliminate use [], trading a tiny bit of efficiency for simpler typing.
Not a demand.
> + obj: _DObject = {'members': [self._gen_member(m) for m in members]}
> if variants:
> obj.update(self._gen_variants(variants.tag_member.name,
> variants.variants))
>
> self._gen_tree(name, 'object', obj, ifcond, features)
>
> - def visit_alternate_type(self, name, info, ifcond, features, variants):
> + def visit_alternate_type(self, name: str, info: QAPISourceInfo,
> + ifcond: List[str],
> + features: List[QAPISchemaFeature],
> + variants: QAPISchemaVariants) -> None:
> self._gen_tree(name, 'alternate',
> {'members': [
> _make_tree({'type': self._use_type(m.type)},
> @@ -225,24 +300,32 @@ def visit_alternate_type(self, name, info, ifcond,
> features, variants):
> for m in variants.variants]},
> ifcond, features)
>
> - def visit_command(self, name, info, ifcond, features,
> - arg_type, ret_type, gen, success_response, boxed,
> - allow_oob, allow_preconfig, coroutine):
> + def visit_command(self, name: str, info: QAPISourceInfo, ifcond:
> List[str],
> + features: List[QAPISchemaFeature],
> + arg_type: QAPISchemaObjectType,
> + ret_type: Optional[QAPISchemaType], gen: bool,
Are you sure arg_type can't be None?
> + success_response: bool, boxed: bool, allow_oob: bool,
> + allow_preconfig: bool, coroutine: bool) -> None:
> arg_type = arg_type or self._schema.the_empty_object_type
> ret_type = ret_type or self._schema.the_empty_object_type
> - obj = {'arg-type': self._use_type(arg_type),
> - 'ret-type': self._use_type(ret_type)}
> + obj: _DObject = {
> + 'arg-type': self._use_type(arg_type),
> + 'ret-type': self._use_type(ret_type)
> + }
> if allow_oob:
> obj['allow-oob'] = allow_oob
> self._gen_tree(name, 'command', obj, ifcond, features)
>
> - def visit_event(self, name, info, ifcond, features, arg_type, boxed):
> + def visit_event(self, name: str, info: QAPISourceInfo,
> + ifcond: List[str], features: List[QAPISchemaFeature],
> + arg_type: QAPISchemaObjectType, boxed: bool) -> None:
> arg_type = arg_type or self._schema.the_empty_object_type
> self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
> ifcond, features)
>
>
> -def gen_introspect(schema, output_dir, prefix, opt_unmask):
> +def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
> + opt_unmask: bool) -> None:
> vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
> schema.visit(vis)
> vis.write(output_dir)
> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> index 74fc6c82153..c0f2a58306d 100644
> --- a/scripts/qapi/mypy.ini
> +++ b/scripts/qapi/mypy.ini
> @@ -14,11 +14,6 @@ disallow_untyped_defs = False
> disallow_incomplete_defs = False
> check_untyped_defs = False
>
> -[mypy-qapi.introspect]
> -disallow_untyped_defs = False
> -disallow_incomplete_defs = False
> -check_untyped_defs = False
> -
> [mypy-qapi.parser]
> disallow_untyped_defs = False
> disallow_incomplete_defs = False
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 720449feee4..e91b77fadc3 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -28,7 +28,7 @@
> class QAPISchemaEntity:
> meta: Optional[str] = None
>
> - def __init__(self, name, info, doc, ifcond=None, features=None):
> + def __init__(self, name: str, info, doc, ifcond=None, features=None):
> assert name is None or isinstance(name, str)
> for f in features or []:
> assert isinstance(f, QAPISchemaFeature)
