Presently, we use a tuple to attach a dict containing annotations
(comments and compile-time conditionals) to a tree node. This is
undesirable because dicts are difficult to strongly type; promoting it
to a real class allows us to name the values and types of the
annotations we are expecting.
In terms of typing, the Annotated<T> type serves as a generic container
where the annotated node's type is preserved, allowing for greater
specificity than we'd be able to provide without a generic.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 77 ++++++++++++++++++++++----------------
1 file changed, 44 insertions(+), 33 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 8e019b4a26a..b9427aba449 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -13,8 +13,12 @@
from typing import (
Any,
Dict,
+ Generic,
+ Iterable,
List,
Optional,
+ Tuple,
+ TypeVar,
Union,
)
@@ -51,15 +55,25 @@
_scalar = Union[str, bool, None]
_nonscalar = Union[Dict[str, _stub], List[_stub]]
_value = Union[_scalar, _nonscalar]
-# TreeValue = TODO, in a forthcoming commit.
+TreeValue = Union[_value, 'Annotated[_value]']
-def _make_tree(obj, ifcond, comment=None):
- extra = {
- 'if': ifcond,
- 'comment': comment
- }
- return (obj, extra)
+_NodeT = TypeVar('_NodeT', bound=_value)
+
+
+class Annotated(Generic[_NodeT]):
+ """
+ Annotated generally contains a SchemaInfo-like type (as a dict),
+ But it also used to wrap comments/ifconds around scalar leaf values,
+ for the benefit of features and enums.
+ """
+ # TODO: Remove after Python 3.7 adds @dataclass:
+ # pylint: disable=too-few-public-methods
+ def __init__(self, value: _NodeT, ifcond: Iterable[str],
+ comment: Optional[str] = None):
+ self.value = value
+ self.comment: Optional[str] = comment
+ self.ifcond: Tuple[str, ...] = tuple(ifcond)
def _tree_to_qlit(obj, level=0, dict_value=False):
@@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, dict_value=False):
def indent(level):
return level * 4 * ' '
- if isinstance(obj, tuple):
- ifobj, extra = obj
- ifcond = extra.get('if')
- comment = extra.get('comment')
-
+ if isinstance(obj, Annotated):
# NB: _tree_to_qlit is called recursively on the values of a key:value
# pair; those values can't be decorated with comments or conditionals.
msg = "dict values cannot have attached comments or if-conditionals."
assert not dict_value, msg
ret = ''
- if comment:
- ret += indent(level) + '/* %s */\n' % comment
- if ifcond:
- ret += gen_if(ifcond)
- ret += _tree_to_qlit(ifobj, level)
- if ifcond:
- ret += '\n' + gen_endif(ifcond)
+ if obj.comment:
+ ret += indent(level) + '/* %s */\n' % obj.comment
+ if obj.ifcond:
+ ret += gen_if(obj.ifcond)
+ ret += _tree_to_qlit(obj.value, level)
+ if obj.ifcond:
+ ret += '\n' + gen_endif(obj.ifcond)
return ret
ret = ''
@@ -201,7 +211,7 @@ def _use_type(self, typ):
@staticmethod
def _gen_features(features):
- return [_make_tree(f.name, f.ifcond) for f in features]
+ return [Annotated(f.name, f.ifcond) for f in features]
def _gen_tree(self, name, mtype, obj, ifcond, features):
comment: Optional[str] = None
@@ -215,7 +225,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
obj['meta-type'] = mtype
if features:
obj['features'] = self._gen_features(features)
- self._trees.append(_make_tree(obj, ifcond, comment))
+ self._trees.append(Annotated(obj, ifcond, comment))
def _gen_member(self, member):
obj = {'name': member.name, 'type': self._use_type(member.type)}
@@ -223,7 +233,7 @@ def _gen_member(self, member):
obj['default'] = None
if member.features:
obj['features'] = self._gen_features(member.features)
- return _make_tree(obj, member.ifcond)
+ return Annotated(obj, member.ifcond)
def _gen_variants(self, tag_name, variants):
return {'tag': tag_name,
@@ -231,16 +241,17 @@ def _gen_variants(self, tag_name, variants):
def _gen_variant(self, variant):
obj = {'case': variant.name, 'type': self._use_type(variant.type)}
- return _make_tree(obj, variant.ifcond)
+ return Annotated(obj, variant.ifcond)
def visit_builtin_type(self, name, info, json_type):
self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
def visit_enum_type(self, name, info, ifcond, features, members, prefix):
- self._gen_tree(name, 'enum',
- {'values': [_make_tree(m.name, m.ifcond, None)
- for m in members]},
- ifcond, features)
+ self._gen_tree(
+ name, 'enum',
+ {'values': [Annotated(m.name, m.ifcond) for m in members]},
+ ifcond, features
+ )
def visit_array_type(self, name, info, ifcond, element_type):
element = self._use_type(element_type)
@@ -257,12 +268,12 @@ def visit_object_type_flat(self, name, info, ifcond, features,
self._gen_tree(name, 'object', obj, ifcond, features)
def visit_alternate_type(self, name, info, ifcond, features, variants):
- self._gen_tree(name, 'alternate',
- {'members': [
- _make_tree({'type': self._use_type(m.type)},
- m.ifcond, None)
- for m in variants.variants]},
- ifcond, features)
+ self._gen_tree(
+ name, 'alternate',
+ {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
+ for m in variants.variants]},
+ ifcond, features
+ )
def visit_command(self, name, info, ifcond, features,
arg_type, ret_type, gen, success_response, boxed,
--
2.29.2
John Snow <jsnow@redhat.com> writes:
> Presently, we use a tuple to attach a dict containing annotations
> (comments and compile-time conditionals) to a tree node. This is
> undesirable because dicts are difficult to strongly type; promoting it
> to a real class allows us to name the values and types of the
> annotations we are expecting.
>
> In terms of typing, the Annotated<T> type serves as a generic container
> where the annotated node's type is preserved, allowing for greater
> specificity than we'd be able to provide without a generic.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 77 ++++++++++++++++++++++----------------
> 1 file changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 8e019b4a26a..b9427aba449 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -13,8 +13,12 @@
> from typing import (
> Any,
> Dict,
> + Generic,
> + Iterable,
> List,
> Optional,
> + Tuple,
> + TypeVar,
> Union,
> )
>
> @@ -51,15 +55,25 @@
> _scalar = Union[str, bool, None]
> _nonscalar = Union[Dict[str, _stub], List[_stub]]
> _value = Union[_scalar, _nonscalar]
> -# TreeValue = TODO, in a forthcoming commit.
> +TreeValue = Union[_value, 'Annotated[_value]']
>
>
> -def _make_tree(obj, ifcond, comment=None):
> - extra = {
> - 'if': ifcond,
> - 'comment': comment
> - }
> - return (obj, extra)
> +_NodeT = TypeVar('_NodeT', bound=_value)
> +
> +
> +class Annotated(Generic[_NodeT]):
My gut feeling is "generic type is overkill for this purpose". Let's go
with it anyway, because
1. It's not wrong.
2. I don't have enough experience with Python type hints for reliable
gut feelings.
3. I plan to overhaul the C generation part relatively soon (after your
work has landed, don't worry), and I can try to make it simpler then.
> + """
> + Annotated generally contains a SchemaInfo-like type (as a dict),
> + But it also used to wrap comments/ifconds around scalar leaf values,
> + for the benefit of features and enums.
> + """
> + # TODO: Remove after Python 3.7 adds @dataclass:
> + # pylint: disable=too-few-public-methods
> + def __init__(self, value: _NodeT, ifcond: Iterable[str],
> + comment: Optional[str] = None):
> + self.value = value
> + self.comment: Optional[str] = comment
> + self.ifcond: Tuple[str, ...] = tuple(ifcond)
>
>
> def _tree_to_qlit(obj, level=0, dict_value=False):
> @@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, dict_value=False):
> def indent(level):
> return level * 4 * ' '
>
> - if isinstance(obj, tuple):
> - ifobj, extra = obj
> - ifcond = extra.get('if')
> - comment = extra.get('comment')
> -
> + if isinstance(obj, Annotated):
> # NB: _tree_to_qlit is called recursively on the values of a key:value
> # pair; those values can't be decorated with comments or conditionals.
> msg = "dict values cannot have attached comments or if-conditionals."
> assert not dict_value, msg
>
> ret = ''
> - if comment:
> - ret += indent(level) + '/* %s */\n' % comment
> - if ifcond:
> - ret += gen_if(ifcond)
> - ret += _tree_to_qlit(ifobj, level)
> - if ifcond:
> - ret += '\n' + gen_endif(ifcond)
> + if obj.comment:
> + ret += indent(level) + '/* %s */\n' % obj.comment
> + if obj.ifcond:
> + ret += gen_if(obj.ifcond)
> + ret += _tree_to_qlit(obj.value, level)
> + if obj.ifcond:
> + ret += '\n' + gen_endif(obj.ifcond)
> return ret
>
> ret = ''
> @@ -201,7 +211,7 @@ def _use_type(self, typ):
>
> @staticmethod
> def _gen_features(features):
> - return [_make_tree(f.name, f.ifcond) for f in features]
> + return [Annotated(f.name, f.ifcond) for f in features]
>
> def _gen_tree(self, name, mtype, obj, ifcond, features):
> comment: Optional[str] = None
> @@ -215,7 +225,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
> obj['meta-type'] = mtype
> if features:
> obj['features'] = self._gen_features(features)
> - self._trees.append(_make_tree(obj, ifcond, comment))
> + self._trees.append(Annotated(obj, ifcond, comment))
>
> def _gen_member(self, member):
> obj = {'name': member.name, 'type': self._use_type(member.type)}
> @@ -223,7 +233,7 @@ def _gen_member(self, member):
> obj['default'] = None
> if member.features:
> obj['features'] = self._gen_features(member.features)
> - return _make_tree(obj, member.ifcond)
> + return Annotated(obj, member.ifcond)
>
> def _gen_variants(self, tag_name, variants):
> return {'tag': tag_name,
> @@ -231,16 +241,17 @@ def _gen_variants(self, tag_name, variants):
>
> def _gen_variant(self, variant):
> obj = {'case': variant.name, 'type': self._use_type(variant.type)}
> - return _make_tree(obj, variant.ifcond)
> + return Annotated(obj, variant.ifcond)
>
> def visit_builtin_type(self, name, info, json_type):
> self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>
> def visit_enum_type(self, name, info, ifcond, features, members, prefix):
> - self._gen_tree(name, 'enum',
> - {'values': [_make_tree(m.name, m.ifcond, None)
> - for m in members]},
> - ifcond, features)
> + self._gen_tree(
> + name, 'enum',
> + {'values': [Annotated(m.name, m.ifcond) for m in members]},
> + ifcond, features
> + )
>
> def visit_array_type(self, name, info, ifcond, element_type):
> element = self._use_type(element_type)
> @@ -257,12 +268,12 @@ def visit_object_type_flat(self, name, info, ifcond, features,
> self._gen_tree(name, 'object', obj, ifcond, features)
>
> def visit_alternate_type(self, name, info, ifcond, features, variants):
> - self._gen_tree(name, 'alternate',
> - {'members': [
> - _make_tree({'type': self._use_type(m.type)},
> - m.ifcond, None)
> - for m in variants.variants]},
> - ifcond, features)
> + self._gen_tree(
> + name, 'alternate',
> + {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
> + for m in variants.variants]},
Slightly more readable, I think:
{'members': [Annotated({'type': self._use_type(m.type)},
m.ifcond)
for m in variants.variants]},
> + ifcond, features
> + )
>
> def visit_command(self, name, info, ifcond, features,
> arg_type, ret_type, gen, success_response, boxed,
On 2/8/21 9:36 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Presently, we use a tuple to attach a dict containing annotations
>> (comments and compile-time conditionals) to a tree node. This is
>> undesirable because dicts are difficult to strongly type; promoting it
>> to a real class allows us to name the values and types of the
>> annotations we are expecting.
>>
>> In terms of typing, the Annotated<T> type serves as a generic container
>> where the annotated node's type is preserved, allowing for greater
>> specificity than we'd be able to provide without a generic.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 77 ++++++++++++++++++++++----------------
>> 1 file changed, 44 insertions(+), 33 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 8e019b4a26a..b9427aba449 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -13,8 +13,12 @@
>> from typing import (
>> Any,
>> Dict,
>> + Generic,
>> + Iterable,
>> List,
>> Optional,
>> + Tuple,
>> + TypeVar,
>> Union,
>> )
>>
>> @@ -51,15 +55,25 @@
>> _scalar = Union[str, bool, None]
>> _nonscalar = Union[Dict[str, _stub], List[_stub]]
>> _value = Union[_scalar, _nonscalar]
>> -# TreeValue = TODO, in a forthcoming commit.
>> +TreeValue = Union[_value, 'Annotated[_value]']
>>
>>
>> -def _make_tree(obj, ifcond, comment=None):
>> - extra = {
>> - 'if': ifcond,
>> - 'comment': comment
>> - }
>> - return (obj, extra)
>> +_NodeT = TypeVar('_NodeT', bound=_value)
>> +
>> +
>> +class Annotated(Generic[_NodeT]):
>
> My gut feeling is "generic type is overkill for this purpose". Let's go
> with it anyway, because
>
> 1. It's not wrong.
>
A famous phrase in Computer Science.
> 2. I don't have enough experience with Python type hints for reliable
> gut feelings.
>
You are exactly correct that the power it offers us here isn't strictly
necessary. An argument might be that removing it makes the types easier
to read, but I think at a certain level of involvement with mypy that it
isn't feasible to escape understanding Generics, and we are at that level.
> 3. I plan to overhaul the C generation part relatively soon (after your
> work has landed, don't worry), and I can try to make it simpler then.
>
Yeah. The generation and typing can likely improve substantially at that
point in time. Hopefully the type hints help guide a design that's nice
to type and nice to read.
>> + """
>> + Annotated generally contains a SchemaInfo-like type (as a dict),
>> + But it also used to wrap comments/ifconds around scalar leaf values,
>> + for the benefit of features and enums.
>> + """
>> + # TODO: Remove after Python 3.7 adds @dataclass:
>> + # pylint: disable=too-few-public-methods
>> + def __init__(self, value: _NodeT, ifcond: Iterable[str],
>> + comment: Optional[str] = None):
>> + self.value = value
>> + self.comment: Optional[str] = comment
>> + self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>
>>
>> def _tree_to_qlit(obj, level=0, dict_value=False):
>> @@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, dict_value=False):
>> def indent(level):
>> return level * 4 * ' '
>>
>> - if isinstance(obj, tuple):
>> - ifobj, extra = obj
>> - ifcond = extra.get('if')
>> - comment = extra.get('comment')
>> -
>> + if isinstance(obj, Annotated):
>> # NB: _tree_to_qlit is called recursively on the values of a key:value
>> # pair; those values can't be decorated with comments or conditionals.
>> msg = "dict values cannot have attached comments or if-conditionals."
>> assert not dict_value, msg
>>
>> ret = ''
>> - if comment:
>> - ret += indent(level) + '/* %s */\n' % comment
>> - if ifcond:
>> - ret += gen_if(ifcond)
>> - ret += _tree_to_qlit(ifobj, level)
>> - if ifcond:
>> - ret += '\n' + gen_endif(ifcond)
>> + if obj.comment:
>> + ret += indent(level) + '/* %s */\n' % obj.comment
>> + if obj.ifcond:
>> + ret += gen_if(obj.ifcond)
>> + ret += _tree_to_qlit(obj.value, level)
>> + if obj.ifcond:
>> + ret += '\n' + gen_endif(obj.ifcond)
>> return ret
>>
>> ret = ''
>> @@ -201,7 +211,7 @@ def _use_type(self, typ):
>>
>> @staticmethod
>> def _gen_features(features):
>> - return [_make_tree(f.name, f.ifcond) for f in features]
>> + return [Annotated(f.name, f.ifcond) for f in features]
>>
>> def _gen_tree(self, name, mtype, obj, ifcond, features):
>> comment: Optional[str] = None
>> @@ -215,7 +225,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>> obj['meta-type'] = mtype
>> if features:
>> obj['features'] = self._gen_features(features)
>> - self._trees.append(_make_tree(obj, ifcond, comment))
>> + self._trees.append(Annotated(obj, ifcond, comment))
>>
>> def _gen_member(self, member):
>> obj = {'name': member.name, 'type': self._use_type(member.type)}
>> @@ -223,7 +233,7 @@ def _gen_member(self, member):
>> obj['default'] = None
>> if member.features:
>> obj['features'] = self._gen_features(member.features)
>> - return _make_tree(obj, member.ifcond)
>> + return Annotated(obj, member.ifcond)
>>
>> def _gen_variants(self, tag_name, variants):
>> return {'tag': tag_name,
>> @@ -231,16 +241,17 @@ def _gen_variants(self, tag_name, variants):
>>
>> def _gen_variant(self, variant):
>> obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>> - return _make_tree(obj, variant.ifcond)
>> + return Annotated(obj, variant.ifcond)
>>
>> def visit_builtin_type(self, name, info, json_type):
>> self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>>
>> def visit_enum_type(self, name, info, ifcond, features, members, prefix):
>> - self._gen_tree(name, 'enum',
>> - {'values': [_make_tree(m.name, m.ifcond, None)
>> - for m in members]},
>> - ifcond, features)
>> + self._gen_tree(
>> + name, 'enum',
>> + {'values': [Annotated(m.name, m.ifcond) for m in members]},
>> + ifcond, features
>> + )
>>
>> def visit_array_type(self, name, info, ifcond, element_type):
>> element = self._use_type(element_type)
>> @@ -257,12 +268,12 @@ def visit_object_type_flat(self, name, info, ifcond, features,
>> self._gen_tree(name, 'object', obj, ifcond, features)
>>
>> def visit_alternate_type(self, name, info, ifcond, features, variants):
>> - self._gen_tree(name, 'alternate',
>> - {'members': [
>> - _make_tree({'type': self._use_type(m.type)},
>> - m.ifcond, None)
>> - for m in variants.variants]},
>> - ifcond, features)
>> + self._gen_tree(
>> + name, 'alternate',
>> + {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
>> + for m in variants.variants]},
>
> Slightly more readable, I think:
>
> {'members': [Annotated({'type': self._use_type(m.type)},
> m.ifcond)
> for m in variants.variants]},
>
OK, but only because I am being annoying about not capitulating
elsewhere on equally trivial matters.
>> + ifcond, features
>> + )
>>
>> def visit_command(self, name, info, ifcond, features,
>> arg_type, ret_type, gen, success_response, boxed,
© 2016 - 2026 Red Hat, Inc.