* [PATCH v6 00/19] qapi: static typing conversion, pt2
@ 2021-02-16 2:17 John Snow
2021-02-16 2:17 ` [PATCH v6 01/19] qapi: Replace List[str] with Sequence[str] for ifcond John Snow
` (18 more replies)
0 siblings, 19 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:17 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
Hi, this series adds static type hints to the QAPI module.
This is part two, and covers introspect.py.
Part 2: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2
Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6
- Requires Python 3.6+
- Requires mypy 0.770 or newer (for type analysis only)
- Requires pylint 2.6.0 or newer (for lint checking only)
(Note: pylint does not like Python 3.9 very much yet. Known problem.)
Type hints are added in patches that add *only* type hints and change no
other behavior. Any necessary changes to behavior to accommodate typing
are split out into their own tiny patches.
Every commit should pass with (from ./scripts):
- flake8 qapi/
- pylint --rcfile=qapi/pylintrc qapi/
- mypy --config-file=qapi/mypy.ini qapi/
- isort -c qapi/
V6:
001/19:[down] 'qapi: Replace List[str] with Sequence[str] for ifcond'
009/19:[0021] [FC] 'qapi/introspect.py: Introduce preliminary tree typing'
010/19:[0013] [FC] 'qapi/introspect.py: create a typed 'Annotated' data strutcure'
013/19:[down] 'qapi/introspect.py: remove _gen_variants helper'
014/19:[0053] [FC] 'qapi/introspect.py: add type hint annotations'
015/19:[down] 'qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit'
017/19:[down] 'qapi/introspect.py: Type _gen_tree variants as Sequence[str]'
018/19:[down] 'qapi/introspect.py: set _gen_tree's default ifcond argument to ()'
019/19:[down] 'qapi/introspect.py: add SchemaMetaType enum'
01: New; consistently type ifcond as Seq[str] in already-typed files.
09: Adjust comment concerning _stub to be more clear (?)
Rename _stub to _Stub, etc.
TreeValue becomes JSONValue.
10: _NodeT becomes _ValueT to match the _Value name.
Change visit_alternate_type whitespace around some more.
13: New, pre-requisite for using SchemaInfo aliases.
(Was not appropriate to go into #14.)
14: Use Sequence[str] instead of List[str] for ifcond
Use SchemaInfo "dummy types" instead of _DObject
15: Adjust comment to mention dict_value limitation.
Add docstring for _gen_tree (from former "dummy types" patch).
Change name of commit to reflect now-multiple docstring additions.
OPTIONAL PATCHES:
17: Use Sequence[QAPISchemaFeature] instead of Optional[List[QAPISchemaFeature]]
18: Set a default argument for ifcond to the empty tuple ().
Stylistically matches the above patch.
19: Create a SchemaMetaType enum and use it instead of the string type.
(Contains an optional blurb that can be removed if desired.)
V5:
04: Rename 'suppress_first_indent' to 'dict_value'
(Docstring added in 014.)
06: Avoid changing the output structure of _make_tree
07: Chance the structure of _make_tree 8-)
08: Change commented TreeValue to include a TODO instead.
09: Change NodeT bound to _value instead of TreeValue
Change "Remove in 3.7" text to include "TODO: "
Remove forwarding suppress_first_indent/dict_value in recursive cases
Change spacing in visit_alternate_type()
11: Consequence of suppress_first_value/dict_value change
12: Commit message note added
Changed _DObject comment
13: Commit notes adjusted
_DObject stuff: Comment near SchemaInfo et al adjusted
14: Changed docstring to reflect dict_value change
15: Updated copyright year for 2021 :~)
V4:
- Rebased on "pt1.5" v4
- signatures updated to use Optional[QAPISourceInfo]
John Snow (19):
qapi: Replace List[str] with Sequence[str] for ifcond
qapi/introspect.py: assert schema is not None
qapi/introspect.py: use _make_tree for features nodes
qapi/introspect.py: add _gen_features helper
qapi/introspect.py: guard against ifcond/comment misuse
qapi/introspect.py: Unify return type of _make_tree()
qapi/introspect.py: replace 'extra' dict with 'comment' argument
qapi/introspect.py: Always define all 'extra' dict keys
qapi/introspect.py: Introduce preliminary tree typing
qapi/introspect.py: create a typed 'Annotated' data strutcure
qapi/introspect.py: improve _tree_to_qlit error message
qapi/introspect.py: improve readability of _tree_to_qlit
qapi/introspect.py: remove _gen_variants helper
qapi/introspect.py: add type hint annotations
qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit
qapi/introspect.py: Update copyright and authors list
qapi/introspect.py: Type _gen_tree variants as Sequence[str]
qapi/introspect.py: set _gen_tree's default ifcond argument to ()
qapi/introspect.py: add SchemaMetaType enum
scripts/qapi/commands.py | 3 +-
scripts/qapi/events.py | 4 +-
scripts/qapi/gen.py | 12 +-
scripts/qapi/introspect.py | 350 +++++++++++++++++++++++++++----------
scripts/qapi/mypy.ini | 5 -
scripts/qapi/schema.py | 2 +-
scripts/qapi/types.py | 12 +-
scripts/qapi/visit.py | 10 +-
8 files changed, 275 insertions(+), 123 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v6 01/19] qapi: Replace List[str] with Sequence[str] for ifcond
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
@ 2021-02-16 2:17 ` John Snow
2021-02-16 8:43 ` Markus Armbruster
2021-02-16 2:17 ` [PATCH v6 02/19] qapi/introspect.py: assert schema is not None John Snow
` (17 subsequent siblings)
18 siblings, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-16 2:17 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
It does happen to be a list (as of now), but we can describe it in more
general terms with no loss in accuracy to allow tuples and other
constructs.
In the future, we can write "ifcond: Sequence[str] = ()" as a default
parameter, which we could not do with a Mutable type like a List.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/commands.py | 3 ++-
scripts/qapi/events.py | 4 ++--
scripts/qapi/gen.py | 12 ++++++------
scripts/qapi/types.py | 12 ++++++------
scripts/qapi/visit.py | 10 +++++-----
5 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 54af519f44d..0a75a9371ba 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -17,6 +17,7 @@
Dict,
List,
Optional,
+ Sequence,
Set,
)
@@ -297,7 +298,7 @@ def visit_end(self) -> None:
def visit_command(self,
name: str,
info: Optional[QAPISourceInfo],
- ifcond: List[str],
+ ifcond: Sequence[str],
features: List[QAPISchemaFeature],
arg_type: Optional[QAPISchemaObjectType],
ret_type: Optional[QAPISchemaType],
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 8c57deb2b89..90d2f6156d8 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -12,7 +12,7 @@
See the COPYING file in the top-level directory.
"""
-from typing import List, Optional
+from typing import List, Optional, Sequence
from .common import c_enum_const, c_name, mcgen
from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
@@ -214,7 +214,7 @@ def visit_end(self) -> None:
def visit_event(self,
name: str,
info: Optional[QAPISourceInfo],
- ifcond: List[str],
+ ifcond: Sequence[str],
features: List[QAPISchemaFeature],
arg_type: Optional[QAPISchemaObjectType],
boxed: bool) -> None:
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 63549cc8d47..1fa503bdbdf 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -17,8 +17,8 @@
from typing import (
Dict,
Iterator,
- List,
Optional,
+ Sequence,
Tuple,
)
@@ -85,7 +85,7 @@ def write(self, output_dir: str) -> None:
fp.write(text)
-def _wrap_ifcond(ifcond: List[str], before: str, after: str) -> str:
+def _wrap_ifcond(ifcond: Sequence[str], before: str, after: str) -> str:
if before == after:
return after # suppress empty #if ... #endif
@@ -127,9 +127,9 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
class QAPIGenCCode(QAPIGen):
def __init__(self, fname: str):
super().__init__(fname)
- self._start_if: Optional[Tuple[List[str], str, str]] = None
+ self._start_if: Optional[Tuple[Sequence[str], str, str]] = None
- def start_if(self, ifcond: List[str]) -> None:
+ def start_if(self, ifcond: Sequence[str]) -> None:
assert self._start_if is None
self._start_if = (ifcond, self._body, self._preamble)
@@ -187,11 +187,11 @@ def _bottom(self) -> str:
@contextmanager
-def ifcontext(ifcond: List[str], *args: QAPIGenCCode) -> Iterator[None]:
+def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]:
"""
A with-statement context manager that wraps with `start_if()` / `end_if()`.
- :param ifcond: A list of conditionals, passed to `start_if()`.
+ :param ifcond: A sequence of conditionals, passed to `start_if()`.
:param args: any number of `QAPIGenCCode`.
Example::
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2bdd6268476..20d572a23aa 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -13,7 +13,7 @@
# See the COPYING file in the top-level directory.
"""
-from typing import List, Optional
+from typing import List, Optional, Sequence
from .common import (
c_enum_const,
@@ -139,7 +139,7 @@ def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str:
return ret
-def gen_object(name: str, ifcond: List[str],
+def gen_object(name: str, ifcond: Sequence[str],
base: Optional[QAPISchemaObjectType],
members: List[QAPISchemaObjectTypeMember],
variants: Optional[QAPISchemaVariants]) -> str:
@@ -307,7 +307,7 @@ def _gen_type_cleanup(self, name: str) -> None:
def visit_enum_type(self,
name: str,
info: Optional[QAPISourceInfo],
- ifcond: List[str],
+ ifcond: Sequence[str],
features: List[QAPISchemaFeature],
members: List[QAPISchemaEnumMember],
prefix: Optional[str]) -> None:
@@ -318,7 +318,7 @@ def visit_enum_type(self,
def visit_array_type(self,
name: str,
info: Optional[QAPISourceInfo],
- ifcond: List[str],
+ ifcond: Sequence[str],
element_type: QAPISchemaType) -> None:
with ifcontext(ifcond, self._genh, self._genc):
self._genh.preamble_add(gen_fwd_object_or_array(name))
@@ -328,7 +328,7 @@ def visit_array_type(self,
def visit_object_type(self,
name: str,
info: Optional[QAPISourceInfo],
- ifcond: List[str],
+ ifcond: Sequence[str],
features: List[QAPISchemaFeature],
base: Optional[QAPISchemaObjectType],
members: List[QAPISchemaObjectTypeMember],
@@ -351,7 +351,7 @@ def visit_object_type(self,
def visit_alternate_type(self,
name: str,
info: Optional[QAPISourceInfo],
- ifcond: List[str],
+ ifcond: Sequence[str],
features: List[QAPISchemaFeature],
variants: QAPISchemaVariants) -> None:
with ifcontext(ifcond, self._genh):
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 22e62df9017..9aa0b1e11e9 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -13,7 +13,7 @@
See the COPYING file in the top-level directory.
"""
-from typing import List, Optional
+from typing import List, Optional, Sequence
from .common import (
c_enum_const,
@@ -337,7 +337,7 @@ def _begin_user_module(self, name: str) -> None:
def visit_enum_type(self,
name: str,
info: Optional[QAPISourceInfo],
- ifcond: List[str],
+ ifcond: Sequence[str],
features: List[QAPISchemaFeature],
members: List[QAPISchemaEnumMember],
prefix: Optional[str]) -> None:
@@ -348,7 +348,7 @@ def visit_enum_type(self,
def visit_array_type(self,
name: str,
info: Optional[QAPISourceInfo],
- ifcond: List[str],
+ ifcond: Sequence[str],
element_type: QAPISchemaType) -> None:
with ifcontext(ifcond, self._genh, self._genc):
self._genh.add(gen_visit_decl(name))
@@ -357,7 +357,7 @@ def visit_array_type(self,
def visit_object_type(self,
name: str,
info: Optional[QAPISourceInfo],
- ifcond: List[str],
+ ifcond: Sequence[str],
features: List[QAPISchemaFeature],
base: Optional[QAPISchemaObjectType],
members: List[QAPISchemaObjectTypeMember],
@@ -379,7 +379,7 @@ def visit_object_type(self,
def visit_alternate_type(self,
name: str,
info: Optional[QAPISourceInfo],
- ifcond: List[str],
+ ifcond: Sequence[str],
features: List[QAPISchemaFeature],
variants: QAPISchemaVariants) -> None:
with ifcontext(ifcond, self._genh, self._genc):
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v6 02/19] qapi/introspect.py: assert schema is not None
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
2021-02-16 2:17 ` [PATCH v6 01/19] qapi: Replace List[str] with Sequence[str] for ifcond John Snow
@ 2021-02-16 2:17 ` John Snow
2021-02-16 2:17 ` [PATCH v6 03/19] qapi/introspect.py: use _make_tree for features nodes John Snow
` (16 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:17 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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] 42+ messages in thread
* [PATCH v6 03/19] qapi/introspect.py: use _make_tree for features nodes
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
2021-02-16 2:17 ` [PATCH v6 01/19] qapi: Replace List[str] with Sequence[str] for ifcond John Snow
2021-02-16 2:17 ` [PATCH v6 02/19] qapi/introspect.py: assert schema is not None John Snow
@ 2021-02-16 2:17 ` John Snow
2021-02-16 2:17 ` [PATCH v6 04/19] qapi/introspect.py: add _gen_features helper John Snow
` (15 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:17 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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] 42+ messages in thread
* [PATCH v6 04/19] qapi/introspect.py: add _gen_features helper
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (2 preceding siblings ...)
2021-02-16 2:17 ` [PATCH v6 03/19] qapi/introspect.py: use _make_tree for features nodes John Snow
@ 2021-02-16 2:17 ` John Snow
2021-02-16 2:17 ` [PATCH v6 05/19] qapi/introspect.py: guard against ifcond/comment misuse John Snow
` (14 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:17 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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] 42+ messages in thread
* [PATCH v6 05/19] qapi/introspect.py: guard against ifcond/comment misuse
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (3 preceding siblings ...)
2021-02-16 2:17 ` [PATCH v6 04/19] qapi/introspect.py: add _gen_features helper John Snow
@ 2021-02-16 2:17 ` John Snow
2021-02-17 12:01 ` Markus Armbruster
2021-02-16 2:17 ` [PATCH v6 06/19] qapi/introspect.py: Unify return type of _make_tree() John Snow
` (13 subsequent siblings)
18 siblings, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-16 2:17 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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] 42+ messages in thread
* [PATCH v6 06/19] qapi/introspect.py: Unify return type of _make_tree()
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (4 preceding siblings ...)
2021-02-16 2:17 ` [PATCH v6 05/19] qapi/introspect.py: guard against ifcond/comment misuse John Snow
@ 2021-02-16 2:17 ` John Snow
2021-02-16 2:17 ` [PATCH v6 07/19] qapi/introspect.py: replace 'extra' dict with 'comment' argument John Snow
` (12 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:17 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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] 42+ messages in thread
* [PATCH v6 07/19] qapi/introspect.py: replace 'extra' dict with 'comment' argument
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (5 preceding siblings ...)
2021-02-16 2:17 ` [PATCH v6 06/19] qapi/introspect.py: Unify return type of _make_tree() John Snow
@ 2021-02-16 2:17 ` John Snow
2021-02-16 2:17 ` [PATCH v6 08/19] qapi/introspect.py: Always define all 'extra' dict keys John Snow
` (11 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:17 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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] 42+ messages in thread
* [PATCH v6 08/19] qapi/introspect.py: Always define all 'extra' dict keys
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (6 preceding siblings ...)
2021-02-16 2:17 ` [PATCH v6 07/19] qapi/introspect.py: replace 'extra' dict with 'comment' argument John Snow
@ 2021-02-16 2:17 ` John Snow
2021-02-16 2:17 ` [PATCH v6 09/19] qapi/introspect.py: Introduce preliminary tree typing John Snow
` (10 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:17 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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] 42+ messages in thread
* [PATCH v6 09/19] qapi/introspect.py: Introduce preliminary tree typing
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (7 preceding siblings ...)
2021-02-16 2:17 ` [PATCH v6 08/19] qapi/introspect.py: Always define all 'extra' dict keys John Snow
@ 2021-02-16 2:17 ` John Snow
2021-02-16 2:18 ` [PATCH v6 10/19] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
` (9 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:17 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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 | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 45231d2abc3..3c37c138013 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,29 @@
)
+# This module constructs a tree data structure that is used to
+# generate the introspection information for QEMU. It is shaped
+# like 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, JSONValue], List[JSONValue]]
+#
+# With optional annotations, the type of all values is:
+# JSONValue = Union[_Value, Annotated[_Value]]
+#
+# Sadly, mypy does not support recursive types; so the _Stub alias is used to
+# mark the imprecision in the type model where we'd otherwise use JSONValue.
+_Stub = Any
+_Scalar = Union[str, bool, None]
+_NonScalar = Union[Dict[str, _Stub], List[_Stub]]
+_Value = Union[_Scalar, _NonScalar]
+# JSONValue = TODO, in a forthcoming commit.
+
+
def _make_tree(obj, ifcond, comment=None):
extra = {
'if': ifcond,
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v6 10/19] qapi/introspect.py: create a typed 'Annotated' data strutcure
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (8 preceding siblings ...)
2021-02-16 2:17 ` [PATCH v6 09/19] qapi/introspect.py: Introduce preliminary tree typing John Snow
@ 2021-02-16 2:18 ` John Snow
2021-02-16 2:18 ` [PATCH v6 11/19] qapi/introspect.py: improve _tree_to_qlit error message John Snow
` (8 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:18 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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 | 78 ++++++++++++++++++++++----------------
1 file changed, 45 insertions(+), 33 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 3c37c138013..5224be1a333 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,
)
@@ -52,15 +56,25 @@
_Scalar = Union[str, bool, None]
_NonScalar = Union[Dict[str, _Stub], List[_Stub]]
_Value = Union[_Scalar, _NonScalar]
-# JSONValue = TODO, in a forthcoming commit.
+JSONValue = Union[_Value, 'Annotated[_Value]']
-def _make_tree(obj, ifcond, comment=None):
- extra = {
- 'if': ifcond,
- 'comment': comment
- }
- return (obj, extra)
+_ValueT = TypeVar('_ValueT', bound=_Value)
+
+
+class Annotated(Generic[_ValueT]):
+ """
+ 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: _ValueT, 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):
@@ -68,24 +82,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 = ''
@@ -202,7 +212,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
@@ -216,7 +226,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)}
@@ -224,7 +234,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,
@@ -232,16 +242,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)
@@ -258,12 +269,13 @@ 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] 42+ messages in thread
* [PATCH v6 11/19] qapi/introspect.py: improve _tree_to_qlit error message
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (9 preceding siblings ...)
2021-02-16 2:18 ` [PATCH v6 10/19] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
@ 2021-02-16 2:18 ` John Snow
2021-02-16 2:18 ` [PATCH v6 12/19] qapi/introspect.py: improve readability of _tree_to_qlit John Snow
` (7 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:18 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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 5224be1a333..2ba0bfec733 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -125,7 +125,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] 42+ messages in thread
* [PATCH v6 12/19] qapi/introspect.py: improve readability of _tree_to_qlit
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (10 preceding siblings ...)
2021-02-16 2:18 ` [PATCH v6 11/19] qapi/introspect.py: improve _tree_to_qlit error message John Snow
@ 2021-02-16 2:18 ` John Snow
2021-02-16 2:18 ` [PATCH v6 13/19] qapi/introspect.py: remove _gen_variants helper John Snow
` (6 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:18 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, 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 2ba0bfec733..afad891bb2b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -90,7 +90,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)
@@ -101,33 +101,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] 42+ messages in thread
* [PATCH v6 13/19] qapi/introspect.py: remove _gen_variants helper
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (11 preceding siblings ...)
2021-02-16 2:18 ` [PATCH v6 12/19] qapi/introspect.py: improve readability of _tree_to_qlit John Snow
@ 2021-02-16 2:18 ` John Snow
2021-02-16 2:18 ` [PATCH v6 14/19] qapi/introspect.py: add type hint annotations John Snow
` (5 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:18 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
It is easier to give a name to all of the dictly-typed objects we pass
around in introspect.py by removing this helper, as it does not return
an object that has any knowable type by itself.
Inline it into its only caller instead.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index afad891bb2b..b0fcc4443c1 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -241,10 +241,6 @@ def _gen_member(self, member):
obj['features'] = self._gen_features(member.features)
return Annotated(obj, member.ifcond)
- def _gen_variants(self, tag_name, variants):
- 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)}
return Annotated(obj, variant.ifcond)
@@ -268,9 +264,8 @@ def visit_object_type_flat(self, name, info, ifcond, features,
members, variants):
obj = {'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, info, ifcond, features, variants):
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v6 14/19] qapi/introspect.py: add type hint annotations
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (12 preceding siblings ...)
2021-02-16 2:18 ` [PATCH v6 13/19] qapi/introspect.py: remove _gen_variants helper John Snow
@ 2021-02-16 2:18 ` John Snow
2021-02-16 8:55 ` Markus Armbruster
2021-02-18 18:56 ` Markus Armbruster
2021-02-16 2:18 ` [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit John Snow
` (4 subsequent siblings)
18 siblings, 2 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:18 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
NB: The type aliases (SchemaInfo et al) declare intent for some of the
"dictly-typed" objects we pass around in introspect.py. They do not
enforce the shape of those objects, and cannot, until Python 3.7 or
later. (And even then, it may not be "worth it".)
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 124 +++++++++++++++++++++++++++----------
scripts/qapi/mypy.ini | 5 --
scripts/qapi/schema.py | 2 +-
3 files changed, 92 insertions(+), 39 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b0fcc4443c1..45284af1330 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -17,6 +17,7 @@
Iterable,
List,
Optional,
+ Sequence,
Tuple,
TypeVar,
Union,
@@ -30,10 +31,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
@@ -58,6 +68,15 @@
_Value = Union[_Scalar, _NonScalar]
JSONValue = Union[_Value, 'Annotated[_Value]']
+# 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]
+
_ValueT = TypeVar('_ValueT', bound=_Value)
@@ -77,9 +96,11 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
self.ifcond: Tuple[str, ...] = tuple(ifcond)
-def _tree_to_qlit(obj, level=0, dict_value=False):
+def _tree_to_qlit(obj: JSONValue,
+ level: int = 0,
+ dict_value: bool = False) -> str:
- def indent(level):
+ def indent(level: int) -> str:
return level * 4 * ' '
if isinstance(obj, Annotated):
@@ -136,21 +157,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[SchemaInfo]] = []
+ self._used_types: List[QAPISchemaType] = []
+ self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
#include "qemu/osdep.h"
#include "%(prefix)sqapi-introspect.h"
@@ -158,10 +179,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)
@@ -183,18 +204,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
@@ -216,10 +237,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: Dict[str, object],
+ ifcond: Sequence[str],
+ features: Optional[List[QAPISchemaFeature]]) -> None:
comment: Optional[str] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
@@ -233,42 +257,65 @@ 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[SchemaInfoObjectMember]:
+ obj: SchemaInfoObjectMember = {
+ '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_variant(self, variant):
- obj = {'case': variant.name, 'type': self._use_type(variant.type)}
+ def _gen_variant(self, variant: QAPISchemaVariant
+ ) -> Annotated[SchemaInfoObjectVariant]:
+ obj: SchemaInfoObjectVariant = {
+ '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: Sequence[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: Sequence[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: Sequence[str],
+ features: List[QAPISchemaFeature],
+ members: List[QAPISchemaObjectTypeMember],
+ variants: Optional[QAPISchemaVariants]) -> None:
+ obj: SchemaInfoObject = {
+ 'members': [self._gen_member(m) for m in members]
+ }
if 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, info, ifcond, features, variants):
+ def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
+ ifcond: Sequence[str],
+ features: List[QAPISchemaFeature],
+ variants: QAPISchemaVariants) -> None:
self._gen_tree(
name, 'alternate',
{'members': [Annotated({'type': self._use_type(m.type)},
@@ -277,27 +324,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: Sequence[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: SchemaInfoCommand = {
+ '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: Sequence[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] 42+ messages in thread
* [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (13 preceding siblings ...)
2021-02-16 2:18 ` [PATCH v6 14/19] qapi/introspect.py: add type hint annotations John Snow
@ 2021-02-16 2:18 ` John Snow
2021-02-17 9:39 ` Markus Armbruster
2021-02-16 2:18 ` [PATCH v6 16/19] qapi/introspect.py: Update copyright and authors list John Snow
` (3 subsequent siblings)
18 siblings, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-16 2:18 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 45284af1330..5d4f5e23f7e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -99,6 +99,15 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
def _tree_to_qlit(obj: JSONValue,
level: int = 0,
dict_value: bool = False) -> str:
+ """
+ Convert the type tree into a QLIT C string, recursively.
+
+ :param obj: The value to convert.
+ This value may not be Annotated when dict_value is True.
+ :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 * ' '
@@ -244,6 +253,15 @@ def _gen_features(features: List[QAPISchemaFeature]
def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
ifcond: Sequence[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: Sequence of conditionals that apply to this entity.
+ :param features: Optional features field for SchemaInfo.
+ """
comment: Optional[str] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v6 16/19] qapi/introspect.py: Update copyright and authors list
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (14 preceding siblings ...)
2021-02-16 2:18 ` [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit John Snow
@ 2021-02-16 2:18 ` John Snow
2021-02-16 2:18 ` [PATCH v6 17/19] qapi/introspect.py: Type _gen_tree variants as Sequence[str] John Snow
` (2 subsequent siblings)
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:18 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
To reflect the work that went into strictly typing introspect.py,
punish myself by claiming credit.
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 5d4f5e23f7e..649225988d1 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] 42+ messages in thread
* [PATCH v6 17/19] qapi/introspect.py: Type _gen_tree variants as Sequence[str]
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (15 preceding siblings ...)
2021-02-16 2:18 ` [PATCH v6 16/19] qapi/introspect.py: Update copyright and authors list John Snow
@ 2021-02-16 2:18 ` John Snow
2021-02-16 9:10 ` Markus Armbruster
2021-02-16 2:18 ` [PATCH v6 18/19] qapi/introspect.py: set _gen_tree's default ifcond argument to () John Snow
2021-02-16 2:18 ` [PATCH v6 19/19] qapi/introspect.py: add SchemaMetaType enum John Snow
18 siblings, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-16 2:18 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
Optional[List] is clunky; an empty sequence can more elegantly convey
"no variants". By downgrading "List" to "Sequence", we can also accept
tuples; this is useful for the empty tuple specifically, which we may
use as a default parameter because it is immutable.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 649225988d1..e4d31a59503 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -247,13 +247,13 @@ def _use_type(self, typ: QAPISchemaType) -> str:
return self._name(typ.name)
@staticmethod
- def _gen_features(features: List[QAPISchemaFeature]
+ def _gen_features(features: Sequence[QAPISchemaFeature]
) -> List[Annotated[str]]:
return [Annotated(f.name, f.ifcond) for f in features]
def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
ifcond: Sequence[str],
- features: Optional[List[QAPISchemaFeature]]) -> None:
+ features: Sequence[QAPISchemaFeature] = ()) -> None:
"""
Build and append a SchemaInfo object to self._trees.
@@ -261,7 +261,8 @@ def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
:param mtype: The entity's meta-type.
:param obj: Additional entity fields, as appropriate for the meta-type.
:param ifcond: Sequence of conditionals that apply to this entity.
- :param features: Optional features field for SchemaInfo.
+ :param features: Optional, The features field for SchemaInfo.
+ Will be omitted from the output if empty.
"""
comment: Optional[str] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
@@ -298,7 +299,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
json_type: str) -> None:
- self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
+ self._gen_tree(name, 'builtin', {'json-type': json_type}, [])
def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
ifcond: Sequence[str],
@@ -316,7 +317,7 @@ def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
element_type: QAPISchemaType) -> None:
element = self._use_type(element_type)
self._gen_tree('[' + element + ']', 'array', {'element-type': element},
- ifcond, None)
+ ifcond)
def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
ifcond: Sequence[str],
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v6 18/19] qapi/introspect.py: set _gen_tree's default ifcond argument to ()
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (16 preceding siblings ...)
2021-02-16 2:18 ` [PATCH v6 17/19] qapi/introspect.py: Type _gen_tree variants as Sequence[str] John Snow
@ 2021-02-16 2:18 ` John Snow
2021-02-16 2:18 ` [PATCH v6 19/19] qapi/introspect.py: add SchemaMetaType enum John Snow
18 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 2:18 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
We don't need to create an empty, mutable list to pass to _gen_tree;
since it is now typed as a Sequence, we can use the empty tuple as a
default and omit the argument.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index e4d31a59503..c6f5cf8d874 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -252,7 +252,7 @@ def _gen_features(features: Sequence[QAPISchemaFeature]
return [Annotated(f.name, f.ifcond) for f in features]
def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
- ifcond: Sequence[str],
+ ifcond: Sequence[str] = (),
features: Sequence[QAPISchemaFeature] = ()) -> None:
"""
Build and append a SchemaInfo object to self._trees.
@@ -299,7 +299,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
json_type: str) -> None:
- self._gen_tree(name, 'builtin', {'json-type': json_type}, [])
+ self._gen_tree(name, 'builtin', {'json-type': json_type})
def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
ifcond: Sequence[str],
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v6 19/19] qapi/introspect.py: add SchemaMetaType enum
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
` (17 preceding siblings ...)
2021-02-16 2:18 ` [PATCH v6 18/19] qapi/introspect.py: set _gen_tree's default ifcond argument to () John Snow
@ 2021-02-16 2:18 ` John Snow
2021-02-16 9:24 ` Markus Armbruster
18 siblings, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-16 2:18 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster
Cc: Michael Roth, John Snow, Eduardo Habkost, Cleber Rosa
Follows the qapi/introspect.py definition of the same; this adds a more
precise typing to _gen_tree's mtype parameter.
NB: print(SchemaMetaType.BUILTIN) would produce the string
"SchemaMetaType.BUILTIN", but when using format strings (.format or f-strings),
it relies on the __format__ method defined in the Enum class, which uses the
"value" of the enum instead, producing the string "builtin".
For consistency with old-style format strings (which simply call the
__str__ method of an object), a __str__ dunder is added, though it is
not actually used here in this code.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index c6f5cf8d874..008a21f5c4c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -11,6 +11,7 @@
See the COPYING file in the top-level directory.
"""
+from enum import Enum
from typing import (
Any,
Dict,
@@ -79,6 +80,23 @@
SchemaInfoCommand = Dict[str, object]
+class SchemaMetaType(str, Enum):
+ """
+ Mimics the SchemaMetaType enum from qapi/introspect.json.
+ """
+ BUILTIN = 'builtin'
+ ENUM = 'enum'
+ ARRAY = 'array'
+ OBJECT = 'object'
+ ALTERNATE = 'alternate'
+ COMMAND = 'command'
+ EVENT = 'event'
+
+ def __str__(self) -> str:
+ # Needed for intuitive behavior with old-style format strings.
+ return str(self.value)
+
+
_ValueT = TypeVar('_ValueT', bound=_Value)
@@ -251,7 +269,8 @@ def _gen_features(features: Sequence[QAPISchemaFeature]
) -> List[Annotated[str]]:
return [Annotated(f.name, f.ifcond) for f in features]
- def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
+ def _gen_tree(self, name: str, mtype: SchemaMetaType,
+ obj: Dict[str, object],
ifcond: Sequence[str] = (),
features: Sequence[QAPISchemaFeature] = ()) -> None:
"""
@@ -299,7 +318,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
json_type: str) -> None:
- self._gen_tree(name, 'builtin', {'json-type': json_type})
+ self._gen_tree(name, SchemaMetaType.BUILTIN, {'json-type': json_type})
def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
ifcond: Sequence[str],
@@ -307,7 +326,7 @@ def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
members: List[QAPISchemaEnumMember],
prefix: Optional[str]) -> None:
self._gen_tree(
- name, 'enum',
+ name, SchemaMetaType.ENUM,
{'values': [Annotated(m.name, m.ifcond) for m in members]},
ifcond, features
)
@@ -316,8 +335,8 @@ def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
ifcond: Sequence[str],
element_type: QAPISchemaType) -> None:
element = self._use_type(element_type)
- self._gen_tree('[' + element + ']', 'array', {'element-type': element},
- ifcond)
+ self._gen_tree('[' + element + ']', SchemaMetaType.ARRAY,
+ {'element-type': element}, ifcond)
def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
ifcond: Sequence[str],
@@ -330,14 +349,14 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
if 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)
+ self._gen_tree(name, SchemaMetaType.OBJECT, obj, ifcond, features)
def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
ifcond: Sequence[str],
features: List[QAPISchemaFeature],
variants: QAPISchemaVariants) -> None:
self._gen_tree(
- name, 'alternate',
+ name, SchemaMetaType.ALTERNATE,
{'members': [Annotated({'type': self._use_type(m.type)},
m.ifcond)
for m in variants.variants]},
@@ -361,7 +380,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
}
if allow_oob:
obj['allow-oob'] = allow_oob
- self._gen_tree(name, 'command', obj, ifcond, features)
+ self._gen_tree(name, SchemaMetaType.COMMAND, obj, ifcond, features)
def visit_event(self, name: str, info: Optional[QAPISourceInfo],
ifcond: Sequence[str], features: List[QAPISchemaFeature],
@@ -370,7 +389,8 @@ def visit_event(self, name: str, info: Optional[QAPISourceInfo],
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)},
+ self._gen_tree(name, SchemaMetaType.EVENT,
+ {'arg-type': self._use_type(arg_type)},
ifcond, features)
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v6 01/19] qapi: Replace List[str] with Sequence[str] for ifcond
2021-02-16 2:17 ` [PATCH v6 01/19] qapi: Replace List[str] with Sequence[str] for ifcond John Snow
@ 2021-02-16 8:43 ` Markus Armbruster
2021-02-16 15:19 ` John Snow
0 siblings, 1 reply; 42+ messages in thread
From: Markus Armbruster @ 2021-02-16 8:43 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> It does happen to be a list (as of now), but we can describe it in more
> general terms with no loss in accuracy to allow tuples and other
> constructs.
>
> In the future, we can write "ifcond: Sequence[str] = ()" as a default
> parameter, which we could not do with a Mutable type like a List.
Well, we could write "= []", but we shouldn't. Worth a commit message
tweak?
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 14/19] qapi/introspect.py: add type hint annotations
2021-02-16 2:18 ` [PATCH v6 14/19] qapi/introspect.py: add type hint annotations John Snow
@ 2021-02-16 8:55 ` Markus Armbruster
2021-02-16 8:58 ` Markus Armbruster
2021-02-16 15:33 ` John Snow
2021-02-18 18:56 ` Markus Armbruster
1 sibling, 2 replies; 42+ messages in thread
From: Markus Armbruster @ 2021-02-16 8:55 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> NB: The type aliases (SchemaInfo et al) declare intent for some of the
> "dictly-typed" objects we pass around in introspect.py. They do not
> enforce the shape of those objects, and cannot, until Python 3.7 or
> later. (And even then, it may not be "worth it".)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 124 +++++++++++++++++++++++++++----------
> scripts/qapi/mypy.ini | 5 --
> scripts/qapi/schema.py | 2 +-
> 3 files changed, 92 insertions(+), 39 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index b0fcc4443c1..45284af1330 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -17,6 +17,7 @@
> Iterable,
> List,
> Optional,
> + Sequence,
> Tuple,
> TypeVar,
> Union,
> @@ -30,10 +31,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
> @@ -58,6 +68,15 @@
> _Value = Union[_Scalar, _NonScalar]
> JSONValue = Union[_Value, 'Annotated[_Value]']
>
> +# 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.
PEP 8: "For flowing long blocks of text with fewer structural
restrictions (docstrings or comments), the line length should be limited
to 72 characters."
> +SchemaInfo = Dict[str, object]
> +SchemaInfoObject = Dict[str, object]
> +SchemaInfoObjectVariant = Dict[str, object]
> +SchemaInfoObjectMember = Dict[str, object]
> +SchemaInfoCommand = Dict[str, object]
> +
>
> _ValueT = TypeVar('_ValueT', bound=_Value)
>
> @@ -77,9 +96,11 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
> self.ifcond: Tuple[str, ...] = tuple(ifcond)
>
>
> -def _tree_to_qlit(obj, level=0, dict_value=False):
> +def _tree_to_qlit(obj: JSONValue,
> + level: int = 0,
> + dict_value: bool = False) -> str:
>
> - def indent(level):
> + def indent(level: int) -> str:
> return level * 4 * ' '
>
> if isinstance(obj, Annotated):
> @@ -136,21 +157,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[SchemaInfo]] = []
> + self._used_types: List[QAPISchemaType] = []
> + self._name_map: Dict[str, str] = {}
> self._genc.add(mcgen('''
> #include "qemu/osdep.h"
> #include "%(prefix)sqapi-introspect.h"
> @@ -158,10 +179,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)
> @@ -183,18 +204,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
> @@ -216,10 +237,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: Dict[str, object],
Schould this be obj: SchemaInfo?
> + ifcond: Sequence[str],
> + features: Optional[List[QAPISchemaFeature]]) -> None:
> comment: Optional[str] = None
> if mtype not in ('command', 'event', 'builtin', 'array'):
> if not self._unmask:
> @@ -233,42 +257,65 @@ 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[SchemaInfoObjectMember]:
> + obj: SchemaInfoObjectMember = {
> + '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_variant(self, variant):
> - obj = {'case': variant.name, 'type': self._use_type(variant.type)}
> + def _gen_variant(self, variant: QAPISchemaVariant
> + ) -> Annotated[SchemaInfoObjectVariant]:
> + obj: SchemaInfoObjectVariant = {
> + '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: Sequence[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: Sequence[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: Sequence[str],
> + features: List[QAPISchemaFeature],
> + members: List[QAPISchemaObjectTypeMember],
> + variants: Optional[QAPISchemaVariants]) -> None:
> + obj: SchemaInfoObject = {
> + 'members': [self._gen_member(m) for m in members]
> + }
> if 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, info, ifcond, features, variants):
> + def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
> + ifcond: Sequence[str],
> + features: List[QAPISchemaFeature],
> + variants: QAPISchemaVariants) -> None:
> self._gen_tree(
> name, 'alternate',
> {'members': [Annotated({'type': self._use_type(m.type)},
> @@ -277,27 +324,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: Sequence[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: SchemaInfoCommand = {
> + '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: Sequence[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)
How is this hunk related to typing introspect.py
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 14/19] qapi/introspect.py: add type hint annotations
2021-02-16 8:55 ` Markus Armbruster
@ 2021-02-16 8:58 ` Markus Armbruster
2021-02-16 15:33 ` John Snow
1 sibling, 0 replies; 42+ messages in thread
From: Markus Armbruster @ 2021-02-16 8:58 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
Markus Armbruster <armbru@redhat.com> writes:
> John Snow <jsnow@redhat.com> writes:
>
>> NB: The type aliases (SchemaInfo et al) declare intent for some of the
>> "dictly-typed" objects we pass around in introspect.py. They do not
>> enforce the shape of those objects, and cannot, until Python 3.7 or
>> later. (And even then, it may not be "worth it".)
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 124 +++++++++++++++++++++++++++----------
>> scripts/qapi/mypy.ini | 5 --
>> scripts/qapi/schema.py | 2 +-
>> 3 files changed, 92 insertions(+), 39 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index b0fcc4443c1..45284af1330 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
[...]
>> @@ -216,10 +237,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: Dict[str, object],
>
> Schould this be obj: SchemaInfo?
No, because @obj is incomplete; _gen_tree() completes it.
>
>> + ifcond: Sequence[str],
>> + features: Optional[List[QAPISchemaFeature]]) -> None:
>> comment: Optional[str] = None
>> if mtype not in ('command', 'event', 'builtin', 'array'):
>> if not self._unmask:
[...]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 17/19] qapi/introspect.py: Type _gen_tree variants as Sequence[str]
2021-02-16 2:18 ` [PATCH v6 17/19] qapi/introspect.py: Type _gen_tree variants as Sequence[str] John Snow
@ 2021-02-16 9:10 ` Markus Armbruster
2021-02-16 15:13 ` John Snow
0 siblings, 1 reply; 42+ messages in thread
From: Markus Armbruster @ 2021-02-16 9:10 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Optional[List] is clunky; an empty sequence can more elegantly convey
> "no variants". By downgrading "List" to "Sequence", we can also accept
> tuples; this is useful for the empty tuple specifically, which we may
> use as a default parameter because it is immutable.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 649225988d1..e4d31a59503 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -247,13 +247,13 @@ def _use_type(self, typ: QAPISchemaType) -> str:
> return self._name(typ.name)
>
> @staticmethod
> - def _gen_features(features: List[QAPISchemaFeature]
> + def _gen_features(features: Sequence[QAPISchemaFeature]
> ) -> List[Annotated[str]]:
> return [Annotated(f.name, f.ifcond) for f in features]
>
> def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
> ifcond: Sequence[str],
> - features: Optional[List[QAPISchemaFeature]]) -> None:
> + features: Sequence[QAPISchemaFeature] = ()) -> None:
> """
> Build and append a SchemaInfo object to self._trees.
>
> @@ -261,7 +261,8 @@ def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
> :param mtype: The entity's meta-type.
> :param obj: Additional entity fields, as appropriate for the meta-type.
> :param ifcond: Sequence of conditionals that apply to this entity.
> - :param features: Optional features field for SchemaInfo.
> + :param features: Optional, The features field for SchemaInfo.
Downcase "The".
> + Will be omitted from the output if empty.
> """
> comment: Optional[str] = None
> if mtype not in ('command', 'event', 'builtin', 'array'):
> @@ -298,7 +299,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
>
> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
> json_type: str) -> None:
> - self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
> + self._gen_tree(name, 'builtin', {'json-type': json_type}, [])
>
> def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
> ifcond: Sequence[str],
> @@ -316,7 +317,7 @@ def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
> element_type: QAPISchemaType) -> None:
> element = self._use_type(element_type)
> self._gen_tree('[' + element + ']', 'array', {'element-type': element},
> - ifcond, None)
> + ifcond)
>
> def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
> ifcond: Sequence[str],
Marginal by itself. Might be worth it as part of a more general move
away from Optional[List[...]]. See also next patch.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 19/19] qapi/introspect.py: add SchemaMetaType enum
2021-02-16 2:18 ` [PATCH v6 19/19] qapi/introspect.py: add SchemaMetaType enum John Snow
@ 2021-02-16 9:24 ` Markus Armbruster
2021-02-16 15:08 ` John Snow
0 siblings, 1 reply; 42+ messages in thread
From: Markus Armbruster @ 2021-02-16 9:24 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Follows the qapi/introspect.py definition of the same; this adds a more
> precise typing to _gen_tree's mtype parameter.
>
> NB: print(SchemaMetaType.BUILTIN) would produce the string
> "SchemaMetaType.BUILTIN", but when using format strings (.format or f-strings),
> it relies on the __format__ method defined in the Enum class, which uses the
> "value" of the enum instead, producing the string "builtin".
>
> For consistency with old-style format strings (which simply call the
> __str__ method of an object), a __str__ dunder is added, though it is
> not actually used here in this code.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 38 +++++++++++++++++++++++++++++---------
> 1 file changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index c6f5cf8d874..008a21f5c4c 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -11,6 +11,7 @@
> See the COPYING file in the top-level directory.
> """
>
> +from enum import Enum
> from typing import (
> Any,
> Dict,
> @@ -79,6 +80,23 @@
> SchemaInfoCommand = Dict[str, object]
>
>
> +class SchemaMetaType(str, Enum):
> + """
> + Mimics the SchemaMetaType enum from qapi/introspect.json.
> + """
> + BUILTIN = 'builtin'
> + ENUM = 'enum'
> + ARRAY = 'array'
> + OBJECT = 'object'
> + ALTERNATE = 'alternate'
> + COMMAND = 'command'
> + EVENT = 'event'
> +
> + def __str__(self) -> str:
> + # Needed for intuitive behavior with old-style format strings.
> + return str(self.value)
> +
> +
The fanciness compared to plain Enum('SchemaMetaType', 'BUILTIN ...')
avoids extra code to map the enum values to the strings with need.
> _ValueT = TypeVar('_ValueT', bound=_Value)
>
>
> @@ -251,7 +269,8 @@ def _gen_features(features: Sequence[QAPISchemaFeature]
> ) -> List[Annotated[str]]:
> return [Annotated(f.name, f.ifcond) for f in features]
>
> - def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
> + def _gen_tree(self, name: str, mtype: SchemaMetaType,
> + obj: Dict[str, object],
> ifcond: Sequence[str] = (),
> features: Sequence[QAPISchemaFeature] = ()) -> None:
> """
> @@ -299,7 +318,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
>
> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
> json_type: str) -> None:
> - self._gen_tree(name, 'builtin', {'json-type': json_type})
> + self._gen_tree(name, SchemaMetaType.BUILTIN, {'json-type': json_type})
>
> def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
> ifcond: Sequence[str],
> @@ -307,7 +326,7 @@ def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
> members: List[QAPISchemaEnumMember],
> prefix: Optional[str]) -> None:
> self._gen_tree(
> - name, 'enum',
> + name, SchemaMetaType.ENUM,
> {'values': [Annotated(m.name, m.ifcond) for m in members]},
> ifcond, features
> )
> @@ -316,8 +335,8 @@ def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
> ifcond: Sequence[str],
> element_type: QAPISchemaType) -> None:
> element = self._use_type(element_type)
> - self._gen_tree('[' + element + ']', 'array', {'element-type': element},
> - ifcond)
> + self._gen_tree('[' + element + ']', SchemaMetaType.ARRAY,
> + {'element-type': element}, ifcond)
>
> def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
> ifcond: Sequence[str],
> @@ -330,14 +349,14 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
> if 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)
> + self._gen_tree(name, SchemaMetaType.OBJECT, obj, ifcond, features)
>
> def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
> ifcond: Sequence[str],
> features: List[QAPISchemaFeature],
> variants: QAPISchemaVariants) -> None:
> self._gen_tree(
> - name, 'alternate',
> + name, SchemaMetaType.ALTERNATE,
> {'members': [Annotated({'type': self._use_type(m.type)},
> m.ifcond)
> for m in variants.variants]},
> @@ -361,7 +380,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
> }
> if allow_oob:
> obj['allow-oob'] = allow_oob
> - self._gen_tree(name, 'command', obj, ifcond, features)
> + self._gen_tree(name, SchemaMetaType.COMMAND, obj, ifcond, features)
>
> def visit_event(self, name: str, info: Optional[QAPISourceInfo],
> ifcond: Sequence[str], features: List[QAPISchemaFeature],
> @@ -370,7 +389,8 @@ def visit_event(self, name: str, info: Optional[QAPISourceInfo],
> 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)},
> + self._gen_tree(name, SchemaMetaType.EVENT,
> + {'arg-type': self._use_type(arg_type)},
> ifcond, features)
Gain: _gen_tree()'s second argument's type now serves as documentation,
and passing crap to it becomes harder.
Gut feeling: too much notational overhead for too little gain.
Opinions?
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 19/19] qapi/introspect.py: add SchemaMetaType enum
2021-02-16 9:24 ` Markus Armbruster
@ 2021-02-16 15:08 ` John Snow
2021-02-16 16:09 ` Markus Armbruster
0 siblings, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-16 15:08 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/16/21 4:24 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Follows the qapi/introspect.py definition of the same; this adds a more
>> precise typing to _gen_tree's mtype parameter.
>>
>> NB: print(SchemaMetaType.BUILTIN) would produce the string
>> "SchemaMetaType.BUILTIN", but when using format strings (.format or f-strings),
>> it relies on the __format__ method defined in the Enum class, which uses the
>> "value" of the enum instead, producing the string "builtin".
>>
>> For consistency with old-style format strings (which simply call the
>> __str__ method of an object), a __str__ dunder is added, though it is
>> not actually used here in this code.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 38 +++++++++++++++++++++++++++++---------
>> 1 file changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index c6f5cf8d874..008a21f5c4c 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -11,6 +11,7 @@
>> See the COPYING file in the top-level directory.
>> """
>>
>> +from enum import Enum
>> from typing import (
>> Any,
>> Dict,
>> @@ -79,6 +80,23 @@
>> SchemaInfoCommand = Dict[str, object]
>>
>>
>> +class SchemaMetaType(str, Enum):
>> + """
>> + Mimics the SchemaMetaType enum from qapi/introspect.json.
>> + """
>> + BUILTIN = 'builtin'
>> + ENUM = 'enum'
>> + ARRAY = 'array'
>> + OBJECT = 'object'
>> + ALTERNATE = 'alternate'
>> + COMMAND = 'command'
>> + EVENT = 'event'
>> +
>> + def __str__(self) -> str:
>> + # Needed for intuitive behavior with old-style format strings.
>> + return str(self.value)
>> +
>> +
>
> The fanciness compared to plain Enum('SchemaMetaType', 'BUILTIN ...')
> avoids extra code to map the enum values to the strings with need.
>
I wasn't even aware there was a short form. (TIL!)
This form allows me to inherit from str and pass the value around
anywhere strings are used. Due to the generalized nature of
tree_to_qlit, using the short constructor form (which creates ints)
would need additional magic to be useful.
You can almost replicate it:
_values = ('builtin', 'enum', 'array')
_nv_pairs = [(value.upper(), value) for value in _values]
SchemaMetaType = enum.Enum('SchemaMetaType', _nv_pairs, type=str)
though this loses out on the __str__ method hack, and I don't think mypy
will be able to introspect into this functional constructor.
>> _ValueT = TypeVar('_ValueT', bound=_Value)
>>
>>
>> @@ -251,7 +269,8 @@ def _gen_features(features: Sequence[QAPISchemaFeature]
>> ) -> List[Annotated[str]]:
>> return [Annotated(f.name, f.ifcond) for f in features]
>>
>> - def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>> + def _gen_tree(self, name: str, mtype: SchemaMetaType,
>> + obj: Dict[str, object],
>> ifcond: Sequence[str] = (),
>> features: Sequence[QAPISchemaFeature] = ()) -> None:
>> """
>> @@ -299,7 +318,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
>>
>> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>> json_type: str) -> None:
>> - self._gen_tree(name, 'builtin', {'json-type': json_type})
>> + self._gen_tree(name, SchemaMetaType.BUILTIN, {'json-type': json_type})
>>
>> def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>> ifcond: Sequence[str],
>> @@ -307,7 +326,7 @@ def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>> members: List[QAPISchemaEnumMember],
>> prefix: Optional[str]) -> None:
>> self._gen_tree(
>> - name, 'enum',
>> + name, SchemaMetaType.ENUM,
>> {'values': [Annotated(m.name, m.ifcond) for m in members]},
>> ifcond, features
>> )
>> @@ -316,8 +335,8 @@ def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
>> ifcond: Sequence[str],
>> element_type: QAPISchemaType) -> None:
>> element = self._use_type(element_type)
>> - self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>> - ifcond)
>> + self._gen_tree('[' + element + ']', SchemaMetaType.ARRAY,
>> + {'element-type': element}, ifcond)
>>
>> def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>> ifcond: Sequence[str],
>> @@ -330,14 +349,14 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>> if 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)
>> + self._gen_tree(name, SchemaMetaType.OBJECT, obj, ifcond, features)
>>
>> def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>> ifcond: Sequence[str],
>> features: List[QAPISchemaFeature],
>> variants: QAPISchemaVariants) -> None:
>> self._gen_tree(
>> - name, 'alternate',
>> + name, SchemaMetaType.ALTERNATE,
>> {'members': [Annotated({'type': self._use_type(m.type)},
>> m.ifcond)
>> for m in variants.variants]},
>> @@ -361,7 +380,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
>> }
>> if allow_oob:
>> obj['allow-oob'] = allow_oob
>> - self._gen_tree(name, 'command', obj, ifcond, features)
>> + self._gen_tree(name, SchemaMetaType.COMMAND, obj, ifcond, features)
>>
>> def visit_event(self, name: str, info: Optional[QAPISourceInfo],
>> ifcond: Sequence[str], features: List[QAPISchemaFeature],
>> @@ -370,7 +389,8 @@ def visit_event(self, name: str, info: Optional[QAPISourceInfo],
>> 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)},
>> + self._gen_tree(name, SchemaMetaType.EVENT,
>> + {'arg-type': self._use_type(arg_type)},
>> ifcond, features)
>
> Gain: _gen_tree()'s second argument's type now serves as documentation,
> and passing crap to it becomes harder.
>
> Gut feeling: too much notational overhead for too little gain.
>
> Opinions?
>
No strong feelings one way or the other, honestly. I wrote this mostly
to see how much the overhead would be based on your comment about the
loose typing of meta-type as str.
If it's too much for too little for you, I'm okay without it too.
--js
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 17/19] qapi/introspect.py: Type _gen_tree variants as Sequence[str]
2021-02-16 9:10 ` Markus Armbruster
@ 2021-02-16 15:13 ` John Snow
0 siblings, 0 replies; 42+ messages in thread
From: John Snow @ 2021-02-16 15:13 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/16/21 4:10 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Optional[List] is clunky; an empty sequence can more elegantly convey
>> "no variants". By downgrading "List" to "Sequence", we can also accept
>> tuples; this is useful for the empty tuple specifically, which we may
>> use as a default parameter because it is immutable.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 649225988d1..e4d31a59503 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -247,13 +247,13 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>> return self._name(typ.name)
>>
>> @staticmethod
>> - def _gen_features(features: List[QAPISchemaFeature]
>> + def _gen_features(features: Sequence[QAPISchemaFeature]
>> ) -> List[Annotated[str]]:
>> return [Annotated(f.name, f.ifcond) for f in features]
>>
>> def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>> ifcond: Sequence[str],
>> - features: Optional[List[QAPISchemaFeature]]) -> None:
>> + features: Sequence[QAPISchemaFeature] = ()) -> None:
>> """
>> Build and append a SchemaInfo object to self._trees.
>>
>> @@ -261,7 +261,8 @@ def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>> :param mtype: The entity's meta-type.
>> :param obj: Additional entity fields, as appropriate for the meta-type.
>> :param ifcond: Sequence of conditionals that apply to this entity.
>> - :param features: Optional features field for SchemaInfo.
>> + :param features: Optional, The features field for SchemaInfo.
>
> Downcase "The".
>
>> + Will be omitted from the output if empty.
>> """
>> comment: Optional[str] = None
>> if mtype not in ('command', 'event', 'builtin', 'array'):
>> @@ -298,7 +299,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
>>
>> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>> json_type: str) -> None:
>> - self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>> + self._gen_tree(name, 'builtin', {'json-type': json_type}, [])
>>
>> def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>> ifcond: Sequence[str],
>> @@ -316,7 +317,7 @@ def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
>> element_type: QAPISchemaType) -> None:
>> element = self._use_type(element_type)
>> self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>> - ifcond, None)
>> + ifcond)
>>
>> def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>> ifcond: Sequence[str],
>
> Marginal by itself. Might be worth it as part of a more general move
> away from Optional[List[...]]. See also next patch.
>
Yep, it's tiny. Still, maybe worth taking just to remove a bad habit
from the code in case of cargo-culting?
Go with whatcha feel, it's a style nit you raised -- I'm not sure I'll
remember to do a more thorough pass after pt6 is done.
ACK to change the comment casing if you take these.
--js
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 01/19] qapi: Replace List[str] with Sequence[str] for ifcond
2021-02-16 8:43 ` Markus Armbruster
@ 2021-02-16 15:19 ` John Snow
2021-02-16 15:58 ` Markus Armbruster
0 siblings, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-16 15:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/16/21 3:43 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> It does happen to be a list (as of now), but we can describe it in more
>> general terms with no loss in accuracy to allow tuples and other
>> constructs.
>>
>> In the future, we can write "ifcond: Sequence[str] = ()" as a default
>> parameter, which we could not do with a Mutable type like a List.
>
> Well, we could write "= []", but we shouldn't. Worth a commit message
> tweak?
>
It would be funny to leave it in to see if anyone tries to disprove me,
and in the act of disproving me, learns for themselves why you "can't"
do that. Rite of passage for Python programming.
Jokes aside:
"which we could not do ^safely^ with a Mutable type like a List."
If that warrants further exposition by Professor Snow:
"(Unsafe due to how Python initializes defaults, see
https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments)"
I leave it to your discretion.
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
These are worth more than BTC!
--js
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 14/19] qapi/introspect.py: add type hint annotations
2021-02-16 8:55 ` Markus Armbruster
2021-02-16 8:58 ` Markus Armbruster
@ 2021-02-16 15:33 ` John Snow
2021-02-16 16:08 ` Markus Armbruster
1 sibling, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-16 15:33 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/16/21 3:55 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> NB: The type aliases (SchemaInfo et al) declare intent for some of the
>> "dictly-typed" objects we pass around in introspect.py. They do not
>> enforce the shape of those objects, and cannot, until Python 3.7 or
>> later. (And even then, it may not be "worth it".)
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 124 +++++++++++++++++++++++++++----------
>> scripts/qapi/mypy.ini | 5 --
>> scripts/qapi/schema.py | 2 +-
>> 3 files changed, 92 insertions(+), 39 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index b0fcc4443c1..45284af1330 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -17,6 +17,7 @@
>> Iterable,
>> List,
>> Optional,
>> + Sequence,
>> Tuple,
>> TypeVar,
>> Union,
>> @@ -30,10 +31,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
>> @@ -58,6 +68,15 @@
>> _Value = Union[_Scalar, _NonScalar]
>> JSONValue = Union[_Value, 'Annotated[_Value]']
>>
>> +# 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.
>
> PEP 8: "For flowing long blocks of text with fewer structural
> restrictions (docstrings or comments), the line length should be limited
> to 72 characters."
>
I'm very likely going to keep violating this until some tool enforces it
on me. I'm also very unlikely to enforce it for anyone else.
You can reflow it as you see fit, but I'll likely need better long-term
assistance for remembering that 72/80 column DANGER ZONE.
>> +SchemaInfo = Dict[str, object]
>> +SchemaInfoObject = Dict[str, object]
>> +SchemaInfoObjectVariant = Dict[str, object]
>> +SchemaInfoObjectMember = Dict[str, object]
>> +SchemaInfoCommand = Dict[str, object]
>> +
>>
>> _ValueT = TypeVar('_ValueT', bound=_Value)
>>
>> @@ -77,9 +96,11 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
>> self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>
>>
>> -def _tree_to_qlit(obj, level=0, dict_value=False):
>> +def _tree_to_qlit(obj: JSONValue,
>> + level: int = 0,
>> + dict_value: bool = False) -> str:
>>
>> - def indent(level):
>> + def indent(level: int) -> str:
>> return level * 4 * ' '
>>
>> if isinstance(obj, Annotated):
>> @@ -136,21 +157,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[SchemaInfo]] = []
>> + self._used_types: List[QAPISchemaType] = []
>> + self._name_map: Dict[str, str] = {}
>> self._genc.add(mcgen('''
>> #include "qemu/osdep.h"
>> #include "%(prefix)sqapi-introspect.h"
>> @@ -158,10 +179,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)
>> @@ -183,18 +204,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
>> @@ -216,10 +237,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: Dict[str, object],
>
> Schould this be obj: SchemaInfo?
>
Yes-ish. It's kind of like the dictly-typed object is being promoted to
a SchemaInfo. In a sense, it isn't one yet (It's missing necessary
keys), but we upgrade it into one in this very function.
I talk about TypedDict a lot and how we don't have it yet; one
interesting feature of TypedDict is that it doesn't allow you to
incrementally build the object -- it requires all of the necessary keys
be present right away.
If we were to have that kind of model in our heads, then this wouldn't
be a SchemaInfo coming in.
So I'll admit here: I don't know. It depends on your perspective,
honestly. It might be the sort of thing that a docstring comment would
be best at addressing, since we're already in the margins for what mypy
can reasonably enforce statically.
>> + ifcond: Sequence[str],
>> + features: Optional[List[QAPISchemaFeature]]) -> None:
>> comment: Optional[str] = None
>> if mtype not in ('command', 'event', 'builtin', 'array'):
>> if not self._unmask:
>> @@ -233,42 +257,65 @@ 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[SchemaInfoObjectMember]:
>> + obj: SchemaInfoObjectMember = {
>> + '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_variant(self, variant):
>> - obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>> + def _gen_variant(self, variant: QAPISchemaVariant
>> + ) -> Annotated[SchemaInfoObjectVariant]:
>> + obj: SchemaInfoObjectVariant = {
>> + '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: Sequence[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: Sequence[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: Sequence[str],
>> + features: List[QAPISchemaFeature],
>> + members: List[QAPISchemaObjectTypeMember],
>> + variants: Optional[QAPISchemaVariants]) -> None:
>> + obj: SchemaInfoObject = {
>> + 'members': [self._gen_member(m) for m in members]
>> + }
>> if 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, info, ifcond, features, variants):
>> + def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>> + ifcond: Sequence[str],
>> + features: List[QAPISchemaFeature],
>> + variants: QAPISchemaVariants) -> None:
>> self._gen_tree(
>> name, 'alternate',
>> {'members': [Annotated({'type': self._use_type(m.type)},
>> @@ -277,27 +324,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: Sequence[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: SchemaInfoCommand = {
>> + '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: Sequence[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)
>
> How is this hunk related to typing introspect.py
>
I forget!
qapi/introspect.py:262: error: Returning Any from function declared to
return "str"
Found 1 error in 1 file (checked 14 source files)
Oh, for this reason:
if isinstance(typ, QAPISchemaBuiltinType):
return typ.name
_use_type has a return type that is dependent upon the type of
"typ.name", which required typing the QAPISchemaEntity initializer.
(Do you want this in its own preceding patch?)
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 01/19] qapi: Replace List[str] with Sequence[str] for ifcond
2021-02-16 15:19 ` John Snow
@ 2021-02-16 15:58 ` Markus Armbruster
0 siblings, 0 replies; 42+ messages in thread
From: Markus Armbruster @ 2021-02-16 15:58 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/16/21 3:43 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> It does happen to be a list (as of now), but we can describe it in more
>>> general terms with no loss in accuracy to allow tuples and other
>>> constructs.
>>>
>>> In the future, we can write "ifcond: Sequence[str] = ()" as a default
>>> parameter, which we could not do with a Mutable type like a List.
>> Well, we could write "= []", but we shouldn't. Worth a commit
>> message
>> tweak?
>>
>
> It would be funny to leave it in to see if anyone tries to disprove
> me, and in the act of disproving me, learns for themselves why you
> "can't" do that. Rite of passage for Python programming.
>
> Jokes aside:
>
> "which we could not do ^safely^ with a Mutable type like a List."
Works for me.
> If that warrants further exposition by Professor Snow:
>
> "(Unsafe due to how Python initializes defaults, see
> https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments)"
I can add that if you prefer.
> I leave it to your discretion.
>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>
> These are worth more than BTC!
;)
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 14/19] qapi/introspect.py: add type hint annotations
2021-02-16 15:33 ` John Snow
@ 2021-02-16 16:08 ` Markus Armbruster
2021-02-16 16:19 ` John Snow
0 siblings, 1 reply; 42+ messages in thread
From: Markus Armbruster @ 2021-02-16 16:08 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/16/21 3:55 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> NB: The type aliases (SchemaInfo et al) declare intent for some of the
>>> "dictly-typed" objects we pass around in introspect.py. They do not
>>> enforce the shape of those objects, and cannot, until Python 3.7 or
>>> later. (And even then, it may not be "worth it".)
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/introspect.py | 124 +++++++++++++++++++++++++++----------
>>> scripts/qapi/mypy.ini | 5 --
>>> scripts/qapi/schema.py | 2 +-
>>> 3 files changed, 92 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index b0fcc4443c1..45284af1330 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -17,6 +17,7 @@
>>> Iterable,
>>> List,
>>> Optional,
>>> + Sequence,
>>> Tuple,
>>> TypeVar,
>>> Union,
>>> @@ -30,10 +31,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
>>> @@ -58,6 +68,15 @@
>>> _Value = Union[_Scalar, _NonScalar]
>>> JSONValue = Union[_Value, 'Annotated[_Value]']
>>> +# 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.
>>
>> PEP 8: "For flowing long blocks of text with fewer structural
>> restrictions (docstrings or comments), the line length should be limited
>> to 72 characters."
>>
>
> I'm very likely going to keep violating this until some tool enforces
> it on me. I'm also very unlikely to enforce it for anyone else.
>
> You can reflow it as you see fit, but I'll likely need better
> long-term assistance for remembering that 72/80 column DANGER ZONE.
Automated assistance would be nice, but not having it is no big deal for
me. I don't mind pointing out the occasional long line I spot in
review.
>>> +SchemaInfo = Dict[str, object]
>>> +SchemaInfoObject = Dict[str, object]
>>> +SchemaInfoObjectVariant = Dict[str, object]
>>> +SchemaInfoObjectMember = Dict[str, object]
>>> +SchemaInfoCommand = Dict[str, object]
>>> +
>>> _ValueT = TypeVar('_ValueT', bound=_Value)
>>> @@ -77,9 +96,11 @@ def __init__(self, value: _ValueT, ifcond:
>>> Iterable[str],
>>> self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>>
>>> -def _tree_to_qlit(obj, level=0, dict_value=False):
>>> +def _tree_to_qlit(obj: JSONValue,
>>> + level: int = 0,
>>> + dict_value: bool = False) -> str:
>>> - def indent(level):
>>> + def indent(level: int) -> str:
>>> return level * 4 * ' '
>>> if isinstance(obj, Annotated):
>>> @@ -136,21 +157,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[SchemaInfo]] = []
>>> + self._used_types: List[QAPISchemaType] = []
>>> + self._name_map: Dict[str, str] = {}
>>> self._genc.add(mcgen('''
>>> #include "qemu/osdep.h"
>>> #include "%(prefix)sqapi-introspect.h"
>>> @@ -158,10 +179,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)
>>> @@ -183,18 +204,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
>>> @@ -216,10 +237,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: Dict[str, object],
>>
>> Schould this be obj: SchemaInfo?
>>
>
> Yes-ish. It's kind of like the dictly-typed object is being promoted
> to a SchemaInfo. In a sense, it isn't one yet (It's missing necessary
> keys), but we upgrade it into one in this very function.
>
> I talk about TypedDict a lot and how we don't have it yet; one
> interesting feature of TypedDict is that it doesn't allow you to
> incrementally build the object -- it requires all of the necessary
> keys be present right away.
>
> If we were to have that kind of model in our heads, then this wouldn't
> be a SchemaInfo coming in.
>
> So I'll admit here: I don't know. It depends on your perspective,
> honestly. It might be the sort of thing that a docstring comment would
> be best at addressing, since we're already in the margins for what
> mypy can reasonably enforce statically.
Let's leave it as is. Rationale: it only becomes a SchemaInfo in
_gen_tree().
>
>>> + ifcond: Sequence[str],
>>> + features: Optional[List[QAPISchemaFeature]]) -> None:
>>> comment: Optional[str] = None
>>> if mtype not in ('command', 'event', 'builtin', 'array'):
>>> if not self._unmask:
>>> @@ -233,42 +257,65 @@ 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[SchemaInfoObjectMember]:
>>> + obj: SchemaInfoObjectMember = {
>>> + '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_variant(self, variant):
>>> - obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>>> + def _gen_variant(self, variant: QAPISchemaVariant
>>> + ) -> Annotated[SchemaInfoObjectVariant]:
>>> + obj: SchemaInfoObjectVariant = {
>>> + '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: Sequence[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: Sequence[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: Sequence[str],
>>> + features: List[QAPISchemaFeature],
>>> + members: List[QAPISchemaObjectTypeMember],
>>> + variants: Optional[QAPISchemaVariants]) -> None:
>>> + obj: SchemaInfoObject = {
>>> + 'members': [self._gen_member(m) for m in members]
>>> + }
>>> if 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, info, ifcond, features,
>>> variants):
>>> + def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>>> + ifcond: Sequence[str],
>>> + features: List[QAPISchemaFeature],
>>> + variants: QAPISchemaVariants) -> None:
>>> self._gen_tree(
>>> name, 'alternate',
>>> {'members': [Annotated({'type': self._use_type(m.type)},
>>> @@ -277,27 +324,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: Sequence[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: SchemaInfoCommand = {
>>> + '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: Sequence[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)
>>
>> How is this hunk related to typing introspect.py
>>
>
> I forget!
>
> qapi/introspect.py:262: error: Returning Any from function declared to
> return "str"
> Found 1 error in 1 file (checked 14 source files)
>
> Oh, for this reason:
>
> if isinstance(typ, QAPISchemaBuiltinType):
> return typ.name
>
> _use_type has a return type that is dependent upon the type of
> "typ.name", which required typing the QAPISchemaEntity initializer.
>
>
> (Do you want this in its own preceding patch?)
That would work.
Keeping it in this patch with a suitable hint in the commit message
would also work. Up to you. If you want me to tweak in my tree, tell
me how.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 19/19] qapi/introspect.py: add SchemaMetaType enum
2021-02-16 15:08 ` John Snow
@ 2021-02-16 16:09 ` Markus Armbruster
0 siblings, 0 replies; 42+ messages in thread
From: Markus Armbruster @ 2021-02-16 16:09 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/16/21 4:24 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Follows the qapi/introspect.py definition of the same; this adds a more
>>> precise typing to _gen_tree's mtype parameter.
>>>
>>> NB: print(SchemaMetaType.BUILTIN) would produce the string
>>> "SchemaMetaType.BUILTIN", but when using format strings (.format or f-strings),
>>> it relies on the __format__ method defined in the Enum class, which uses the
>>> "value" of the enum instead, producing the string "builtin".
>>>
>>> For consistency with old-style format strings (which simply call the
>>> __str__ method of an object), a __str__ dunder is added, though it is
>>> not actually used here in this code.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
[...]
>> Gain: _gen_tree()'s second argument's type now serves as documentation,
>> and passing crap to it becomes harder.
>>
>> Gut feeling: too much notational overhead for too little gain.
>>
>> Opinions?
>>
>
> No strong feelings one way or the other, honestly. I wrote this mostly
> to see how much the overhead would be based on your comment about the
> loose typing of meta-type as str.
>
> If it's too much for too little for you, I'm okay without it too.
Let's put it aside for now.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 14/19] qapi/introspect.py: add type hint annotations
2021-02-16 16:08 ` Markus Armbruster
@ 2021-02-16 16:19 ` John Snow
2021-02-17 9:21 ` Markus Armbruster
0 siblings, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-16 16:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
On 2/16/21 11:08 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 2/16/21 3:55 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> NB: The type aliases (SchemaInfo et al) declare intent for some of the
>>>> "dictly-typed" objects we pass around in introspect.py. They do not
>>>> enforce the shape of those objects, and cannot, until Python 3.7 or
>>>> later. (And even then, it may not be "worth it".)
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> scripts/qapi/introspect.py | 124 +++++++++++++++++++++++++++----------
>>>> scripts/qapi/mypy.ini | 5 --
>>>> scripts/qapi/schema.py | 2 +-
>>>> 3 files changed, 92 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>>> index b0fcc4443c1..45284af1330 100644
>>>> --- a/scripts/qapi/introspect.py
>>>> +++ b/scripts/qapi/introspect.py
>>>> @@ -17,6 +17,7 @@
>>>> Iterable,
>>>> List,
>>>> Optional,
>>>> + Sequence,
>>>> Tuple,
>>>> TypeVar,
>>>> Union,
>>>> @@ -30,10 +31,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
>>>> @@ -58,6 +68,15 @@
>>>> _Value = Union[_Scalar, _NonScalar]
>>>> JSONValue = Union[_Value, 'Annotated[_Value]']
>>>> +# 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.
>>>
>>> PEP 8: "For flowing long blocks of text with fewer structural
>>> restrictions (docstrings or comments), the line length should be limited
>>> to 72 characters."
>>>
>>
>> I'm very likely going to keep violating this until some tool enforces
>> it on me. I'm also very unlikely to enforce it for anyone else.
>>
>> You can reflow it as you see fit, but I'll likely need better
>> long-term assistance for remembering that 72/80 column DANGER ZONE.
>
> Automated assistance would be nice, but not having it is no big deal for
> me. I don't mind pointing out the occasional long line I spot in
> review.
>
>>>> +SchemaInfo = Dict[str, object]
>>>> +SchemaInfoObject = Dict[str, object]
>>>> +SchemaInfoObjectVariant = Dict[str, object]
>>>> +SchemaInfoObjectMember = Dict[str, object]
>>>> +SchemaInfoCommand = Dict[str, object]
>>>> +
>>>> _ValueT = TypeVar('_ValueT', bound=_Value)
>>>> @@ -77,9 +96,11 @@ def __init__(self, value: _ValueT, ifcond:
>>>> Iterable[str],
>>>> self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>>>
>>>> -def _tree_to_qlit(obj, level=0, dict_value=False):
>>>> +def _tree_to_qlit(obj: JSONValue,
>>>> + level: int = 0,
>>>> + dict_value: bool = False) -> str:
>>>> - def indent(level):
>>>> + def indent(level: int) -> str:
>>>> return level * 4 * ' '
>>>> if isinstance(obj, Annotated):
>>>> @@ -136,21 +157,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[SchemaInfo]] = []
>>>> + self._used_types: List[QAPISchemaType] = []
>>>> + self._name_map: Dict[str, str] = {}
>>>> self._genc.add(mcgen('''
>>>> #include "qemu/osdep.h"
>>>> #include "%(prefix)sqapi-introspect.h"
>>>> @@ -158,10 +179,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)
>>>> @@ -183,18 +204,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
>>>> @@ -216,10 +237,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: Dict[str, object],
>>>
>>> Schould this be obj: SchemaInfo?
>>>
>>
>> Yes-ish. It's kind of like the dictly-typed object is being promoted
>> to a SchemaInfo. In a sense, it isn't one yet (It's missing necessary
>> keys), but we upgrade it into one in this very function.
>>
>> I talk about TypedDict a lot and how we don't have it yet; one
>> interesting feature of TypedDict is that it doesn't allow you to
>> incrementally build the object -- it requires all of the necessary
>> keys be present right away.
>>
>> If we were to have that kind of model in our heads, then this wouldn't
>> be a SchemaInfo coming in.
>>
>> So I'll admit here: I don't know. It depends on your perspective,
>> honestly. It might be the sort of thing that a docstring comment would
>> be best at addressing, since we're already in the margins for what
>> mypy can reasonably enforce statically.
>
> Let's leave it as is. Rationale: it only becomes a SchemaInfo in
> _gen_tree().
>
>>
>>>> + ifcond: Sequence[str],
>>>> + features: Optional[List[QAPISchemaFeature]]) -> None:
>>>> comment: Optional[str] = None
>>>> if mtype not in ('command', 'event', 'builtin', 'array'):
>>>> if not self._unmask:
>>>> @@ -233,42 +257,65 @@ 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[SchemaInfoObjectMember]:
>>>> + obj: SchemaInfoObjectMember = {
>>>> + '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_variant(self, variant):
>>>> - obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>>>> + def _gen_variant(self, variant: QAPISchemaVariant
>>>> + ) -> Annotated[SchemaInfoObjectVariant]:
>>>> + obj: SchemaInfoObjectVariant = {
>>>> + '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: Sequence[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: Sequence[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: Sequence[str],
>>>> + features: List[QAPISchemaFeature],
>>>> + members: List[QAPISchemaObjectTypeMember],
>>>> + variants: Optional[QAPISchemaVariants]) -> None:
>>>> + obj: SchemaInfoObject = {
>>>> + 'members': [self._gen_member(m) for m in members]
>>>> + }
>>>> if 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, info, ifcond, features,
>>>> variants):
>>>> + def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>>>> + ifcond: Sequence[str],
>>>> + features: List[QAPISchemaFeature],
>>>> + variants: QAPISchemaVariants) -> None:
>>>> self._gen_tree(
>>>> name, 'alternate',
>>>> {'members': [Annotated({'type': self._use_type(m.type)},
>>>> @@ -277,27 +324,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: Sequence[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: SchemaInfoCommand = {
>>>> + '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: Sequence[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)
>>>
>>> How is this hunk related to typing introspect.py
>>>
>>
>> I forget!
>>
>> qapi/introspect.py:262: error: Returning Any from function declared to
>> return "str"
>> Found 1 error in 1 file (checked 14 source files)
>>
>> Oh, for this reason:
>>
>> if isinstance(typ, QAPISchemaBuiltinType):
>> return typ.name
>>
>> _use_type has a return type that is dependent upon the type of
>> "typ.name", which required typing the QAPISchemaEntity initializer.
>>
>>
>> (Do you want this in its own preceding patch?)
>
> That would work.
>
> Keeping it in this patch with a suitable hint in the commit message
> would also work. Up to you. If you want me to tweak in my tree, tell
> me how.
>
We can try:
"Annotations are also added to the QAPISchemaEntity __init__ method in
schema.py to allow mypy to statically prove the type of typ.name, needed
to prove the return type of QAPISchemaGenIntrospectVisitor._use_type()."
--js
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 14/19] qapi/introspect.py: add type hint annotations
2021-02-16 16:19 ` John Snow
@ 2021-02-17 9:21 ` Markus Armbruster
0 siblings, 0 replies; 42+ messages in thread
From: Markus Armbruster @ 2021-02-17 9:21 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> On 2/16/21 11:08 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 2/16/21 3:55 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
[...]
>>>>> 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)
>>>>
>>>> How is this hunk related to typing introspect.py
>>>>
>>>
>>> I forget!
>>>
>>> qapi/introspect.py:262: error: Returning Any from function declared to
>>> return "str"
>>> Found 1 error in 1 file (checked 14 source files)
>>>
>>> Oh, for this reason:
>>>
>>> if isinstance(typ, QAPISchemaBuiltinType):
>>> return typ.name
>>>
>>> _use_type has a return type that is dependent upon the type of
>>> "typ.name", which required typing the QAPISchemaEntity initializer.
>>>
>>>
>>> (Do you want this in its own preceding patch?)
>>
>> That would work.
>> Keeping it in this patch with a suitable hint in the commit message
>> would also work. Up to you. If you want me to tweak in my tree, tell
>> me how.
>>
>
> We can try:
>
> "Annotations are also added to the QAPISchemaEntity __init__ method in
> schema.py to allow mypy to statically prove the type of typ.name,
> needed to prove the return type of
> QAPISchemaGenIntrospectVisitor._use_type()."
Sold!
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit
2021-02-16 2:18 ` [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit John Snow
@ 2021-02-17 9:39 ` Markus Armbruster
2021-02-17 16:07 ` John Snow
0 siblings, 1 reply; 42+ messages in thread
From: Markus Armbruster @ 2021-02-17 9:39 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 45284af1330..5d4f5e23f7e 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -99,6 +99,15 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
> def _tree_to_qlit(obj: JSONValue,
> level: int = 0,
> dict_value: bool = False) -> str:
> + """
> + Convert the type tree into a QLIT C string, recursively.
> +
> + :param obj: The value to convert.
> + This value may not be Annotated when dict_value is True.
> + :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 * ' '
> @@ -244,6 +253,15 @@ def _gen_features(features: List[QAPISchemaFeature]
> def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
> ifcond: Sequence[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.
"Additional members", since we're talking about a JSON object.
> + :param ifcond: Sequence of conditionals that apply to this entity.
> + :param features: Optional features field for SchemaInfo.
Likewise.
Sure we want to restate parts of the type ("Sequence of", "Optional") in
the doc string?
> + """
> comment: Optional[str] = None
> if mtype not in ('command', 'event', 'builtin', 'array'):
> if not self._unmask:
Also: more line-wrapping for PEP 8.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 05/19] qapi/introspect.py: guard against ifcond/comment misuse
2021-02-16 2:17 ` [PATCH v6 05/19] qapi/introspect.py: guard against ifcond/comment misuse John Snow
@ 2021-02-17 12:01 ` Markus Armbruster
0 siblings, 0 replies; 42+ messages in thread
From: Markus Armbruster @ 2021-02-17 12:01 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> _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."
I'm wrapping this comment to conform to PEP 8.
> + 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'
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit
2021-02-17 9:39 ` Markus Armbruster
@ 2021-02-17 16:07 ` John Snow
2021-02-17 16:35 ` Markus Armbruster
0 siblings, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-17 16:07 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
On 2/17/21 4:39 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 45284af1330..5d4f5e23f7e 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -99,6 +99,15 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
>> def _tree_to_qlit(obj: JSONValue,
>> level: int = 0,
>> dict_value: bool = False) -> str:
>> + """
>> + Convert the type tree into a QLIT C string, recursively.
>> +
>> + :param obj: The value to convert.
>> + This value may not be Annotated when dict_value is True.
>> + :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 * ' '
>> @@ -244,6 +253,15 @@ def _gen_features(features: List[QAPISchemaFeature]
>> def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>> ifcond: Sequence[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.
>
> "Additional members", since we're talking about a JSON object.
>
I thought "field" was also appropriate for JSON, but I suppose the spec
doesn't use that word. Over time, "field", "member" and "property" have
become just meaningless word-slurry to me.
OK.
"Additional members as appropriate for the meta-type."
>> + :param ifcond: Sequence of conditionals that apply to this entity.
>> + :param features: Optional features field for SchemaInfo.
>
> Likewise.
>
"Optional features member for SchemaInfo" ?
Sure.
> Sure we want to restate parts of the type ("Sequence of", "Optional") in
> the doc string?
>
I usually avoid it, but sometimes for non-scalars I found that it read
better to give a nod to the plural, as in:
[ifcond is a] sequence of conditionals ...
but, yes, I haven't been consistent about it. right below for @obj I
omit the type of the container.
"Conditionals that apply to this entity" feels almost too terse in
isolation.
I don't feel like it's a requisite to state the type, but in some cases
I unconsciously chose to mention the structure.
With regards to "Optional", I use this word specifically to indicate
parameters that have default values -- distinct from a type that's
Optional[], which is really actually like Nullable[T] ... If it makes
you feel better, Guido says he regrets that naming decision. Oops!
I'm probably not consistent about when I decided to write it, though.
Ehm. If it's not harmful to leave it as-is, I think it'd be okay to do
so. If you prefer a consistency all one way or all the other, I'd have
to run the vacuum back over the series to check for it.
>> + """
>> comment: Optional[str] = None
>> if mtype not in ('command', 'event', 'builtin', 'array'):
>> if not self._unmask:
>
> Also: more line-wrapping for PEP 8.
>
I thought the 72 column limit was for things like comments and docstrings.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit
2021-02-17 16:07 ` John Snow
@ 2021-02-17 16:35 ` Markus Armbruster
2021-02-17 16:55 ` John Snow
0 siblings, 1 reply; 42+ messages in thread
From: Markus Armbruster @ 2021-02-17 16:35 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 2/17/21 4:39 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/introspect.py | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index 45284af1330..5d4f5e23f7e 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -99,6 +99,15 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
>>> def _tree_to_qlit(obj: JSONValue,
>>> level: int = 0,
>>> dict_value: bool = False) -> str:
>>> + """
>>> + Convert the type tree into a QLIT C string, recursively.
>>> +
>>> + :param obj: The value to convert.
>>> + This value may not be Annotated when dict_value is True.
>>> + :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 * ' '
>>> @@ -244,6 +253,15 @@ def _gen_features(features: List[QAPISchemaFeature]
>>> def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>>> ifcond: Sequence[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.
>>
>> "Additional members", since we're talking about a JSON object.
>>
>
> I thought "field" was also appropriate for JSON, but I suppose the spec
> doesn't use that word.
Correct.
> Over time, "field", "member" and "property" have
> become just meaningless word-slurry to me.
Perfectly understandable.
> OK.
>
> "Additional members as appropriate for the meta-type."
Let's stick in a SchemaInfo for clarity:
:param obj: Additional SchemaInfo members, as appropriate for
the meta-type.
>>> + :param ifcond: Sequence of conditionals that apply to this entity.
>>> + :param features: Optional features field for SchemaInfo.
>>
>> Likewise.
>>
>
> "Optional features member for SchemaInfo" ?
>
> Sure.
What about
:param features: The SchemaInfo's features.
>> Sure we want to restate parts of the type ("Sequence of", "Optional") in
>> the doc string?
>>
>
> I usually avoid it, but sometimes for non-scalars I found that it read
> better to give a nod to the plural, as in:
>
> [ifcond is a] sequence of conditionals ...
>
> but, yes, I haven't been consistent about it. right below for @obj I
> omit the type of the container.
>
> "Conditionals that apply to this entity" feels almost too terse in
> isolation.
Similarly terse, just with SchemaInfo:
:param ifcond: Conditionals to apply to the SchemaInfo.
Or "Conditionals to guard the SchemaInfo with". Doesn't read any
better, I fear. Ideas?
> I don't feel like it's a requisite to state the type, but in some cases
> I unconsciously chose to mention the structure.
Then let's work with the informal rule not to repeat types, unless where
repeating them makes the text easier to read. Makes sense to you?
I suspect the answer we'll give in the long term depends on tooling.
When all the tools can show you is the doc string, the doc string better
includes type information. But if the tools show you the types,
repeating them wastes precious reader bandwidth.
> With regards to "Optional", I use this word specifically to indicate
> parameters that have default values -- distinct from a type that's
> Optional[], which is really actually like Nullable[T] ... If it makes
> you feel better, Guido says he regrets that naming decision. Oops!
He's right.
The "has default value" kind of optional is not part of the type, thus
not covered by the proposed informal rule. Similar, if separate
question: sure we want to restate the (presence of a) default value in
the doc string?
Again, the long-term answer will likely depend on tooling.
> I'm probably not consistent about when I decided to write it, though.
>
> Ehm. If it's not harmful to leave it as-is, I think it'd be okay to do
> so. If you prefer a consistency all one way or all the other, I'd have
> to run the vacuum back over the series to check for it.
Just five patches add comments, and just two doc strings. I had a look
at all of them, and found nothing else in need of vacuuming.
>>> + """
>>> comment: Optional[str] = None
>>> if mtype not in ('command', 'event', 'builtin', 'array'):
>>> if not self._unmask:
>>
>> Also: more line-wrapping for PEP 8.
>>
>
> I thought the 72 column limit was for things like comments and docstrings.
I put this in the wrong spot, I meant the doc string, not the code.
sorry for the confusion!
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit
2021-02-17 16:35 ` Markus Armbruster
@ 2021-02-17 16:55 ` John Snow
2021-02-18 7:53 ` Markus Armbruster
0 siblings, 1 reply; 42+ messages in thread
From: John Snow @ 2021-02-17 16:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, qemu-devel, Eduardo Habkost, Cleber Rosa
On 2/17/21 11:35 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 2/17/21 4:39 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> scripts/qapi/introspect.py | 18 ++++++++++++++++++
>>>> 1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>>> index 45284af1330..5d4f5e23f7e 100644
>>>> --- a/scripts/qapi/introspect.py
>>>> +++ b/scripts/qapi/introspect.py
>>>> @@ -99,6 +99,15 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
>>>> def _tree_to_qlit(obj: JSONValue,
>>>> level: int = 0,
>>>> dict_value: bool = False) -> str:
>>>> + """
>>>> + Convert the type tree into a QLIT C string, recursively.
>>>> +
>>>> + :param obj: The value to convert.
>>>> + This value may not be Annotated when dict_value is True.
>>>> + :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 * ' '
>>>> @@ -244,6 +253,15 @@ def _gen_features(features: List[QAPISchemaFeature]
>>>> def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>>>> ifcond: Sequence[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.
>>>
>>> "Additional members", since we're talking about a JSON object.
>>>
>>
>> I thought "field" was also appropriate for JSON, but I suppose the spec
>> doesn't use that word.
>
> Correct.
>
>> Over time, "field", "member" and "property" have
>> become just meaningless word-slurry to me.
>
> Perfectly understandable.
>
>> OK.
>>
>> "Additional members as appropriate for the meta-type."
>
> Let's stick in a SchemaInfo for clarity:
>
> :param obj: Additional SchemaInfo members, as appropriate for
> the meta-type.
>
Sure, why not?
>>>> + :param ifcond: Sequence of conditionals that apply to this entity.
>>>> + :param features: Optional features field for SchemaInfo.
>>>
>>> Likewise.
>>>
>>
>> "Optional features member for SchemaInfo" ?
>>
>> Sure.
>
> What about
>
> :param features: The SchemaInfo's features.
>
Sure, why not? x2
>>> Sure we want to restate parts of the type ("Sequence of", "Optional") in
>>> the doc string?
>>>
>>
>> I usually avoid it, but sometimes for non-scalars I found that it read
>> better to give a nod to the plural, as in:
>>
>> [ifcond is a] sequence of conditionals ...
>>
>> but, yes, I haven't been consistent about it. right below for @obj I
>> omit the type of the container.
>>
>> "Conditionals that apply to this entity" feels almost too terse in
>> isolation.
>
> Similarly terse, just with SchemaInfo:
>
> :param ifcond: Conditionals to apply to the SchemaInfo.
>
Sure, why not! x3
> Or "Conditionals to guard the SchemaInfo with". Doesn't read any
> better, I fear. Ideas?
>
>> I don't feel like it's a requisite to state the type, but in some cases
>> I unconsciously chose to mention the structure.
>
> Then let's work with the informal rule not to repeat types, unless where
> repeating them makes the text easier to read. Makes sense to you?
>
Subjectively I was doing this. Maybe half-heartedly. :~)
> I suspect the answer we'll give in the long term depends on tooling.
> When all the tools can show you is the doc string, the doc string better
> includes type information. But if the tools show you the types,
> repeating them wastes precious reader bandwidth.
>
Yep. Sphinx shows type in the signature, but it doesn't necessarily
repeat it for the :param list: entries; So you can correlate it with
your eyeballs, but it isn't done for you.
Maybe that'll change. Maybe I'll change it with my own Sphinx plugin
eventually that does the cool stuff I dream about.
Maybe Maybe Maybe. I need to study the docutils API and learn how to
make even a simple plugin... There are definitely a few ideas that I
have that I want to bring to life that I think will help people like me
adhere to a more consistent style.
>> With regards to "Optional", I use this word specifically to indicate
>> parameters that have default values -- distinct from a type that's
>> Optional[], which is really actually like Nullable[T] ... If it makes
>> you feel better, Guido says he regrets that naming decision. Oops!
>
> He's right.
>
> The "has default value" kind of optional is not part of the type, thus
> not covered by the proposed informal rule. Similar, if separate
> question: sure we want to restate the (presence of a) default value in
> the doc string?
>
I tend to state what a default is if the default is a special value that
implies something else. Like: "The default is to not add this member." I
have generally avoided "The default is 3."
I do sometimes say "Defaults to true/false" for boolean options just to
add emphasis on "which way" the boolean leans, in case the name doesn't
make that clear in isolation.
I haven't been consistent, but I will try to be a bit more conscious
about it going forward.
(the expr.py series, up next, is gonna be a playground for docstring
style reviews, because I added a ton.)
> Again, the long-term answer will likely depend on tooling.
>
If it reads better to you to remove the "Optional, " then go ahead and
make those cuts for the time-being, and I can try to hit things with a
docstring beautifying beam later when we actually try to develop and
publish a manual.
(After my python packaging series and after this QAPI cleanup series.)
>> I'm probably not consistent about when I decided to write it, though.
>>
>> Ehm. If it's not harmful to leave it as-is, I think it'd be okay to do
>> so. If you prefer a consistency all one way or all the other, I'd have
>> to run the vacuum back over the series to check for it.
>
> Just five patches add comments, and just two doc strings. I had a look
> at all of them, and found nothing else in need of vacuuming.
>
Go with whatcha feel, but I'll try to write that "style guide" we
discussed during part 1 and we can hem and haw over the guidelines for
ourselves.
I will want to apply it to all of ./scripts/qapi and ./python/qemu both.
>>>> + """
>>>> comment: Optional[str] = None
>>>> if mtype not in ('command', 'event', 'builtin', 'array'):
>>>> if not self._unmask:
>>>
>>> Also: more line-wrapping for PEP 8.
>>>
>>
>> I thought the 72 column limit was for things like comments and docstrings.
>
> I put this in the wrong spot, I meant the doc string, not the code.
> sorry for the confusion!
>
Ah, phew. OK, yes, I've already capitulated on the comment line lengths,
have at those :)
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit
2021-02-17 16:55 ` John Snow
@ 2021-02-18 7:53 ` Markus Armbruster
0 siblings, 0 replies; 42+ messages in thread
From: Markus Armbruster @ 2021-02-18 7:53 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> On 2/17/21 11:35 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 2/17/21 4:39 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>> scripts/qapi/introspect.py | 18 ++++++++++++++++++
>>>>> 1 file changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>>>> index 45284af1330..5d4f5e23f7e 100644
>>>>> --- a/scripts/qapi/introspect.py
>>>>> +++ b/scripts/qapi/introspect.py
>>>>> @@ -99,6 +99,15 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
>>>>> def _tree_to_qlit(obj: JSONValue,
>>>>> level: int = 0,
>>>>> dict_value: bool = False) -> str:
>>>>> + """
>>>>> + Convert the type tree into a QLIT C string, recursively.
>>>>> +
>>>>> + :param obj: The value to convert.
>>>>> + This value may not be Annotated when dict_value is True.
>>>>> + :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 * ' '
>>>>> @@ -244,6 +253,15 @@ def _gen_features(features: List[QAPISchemaFeature]
>>>>> def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>>>>> ifcond: Sequence[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.
>>>>
>>>> "Additional members", since we're talking about a JSON object.
>>>>
>>>
>>> I thought "field" was also appropriate for JSON, but I suppose the spec
>>> doesn't use that word.
>>
>> Correct.
>>
>>> Over time, "field", "member" and "property" have
>>> become just meaningless word-slurry to me.
>>
>> Perfectly understandable.
>>
>>> OK.
>>>
>>> "Additional members as appropriate for the meta-type."
>>
>> Let's stick in a SchemaInfo for clarity:
>>
>> :param obj: Additional SchemaInfo members, as appropriate for
>> the meta-type.
>>
>
> Sure, why not?
>
>>>>> + :param ifcond: Sequence of conditionals that apply to this entity.
>>>>> + :param features: Optional features field for SchemaInfo.
>>>>
>>>> Likewise.
>>>>
>>>
>>> "Optional features member for SchemaInfo" ?
>>>
>>> Sure.
>>
>> What about
>>
>> :param features: The SchemaInfo's features.
>>
>
> Sure, why not? x2
>
>>>> Sure we want to restate parts of the type ("Sequence of", "Optional") in
>>>> the doc string?
>>>>
>>>
>>> I usually avoid it, but sometimes for non-scalars I found that it read
>>> better to give a nod to the plural, as in:
>>>
>>> [ifcond is a] sequence of conditionals ...
>>>
>>> but, yes, I haven't been consistent about it. right below for @obj I
>>> omit the type of the container.
>>>
>>> "Conditionals that apply to this entity" feels almost too terse in
>>> isolation.
>>
>> Similarly terse, just with SchemaInfo:
>>
>> :param ifcond: Conditionals to apply to the SchemaInfo.
>>
>
> Sure, why not! x3
>
>> Or "Conditionals to guard the SchemaInfo with". Doesn't read any
>> better, I fear. Ideas?
>>
>>> I don't feel like it's a requisite to state the type, but in some cases
>>> I unconsciously chose to mention the structure.
>>
>> Then let's work with the informal rule not to repeat types, unless where
>> repeating them makes the text easier to read. Makes sense to you?
>
> Subjectively I was doing this. Maybe half-heartedly. :~)
I'm not criticizing any half-heartedness here. I just want to get to a
common understanding of how we want to do doc strings. Preliminary and
somewhat vague is fine; it's *understanding*, not *law*. "In John's
head" is not fine :)
>> I suspect the answer we'll give in the long term depends on tooling.
>> When all the tools can show you is the doc string, the doc string better
>> includes type information. But if the tools show you the types,
>> repeating them wastes precious reader bandwidth.
>>
>
> Yep. Sphinx shows type in the signature, but it doesn't necessarily
> repeat it for the :param list: entries; So you can correlate it with
> your eyeballs, but it isn't done for you.
>
> Maybe that'll change. Maybe I'll change it with my own Sphinx plugin
> eventually that does the cool stuff I dream about.
>
> Maybe Maybe Maybe. I need to study the docutils API and learn how to
> make even a simple plugin... There are definitely a few ideas that I
> have that I want to bring to life that I think will help people like me
> adhere to a more consistent style.
>
>>> With regards to "Optional", I use this word specifically to indicate
>>> parameters that have default values -- distinct from a type that's
>>> Optional[], which is really actually like Nullable[T] ... If it makes
>>> you feel better, Guido says he regrets that naming decision. Oops!
>>
>> He's right.
>>
>> The "has default value" kind of optional is not part of the type, thus
>> not covered by the proposed informal rule. Similar, if separate
>> question: sure we want to restate the (presence of a) default value in
>> the doc string?
>>
>
> I tend to state what a default is if the default is a special value that
> implies something else. Like: "The default is to not add this member." I
> have generally avoided "The default is 3."
>
> I do sometimes say "Defaults to true/false" for boolean options just to
> add emphasis on "which way" the boolean leans, in case the name doesn't
> make that clear in isolation.
Documentation should explain defaults. But what exactly counts as
documentation today? Just the doc string? The doc string plus the type
hints? Plus the default values?
> I haven't been consistent, but I will try to be a bit more conscious
> about it going forward.
>
> (the expr.py series, up next, is gonna be a playground for docstring
> style reviews, because I added a ton.)
I feel we should to pick some rules that work for us with the tooling we
have. Even imperfect rules that aren't enforced automatically should
help us maintain a useful level of consistency. Also liberate us from
debating the same doc questions over and over. Liberate *me* from
debating them in my head.
>> Again, the long-term answer will likely depend on tooling.
>>
>
> If it reads better to you to remove the "Optional, " then go ahead and
> make those cuts for the time-being, and I can try to hit things with a
> docstring beautifying beam later when we actually try to develop and
> publish a manual.
>
> (After my python packaging series and after this QAPI cleanup series.)
Here's what I have in my tree now:
def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
ifcond: Sequence[str] = (),
features: Sequence[QAPISchemaFeature] = ()) -> None:
"""
Build and append a SchemaInfo object to self._trees.
:param name: The SchemaInfo's name.
:param mtype: The SchemaInfo's meta-type.
:param obj: Additional SchemaInfo members, as appropriate for
the meta-type.
:param ifcond: Conditionals to apply to the SchemaInfo.
:param features: The SchemaInfo's features.
"""
PATCH 17 adds
Will be omitted from the output if empty.
Hmm, I think that should actually go right into this patch instead.
>>> I'm probably not consistent about when I decided to write it, though.
>>>
>>> Ehm. If it's not harmful to leave it as-is, I think it'd be okay to do
>>> so. If you prefer a consistency all one way or all the other, I'd have
>>> to run the vacuum back over the series to check for it.
>>
>> Just five patches add comments, and just two doc strings. I had a look
>> at all of them, and found nothing else in need of vacuuming.
>>
>
> Go with whatcha feel, but I'll try to write that "style guide" we
> discussed during part 1 and we can hem and haw over the guidelines for
> ourselves.
>
> I will want to apply it to all of ./scripts/qapi and ./python/qemu both.
Makes sense.
Perhaps we can steal from existing style guide(s). Sadly, PEP 257 is
next to useless.
>>>>> + """
>>>>> comment: Optional[str] = None
>>>>> if mtype not in ('command', 'event', 'builtin', 'array'):
>>>>> if not self._unmask:
>>>>
>>>> Also: more line-wrapping for PEP 8.
>>>>
>>>
>>> I thought the 72 column limit was for things like comments and docstrings.
>>
>> I put this in the wrong spot, I meant the doc string, not the code.
>> sorry for the confusion!
>>
>
> Ah, phew. OK, yes, I've already capitulated on the comment line lengths,
> have at those :)
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 14/19] qapi/introspect.py: add type hint annotations
2021-02-16 2:18 ` [PATCH v6 14/19] qapi/introspect.py: add type hint annotations John Snow
2021-02-16 8:55 ` Markus Armbruster
@ 2021-02-18 18:56 ` Markus Armbruster
2021-02-18 19:01 ` Markus Armbruster
1 sibling, 1 reply; 42+ messages in thread
From: Markus Armbruster @ 2021-02-18 18:56 UTC (permalink / raw)
To: John Snow; +Cc: Michael Roth, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> NB: The type aliases (SchemaInfo et al) declare intent for some of the
> "dictly-typed" objects we pass around in introspect.py. They do not
> enforce the shape of those objects, and cannot, until Python 3.7 or
> later. (And even then, it may not be "worth it".)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Series:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
I'm queuing all but the last patch.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v6 14/19] qapi/introspect.py: add type hint annotations
2021-02-18 18:56 ` Markus Armbruster
@ 2021-02-18 19:01 ` Markus Armbruster
0 siblings, 0 replies; 42+ messages in thread
From: Markus Armbruster @ 2021-02-18 19:01 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael Roth, Cleber Rosa, John Snow, qemu-devel, Eduardo Habkost
Markus Armbruster <armbru@redhat.com> writes:
> John Snow <jsnow@redhat.com> writes:
>
>> NB: The type aliases (SchemaInfo et al) declare intent for some of the
>> "dictly-typed" objects we pass around in introspect.py. They do not
>> enforce the shape of those objects, and cannot, until Python 3.7 or
>> later. (And even then, it may not be "worth it".)
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Series:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> I'm queuing all but the last patch.
Meant to send this in reply to the cover letter.
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2021-02-18 19:02 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-16 2:17 [PATCH v6 00/19] qapi: static typing conversion, pt2 John Snow
2021-02-16 2:17 ` [PATCH v6 01/19] qapi: Replace List[str] with Sequence[str] for ifcond John Snow
2021-02-16 8:43 ` Markus Armbruster
2021-02-16 15:19 ` John Snow
2021-02-16 15:58 ` Markus Armbruster
2021-02-16 2:17 ` [PATCH v6 02/19] qapi/introspect.py: assert schema is not None John Snow
2021-02-16 2:17 ` [PATCH v6 03/19] qapi/introspect.py: use _make_tree for features nodes John Snow
2021-02-16 2:17 ` [PATCH v6 04/19] qapi/introspect.py: add _gen_features helper John Snow
2021-02-16 2:17 ` [PATCH v6 05/19] qapi/introspect.py: guard against ifcond/comment misuse John Snow
2021-02-17 12:01 ` Markus Armbruster
2021-02-16 2:17 ` [PATCH v6 06/19] qapi/introspect.py: Unify return type of _make_tree() John Snow
2021-02-16 2:17 ` [PATCH v6 07/19] qapi/introspect.py: replace 'extra' dict with 'comment' argument John Snow
2021-02-16 2:17 ` [PATCH v6 08/19] qapi/introspect.py: Always define all 'extra' dict keys John Snow
2021-02-16 2:17 ` [PATCH v6 09/19] qapi/introspect.py: Introduce preliminary tree typing John Snow
2021-02-16 2:18 ` [PATCH v6 10/19] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
2021-02-16 2:18 ` [PATCH v6 11/19] qapi/introspect.py: improve _tree_to_qlit error message John Snow
2021-02-16 2:18 ` [PATCH v6 12/19] qapi/introspect.py: improve readability of _tree_to_qlit John Snow
2021-02-16 2:18 ` [PATCH v6 13/19] qapi/introspect.py: remove _gen_variants helper John Snow
2021-02-16 2:18 ` [PATCH v6 14/19] qapi/introspect.py: add type hint annotations John Snow
2021-02-16 8:55 ` Markus Armbruster
2021-02-16 8:58 ` Markus Armbruster
2021-02-16 15:33 ` John Snow
2021-02-16 16:08 ` Markus Armbruster
2021-02-16 16:19 ` John Snow
2021-02-17 9:21 ` Markus Armbruster
2021-02-18 18:56 ` Markus Armbruster
2021-02-18 19:01 ` Markus Armbruster
2021-02-16 2:18 ` [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit John Snow
2021-02-17 9:39 ` Markus Armbruster
2021-02-17 16:07 ` John Snow
2021-02-17 16:35 ` Markus Armbruster
2021-02-17 16:55 ` John Snow
2021-02-18 7:53 ` Markus Armbruster
2021-02-16 2:18 ` [PATCH v6 16/19] qapi/introspect.py: Update copyright and authors list John Snow
2021-02-16 2:18 ` [PATCH v6 17/19] qapi/introspect.py: Type _gen_tree variants as Sequence[str] John Snow
2021-02-16 9:10 ` Markus Armbruster
2021-02-16 15:13 ` John Snow
2021-02-16 2:18 ` [PATCH v6 18/19] qapi/introspect.py: set _gen_tree's default ifcond argument to () John Snow
2021-02-16 2:18 ` [PATCH v6 19/19] qapi/introspect.py: add SchemaMetaType enum John Snow
2021-02-16 9:24 ` Markus Armbruster
2021-02-16 15:08 ` John Snow
2021-02-16 16:09 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.