* [PATCH v5 01/15] qapi/introspect.py: assert schema is not None
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
@ 2021-02-04 0:31 ` John Snow
2021-02-04 0:31 ` [PATCH v5 02/15] qapi/introspect.py: use _make_tree for features nodes John Snow
` (13 subsequent siblings)
14 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-04 0:31 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
The introspect visitor is stateful, but expects that it will have a
schema to refer to. Add assertions that state this.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index fafec94e022..43ab4be1f77 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -147,6 +147,8 @@ def _name(self, name):
return self._name_map[name]
def _use_type(self, typ):
+ assert self._schema is not None
+
# Map the various integer types to plain int
if typ.json_type() == 'int':
typ = self._schema.lookup_type('int')
@@ -225,6 +227,8 @@ def visit_alternate_type(self, name, info, ifcond, features, variants):
def visit_command(self, name, info, ifcond, features,
arg_type, ret_type, gen, success_response, boxed,
allow_oob, allow_preconfig, coroutine):
+ assert self._schema is not 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),
@@ -234,6 +238,7 @@ def visit_command(self, name, info, ifcond, features,
self._gen_tree(name, 'command', obj, ifcond, features)
def visit_event(self, name, info, ifcond, features, arg_type, boxed):
+ assert self._schema is not 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)
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 02/15] qapi/introspect.py: use _make_tree for features nodes
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
2021-02-04 0:31 ` [PATCH v5 01/15] qapi/introspect.py: assert schema is not None John Snow
@ 2021-02-04 0:31 ` John Snow
2021-02-04 0:31 ` [PATCH v5 03/15] qapi/introspect.py: add _gen_features helper John Snow
` (12 subsequent siblings)
14 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-04 0:31 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
At present, we open-code this in _make_tree itself; but if the structure
of the tree changes, this is brittle. Use an explicit recursive call to
_make_tree when appropriate to help keep the interior node typing
consistent.
A consequence of doing this is that the 'ifcond' key of the features
dict will be omitted when ifcond is false-ish, just like it is omitted
in top-level calls to _make_tree. This also increases consistency in our
handling of this property.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 43ab4be1f77..3295a15c98e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -30,7 +30,9 @@ def _make_tree(obj, ifcond, features, extra=None):
if ifcond:
extra['if'] = ifcond
if features:
- obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
+ obj['features'] = [
+ _make_tree(f.name, f.ifcond, None) for f in features
+ ]
if extra:
return (obj, extra)
return obj
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 03/15] qapi/introspect.py: add _gen_features helper
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
2021-02-04 0:31 ` [PATCH v5 01/15] qapi/introspect.py: assert schema is not None John Snow
2021-02-04 0:31 ` [PATCH v5 02/15] qapi/introspect.py: use _make_tree for features nodes John Snow
@ 2021-02-04 0:31 ` John Snow
2021-02-04 0:31 ` [PATCH v5 04/15] qapi/introspect.py: guard against ifcond/comment misuse John Snow
` (11 subsequent siblings)
14 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-04 0:31 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
_make_tree might receive a dict (a SchemaInfo object) or some other type
(usually, a string) for its obj parameter. Adding features information
should arguably be performed by the caller at such a time when we know
the type of the object and don't have to re-interrogate it.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 3295a15c98e..4749f65ea3c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -24,15 +24,11 @@
)
-def _make_tree(obj, ifcond, features, extra=None):
+def _make_tree(obj, ifcond, extra=None):
if extra is None:
extra = {}
if ifcond:
extra['if'] = ifcond
- if features:
- obj['features'] = [
- _make_tree(f.name, f.ifcond, None) for f in features
- ]
if extra:
return (obj, extra)
return obj
@@ -169,6 +165,10 @@ def _use_type(self, typ):
return '[' + self._use_type(typ.element_type) + ']'
return self._name(typ.name)
+ @staticmethod
+ def _gen_features(features):
+ return [_make_tree(f.name, f.ifcond) for f in features]
+
def _gen_tree(self, name, mtype, obj, ifcond, features):
extra = None
if mtype not in ('command', 'event', 'builtin', 'array'):
@@ -179,13 +179,17 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
name = self._name(name)
obj['name'] = name
obj['meta-type'] = mtype
- self._trees.append(_make_tree(obj, ifcond, features, extra))
+ if features:
+ obj['features'] = self._gen_features(features)
+ self._trees.append(_make_tree(obj, ifcond, extra))
def _gen_member(self, member):
obj = {'name': member.name, 'type': self._use_type(member.type)}
if member.optional:
obj['default'] = None
- return _make_tree(obj, member.ifcond, member.features)
+ if member.features:
+ obj['features'] = self._gen_features(member.features)
+ return _make_tree(obj, member.ifcond)
def _gen_variants(self, tag_name, variants):
return {'tag': tag_name,
@@ -193,7 +197,7 @@ def _gen_variants(self, tag_name, variants):
def _gen_variant(self, variant):
obj = {'case': variant.name, 'type': self._use_type(variant.type)}
- return _make_tree(obj, variant.ifcond, None)
+ return _make_tree(obj, variant.ifcond)
def visit_builtin_type(self, name, info, json_type):
self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 04/15] qapi/introspect.py: guard against ifcond/comment misuse
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (2 preceding siblings ...)
2021-02-04 0:31 ` [PATCH v5 03/15] qapi/introspect.py: add _gen_features helper John Snow
@ 2021-02-04 0:31 ` John Snow
2021-02-04 0:31 ` [PATCH v5 05/15] qapi/introspect.py: Unify return type of _make_tree() John Snow
` (10 subsequent siblings)
14 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-04 0:31 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
_tree_to_qlit is called recursively on dict values (isolated from their
keys); at such a point in generating output it is too late to apply an
ifcond. Similarly, comments do not necessarily have a "tidy" place they
can be printed in such a circumstance.
Forbid this usage by renaming "suppress_first_indent" to "dict_value" to
emphasize that indents are suppressed only for the benefit of dict
values; then add an assertion assuring we do not pass ifcond/comments
in this case.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 4749f65ea3c..a7ccda5ab92 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -34,7 +34,7 @@ def _make_tree(obj, ifcond, extra=None):
return obj
-def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
+def _tree_to_qlit(obj, level=0, dict_value=False):
def indent(level):
return level * 4 * ' '
@@ -43,6 +43,12 @@ def indent(level):
ifobj, extra = obj
ifcond = extra.get('if')
comment = extra.get('comment')
+
+ # NB: _tree_to_qlit is called recursively on the values of a key:value
+ # pair; those values can't be decorated with comments or conditionals.
+ msg = "dict values cannot have attached comments or if-conditionals."
+ assert not dict_value, msg
+
ret = ''
if comment:
ret += indent(level) + '/* %s */\n' % comment
@@ -54,7 +60,7 @@ def indent(level):
return ret
ret = ''
- if not suppress_first_indent:
+ if not dict_value:
ret += indent(level)
if obj is None:
ret += 'QLIT_QNULL'
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 05/15] qapi/introspect.py: Unify return type of _make_tree()
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (3 preceding siblings ...)
2021-02-04 0:31 ` [PATCH v5 04/15] qapi/introspect.py: guard against ifcond/comment misuse John Snow
@ 2021-02-04 0:31 ` John Snow
2021-02-04 0:31 ` [PATCH v5 06/15] qapi/introspect.py: replace 'extra' dict with 'comment' argument John Snow
` (9 subsequent siblings)
14 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-04 0:31 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Returning two different types conditionally can be complicated to
type. Return one type for consistency.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index a7ccda5ab92..7730d8ed6b2 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -29,9 +29,7 @@ def _make_tree(obj, ifcond, extra=None):
extra = {}
if ifcond:
extra['if'] = ifcond
- if extra:
- return (obj, extra)
- return obj
+ return (obj, extra)
def _tree_to_qlit(obj, level=0, dict_value=False):
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 06/15] qapi/introspect.py: replace 'extra' dict with 'comment' argument
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (4 preceding siblings ...)
2021-02-04 0:31 ` [PATCH v5 05/15] qapi/introspect.py: Unify return type of _make_tree() John Snow
@ 2021-02-04 0:31 ` John Snow
2021-02-04 0:31 ` [PATCH v5 07/15] qapi/introspect.py: Always define all 'extra' dict keys John Snow
` (8 subsequent siblings)
14 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-04 0:31 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
This is only used to pass in a dictionary with a comment already set, so
skip the runaround and just accept the (optional) comment.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 7730d8ed6b2..1655a21f85b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,6 +10,8 @@
See the COPYING file in the top-level directory.
"""
+from typing import Optional
+
from .common import (
c_name,
gen_endif,
@@ -24,11 +26,12 @@
)
-def _make_tree(obj, ifcond, extra=None):
- if extra is None:
- extra = {}
+def _make_tree(obj, ifcond, comment=None):
+ extra = {}
if ifcond:
extra['if'] = ifcond
+ if comment:
+ extra['comment'] = comment
return (obj, extra)
@@ -174,18 +177,18 @@ def _gen_features(features):
return [_make_tree(f.name, f.ifcond) for f in features]
def _gen_tree(self, name, mtype, obj, ifcond, features):
- extra = None
+ comment: Optional[str] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
# Output a comment to make it easy to map masked names
# back to the source when reading the generated output.
- extra = {'comment': '"%s" = %s' % (self._name(name), name)}
+ comment = f'"{self._name(name)}" = {name}'
name = self._name(name)
obj['name'] = name
obj['meta-type'] = mtype
if features:
obj['features'] = self._gen_features(features)
- self._trees.append(_make_tree(obj, ifcond, extra))
+ self._trees.append(_make_tree(obj, ifcond, comment))
def _gen_member(self, member):
obj = {'name': member.name, 'type': self._use_type(member.type)}
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 07/15] qapi/introspect.py: Always define all 'extra' dict keys
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (5 preceding siblings ...)
2021-02-04 0:31 ` [PATCH v5 06/15] qapi/introspect.py: replace 'extra' dict with 'comment' argument John Snow
@ 2021-02-04 0:31 ` John Snow
2021-02-04 0:32 ` [PATCH v5 08/15] qapi/introspect.py: Introduce preliminary tree typing John Snow
` (7 subsequent siblings)
14 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-04 0:31 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
This mimics how a typed object works, where 'if' and 'comment' are
always set, regardless of if they have a value set or not.
It is safe to do this because of the way that _tree_to_qlit processes
these values (using dict.get with a default of None), resulting in no
change of output from _tree_to_qlit. There are no other users of this
data.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 1655a21f85b..45231d2abc3 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -27,11 +27,10 @@
def _make_tree(obj, ifcond, comment=None):
- extra = {}
- if ifcond:
- extra['if'] = ifcond
- if comment:
- extra['comment'] = comment
+ extra = {
+ 'if': ifcond,
+ 'comment': comment
+ }
return (obj, extra)
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 08/15] qapi/introspect.py: Introduce preliminary tree typing
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (6 preceding siblings ...)
2021-02-04 0:31 ` [PATCH v5 07/15] qapi/introspect.py: Always define all 'extra' dict keys John Snow
@ 2021-02-04 0:32 ` John Snow
2021-02-08 14:29 ` Markus Armbruster
2021-02-04 0:32 ` [PATCH v5 09/15] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
` (6 subsequent siblings)
14 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2021-02-04 0:32 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
The types will be used in forthcoming patches to add typing. These types
describe the layout and structure of the objects passed to
_tree_to_qlit, but lack the power to describe annotations until the next
commit.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 45231d2abc3..8e019b4a26a 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,7 +10,13 @@
See the COPYING file in the top-level directory.
"""
-from typing import Optional
+from typing import (
+ Any,
+ Dict,
+ List,
+ Optional,
+ Union,
+)
from .common import (
c_name,
@@ -26,6 +32,28 @@
)
+# This module constructs a tree data structure that is used to
+# 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.
+#
+# Un-annotated values may be:
+# Scalar: str, bool, None.
+# Non-scalar: List, Dict
+# _value = Union[str, bool, None, Dict[str, TreeValue], List[TreeValue]]
+#
+# 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 = TODO, in a forthcoming commit.
+
+
def _make_tree(obj, ifcond, comment=None):
extra = {
'if': ifcond,
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 08/15] qapi/introspect.py: Introduce preliminary tree typing
2021-02-04 0:32 ` [PATCH v5 08/15] qapi/introspect.py: Introduce preliminary tree typing John Snow
@ 2021-02-08 14:29 ` Markus Armbruster
0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-08 14:29 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> The types will be used in forthcoming patches to add typing. These types
> describe the layout and structure of the objects passed to
> _tree_to_qlit, but lack the power to describe annotations until the next
> commit.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 45231d2abc3..8e019b4a26a 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -10,7 +10,13 @@
> See the COPYING file in the top-level directory.
> """
>
> -from typing import Optional
> +from typing import (
> + Any,
> + Dict,
> + List,
> + Optional,
> + Union,
> +)
>
> from .common import (
> c_name,
> @@ -26,6 +32,28 @@
> )
>
>
> +# This module constructs a tree data structure that is used to
> +# 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.
> +#
> +# Un-annotated values may be:
> +# Scalar: str, bool, None.
> +# Non-scalar: List, Dict
> +# _value = Union[str, bool, None, Dict[str, TreeValue], List[TreeValue]]
> +#
> +# 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 = TODO, in a forthcoming commit.
> +
> +
> def _make_tree(obj, ifcond, comment=None):
> extra = {
> 'if': ifcond,
We discussed the naming of these type aliases review of v4. v5 arrived
before we reached a conclusion. That's okay. I'm still interested in
finishing the discussion, of course.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 09/15] qapi/introspect.py: create a typed 'Annotated' data strutcure
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (7 preceding siblings ...)
2021-02-04 0:32 ` [PATCH v5 08/15] qapi/introspect.py: Introduce preliminary tree typing John Snow
@ 2021-02-04 0:32 ` John Snow
2021-02-08 14:36 ` Markus Armbruster
2021-02-04 0:32 ` [PATCH v5 10/15] qapi/introspect.py: improve _tree_to_qlit error message John Snow
` (5 subsequent siblings)
14 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2021-02-04 0:32 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Presently, we use a tuple to attach a dict containing annotations
(comments and compile-time conditionals) to a tree node. This is
undesirable because dicts are difficult to strongly type; promoting it
to a real class allows us to name the values and types of the
annotations we are expecting.
In terms of typing, the Annotated<T> type serves as a generic container
where the annotated node's type is preserved, allowing for greater
specificity than we'd be able to provide without a generic.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 77 ++++++++++++++++++++++----------------
1 file changed, 44 insertions(+), 33 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 8e019b4a26a..b9427aba449 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -13,8 +13,12 @@
from typing import (
Any,
Dict,
+ Generic,
+ Iterable,
List,
Optional,
+ Tuple,
+ TypeVar,
Union,
)
@@ -51,15 +55,25 @@
_scalar = Union[str, bool, None]
_nonscalar = Union[Dict[str, _stub], List[_stub]]
_value = Union[_scalar, _nonscalar]
-# TreeValue = TODO, in a forthcoming commit.
+TreeValue = Union[_value, 'Annotated[_value]']
-def _make_tree(obj, ifcond, comment=None):
- extra = {
- 'if': ifcond,
- 'comment': comment
- }
- return (obj, extra)
+_NodeT = TypeVar('_NodeT', bound=_value)
+
+
+class Annotated(Generic[_NodeT]):
+ """
+ Annotated generally contains a SchemaInfo-like type (as a dict),
+ But it also used to wrap comments/ifconds around scalar leaf values,
+ for the benefit of features and enums.
+ """
+ # TODO: Remove after Python 3.7 adds @dataclass:
+ # pylint: disable=too-few-public-methods
+ def __init__(self, value: _NodeT, ifcond: Iterable[str],
+ comment: Optional[str] = None):
+ self.value = value
+ self.comment: Optional[str] = comment
+ self.ifcond: Tuple[str, ...] = tuple(ifcond)
def _tree_to_qlit(obj, level=0, dict_value=False):
@@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, dict_value=False):
def indent(level):
return level * 4 * ' '
- if isinstance(obj, tuple):
- ifobj, extra = obj
- ifcond = extra.get('if')
- comment = extra.get('comment')
-
+ if isinstance(obj, Annotated):
# NB: _tree_to_qlit is called recursively on the values of a key:value
# pair; those values can't be decorated with comments or conditionals.
msg = "dict values cannot have attached comments or if-conditionals."
assert not dict_value, msg
ret = ''
- if comment:
- ret += indent(level) + '/* %s */\n' % comment
- if ifcond:
- ret += gen_if(ifcond)
- ret += _tree_to_qlit(ifobj, level)
- if ifcond:
- ret += '\n' + gen_endif(ifcond)
+ if obj.comment:
+ ret += indent(level) + '/* %s */\n' % obj.comment
+ if obj.ifcond:
+ ret += gen_if(obj.ifcond)
+ ret += _tree_to_qlit(obj.value, level)
+ if obj.ifcond:
+ ret += '\n' + gen_endif(obj.ifcond)
return ret
ret = ''
@@ -201,7 +211,7 @@ def _use_type(self, typ):
@staticmethod
def _gen_features(features):
- return [_make_tree(f.name, f.ifcond) for f in features]
+ return [Annotated(f.name, f.ifcond) for f in features]
def _gen_tree(self, name, mtype, obj, ifcond, features):
comment: Optional[str] = None
@@ -215,7 +225,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
obj['meta-type'] = mtype
if features:
obj['features'] = self._gen_features(features)
- self._trees.append(_make_tree(obj, ifcond, comment))
+ self._trees.append(Annotated(obj, ifcond, comment))
def _gen_member(self, member):
obj = {'name': member.name, 'type': self._use_type(member.type)}
@@ -223,7 +233,7 @@ def _gen_member(self, member):
obj['default'] = None
if member.features:
obj['features'] = self._gen_features(member.features)
- return _make_tree(obj, member.ifcond)
+ return Annotated(obj, member.ifcond)
def _gen_variants(self, tag_name, variants):
return {'tag': tag_name,
@@ -231,16 +241,17 @@ def _gen_variants(self, tag_name, variants):
def _gen_variant(self, variant):
obj = {'case': variant.name, 'type': self._use_type(variant.type)}
- return _make_tree(obj, variant.ifcond)
+ return Annotated(obj, variant.ifcond)
def visit_builtin_type(self, name, info, json_type):
self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
def visit_enum_type(self, name, info, ifcond, features, members, prefix):
- self._gen_tree(name, 'enum',
- {'values': [_make_tree(m.name, m.ifcond, None)
- for m in members]},
- ifcond, features)
+ self._gen_tree(
+ name, 'enum',
+ {'values': [Annotated(m.name, m.ifcond) for m in members]},
+ ifcond, features
+ )
def visit_array_type(self, name, info, ifcond, element_type):
element = self._use_type(element_type)
@@ -257,12 +268,12 @@ def visit_object_type_flat(self, name, info, ifcond, features,
self._gen_tree(name, 'object', obj, ifcond, features)
def visit_alternate_type(self, name, info, ifcond, features, variants):
- self._gen_tree(name, 'alternate',
- {'members': [
- _make_tree({'type': self._use_type(m.type)},
- m.ifcond, None)
- for m in variants.variants]},
- ifcond, features)
+ self._gen_tree(
+ name, 'alternate',
+ {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
+ for m in variants.variants]},
+ ifcond, features
+ )
def visit_command(self, name, info, ifcond, features,
arg_type, ret_type, gen, success_response, boxed,
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 09/15] qapi/introspect.py: create a typed 'Annotated' data strutcure
2021-02-04 0:32 ` [PATCH v5 09/15] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
@ 2021-02-08 14:36 ` Markus Armbruster
2021-02-08 20:53 ` John Snow
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-08 14:36 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Presently, we use a tuple to attach a dict containing annotations
> (comments and compile-time conditionals) to a tree node. This is
> undesirable because dicts are difficult to strongly type; promoting it
> to a real class allows us to name the values and types of the
> annotations we are expecting.
>
> In terms of typing, the Annotated<T> type serves as a generic container
> where the annotated node's type is preserved, allowing for greater
> specificity than we'd be able to provide without a generic.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 77 ++++++++++++++++++++++----------------
> 1 file changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 8e019b4a26a..b9427aba449 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -13,8 +13,12 @@
> from typing import (
> Any,
> Dict,
> + Generic,
> + Iterable,
> List,
> Optional,
> + Tuple,
> + TypeVar,
> Union,
> )
>
> @@ -51,15 +55,25 @@
> _scalar = Union[str, bool, None]
> _nonscalar = Union[Dict[str, _stub], List[_stub]]
> _value = Union[_scalar, _nonscalar]
> -# TreeValue = TODO, in a forthcoming commit.
> +TreeValue = Union[_value, 'Annotated[_value]']
>
>
> -def _make_tree(obj, ifcond, comment=None):
> - extra = {
> - 'if': ifcond,
> - 'comment': comment
> - }
> - return (obj, extra)
> +_NodeT = TypeVar('_NodeT', bound=_value)
> +
> +
> +class Annotated(Generic[_NodeT]):
My gut feeling is "generic type is overkill for this purpose". Let's go
with it anyway, because
1. It's not wrong.
2. I don't have enough experience with Python type hints for reliable
gut feelings.
3. I plan to overhaul the C generation part relatively soon (after your
work has landed, don't worry), and I can try to make it simpler then.
> + """
> + Annotated generally contains a SchemaInfo-like type (as a dict),
> + But it also used to wrap comments/ifconds around scalar leaf values,
> + for the benefit of features and enums.
> + """
> + # TODO: Remove after Python 3.7 adds @dataclass:
> + # pylint: disable=too-few-public-methods
> + def __init__(self, value: _NodeT, ifcond: Iterable[str],
> + comment: Optional[str] = None):
> + self.value = value
> + self.comment: Optional[str] = comment
> + self.ifcond: Tuple[str, ...] = tuple(ifcond)
>
>
> def _tree_to_qlit(obj, level=0, dict_value=False):
> @@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, dict_value=False):
> def indent(level):
> return level * 4 * ' '
>
> - if isinstance(obj, tuple):
> - ifobj, extra = obj
> - ifcond = extra.get('if')
> - comment = extra.get('comment')
> -
> + if isinstance(obj, Annotated):
> # NB: _tree_to_qlit is called recursively on the values of a key:value
> # pair; those values can't be decorated with comments or conditionals.
> msg = "dict values cannot have attached comments or if-conditionals."
> assert not dict_value, msg
>
> ret = ''
> - if comment:
> - ret += indent(level) + '/* %s */\n' % comment
> - if ifcond:
> - ret += gen_if(ifcond)
> - ret += _tree_to_qlit(ifobj, level)
> - if ifcond:
> - ret += '\n' + gen_endif(ifcond)
> + if obj.comment:
> + ret += indent(level) + '/* %s */\n' % obj.comment
> + if obj.ifcond:
> + ret += gen_if(obj.ifcond)
> + ret += _tree_to_qlit(obj.value, level)
> + if obj.ifcond:
> + ret += '\n' + gen_endif(obj.ifcond)
> return ret
>
> ret = ''
> @@ -201,7 +211,7 @@ def _use_type(self, typ):
>
> @staticmethod
> def _gen_features(features):
> - return [_make_tree(f.name, f.ifcond) for f in features]
> + return [Annotated(f.name, f.ifcond) for f in features]
>
> def _gen_tree(self, name, mtype, obj, ifcond, features):
> comment: Optional[str] = None
> @@ -215,7 +225,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
> obj['meta-type'] = mtype
> if features:
> obj['features'] = self._gen_features(features)
> - self._trees.append(_make_tree(obj, ifcond, comment))
> + self._trees.append(Annotated(obj, ifcond, comment))
>
> def _gen_member(self, member):
> obj = {'name': member.name, 'type': self._use_type(member.type)}
> @@ -223,7 +233,7 @@ def _gen_member(self, member):
> obj['default'] = None
> if member.features:
> obj['features'] = self._gen_features(member.features)
> - return _make_tree(obj, member.ifcond)
> + return Annotated(obj, member.ifcond)
>
> def _gen_variants(self, tag_name, variants):
> return {'tag': tag_name,
> @@ -231,16 +241,17 @@ def _gen_variants(self, tag_name, variants):
>
> def _gen_variant(self, variant):
> obj = {'case': variant.name, 'type': self._use_type(variant.type)}
> - return _make_tree(obj, variant.ifcond)
> + return Annotated(obj, variant.ifcond)
>
> def visit_builtin_type(self, name, info, json_type):
> self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>
> def visit_enum_type(self, name, info, ifcond, features, members, prefix):
> - self._gen_tree(name, 'enum',
> - {'values': [_make_tree(m.name, m.ifcond, None)
> - for m in members]},
> - ifcond, features)
> + self._gen_tree(
> + name, 'enum',
> + {'values': [Annotated(m.name, m.ifcond) for m in members]},
> + ifcond, features
> + )
>
> def visit_array_type(self, name, info, ifcond, element_type):
> element = self._use_type(element_type)
> @@ -257,12 +268,12 @@ def visit_object_type_flat(self, name, info, ifcond, features,
> self._gen_tree(name, 'object', obj, ifcond, features)
>
> def visit_alternate_type(self, name, info, ifcond, features, variants):
> - self._gen_tree(name, 'alternate',
> - {'members': [
> - _make_tree({'type': self._use_type(m.type)},
> - m.ifcond, None)
> - for m in variants.variants]},
> - ifcond, features)
> + self._gen_tree(
> + name, 'alternate',
> + {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
> + for m in variants.variants]},
Slightly more readable, I think:
{'members': [Annotated({'type': self._use_type(m.type)},
m.ifcond)
for m in variants.variants]},
> + ifcond, features
> + )
>
> def visit_command(self, name, info, ifcond, features,
> arg_type, ret_type, gen, success_response, boxed,
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 09/15] qapi/introspect.py: create a typed 'Annotated' data strutcure
2021-02-08 14:36 ` Markus Armbruster
@ 2021-02-08 20:53 ` John Snow
0 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-08 20:53 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/8/21 9:36 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Presently, we use a tuple to attach a dict containing annotations
>> (comments and compile-time conditionals) to a tree node. This is
>> undesirable because dicts are difficult to strongly type; promoting it
>> to a real class allows us to name the values and types of the
>> annotations we are expecting.
>>
>> In terms of typing, the Annotated<T> type serves as a generic container
>> where the annotated node's type is preserved, allowing for greater
>> specificity than we'd be able to provide without a generic.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 77 ++++++++++++++++++++++----------------
>> 1 file changed, 44 insertions(+), 33 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 8e019b4a26a..b9427aba449 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -13,8 +13,12 @@
>> from typing import (
>> Any,
>> Dict,
>> + Generic,
>> + Iterable,
>> List,
>> Optional,
>> + Tuple,
>> + TypeVar,
>> Union,
>> )
>>
>> @@ -51,15 +55,25 @@
>> _scalar = Union[str, bool, None]
>> _nonscalar = Union[Dict[str, _stub], List[_stub]]
>> _value = Union[_scalar, _nonscalar]
>> -# TreeValue = TODO, in a forthcoming commit.
>> +TreeValue = Union[_value, 'Annotated[_value]']
>>
>>
>> -def _make_tree(obj, ifcond, comment=None):
>> - extra = {
>> - 'if': ifcond,
>> - 'comment': comment
>> - }
>> - return (obj, extra)
>> +_NodeT = TypeVar('_NodeT', bound=_value)
>> +
>> +
>> +class Annotated(Generic[_NodeT]):
>
> My gut feeling is "generic type is overkill for this purpose". Let's go
> with it anyway, because
>
> 1. It's not wrong.
>
A famous phrase in Computer Science.
> 2. I don't have enough experience with Python type hints for reliable
> gut feelings.
>
You are exactly correct that the power it offers us here isn't strictly
necessary. An argument might be that removing it makes the types easier
to read, but I think at a certain level of involvement with mypy that it
isn't feasible to escape understanding Generics, and we are at that level.
> 3. I plan to overhaul the C generation part relatively soon (after your
> work has landed, don't worry), and I can try to make it simpler then.
>
Yeah. The generation and typing can likely improve substantially at that
point in time. Hopefully the type hints help guide a design that's nice
to type and nice to read.
>> + """
>> + Annotated generally contains a SchemaInfo-like type (as a dict),
>> + But it also used to wrap comments/ifconds around scalar leaf values,
>> + for the benefit of features and enums.
>> + """
>> + # TODO: Remove after Python 3.7 adds @dataclass:
>> + # pylint: disable=too-few-public-methods
>> + def __init__(self, value: _NodeT, ifcond: Iterable[str],
>> + comment: Optional[str] = None):
>> + self.value = value
>> + self.comment: Optional[str] = comment
>> + self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>
>>
>> def _tree_to_qlit(obj, level=0, dict_value=False):
>> @@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, dict_value=False):
>> def indent(level):
>> return level * 4 * ' '
>>
>> - if isinstance(obj, tuple):
>> - ifobj, extra = obj
>> - ifcond = extra.get('if')
>> - comment = extra.get('comment')
>> -
>> + if isinstance(obj, Annotated):
>> # NB: _tree_to_qlit is called recursively on the values of a key:value
>> # pair; those values can't be decorated with comments or conditionals.
>> msg = "dict values cannot have attached comments or if-conditionals."
>> assert not dict_value, msg
>>
>> ret = ''
>> - if comment:
>> - ret += indent(level) + '/* %s */\n' % comment
>> - if ifcond:
>> - ret += gen_if(ifcond)
>> - ret += _tree_to_qlit(ifobj, level)
>> - if ifcond:
>> - ret += '\n' + gen_endif(ifcond)
>> + if obj.comment:
>> + ret += indent(level) + '/* %s */\n' % obj.comment
>> + if obj.ifcond:
>> + ret += gen_if(obj.ifcond)
>> + ret += _tree_to_qlit(obj.value, level)
>> + if obj.ifcond:
>> + ret += '\n' + gen_endif(obj.ifcond)
>> return ret
>>
>> ret = ''
>> @@ -201,7 +211,7 @@ def _use_type(self, typ):
>>
>> @staticmethod
>> def _gen_features(features):
>> - return [_make_tree(f.name, f.ifcond) for f in features]
>> + return [Annotated(f.name, f.ifcond) for f in features]
>>
>> def _gen_tree(self, name, mtype, obj, ifcond, features):
>> comment: Optional[str] = None
>> @@ -215,7 +225,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>> obj['meta-type'] = mtype
>> if features:
>> obj['features'] = self._gen_features(features)
>> - self._trees.append(_make_tree(obj, ifcond, comment))
>> + self._trees.append(Annotated(obj, ifcond, comment))
>>
>> def _gen_member(self, member):
>> obj = {'name': member.name, 'type': self._use_type(member.type)}
>> @@ -223,7 +233,7 @@ def _gen_member(self, member):
>> obj['default'] = None
>> if member.features:
>> obj['features'] = self._gen_features(member.features)
>> - return _make_tree(obj, member.ifcond)
>> + return Annotated(obj, member.ifcond)
>>
>> def _gen_variants(self, tag_name, variants):
>> return {'tag': tag_name,
>> @@ -231,16 +241,17 @@ def _gen_variants(self, tag_name, variants):
>>
>> def _gen_variant(self, variant):
>> obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>> - return _make_tree(obj, variant.ifcond)
>> + return Annotated(obj, variant.ifcond)
>>
>> def visit_builtin_type(self, name, info, json_type):
>> self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>>
>> def visit_enum_type(self, name, info, ifcond, features, members, prefix):
>> - self._gen_tree(name, 'enum',
>> - {'values': [_make_tree(m.name, m.ifcond, None)
>> - for m in members]},
>> - ifcond, features)
>> + self._gen_tree(
>> + name, 'enum',
>> + {'values': [Annotated(m.name, m.ifcond) for m in members]},
>> + ifcond, features
>> + )
>>
>> def visit_array_type(self, name, info, ifcond, element_type):
>> element = self._use_type(element_type)
>> @@ -257,12 +268,12 @@ def visit_object_type_flat(self, name, info, ifcond, features,
>> self._gen_tree(name, 'object', obj, ifcond, features)
>>
>> def visit_alternate_type(self, name, info, ifcond, features, variants):
>> - self._gen_tree(name, 'alternate',
>> - {'members': [
>> - _make_tree({'type': self._use_type(m.type)},
>> - m.ifcond, None)
>> - for m in variants.variants]},
>> - ifcond, features)
>> + self._gen_tree(
>> + name, 'alternate',
>> + {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
>> + for m in variants.variants]},
>
> Slightly more readable, I think:
>
> {'members': [Annotated({'type': self._use_type(m.type)},
> m.ifcond)
> for m in variants.variants]},
>
OK, but only because I am being annoying about not capitulating
elsewhere on equally trivial matters.
>> + ifcond, features
>> + )
>>
>> def visit_command(self, name, info, ifcond, features,
>> arg_type, ret_type, gen, success_response, boxed,
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 10/15] qapi/introspect.py: improve _tree_to_qlit error message
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (8 preceding siblings ...)
2021-02-04 0:32 ` [PATCH v5 09/15] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
@ 2021-02-04 0:32 ` John Snow
2021-02-04 0:32 ` [PATCH v5 11/15] qapi/introspect.py: improve readability of _tree_to_qlit John Snow
` (4 subsequent siblings)
14 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-04 0:32 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Trivial; make the error message just a pinch more explicit in case we
trip this by accident in the future.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b9427aba449..0146e4c6a0c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -124,7 +124,9 @@ def indent(level):
elif isinstance(obj, bool):
ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
else:
- assert False # not implemented
+ raise NotImplementedError(
+ f"type '{type(obj).__name__}' not implemented"
+ )
if level > 0:
ret += ','
return ret
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 11/15] qapi/introspect.py: improve readability of _tree_to_qlit
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (9 preceding siblings ...)
2021-02-04 0:32 ` [PATCH v5 10/15] qapi/introspect.py: improve _tree_to_qlit error message John Snow
@ 2021-02-04 0:32 ` John Snow
2021-02-04 0:32 ` [PATCH v5 12/15] qapi/introspect.py: add type hint annotations John Snow
` (3 subsequent siblings)
14 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-04 0:32 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Subjective, but I find getting rid of the comprehensions helps. Also,
divide the sections into scalar and non-scalar sections, and remove
old-style string formatting.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 0146e4c6a0c..cf0e4e05c5c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -89,7 +89,7 @@ def indent(level):
ret = ''
if obj.comment:
- ret += indent(level) + '/* %s */\n' % obj.comment
+ ret += indent(level) + f"/* {obj.comment} */\n"
if obj.ifcond:
ret += gen_if(obj.ifcond)
ret += _tree_to_qlit(obj.value, level)
@@ -100,33 +100,36 @@ def indent(level):
ret = ''
if not dict_value:
ret += indent(level)
+
+ # Scalars:
if obj is None:
ret += 'QLIT_QNULL'
elif isinstance(obj, str):
- ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
+ ret += f"QLIT_QSTR({to_c_string(obj)})"
+ elif isinstance(obj, bool):
+ ret += f"QLIT_QBOOL({str(obj).lower()})"
+
+ # Non-scalars:
elif isinstance(obj, list):
- elts = [_tree_to_qlit(elt, level + 1).strip('\n')
- for elt in obj]
- elts.append(indent(level + 1) + "{}")
ret += 'QLIT_QLIST(((QLitObject[]) {\n'
- ret += '\n'.join(elts) + '\n'
+ for value in obj:
+ ret += _tree_to_qlit(value, level + 1).strip('\n') + '\n'
+ ret += indent(level + 1) + '{}\n'
ret += indent(level) + '}))'
elif isinstance(obj, dict):
- elts = []
- for key, value in sorted(obj.items()):
- elts.append(indent(level + 1) + '{ %s, %s }' %
- (to_c_string(key),
- _tree_to_qlit(value, level + 1, True)))
- elts.append(indent(level + 1) + '{}')
ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n'
- ret += ',\n'.join(elts) + '\n'
+ for key, value in sorted(obj.items()):
+ ret += indent(level + 1) + "{{ {:s}, {:s} }},\n".format(
+ to_c_string(key),
+ _tree_to_qlit(value, level + 1, dict_value=True)
+ )
+ ret += indent(level + 1) + '{}\n'
ret += indent(level) + '}))'
- elif isinstance(obj, bool):
- ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
else:
raise NotImplementedError(
f"type '{type(obj).__name__}' not implemented"
)
+
if level > 0:
ret += ','
return ret
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5 12/15] qapi/introspect.py: add type hint annotations
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (10 preceding siblings ...)
2021-02-04 0:32 ` [PATCH v5 11/15] qapi/introspect.py: improve readability of _tree_to_qlit John Snow
@ 2021-02-04 0:32 ` John Snow
2021-02-08 15:34 ` Markus Armbruster
2021-02-04 0:32 ` [PATCH v5 13/15] qapi/introspect.py: add introspect.json dummy types John Snow
` (2 subsequent siblings)
14 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2021-02-04 0:32 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
---
See the next patch for an optional amendment that helps to clarify what
_DObject is meant to be.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 117 ++++++++++++++++++++++++++-----------
scripts/qapi/mypy.ini | 5 --
scripts/qapi/schema.py | 2 +-
3 files changed, 84 insertions(+), 40 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index cf0e4e05c5c..3afcdda7446 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -30,10 +30,19 @@
)
from .gen import QAPISchemaMonolithicCVisitor
from .schema import (
+ QAPISchema,
QAPISchemaArrayType,
QAPISchemaBuiltinType,
+ QAPISchemaEntity,
+ QAPISchemaEnumMember,
+ QAPISchemaFeature,
+ QAPISchemaObjectType,
+ QAPISchemaObjectTypeMember,
QAPISchemaType,
+ QAPISchemaVariant,
+ QAPISchemaVariants,
)
+from .source import QAPISourceInfo
# This module constructs a tree data structure that is used to
@@ -57,6 +66,10 @@
_value = Union[_scalar, _nonscalar]
TreeValue = Union[_value, 'Annotated[_value]']
+# This is an alias for an arbitrary JSON object, represented here as a Dict.
+# It is stricter on the value type than the recursive definition above.
+# It is used to represent SchemaInfo-related structures exclusively.
+_DObject = Dict[str, object]
_NodeT = TypeVar('_NodeT', bound=_value)
@@ -76,9 +89,11 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
self.ifcond: Tuple[str, ...] = tuple(ifcond)
-def _tree_to_qlit(obj, level=0, dict_value=False):
+def _tree_to_qlit(obj: TreeValue,
+ level: int = 0,
+ dict_value: bool = False) -> str:
- def indent(level):
+ def indent(level: int) -> str:
return level * 4 * ' '
if isinstance(obj, Annotated):
@@ -135,21 +150,21 @@ 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):
- 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[Annotated[_DObject]] = []
+ self._used_types: List[QAPISchemaType] = []
+ self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
#include "qemu/osdep.h"
#include "%(prefix)sqapi-introspect.h"
@@ -157,10 +172,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)
@@ -182,18 +197,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:
assert self._schema is not None
# Map the various integer types to plain int
@@ -215,10 +230,13 @@ def _use_type(self, typ):
return self._name(typ.name)
@staticmethod
- def _gen_features(features):
+ def _gen_features(features: List[QAPISchemaFeature]
+ ) -> List[Annotated[str]]:
return [Annotated(f.name, f.ifcond) for f in features]
- def _gen_tree(self, name, mtype, obj, ifcond, features):
+ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
+ ifcond: List[str],
+ features: Optional[List[QAPISchemaFeature]]) -> None:
comment: Optional[str] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
@@ -232,47 +250,67 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
obj['features'] = self._gen_features(features)
self._trees.append(Annotated(obj, ifcond, comment))
- def _gen_member(self, member):
- obj = {'name': member.name, 'type': self._use_type(member.type)}
+ def _gen_member(self, member: QAPISchemaObjectTypeMember
+ ) -> Annotated[_DObject]:
+ obj: _DObject = {
+ 'name': member.name,
+ 'type': self._use_type(member.type)
+ }
if member.optional:
obj['default'] = None
if member.features:
obj['features'] = self._gen_features(member.features)
return Annotated(obj, member.ifcond)
- 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) -> Annotated[_DObject]:
+ obj: _DObject = {
+ 'case': variant.name,
+ 'type': self._use_type(variant.type)
+ }
return Annotated(obj, variant.ifcond)
- def visit_builtin_type(self, name, info, json_type):
+ def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
+ 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: Optional[QAPISourceInfo],
+ ifcond: List[str], features: List[QAPISchemaFeature],
+ members: List[QAPISchemaEnumMember],
+ prefix: Optional[str]) -> None:
self._gen_tree(
name, 'enum',
{'values': [Annotated(m.name, m.ifcond) 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],
+ 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],
+ ifcond: List[str],
+ features: List[QAPISchemaFeature],
+ members: List[QAPISchemaObjectTypeMember],
+ variants: Optional[QAPISchemaVariants]) -> None:
+ 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: Optional[QAPISourceInfo],
+ ifcond: List[str],
+ features: List[QAPISchemaFeature],
+ variants: QAPISchemaVariants) -> None:
self._gen_tree(
name, 'alternate',
{'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
@@ -280,27 +318,38 @@ def visit_alternate_type(self, name, info, ifcond, features, 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: Optional[QAPISourceInfo],
+ ifcond: List[str],
+ features: List[QAPISchemaFeature],
+ arg_type: Optional[QAPISchemaObjectType],
+ ret_type: Optional[QAPISchemaType], gen: bool,
+ success_response: bool, boxed: bool, allow_oob: bool,
+ allow_preconfig: bool, coroutine: bool) -> None:
assert self._schema is not 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: Optional[QAPISourceInfo],
+ ifcond: List[str], features: List[QAPISchemaFeature],
+ arg_type: Optional[QAPISchemaObjectType],
+ boxed: bool) -> None:
assert self._schema is not 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 04bd5db5278..0a000d58b37 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -13,11 +13,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 353e8020a27..ff16578f6de 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)
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 12/15] qapi/introspect.py: add type hint annotations
2021-02-04 0:32 ` [PATCH v5 12/15] qapi/introspect.py: add type hint annotations John Snow
@ 2021-02-08 15:34 ` Markus Armbruster
2021-02-08 20:45 ` John Snow
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-08 15:34 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> See the next patch for an optional amendment that helps to clarify what
> _DObject is meant to be.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 117 ++++++++++++++++++++++++++-----------
> scripts/qapi/mypy.ini | 5 --
> scripts/qapi/schema.py | 2 +-
> 3 files changed, 84 insertions(+), 40 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index cf0e4e05c5c..3afcdda7446 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -30,10 +30,19 @@
> )
> from .gen import QAPISchemaMonolithicCVisitor
> from .schema import (
> + QAPISchema,
> QAPISchemaArrayType,
> QAPISchemaBuiltinType,
> + QAPISchemaEntity,
> + QAPISchemaEnumMember,
> + QAPISchemaFeature,
> + QAPISchemaObjectType,
> + QAPISchemaObjectTypeMember,
> QAPISchemaType,
> + QAPISchemaVariant,
> + QAPISchemaVariants,
> )
> +from .source import QAPISourceInfo
>
>
> # This module constructs a tree data structure that is used to
> @@ -57,6 +66,10 @@
_stub = Any
_scalar = Union[str, bool, None]
_nonscalar = Union[Dict[str, _stub], List[_stub]]
> _value = Union[_scalar, _nonscalar]
> TreeValue = Union[_value, 'Annotated[_value]']
>
> +# This is an alias for an arbitrary JSON object, represented here as a Dict.
> +# It is stricter on the value type than the recursive definition above.
> +# It is used to represent SchemaInfo-related structures exclusively.
> +_DObject = Dict[str, object]
Please work in an abridged version of your helpful reply to my ignorant
questions in review of v4.
My comments below are based on the following understanding:
_value has a Dict[str, Any] branch.
_DObject is Dict[str, object].
Both types are for the tree's dict nodes. Both under-constrain the
dict's values in the sense that anything can go into the dict. The
difference is static type checking on use of dict values: none with the
former, overly strict with the latter (to actually do something
interesting with a value, you usually have to narrow its static type to
a more specific one than object).
So, having _DObject in addition to the _value branch buys us a little
more static type checking. I don't understand what type checking
exactly, and therefore can't judge whether it's worth the price in
complexity. If you can enlighten me, I'm all ears.
>
> _NodeT = TypeVar('_NodeT', bound=_value)
>
> @@ -76,9 +89,11 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
> self.ifcond: Tuple[str, ...] = tuple(ifcond)
>
>
> -def _tree_to_qlit(obj, level=0, dict_value=False):
> +def _tree_to_qlit(obj: TreeValue,
> + level: int = 0,
> + dict_value: bool = False) -> str:
>
> - def indent(level):
> + def indent(level: int) -> str:
> return level * 4 * ' '
>
> if isinstance(obj, Annotated):
> @@ -135,21 +150,21 @@ 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):
>
> - 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[Annotated[_DObject]] = []
> + self._used_types: List[QAPISchemaType] = []
> + self._name_map: Dict[str, str] = {}
> self._genc.add(mcgen('''
> #include "qemu/osdep.h"
> #include "%(prefix)sqapi-introspect.h"
> @@ -157,10 +172,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)
> @@ -182,18 +197,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:
> assert self._schema is not None
>
> # Map the various integer types to plain int
> @@ -215,10 +230,13 @@ def _use_type(self, typ):
> return self._name(typ.name)
>
> @staticmethod
> - def _gen_features(features):
> + def _gen_features(features: List[QAPISchemaFeature]
> + ) -> List[Annotated[str]]:
> return [Annotated(f.name, f.ifcond) for f in features]
>
> - def _gen_tree(self, name, mtype, obj, ifcond, features):
> + def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> + ifcond: List[str],
> + features: Optional[List[QAPISchemaFeature]]) -> None:
Recycling my review of v2...
* @name corresponds to SchemaInfo member name, which is indeed str.
Okay.
* @mtype corresponds to member meta-type, which is enum SchemaMetaType.
It's not a Python Enum, because those were off limits when this code
was written. We type what we have, and we have str. Okay.
* @obj will be turned by _gen_tree() into the tree for a SchemaInfo.
_DObject fits the bill.
* ifcond: List[str] should work, but gen_if(), gen_endif() use
Sequence[str]. When I pointed this out for v2, you replied "should
probable be using Sequence".
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.
> comment: Optional[str] = None
"No annotations" is represented as None here, not {}. I guess we could
use {} for simpler typing. When I pointed this out for v2, you replied
it'll go away later. Good enough for me.
> if mtype not in ('command', 'event', 'builtin', 'array'):
> if not self._unmask:
> @@ -232,47 +250,67 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
> obj['features'] = self._gen_features(features)
> self._trees.append(Annotated(obj, ifcond, comment))
>
> - def _gen_member(self, member):
> - obj = {'name': member.name, 'type': self._use_type(member.type)}
> + def _gen_member(self, member: QAPISchemaObjectTypeMember
> + ) -> Annotated[_DObject]:
> + obj: _DObject = {
> + 'name': member.name,
> + 'type': self._use_type(member.type)
> + }
> if member.optional:
> obj['default'] = None
> if member.features:
> obj['features'] = self._gen_features(member.features)
> return Annotated(obj, member.ifcond)
>
> - 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) -> Annotated[_DObject]:
> + obj: _DObject = {
> + 'case': variant.name,
> + 'type': self._use_type(variant.type)
> + }
> return Annotated(obj, variant.ifcond)
>
> - def visit_builtin_type(self, name, info, json_type):
> + def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
> + json_type: str) -> None:
A built-in type's info is always None. Perhaps we should drop the
parameter.
> 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: Optional[QAPISourceInfo],
> + ifcond: List[str], features: List[QAPISchemaFeature],
> + members: List[QAPISchemaEnumMember],
> + prefix: Optional[str]) -> None:
> self._gen_tree(
> name, 'enum',
> {'values': [Annotated(m.name, m.ifcond) 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],
> + 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],
> + ifcond: List[str],
> + features: List[QAPISchemaFeature],
> + members: List[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.
When I pointed this out for v2, you replied we should turn
QAPISchemaVariants into an extension of Sequence[QAPISchemaVariant] in a
later series. Okay.
> + 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: Optional[QAPISourceInfo],
> + ifcond: List[str],
> + features: List[QAPISchemaFeature],
> + variants: QAPISchemaVariants) -> None:
> self._gen_tree(
> name, 'alternate',
> {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
> @@ -280,27 +318,38 @@ def visit_alternate_type(self, name, info, ifcond, features, 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: Optional[QAPISourceInfo],
> + ifcond: List[str],
> + features: List[QAPISchemaFeature],
> + arg_type: Optional[QAPISchemaObjectType],
> + ret_type: Optional[QAPISchemaType], gen: bool,
> + success_response: bool, boxed: bool, allow_oob: bool,
> + allow_preconfig: bool, coroutine: bool) -> None:
> assert self._schema is not 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: Optional[QAPISourceInfo],
> + ifcond: List[str], features: List[QAPISchemaFeature],
> + arg_type: Optional[QAPISchemaObjectType],
> + boxed: bool) -> None:
> assert self._schema is not 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 04bd5db5278..0a000d58b37 100644
> --- a/scripts/qapi/mypy.ini
> +++ b/scripts/qapi/mypy.ini
> @@ -13,11 +13,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 353e8020a27..ff16578f6de 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)
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 12/15] qapi/introspect.py: add type hint annotations
2021-02-08 15:34 ` Markus Armbruster
@ 2021-02-08 20:45 ` John Snow
2021-02-09 9:17 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2021-02-08 20:45 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/8/21 10:34 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> ---
>>
>> See the next patch for an optional amendment that helps to clarify what
>> _DObject is meant to be.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 117 ++++++++++++++++++++++++++-----------
>> scripts/qapi/mypy.ini | 5 --
>> scripts/qapi/schema.py | 2 +-
>> 3 files changed, 84 insertions(+), 40 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index cf0e4e05c5c..3afcdda7446 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -30,10 +30,19 @@
>> )
>> from .gen import QAPISchemaMonolithicCVisitor
>> from .schema import (
>> + QAPISchema,
>> QAPISchemaArrayType,
>> QAPISchemaBuiltinType,
>> + QAPISchemaEntity,
>> + QAPISchemaEnumMember,
>> + QAPISchemaFeature,
>> + QAPISchemaObjectType,
>> + QAPISchemaObjectTypeMember,
>> QAPISchemaType,
>> + QAPISchemaVariant,
>> + QAPISchemaVariants,
>> )
>> +from .source import QAPISourceInfo
>>
>>
>> # This module constructs a tree data structure that is used to
>> @@ -57,6 +66,10 @@
> _stub = Any
> _scalar = Union[str, bool, None]
> _nonscalar = Union[Dict[str, _stub], List[_stub]]
>> _value = Union[_scalar, _nonscalar]
>> TreeValue = Union[_value, 'Annotated[_value]']
>>
>> +# This is an alias for an arbitrary JSON object, represented here as a Dict.
>> +# It is stricter on the value type than the recursive definition above.
>> +# It is used to represent SchemaInfo-related structures exclusively.
>> +_DObject = Dict[str, object]
>
> Please work in an abridged version of your helpful reply to my ignorant
> questions in review of v4.
>
Maybe not needed after we squash the named aliases patch in?.
> My comments below are based on the following understanding:
>
> _value has a Dict[str, Any] branch.
>
> _DObject is Dict[str, object].
>
> Both types are for the tree's dict nodes. Both under-constrain the
> dict's values in the sense that anything can go into the dict. The
> difference is static type checking on use of dict values: none with the
> former, overly strict with the latter (to actually do something
> interesting with a value, you usually have to narrow its static type to
> a more specific one than object).
>
> So, having _DObject in addition to the _value branch buys us a little
> more static type checking. I don't understand what type checking
> exactly, and therefore can't judge whether it's worth the price in
> complexity. If you can enlighten me, I'm all ears.
>
It enables type checking on any value that a _DObject (SchemaInfo and
its union arm types) may have.
foo: SchemaInfo = {} # Dict[str, object]
says that the values here must be an object. Everything is an object, so
this doesn't constrain much in the write direction, but it constrains a
lot in the read direction.
the type of this expression:
foo.get('key')
is object, not "Any". Which means if we do this:
x = foo.get('key') + 5
that is a type error for mypy. If we pass this value on to any function
that is looking for or expects a specific type, it will also be an
error. If we treat this value in any way beyond the capabilities of
Python's most basic, abstract type, it will be an error.
When you use 'Any', it actually *disables* type checking at that
boundary instead. If you pass an "Any" on to an interface that expects
"int", mypy will just assume you're right about that.
When I started this series I wasn't as clear on the distinction myself,
but during review and re-iteration I've shifted to using 'object'
absolutely whenever I can if I need to describe an abstract value.
Summary:
- I prefer "object" to "Any", when possible
- We have to use "Any" for the recursive stubs
- I use _DObject (and later SchemaInfo et al) to represent specific
types, not abstract recursive types, so they get the stricter 'object'.
>>
>> _NodeT = TypeVar('_NodeT', bound=_value)
>>
>> @@ -76,9 +89,11 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
>> self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>
>>
>> -def _tree_to_qlit(obj, level=0, dict_value=False):
>> +def _tree_to_qlit(obj: TreeValue,
>> + level: int = 0,
>> + dict_value: bool = False) -> str:
>>
>> - def indent(level):
>> + def indent(level: int) -> str:
>> return level * 4 * ' '
>>
>> if isinstance(obj, Annotated):
>> @@ -135,21 +150,21 @@ 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):
>>
>> - 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[Annotated[_DObject]] = []
>> + self._used_types: List[QAPISchemaType] = []
>> + self._name_map: Dict[str, str] = {}
>> self._genc.add(mcgen('''
>> #include "qemu/osdep.h"
>> #include "%(prefix)sqapi-introspect.h"
>> @@ -157,10 +172,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)
>> @@ -182,18 +197,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:
>> assert self._schema is not None
>>
>> # Map the various integer types to plain int
>> @@ -215,10 +230,13 @@ def _use_type(self, typ):
>> return self._name(typ.name)
>>
>> @staticmethod
>> - def _gen_features(features):
>> + def _gen_features(features: List[QAPISchemaFeature]
>> + ) -> List[Annotated[str]]:
>> return [Annotated(f.name, f.ifcond) for f in features]
>>
>> - def _gen_tree(self, name, mtype, obj, ifcond, features):
>> + def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> + ifcond: List[str],
>> + features: Optional[List[QAPISchemaFeature]]) -> None:
>
> Recycling my review of v2...
>
> * @name corresponds to SchemaInfo member name, which is indeed str.
> Okay.
>
> * @mtype corresponds to member meta-type, which is enum SchemaMetaType.
> It's not a Python Enum, because those were off limits when this code
> was written. We type what we have, and we have str. Okay.
>
I can *make* it an enum if you'd like. I'll throw it on the end of the
series as an optional patch that can be kept or dropped at your discretion.
> * @obj will be turned by _gen_tree() into the tree for a SchemaInfo.
> _DObject fits the bill.
>
> * ifcond: List[str] should work, but gen_if(), gen_endif() use
> Sequence[str]. When I pointed this out for v2, you replied "should
> probable be using Sequence".
>
> More instances of ifcond: List[str] elsewhere; I'm not flagging them.
>
Oh, yes.
List winds up being correct, but is unnecessarily restrictive. It
doesn't wind up mattering, but we can use Sequence[str].
I'll add a patch to fix the types-so-far at the beginning of the series,
and then squash the other annotation changes into this patch.
> * 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.
>
This is easy enough to make happen. I didn't notice.
I'll include it as an extra patch at the end of the series.
>> comment: Optional[str] = None
>
> "No annotations" is represented as None here, not {}. I guess we could
> use {} for simpler typing. When I pointed this out for v2, you replied
> it'll go away later. Good enough for me.
>
Something might have gotten garbled between then and now. I consider
"Annotations" to be both the if conditionals and comments; this field is
just the comment. Optional[str] is the right type for it here:
self._trees.append(Annotated(obj, ifcond, comment))
>> if mtype not in ('command', 'event', 'builtin', 'array'):
>> if not self._unmask:
>> @@ -232,47 +250,67 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>> obj['features'] = self._gen_features(features)
>> self._trees.append(Annotated(obj, ifcond, comment))
>>
>> - def _gen_member(self, member):
>> - obj = {'name': member.name, 'type': self._use_type(member.type)}
>> + def _gen_member(self, member: QAPISchemaObjectTypeMember
>> + ) -> Annotated[_DObject]:
>> + obj: _DObject = {
>> + 'name': member.name,
>> + 'type': self._use_type(member.type)
>> + }
>> if member.optional:
>> obj['default'] = None
>> if member.features:
>> obj['features'] = self._gen_features(member.features)
>> return Annotated(obj, member.ifcond)
>>
>> - 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) -> Annotated[_DObject]:
>> + obj: _DObject = {
>> + 'case': variant.name,
>> + 'type': self._use_type(variant.type)
>> + }
>> return Annotated(obj, variant.ifcond)
>>
>> - def visit_builtin_type(self, name, info, json_type):
>> + def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>> + json_type: str) -> None:
>
> A built-in type's info is always None. Perhaps we should drop the
> parameter.
>
We could.
For the moment, info cannot be "None" here because mypy thinks it
*might* be something.
...but we could just remove it for clarity. (As long as we don't later
re-add that built-in source info object style idea and then we'd want to
put it back, but ...)
Later?
>> 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: Optional[QAPISourceInfo],
>> + ifcond: List[str], features: List[QAPISchemaFeature],
>> + members: List[QAPISchemaEnumMember],
>> + prefix: Optional[str]) -> None:
>> self._gen_tree(
>> name, 'enum',
>> {'values': [Annotated(m.name, m.ifcond) 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],
>> + 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],
>> + ifcond: List[str],
>> + features: List[QAPISchemaFeature],
>> + members: List[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.
> When I pointed this out for v2, you replied we should turn
> QAPISchemaVariants into an extension of Sequence[QAPISchemaVariant] in a
> later series. Okay.
>
Yeah, that's a longer-term fix I think. Not for doing right now -- but
you're right, it stands out as suspect.
I think ideally we'd have a Variants object that behaves like a
collection where the empty collection is false-ish and we'd always
create such a collection.
(Though, that might create problems for tag_name vs tag_member where we
might now have circumstances where we have neither, maybe that makes
typing harder instead of easier. I'll need to experiment ... I have some
questions about how to achieve better typing for our collections class
that clash with the delayed-type-evaluation mechanisms we have. Ongoing
research.)
>> + 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: Optional[QAPISourceInfo],
>> + ifcond: List[str],
>> + features: List[QAPISchemaFeature],
>> + variants: QAPISchemaVariants) -> None:
>> self._gen_tree(
>> name, 'alternate',
>> {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
>> @@ -280,27 +318,38 @@ def visit_alternate_type(self, name, info, ifcond, features, 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: Optional[QAPISourceInfo],
>> + ifcond: List[str],
>> + features: List[QAPISchemaFeature],
>> + arg_type: Optional[QAPISchemaObjectType],
>> + ret_type: Optional[QAPISchemaType], gen: bool,
>> + success_response: bool, boxed: bool, allow_oob: bool,
>> + allow_preconfig: bool, coroutine: bool) -> None:
>> assert self._schema is not 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: Optional[QAPISourceInfo],
>> + ifcond: List[str], features: List[QAPISchemaFeature],
>> + arg_type: Optional[QAPISchemaObjectType],
>> + boxed: bool) -> None:
>> assert self._schema is not 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 04bd5db5278..0a000d58b37 100644
>> --- a/scripts/qapi/mypy.ini
>> +++ b/scripts/qapi/mypy.ini
>> @@ -13,11 +13,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 353e8020a27..ff16578f6de 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)
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 12/15] qapi/introspect.py: add type hint annotations
2021-02-08 20:45 ` John Snow
@ 2021-02-09 9:17 ` Markus Armbruster
2021-02-10 17:05 ` John Snow
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-09 9:17 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/8/21 10:34 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> ---
>>>
>>> See the next patch for an optional amendment that helps to clarify what
>>> _DObject is meant to be.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/introspect.py | 117 ++++++++++++++++++++++++++-----------
>>> scripts/qapi/mypy.ini | 5 --
>>> scripts/qapi/schema.py | 2 +-
>>> 3 files changed, 84 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index cf0e4e05c5c..3afcdda7446 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -30,10 +30,19 @@
>>> )
>>> from .gen import QAPISchemaMonolithicCVisitor
>>> from .schema import (
>>> + QAPISchema,
>>> QAPISchemaArrayType,
>>> QAPISchemaBuiltinType,
>>> + QAPISchemaEntity,
>>> + QAPISchemaEnumMember,
>>> + QAPISchemaFeature,
>>> + QAPISchemaObjectType,
>>> + QAPISchemaObjectTypeMember,
>>> QAPISchemaType,
>>> + QAPISchemaVariant,
>>> + QAPISchemaVariants,
>>> )
>>> +from .source import QAPISourceInfo
>>>
>>>
>>> # This module constructs a tree data structure that is used to
>>> @@ -57,6 +66,10 @@
>> _stub = Any
>> _scalar = Union[str, bool, None]
>> _nonscalar = Union[Dict[str, _stub], List[_stub]]
>>> _value = Union[_scalar, _nonscalar]
>>> TreeValue = Union[_value, 'Annotated[_value]']
>>>
>>> +# This is an alias for an arbitrary JSON object, represented here as a Dict.
>>> +# It is stricter on the value type than the recursive definition above.
>>> +# It is used to represent SchemaInfo-related structures exclusively.
>>> +_DObject = Dict[str, object]
>>
>> Please work in an abridged version of your helpful reply to my ignorant
>> questions in review of v4.
>>
>
> Maybe not needed after we squash the named aliases patch in?.
>
>> My comments below are based on the following understanding:
>>
>> _value has a Dict[str, Any] branch.
>>
>> _DObject is Dict[str, object].
>>
>> Both types are for the tree's dict nodes. Both under-constrain the
>> dict's values in the sense that anything can go into the dict. The
>> difference is static type checking on use of dict values: none with the
>> former, overly strict with the latter (to actually do something
>> interesting with a value, you usually have to narrow its static type to
>> a more specific one than object).
>>
>> So, having _DObject in addition to the _value branch buys us a little
>> more static type checking. I don't understand what type checking
>> exactly, and therefore can't judge whether it's worth the price in
>> complexity. If you can enlighten me, I'm all ears.
>>
>
> It enables type checking on any value that a _DObject (SchemaInfo and
> its union arm types) may have.
>
> foo: SchemaInfo = {} # Dict[str, object]
>
> says that the values here must be an object. Everything is an object, so
> this doesn't constrain much in the write direction, but it constrains a
> lot in the read direction.
>
> the type of this expression:
>
> foo.get('key')
>
> is object, not "Any". Which means if we do this:
>
> x = foo.get('key') + 5
>
> that is a type error for mypy. If we pass this value on to any function
> that is looking for or expects a specific type, it will also be an
> error. If we treat this value in any way beyond the capabilities of
> Python's most basic, abstract type, it will be an error.
>
> When you use 'Any', it actually *disables* type checking at that
> boundary instead. If you pass an "Any" on to an interface that expects
> "int", mypy will just assume you're right about that.
>
> When I started this series I wasn't as clear on the distinction myself,
> but during review and re-iteration I've shifted to using 'object'
> absolutely whenever I can if I need to describe an abstract value.
>
> Summary:
>
> - I prefer "object" to "Any", when possible
> - We have to use "Any" for the recursive stubs
> - I use _DObject (and later SchemaInfo et al) to represent specific
> types, not abstract recursive types, so they get the stricter 'object'.
I understand that now. I doubt it's worth the extra complexity here.
Being discussed in reply to PATCH 11. Let's continue there.
>>>
>>> _NodeT = TypeVar('_NodeT', bound=_value)
>>>
>>> @@ -76,9 +89,11 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
>>> self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>>
>>>
>>> -def _tree_to_qlit(obj, level=0, dict_value=False):
>>> +def _tree_to_qlit(obj: TreeValue,
>>> + level: int = 0,
>>> + dict_value: bool = False) -> str:
>>>
>>> - def indent(level):
>>> + def indent(level: int) -> str:
>>> return level * 4 * ' '
>>>
>>> if isinstance(obj, Annotated):
>>> @@ -135,21 +150,21 @@ 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):
>>>
>>> - 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[Annotated[_DObject]] = []
>>> + self._used_types: List[QAPISchemaType] = []
>>> + self._name_map: Dict[str, str] = {}
>>> self._genc.add(mcgen('''
>>> #include "qemu/osdep.h"
>>> #include "%(prefix)sqapi-introspect.h"
>>> @@ -157,10 +172,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)
>>> @@ -182,18 +197,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:
>>> assert self._schema is not None
>>>
>>> # Map the various integer types to plain int
>>> @@ -215,10 +230,13 @@ def _use_type(self, typ):
>>> return self._name(typ.name)
>>>
>>> @staticmethod
>>> - def _gen_features(features):
>>> + def _gen_features(features: List[QAPISchemaFeature]
>>> + ) -> List[Annotated[str]]:
>>> return [Annotated(f.name, f.ifcond) for f in features]
>>>
>>> - def _gen_tree(self, name, mtype, obj, ifcond, features):
>>> + def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>>> + ifcond: List[str],
>>> + features: Optional[List[QAPISchemaFeature]]) -> None:
>>
>> Recycling my review of v2...
>>
>> * @name corresponds to SchemaInfo member name, which is indeed str.
>> Okay.
>>
>> * @mtype corresponds to member meta-type, which is enum SchemaMetaType.
>> It's not a Python Enum, because those were off limits when this code
>> was written. We type what we have, and we have str. Okay.
>>
>
> I can *make* it an enum if you'd like. I'll throw it on the end of the
> series as an optional patch that can be kept or dropped at your discretion.
Let's stick stick to "type what we have", and also take notes on
possible improvements for later. We need this series to converge.
>> * @obj will be turned by _gen_tree() into the tree for a SchemaInfo.
>> _DObject fits the bill.
>>
>> * ifcond: List[str] should work, but gen_if(), gen_endif() use
>> Sequence[str]. When I pointed this out for v2, you replied "should
>> probable be using Sequence".
>>
>> More instances of ifcond: List[str] elsewhere; I'm not flagging them.
>>
>
> Oh, yes.
>
> List winds up being correct, but is unnecessarily restrictive. It
> doesn't wind up mattering, but we can use Sequence[str].
>
> I'll add a patch to fix the types-so-far at the beginning of the series,
> and then squash the other annotation changes into this patch.
Sounds good.
>> * 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.
>>
>
> This is easy enough to make happen. I didn't notice.
>
> I'll include it as an extra patch at the end of the series.
Only if it's really, really simple. Else, stick to "type what we have".
>>> comment: Optional[str] = None
>>
>> "No annotations" is represented as None here, not {}. I guess we could
>> use {} for simpler typing. When I pointed this out for v2, you replied
>> it'll go away later. Good enough for me.
>>
>
> Something might have gotten garbled between then and now. I consider
> "Annotations" to be both the if conditionals and comments; this field is
> just the comment. Optional[str] is the right type for it here:
>
> self._trees.append(Annotated(obj, ifcond, comment))
You're right.
>>> if mtype not in ('command', 'event', 'builtin', 'array'):
>>> if not self._unmask:
>>> @@ -232,47 +250,67 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>>> obj['features'] = self._gen_features(features)
>>> self._trees.append(Annotated(obj, ifcond, comment))
>>>
>>> - def _gen_member(self, member):
>>> - obj = {'name': member.name, 'type': self._use_type(member.type)}
>>> + def _gen_member(self, member: QAPISchemaObjectTypeMember
>>> + ) -> Annotated[_DObject]:
>>> + obj: _DObject = {
>>> + 'name': member.name,
>>> + 'type': self._use_type(member.type)
>>> + }
>>> if member.optional:
>>> obj['default'] = None
>>> if member.features:
>>> obj['features'] = self._gen_features(member.features)
>>> return Annotated(obj, member.ifcond)
>>>
>>> - 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) -> Annotated[_DObject]:
>>> + obj: _DObject = {
>>> + 'case': variant.name,
>>> + 'type': self._use_type(variant.type)
>>> + }
>>> return Annotated(obj, variant.ifcond)
>>>
>>> - def visit_builtin_type(self, name, info, json_type):
>>> + def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>>> + json_type: str) -> None:
>>
>> A built-in type's info is always None. Perhaps we should drop the
>> parameter.
>>
>
> We could.
>
> For the moment, info cannot be "None" here because mypy thinks it
> *might* be something.
>
> ...but we could just remove it for clarity. (As long as we don't later
> re-add that built-in source info object style idea and then we'd want to
> put it back, but ...)
>
> Later?
Okay.
>>> 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: Optional[QAPISourceInfo],
>>> + ifcond: List[str], features: List[QAPISchemaFeature],
>>> + members: List[QAPISchemaEnumMember],
>>> + prefix: Optional[str]) -> None:
>>> self._gen_tree(
>>> name, 'enum',
>>> {'values': [Annotated(m.name, m.ifcond) 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],
>>> + 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],
>>> + ifcond: List[str],
>>> + features: List[QAPISchemaFeature],
>>> + members: List[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.
>> When I pointed this out for v2, you replied we should turn
>> QAPISchemaVariants into an extension of Sequence[QAPISchemaVariant] in a
>> later series. Okay.
>>
>
> Yeah, that's a longer-term fix I think. Not for doing right now -- but
> you're right, it stands out as suspect.
>
> I think ideally we'd have a Variants object that behaves like a
> collection where the empty collection is false-ish and we'd always
> create such a collection.
>
> (Though, that might create problems for tag_name vs tag_member where we
> might now have circumstances where we have neither, maybe that makes
> typing harder instead of easier. I'll need to experiment ... I have some
> questions about how to achieve better typing for our collections class
> that clash with the delayed-type-evaluation mechanisms we have. Ongoing
> research.)
In short: later (maybe).
[...]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 12/15] qapi/introspect.py: add type hint annotations
2021-02-09 9:17 ` Markus Armbruster
@ 2021-02-10 17:05 ` John Snow
0 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-10 17:05 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Eduardo Habkost, Cleber Rosa
On 2/9/21 4:17 AM, Markus Armbruster wrote:
> Let's stick stick to "type what we have", and also take notes on
> possible improvements for later. We need this series to converge.
Yup. Consider the optional trailing patches my attempt at saying "Later,
maybe?" as a genuine to-do and not dismissive "I will never do this! The
fool! The rube!"
--js
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 13/15] qapi/introspect.py: add introspect.json dummy types
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (11 preceding siblings ...)
2021-02-04 0:32 ` [PATCH v5 12/15] qapi/introspect.py: add type hint annotations John Snow
@ 2021-02-04 0:32 ` John Snow
2021-02-08 15:43 ` Markus Armbruster
2021-02-04 0:32 ` [PATCH v5 14/15] qapi/introspect.py: Add docstring to _tree_to_qlit John Snow
2021-02-04 0:32 ` [PATCH v5 15/15] qapi/introspect.py: Update copyright and authors list John Snow
14 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2021-02-04 0:32 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Add some aliases that declare intent for some of the "dictly-typed"
objects we pass around in introspect.py.
Signed-off-by: John Snow <jsnow@redhat.com>
---
This patch is optional, it can be dropped if desired. If it's taken,
it's probably best to squash it with the prior patch. It is purely for
the sake of demonstrating what the _DObject was meant to convey: a
Python Dict that represents some JSON Object. It does not add any type
safety in and of itself, but does have some (light) annotational
benefit. In this case, it's usually a specific data structure from the
QAPI Schema we are referencing, but do not have "compile-time" access to
in Python.
These are loosely typed and don't bother reproducing the exact structure
of the real types. Python 3.6 does not have support for TypedDict
structures, so this is as good as we can do without involving a
third-party library (e.g. Pydantic), in which we might be able to say:
class SchemaMetaType(str, enum.Enum):
BUILTIN = "builtin"
ENUM = "enum"
ARRAY = "array"
OBJECT = "object"
ALTERNATE = "alternate"
COMMAND = "command"
EVENT = "event"
class SchemaInfo(pydantic.BaseModel):
name: str
meta-type: SchemaMetaType
features: Optional[List[str]]
data: Union[SchemaInfoBuiltin, SchemaInfoEnum, SchemaInfoArray,
SchemaInfoObject, SchemaInfoAlternate, SchemaInfoCommand,
SchemaInfoEvent]
However, the cost of reproducing and keeping these structure definitions
in-sync between the user-defined portion of the schema and the code
generator is likely not worth doing any such thing. However, it does
illustrate an interesting dependency the generator has on the
user-defined schema itself in terms of types.
So, I settled on using some light types that suggest the form of the
object instead of enforcing the form.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 51 +++++++++++++++++++++++---------------
1 file changed, 31 insertions(+), 20 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 3afcdda7446..2a39726f40a 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -66,10 +66,15 @@
_value = Union[_scalar, _nonscalar]
TreeValue = Union[_value, 'Annotated[_value]']
-# This is an alias for an arbitrary JSON object, represented here as a Dict.
-# It is stricter on the value type than the recursive definition above.
-# It is used to represent SchemaInfo-related structures exclusively.
-_DObject = Dict[str, object]
+# These types are based on structures defined in QEMU's schema, so we lack
+# precise types for them here. Python 3.6 does not offer TypedDict constructs,
+# so they are broadly typed here as simple Python Dicts.
+SchemaInfo = Dict[str, object]
+SchemaInfoObject = Dict[str, object]
+SchemaInfoObjectVariant = Dict[str, object]
+SchemaInfoObjectMember = Dict[str, object]
+SchemaInfoCommand = Dict[str, object]
+
_NodeT = TypeVar('_NodeT', bound=_value)
@@ -162,7 +167,7 @@ def __init__(self, prefix: str, unmask: bool):
' * QAPI/QMP schema introspection', __doc__)
self._unmask = unmask
self._schema: Optional[QAPISchema] = None
- self._trees: List[Annotated[_DObject]] = []
+ self._trees: List[Annotated[SchemaInfo]] = []
self._used_types: List[QAPISchemaType] = []
self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
@@ -234,9 +239,18 @@ def _gen_features(features: List[QAPISchemaFeature]
) -> List[Annotated[str]]:
return [Annotated(f.name, f.ifcond) for f in features]
- def _gen_tree(self, name: str, mtype: str, obj: _DObject,
+ def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
ifcond: List[str],
features: Optional[List[QAPISchemaFeature]]) -> None:
+ """
+ Build and append a SchemaInfo object to self._trees.
+
+ :param name: The entity's name.
+ :param mtype: The entity's meta-type.
+ :param obj: Additional entity fields, as appropriate for the meta-type.
+ :param ifcond: List of conditionals that apply to this entire entity.
+ :param features: Optional features field for SchemaInfo.
+ """
comment: Optional[str] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
@@ -251,8 +265,8 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
self._trees.append(Annotated(obj, ifcond, comment))
def _gen_member(self, member: QAPISchemaObjectTypeMember
- ) -> Annotated[_DObject]:
- obj: _DObject = {
+ ) -> Annotated[SchemaInfoObjectMember]:
+ obj: SchemaInfoObjectMember = {
'name': member.name,
'type': self._use_type(member.type)
}
@@ -262,13 +276,9 @@ def _gen_member(self, member: QAPISchemaObjectTypeMember
obj['features'] = self._gen_features(member.features)
return Annotated(obj, member.ifcond)
- 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: QAPISchemaVariant) -> Annotated[_DObject]:
- obj: _DObject = {
+ def _gen_variant(self, variant: QAPISchemaVariant
+ ) -> Annotated[SchemaInfoObjectVariant]:
+ obj: SchemaInfoObjectVariant = {
'case': variant.name,
'type': self._use_type(variant.type)
}
@@ -300,11 +310,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
features: List[QAPISchemaFeature],
members: List[QAPISchemaObjectTypeMember],
variants: Optional[QAPISchemaVariants]) -> None:
- obj: _DObject = {'members': [self._gen_member(m) for m in members]}
+ obj: SchemaInfoObject = {
+ 'members': [self._gen_member(m) for m in members]
+ }
if variants:
- obj.update(self._gen_variants(variants.tag_member.name,
- variants.variants))
-
+ obj['tag'] = variants.tag_member.name
+ obj['variants'] = [self._gen_variant(v) for v in variants.variants]
self._gen_tree(name, 'object', obj, ifcond, features)
def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
@@ -329,7 +340,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
arg_type = arg_type or self._schema.the_empty_object_type
ret_type = ret_type or self._schema.the_empty_object_type
- obj: _DObject = {
+ obj: SchemaInfoCommand = {
'arg-type': self._use_type(arg_type),
'ret-type': self._use_type(ret_type)
}
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 13/15] qapi/introspect.py: add introspect.json dummy types
2021-02-04 0:32 ` [PATCH v5 13/15] qapi/introspect.py: add introspect.json dummy types John Snow
@ 2021-02-08 15:43 ` Markus Armbruster
2021-02-08 15:56 ` John Snow
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-08 15:43 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Add some aliases that declare intent for some of the "dictly-typed"
> objects we pass around in introspect.py.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> This patch is optional, it can be dropped if desired. If it's taken,
> it's probably best to squash it with the prior patch. It is purely for
> the sake of demonstrating what the _DObject was meant to convey: a
> Python Dict that represents some JSON Object. It does not add any type
> safety in and of itself, but does have some (light) annotational
> benefit. In this case, it's usually a specific data structure from the
> QAPI Schema we are referencing, but do not have "compile-time" access to
> in Python.
>
> These are loosely typed and don't bother reproducing the exact structure
> of the real types. Python 3.6 does not have support for TypedDict
> structures, so this is as good as we can do without involving a
> third-party library (e.g. Pydantic), in which we might be able to say:
>
>
> class SchemaMetaType(str, enum.Enum):
> BUILTIN = "builtin"
> ENUM = "enum"
> ARRAY = "array"
> OBJECT = "object"
> ALTERNATE = "alternate"
> COMMAND = "command"
> EVENT = "event"
>
>
> class SchemaInfo(pydantic.BaseModel):
> name: str
> meta-type: SchemaMetaType
> features: Optional[List[str]]
> data: Union[SchemaInfoBuiltin, SchemaInfoEnum, SchemaInfoArray,
> SchemaInfoObject, SchemaInfoAlternate, SchemaInfoCommand,
> SchemaInfoEvent]
>
>
> However, the cost of reproducing and keeping these structure definitions
> in-sync between the user-defined portion of the schema and the code
> generator is likely not worth doing any such thing. However, it does
> illustrate an interesting dependency the generator has on the
> user-defined schema itself in terms of types.
>
> So, I settled on using some light types that suggest the form of the
> object instead of enforcing the form.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 51 +++++++++++++++++++++++---------------
> 1 file changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 3afcdda7446..2a39726f40a 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -66,10 +66,15 @@
> _value = Union[_scalar, _nonscalar]
> TreeValue = Union[_value, 'Annotated[_value]']
>
> -# This is an alias for an arbitrary JSON object, represented here as a Dict.
> -# It is stricter on the value type than the recursive definition above.
> -# It is used to represent SchemaInfo-related structures exclusively.
> -_DObject = Dict[str, object]
> +# These types are based on structures defined in QEMU's schema, so we lack
> +# precise types for them here. Python 3.6 does not offer TypedDict constructs,
> +# so they are broadly typed here as simple Python Dicts.
> +SchemaInfo = Dict[str, object]
> +SchemaInfoObject = Dict[str, object]
> +SchemaInfoObjectVariant = Dict[str, object]
> +SchemaInfoObjectMember = Dict[str, object]
> +SchemaInfoCommand = Dict[str, object]
> +
>
> _NodeT = TypeVar('_NodeT', bound=_value)
>
> @@ -162,7 +167,7 @@ def __init__(self, prefix: str, unmask: bool):
> ' * QAPI/QMP schema introspection', __doc__)
> self._unmask = unmask
> self._schema: Optional[QAPISchema] = None
> - self._trees: List[Annotated[_DObject]] = []
> + self._trees: List[Annotated[SchemaInfo]] = []
> self._used_types: List[QAPISchemaType] = []
> self._name_map: Dict[str, str] = {}
> self._genc.add(mcgen('''
> @@ -234,9 +239,18 @@ def _gen_features(features: List[QAPISchemaFeature]
> ) -> List[Annotated[str]]:
> return [Annotated(f.name, f.ifcond) for f in features]
>
> - def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> + def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
> ifcond: List[str],
> features: Optional[List[QAPISchemaFeature]]) -> None:
> + """
> + Build and append a SchemaInfo object to self._trees.
> +
> + :param name: The entity's name.
> + :param mtype: The entity's meta-type.
> + :param obj: Additional entity fields, as appropriate for the meta-type.
> + :param ifcond: List of conditionals that apply to this entire entity.
> + :param features: Optional features field for SchemaInfo.
> + """
> comment: Optional[str] = None
> if mtype not in ('command', 'event', 'builtin', 'array'):
> if not self._unmask:
> @@ -251,8 +265,8 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> self._trees.append(Annotated(obj, ifcond, comment))
>
> def _gen_member(self, member: QAPISchemaObjectTypeMember
> - ) -> Annotated[_DObject]:
> - obj: _DObject = {
> + ) -> Annotated[SchemaInfoObjectMember]:
> + obj: SchemaInfoObjectMember = {
> 'name': member.name,
> 'type': self._use_type(member.type)
> }
> @@ -262,13 +276,9 @@ def _gen_member(self, member: QAPISchemaObjectTypeMember
> obj['features'] = self._gen_features(member.features)
> return Annotated(obj, member.ifcond)
>
> - 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: QAPISchemaVariant) -> Annotated[_DObject]:
> - obj: _DObject = {
> + def _gen_variant(self, variant: QAPISchemaVariant
> + ) -> Annotated[SchemaInfoObjectVariant]:
> + obj: SchemaInfoObjectVariant = {
> 'case': variant.name,
> 'type': self._use_type(variant.type)
> }
> @@ -300,11 +310,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
> features: List[QAPISchemaFeature],
> members: List[QAPISchemaObjectTypeMember],
> variants: Optional[QAPISchemaVariants]) -> None:
> - obj: _DObject = {'members': [self._gen_member(m) for m in members]}
> + obj: SchemaInfoObject = {
> + 'members': [self._gen_member(m) for m in members]
> + }
> if variants:
> - obj.update(self._gen_variants(variants.tag_member.name,
> - variants.variants))
> -
> + obj['tag'] = variants.tag_member.name
> + obj['variants'] = [self._gen_variant(v) for v in variants.variants]
> self._gen_tree(name, 'object', obj, ifcond, features)
>
> def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
> @@ -329,7 +340,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
>
> arg_type = arg_type or self._schema.the_empty_object_type
> ret_type = ret_type or self._schema.the_empty_object_type
> - obj: _DObject = {
> + obj: SchemaInfoCommand = {
> 'arg-type': self._use_type(arg_type),
> 'ret-type': self._use_type(ret_type)
> }
I see what you mean by "some (light) annotational benefit".
I figure some of is simply due to going from _DObject, a name that tells
me nothing, to SchemaInfo, which makes me go aha!
The remainder is having the subtypes of SchemaInfo, too. albeit without
actual type checking. Worthwhile?
I agree it should be squashed if we want it, or parts of it.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 13/15] qapi/introspect.py: add introspect.json dummy types
2021-02-08 15:43 ` Markus Armbruster
@ 2021-02-08 15:56 ` John Snow
0 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-08 15:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/8/21 10:43 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Add some aliases that declare intent for some of the "dictly-typed"
>> objects we pass around in introspect.py.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> ---
>>
>> This patch is optional, it can be dropped if desired. If it's taken,
>> it's probably best to squash it with the prior patch. It is purely for
>> the sake of demonstrating what the _DObject was meant to convey: a
>> Python Dict that represents some JSON Object. It does not add any type
>> safety in and of itself, but does have some (light) annotational
>> benefit. In this case, it's usually a specific data structure from the
>> QAPI Schema we are referencing, but do not have "compile-time" access to
>> in Python.
>>
>> These are loosely typed and don't bother reproducing the exact structure
>> of the real types. Python 3.6 does not have support for TypedDict
>> structures, so this is as good as we can do without involving a
>> third-party library (e.g. Pydantic), in which we might be able to say:
>>
>>
>> class SchemaMetaType(str, enum.Enum):
>> BUILTIN = "builtin"
>> ENUM = "enum"
>> ARRAY = "array"
>> OBJECT = "object"
>> ALTERNATE = "alternate"
>> COMMAND = "command"
>> EVENT = "event"
>>
>>
>> class SchemaInfo(pydantic.BaseModel):
>> name: str
>> meta-type: SchemaMetaType
>> features: Optional[List[str]]
>> data: Union[SchemaInfoBuiltin, SchemaInfoEnum, SchemaInfoArray,
>> SchemaInfoObject, SchemaInfoAlternate, SchemaInfoCommand,
>> SchemaInfoEvent]
>>
>>
>> However, the cost of reproducing and keeping these structure definitions
>> in-sync between the user-defined portion of the schema and the code
>> generator is likely not worth doing any such thing. However, it does
>> illustrate an interesting dependency the generator has on the
>> user-defined schema itself in terms of types.
>>
>> So, I settled on using some light types that suggest the form of the
>> object instead of enforcing the form.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 51 +++++++++++++++++++++++---------------
>> 1 file changed, 31 insertions(+), 20 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 3afcdda7446..2a39726f40a 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -66,10 +66,15 @@
>> _value = Union[_scalar, _nonscalar]
>> TreeValue = Union[_value, 'Annotated[_value]']
>>
>> -# This is an alias for an arbitrary JSON object, represented here as a Dict.
>> -# It is stricter on the value type than the recursive definition above.
>> -# It is used to represent SchemaInfo-related structures exclusively.
>> -_DObject = Dict[str, object]
>> +# These types are based on structures defined in QEMU's schema, so we lack
>> +# precise types for them here. Python 3.6 does not offer TypedDict constructs,
>> +# so they are broadly typed here as simple Python Dicts.
>> +SchemaInfo = Dict[str, object]
>> +SchemaInfoObject = Dict[str, object]
>> +SchemaInfoObjectVariant = Dict[str, object]
>> +SchemaInfoObjectMember = Dict[str, object]
>> +SchemaInfoCommand = Dict[str, object]
>> +
>>
>> _NodeT = TypeVar('_NodeT', bound=_value)
>>
>> @@ -162,7 +167,7 @@ def __init__(self, prefix: str, unmask: bool):
>> ' * QAPI/QMP schema introspection', __doc__)
>> self._unmask = unmask
>> self._schema: Optional[QAPISchema] = None
>> - self._trees: List[Annotated[_DObject]] = []
>> + self._trees: List[Annotated[SchemaInfo]] = []
>> self._used_types: List[QAPISchemaType] = []
>> self._name_map: Dict[str, str] = {}
>> self._genc.add(mcgen('''
>> @@ -234,9 +239,18 @@ def _gen_features(features: List[QAPISchemaFeature]
>> ) -> List[Annotated[str]]:
>> return [Annotated(f.name, f.ifcond) for f in features]
>>
>> - def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> + def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>> ifcond: List[str],
>> features: Optional[List[QAPISchemaFeature]]) -> None:
>> + """
>> + Build and append a SchemaInfo object to self._trees.
>> +
>> + :param name: The entity's name.
>> + :param mtype: The entity's meta-type.
>> + :param obj: Additional entity fields, as appropriate for the meta-type.
>> + :param ifcond: List of conditionals that apply to this entire entity.
>> + :param features: Optional features field for SchemaInfo.
>> + """
>> comment: Optional[str] = None
>> if mtype not in ('command', 'event', 'builtin', 'array'):
>> if not self._unmask:
>> @@ -251,8 +265,8 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> self._trees.append(Annotated(obj, ifcond, comment))
>>
>> def _gen_member(self, member: QAPISchemaObjectTypeMember
>> - ) -> Annotated[_DObject]:
>> - obj: _DObject = {
>> + ) -> Annotated[SchemaInfoObjectMember]:
>> + obj: SchemaInfoObjectMember = {
>> 'name': member.name,
>> 'type': self._use_type(member.type)
>> }
>> @@ -262,13 +276,9 @@ def _gen_member(self, member: QAPISchemaObjectTypeMember
>> obj['features'] = self._gen_features(member.features)
>> return Annotated(obj, member.ifcond)
>>
>> - 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: QAPISchemaVariant) -> Annotated[_DObject]:
>> - obj: _DObject = {
>> + def _gen_variant(self, variant: QAPISchemaVariant
>> + ) -> Annotated[SchemaInfoObjectVariant]:
>> + obj: SchemaInfoObjectVariant = {
>> 'case': variant.name,
>> 'type': self._use_type(variant.type)
>> }
>> @@ -300,11 +310,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>> features: List[QAPISchemaFeature],
>> members: List[QAPISchemaObjectTypeMember],
>> variants: Optional[QAPISchemaVariants]) -> None:
>> - obj: _DObject = {'members': [self._gen_member(m) for m in members]}
>> + obj: SchemaInfoObject = {
>> + 'members': [self._gen_member(m) for m in members]
>> + }
>> if variants:
>> - obj.update(self._gen_variants(variants.tag_member.name,
>> - variants.variants))
>> -
>> + obj['tag'] = variants.tag_member.name
>> + obj['variants'] = [self._gen_variant(v) for v in variants.variants]
>> self._gen_tree(name, 'object', obj, ifcond, features)
>>
>> def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>> @@ -329,7 +340,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
>>
>> arg_type = arg_type or self._schema.the_empty_object_type
>> ret_type = ret_type or self._schema.the_empty_object_type
>> - obj: _DObject = {
>> + obj: SchemaInfoCommand = {
>> 'arg-type': self._use_type(arg_type),
>> 'ret-type': self._use_type(ret_type)
>> }
>
> I see what you mean by "some (light) annotational benefit".
>
> I figure some of is simply due to going from _DObject, a name that tells
> me nothing, to SchemaInfo, which makes me go aha!
>
Naming is hard! Using the *exact* names seems helpful.
> The remainder is having the subtypes of SchemaInfo, too. albeit without
> actual type checking. Worthwhile?
>
For referential purposes it seems useful, at least in so far as
declaring intent. When I was trying to type this series, I was not aware
this module was building structures defined in introspect.json until
quite late into the series.
I kept wondering: "What are the valid keys here? Who consumes this?" --
It wasn't obvious.
I'm leaning towards just keeping them all for the documentation benefit,
though I admit it *could* mislead someone into believing it's type
checked when it isn't, really. I tried to call that out with the comment
at the top, which I hope suffices to diffuse that potential
faith-based-landmine.
> I agree it should be squashed if we want it, or parts of it.
>
Squash all of it, IMO.
--js
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 14/15] qapi/introspect.py: Add docstring to _tree_to_qlit
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (12 preceding siblings ...)
2021-02-04 0:32 ` [PATCH v5 13/15] qapi/introspect.py: add introspect.json dummy types John Snow
@ 2021-02-04 0:32 ` John Snow
2021-02-08 15:45 ` Markus Armbruster
2021-02-04 0:32 ` [PATCH v5 15/15] qapi/introspect.py: Update copyright and authors list John Snow
14 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2021-02-04 0:32 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 2a39726f40a..2b338abe2cf 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -97,6 +97,14 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
def _tree_to_qlit(obj: TreeValue,
level: int = 0,
dict_value: bool = False) -> str:
+ """
+ Convert the type tree into a QLIT C string, recursively.
+
+ :param obj: The value to convert.
+ :param level: The indentation level for this particular value.
+ :param dict_value: True when the value being processed belongs to a
+ dict key; which suppresses the output indent.
+ """
def indent(level: int) -> str:
return level * 4 * ' '
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 14/15] qapi/introspect.py: Add docstring to _tree_to_qlit
2021-02-04 0:32 ` [PATCH v5 14/15] qapi/introspect.py: Add docstring to _tree_to_qlit John Snow
@ 2021-02-08 15:45 ` Markus Armbruster
2021-02-08 15:57 ` John Snow
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-08 15:45 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 2a39726f40a..2b338abe2cf 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -97,6 +97,14 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
> def _tree_to_qlit(obj: TreeValue,
> level: int = 0,
> dict_value: bool = False) -> str:
> + """
> + Convert the type tree into a QLIT C string, recursively.
> +
> + :param obj: The value to convert.
> + :param level: The indentation level for this particular value.
> + :param dict_value: True when the value being processed belongs to a
> + dict key; which suppresses the output indent.
> + """
>
> def indent(level: int) -> str:
> return level * 4 * ' '
Might want to mention @obj may not be Annotated when dict_value=True.
Not a demand.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v5 14/15] qapi/introspect.py: Add docstring to _tree_to_qlit
2021-02-08 15:45 ` Markus Armbruster
@ 2021-02-08 15:57 ` John Snow
0 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-08 15:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/8/21 10:45 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 2a39726f40a..2b338abe2cf 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -97,6 +97,14 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
>> def _tree_to_qlit(obj: TreeValue,
>> level: int = 0,
>> dict_value: bool = False) -> str:
>> + """
>> + Convert the type tree into a QLIT C string, recursively.
>> +
>> + :param obj: The value to convert.
>> + :param level: The indentation level for this particular value.
>> + :param dict_value: True when the value being processed belongs to a
>> + dict key; which suppresses the output indent.
>> + """
>>
>> def indent(level: int) -> str:
>> return level * 4 * ' '
>
> Might want to mention @obj may not be Annotated when dict_value=True.
> Not a demand.
>
No, that's a good point.
--js
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v5 15/15] qapi/introspect.py: Update copyright and authors list
2021-02-04 0:31 [PATCH v5 00/15] qapi: static typing conversion, pt2 John Snow
` (13 preceding siblings ...)
2021-02-04 0:32 ` [PATCH v5 14/15] qapi/introspect.py: Add docstring to _tree_to_qlit John Snow
@ 2021-02-04 0:32 ` John Snow
14 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2021-02-04 0:32 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 2b338abe2cf..fd0ca0aba21 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -1,10 +1,11 @@
"""
QAPI introspection generator
-Copyright (C) 2015-2018 Red Hat, Inc.
+Copyright (C) 2015-2021 Red Hat, Inc.
Authors:
Markus Armbruster <armbru@redhat.com>
+ John Snow <jsnow@redhat.com>
This work is licensed under the terms of the GNU GPL, version 2.
See the COPYING file in the top-level directory.
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread