| Message ID | 20201026194251.11075-10-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,01/11,DO-NOT-MERGE] docs: replace single backtick (`) with double-backtick (``) | expand |
On Mon, Oct 26, 2020 at 03:42:49PM -0400, John Snow wrote: > This replaces _make_tree with Annotated(). By creating it as a generic > container, we can more accurately describe the exact nature of this > particular value. i.e., each Annotated object is actually an > Annotated<T>, describing its contained value. > > This adds stricter typing to Annotated nodes and extra annotated > information. It also replaces a check of "isinstance tuple" with the > much more explicit "isinstance Annotated" which is guaranteed not to > break if a tuple is accidentally introduced into the type tree. (Perhaps > as a result of a bad conversion from a list.) > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/introspect.py | 97 +++++++++++++++++++------------------- > 1 file changed, 48 insertions(+), 49 deletions(-) > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index a0978cb3adb..a261e402d69 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -13,12 +13,13 @@ > from typing import ( > Any, > Dict, > + Generic, > + Iterable, > List, > Optional, > Sequence, > - Tuple, > + TypeVar, > Union, > - cast, > ) > > from .common import ( > @@ -63,50 +64,48 @@ > _scalar = Union[str, bool, None] > _nonscalar = Union[Dict[str, _stub], List[_stub]] > _value = Union[_scalar, _nonscalar] > -TreeValue = Union[_value, 'Annotated'] > +TreeValue = Union[_value, 'Annotated[_value]'] > > # This is just an alias for an object in the structure described above: > _DObject = Dict[str, object] > > -# Represents the annotations themselves: > -Annotations = Dict[str, object] > > -# Represents an annotated node (of some kind). > -Annotated = Tuple[_value, Annotations] > +_AnnoType = TypeVar('_AnnoType', bound=TreeValue) Here it becomes much harder to keep the suggestions I made on patch 5 because of forward and backward references. Reviewed-by: Cleber Rosa <crosa@redhat.com>
John Snow <jsnow@redhat.com> writes: > This replaces _make_tree with Annotated(). By creating it as a generic > container, we can more accurately describe the exact nature of this > particular value. i.e., each Annotated object is actually an > Annotated<T>, describing its contained value. > > This adds stricter typing to Annotated nodes and extra annotated > information. Inhowfar? > It also replaces a check of "isinstance tuple" with the > much more explicit "isinstance Annotated" which is guaranteed not to > break if a tuple is accidentally introduced into the type tree. (Perhaps > as a result of a bad conversion from a list.) Sure this is worth writing home about? Such accidents seem quite unlikely. For me, the commit's benefit is making the structure of the annotated tree node more explicit (your first paragraph, I guess). It's a bit of a pattern in developing Python code: we start with a Tuple because it's terse and easy, then things get more complex, terse becomes too terse, and we're replacing the Tuple with a class. > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/introspect.py | 97 +++++++++++++++++++------------------- > 1 file changed, 48 insertions(+), 49 deletions(-) > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index a0978cb3adb..a261e402d69 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -13,12 +13,13 @@ > from typing import ( > Any, > Dict, > + Generic, > + Iterable, > List, > Optional, > Sequence, > - Tuple, > + TypeVar, > Union, > - cast, > ) > > from .common import ( > @@ -63,50 +64,48 @@ > _scalar = Union[str, bool, None] > _nonscalar = Union[Dict[str, _stub], List[_stub]] > _value = Union[_scalar, _nonscalar] > -TreeValue = Union[_value, 'Annotated'] > +TreeValue = Union[_value, 'Annotated[_value]'] > > # This is just an alias for an object in the structure described above: > _DObject = Dict[str, object] > > -# Represents the annotations themselves: > -Annotations = Dict[str, object] > > -# Represents an annotated node (of some kind). > -Annotated = Tuple[_value, Annotations] > +_AnnoType = TypeVar('_AnnoType', bound=TreeValue) > > > -def _make_tree(obj: Union[_DObject, str], ifcond: List[str], > - comment: Optional[str] = None) -> Annotated: > - extra: Annotations = { > - 'if': ifcond, > - 'comment': comment, > - } > - return (obj, extra) > +class Annotated(Generic[_AnnoType]): > + """ > + 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. > + """ > + # Remove after 3.7 adds @dataclass: > + # pylint: disable=too-few-public-methods > + def __init__(self, value: _AnnoType, ifcond: Iterable[str], > + comment: Optional[str] = None): > + self.value = value > + self.comment: Optional[str] = comment > + self.ifcond: Sequence[str] = tuple(ifcond) > > > -def _tree_to_qlit(obj: TreeValue, > - level: int = 0, > +def _tree_to_qlit(obj: TreeValue, level: int = 0, > suppress_first_indent: bool = False) -> str: > > def indent(level: int) -> str: > return level * 4 * ' ' > > - if isinstance(obj, tuple): > - ifobj, extra = obj > - ifcond = cast(Optional[Sequence[str]], extra.get('if')) > - comment = extra.get('comment') > - > + if isinstance(obj, Annotated): > msg = "Comments and Conditionals not implemented for dict values" > - assert not (suppress_first_indent and (ifcond or comment)), msg > + assert not (suppress_first_indent and (obj.comment or obj.ifcond)), msg > > ret = '' > - if comment: > - ret += indent(level) + '/* %s */\n' % comment > - if ifcond: > - ret += gen_if(ifcond) > - ret += _tree_to_qlit(ifobj, level, suppress_first_indent) > - 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, suppress_first_indent) > + if obj.ifcond: > + ret += '\n' + gen_endif(obj.ifcond) > return ret > > ret = '' > @@ -153,7 +152,7 @@ def __init__(self, prefix: str, unmask: bool): > ' * QAPI/QMP schema introspection', __doc__) > self._unmask = unmask > self._schema: Optional[QAPISchema] = None > - self._trees: List[Annotated] = [] > + self._trees: List[Annotated[_DObject]] = [] > self._used_types: List[QAPISchemaType] = [] > self._name_map: Dict[str, str] = {} > self._genc.add(mcgen(''' > @@ -219,10 +218,9 @@ def _use_type(self, typ: QAPISchemaType) -> str: > return self._name(typ.name) > > @classmethod > - def _gen_features(cls, > - features: List[QAPISchemaFeature] > - ) -> List[Annotated]: > - return [_make_tree(f.name, f.ifcond) for f in features] > + def _gen_features( > + cls, features: List[QAPISchemaFeature]) -> List[Annotated[str]]: Indent this way from the start for lesser churn. > + return [Annotated(f.name, f.ifcond) for f in features] > > def _gen_tree(self, name: str, mtype: str, obj: _DObject, > ifcond: List[str], > @@ -238,10 +236,10 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject, > 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: QAPISchemaObjectTypeMember) -> Annotated: > + member: QAPISchemaObjectTypeMember) -> Annotated[_DObject]: Long line. Ty hanging indent. > obj: _DObject = { > 'name': member.name, > 'type': self._use_type(member.type) > @@ -250,19 +248,19 @@ def _gen_member(self, > 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: str, > variants: List[QAPISchemaVariant]) -> _DObject: > return {'tag': tag_name, > 'variants': [self._gen_variant(v) for v in variants]} > > - def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated: > + def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]: > obj: _DObject = { > '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: str, info: Optional[QAPISourceInfo], > json_type: str) -> None: > @@ -272,10 +270,11 @@ def visit_enum_type(self, name: str, info: QAPISourceInfo, > ifcond: List[str], features: List[QAPISchemaFeature], > members: List[QAPISchemaEnumMember], > prefix: Optional[str]) -> None: > - self._gen_tree(name, 'enum', > - {'values': [_make_tree(m.name, m.ifcond, None) > - for m in members]}, > - ifcond, features) > + self._gen_tree( > + name, 'enum', > + {'values': [Annotated(m.name, m.ifcond) for m in members]}, > + ifcond, features > + ) > > def visit_array_type(self, name: str, info: Optional[QAPISourceInfo], > ifcond: List[str], > @@ -300,12 +299,12 @@ def visit_alternate_type(self, name: str, info: QAPISourceInfo, > ifcond: List[str], > features: List[QAPISchemaFeature], > variants: QAPISchemaVariants) -> None: > - self._gen_tree(name, 'alternate', > - {'members': [ > - _make_tree({'type': self._use_type(m.type)}, > - 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) Long line. Try breaking the line before m.ifcond, or before Annotated. > + for m in variants.variants]}, > + ifcond, features > + ) > > def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str], > features: List[QAPISchemaFeature],
On 11/16/20 5:12 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> This replaces _make_tree with Annotated(). By creating it as a generic >> container, we can more accurately describe the exact nature of this >> particular value. i.e., each Annotated object is actually an >> Annotated<T>, describing its contained value. >> >> This adds stricter typing to Annotated nodes and extra annotated >> information. > > Inhowfar? > The Generic[T] trick lets us express the type of the annotated node itself, which is more specific than Tuple[_something, ...etc...] and this type can be preserved when we peel the annotations off. It's not super crucial, but like you say, the big benefit is the field names and strict types for the special-purpose structure. >> It also replaces a check of "isinstance tuple" with the >> much more explicit "isinstance Annotated" which is guaranteed not to >> break if a tuple is accidentally introduced into the type tree. (Perhaps >> as a result of a bad conversion from a list.) > > Sure this is worth writing home about? Such accidents seem quite > unlikely. > We all have our phobias. I find "isinstance(x, extremely_common_stdlib_type)" to be extremely fragile and likely to frustrate. Maybe what's unlikely is anyone editing this code ever again. You've mentioned wanting to look into changing how the schema information is stored in QEMU before, so a lot of this might not matter for too much longer, who knows. > For me, the commit's benefit is making the structure of the annotated > tree node more explicit (your first paragraph, I guess). It's a bit of > a pattern in developing Python code: we start with a Tuple because it's > terse and easy, then things get more complex, terse becomes too terse, > and we're replacing the Tuple with a class. > Yep. >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/introspect.py | 97 +++++++++++++++++++------------------- >> 1 file changed, 48 insertions(+), 49 deletions(-) >> >> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >> index a0978cb3adb..a261e402d69 100644 >> --- a/scripts/qapi/introspect.py >> +++ b/scripts/qapi/introspect.py >> @@ -13,12 +13,13 @@ >> from typing import ( >> Any, >> Dict, >> + Generic, >> + Iterable, >> List, >> Optional, >> Sequence, >> - Tuple, >> + TypeVar, >> Union, >> - cast, >> ) >> >> from .common import ( >> @@ -63,50 +64,48 @@ >> _scalar = Union[str, bool, None] >> _nonscalar = Union[Dict[str, _stub], List[_stub]] >> _value = Union[_scalar, _nonscalar] >> -TreeValue = Union[_value, 'Annotated'] >> +TreeValue = Union[_value, 'Annotated[_value]'] >> >> # This is just an alias for an object in the structure described above: >> _DObject = Dict[str, object] >> >> -# Represents the annotations themselves: >> -Annotations = Dict[str, object] >> >> -# Represents an annotated node (of some kind). >> -Annotated = Tuple[_value, Annotations] >> +_AnnoType = TypeVar('_AnnoType', bound=TreeValue) >> >> >> -def _make_tree(obj: Union[_DObject, str], ifcond: List[str], >> - comment: Optional[str] = None) -> Annotated: >> - extra: Annotations = { >> - 'if': ifcond, >> - 'comment': comment, >> - } >> - return (obj, extra) >> +class Annotated(Generic[_AnnoType]): >> + """ >> + 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. >> + """ >> + # Remove after 3.7 adds @dataclass: >> + # pylint: disable=too-few-public-methods >> + def __init__(self, value: _AnnoType, ifcond: Iterable[str], >> + comment: Optional[str] = None): >> + self.value = value >> + self.comment: Optional[str] = comment >> + self.ifcond: Sequence[str] = tuple(ifcond) >> >> >> -def _tree_to_qlit(obj: TreeValue, >> - level: int = 0, >> +def _tree_to_qlit(obj: TreeValue, level: int = 0, >> suppress_first_indent: bool = False) -> str: >> >> def indent(level: int) -> str: >> return level * 4 * ' ' >> >> - if isinstance(obj, tuple): >> - ifobj, extra = obj >> - ifcond = cast(Optional[Sequence[str]], extra.get('if')) >> - comment = extra.get('comment') >> - >> + if isinstance(obj, Annotated): >> msg = "Comments and Conditionals not implemented for dict values" >> - assert not (suppress_first_indent and (ifcond or comment)), msg >> + assert not (suppress_first_indent and (obj.comment or obj.ifcond)), msg >> >> ret = '' >> - if comment: >> - ret += indent(level) + '/* %s */\n' % comment >> - if ifcond: >> - ret += gen_if(ifcond) >> - ret += _tree_to_qlit(ifobj, level, suppress_first_indent) >> - 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, suppress_first_indent) >> + if obj.ifcond: >> + ret += '\n' + gen_endif(obj.ifcond) >> return ret >> >> ret = '' >> @@ -153,7 +152,7 @@ def __init__(self, prefix: str, unmask: bool): >> ' * QAPI/QMP schema introspection', __doc__) >> self._unmask = unmask >> self._schema: Optional[QAPISchema] = None >> - self._trees: List[Annotated] = [] >> + self._trees: List[Annotated[_DObject]] = [] >> self._used_types: List[QAPISchemaType] = [] >> self._name_map: Dict[str, str] = {} >> self._genc.add(mcgen(''' >> @@ -219,10 +218,9 @@ def _use_type(self, typ: QAPISchemaType) -> str: >> return self._name(typ.name) >> >> @classmethod >> - def _gen_features(cls, >> - features: List[QAPISchemaFeature] >> - ) -> List[Annotated]: >> - return [_make_tree(f.name, f.ifcond) for f in features] >> + def _gen_features( >> + cls, features: List[QAPISchemaFeature]) -> List[Annotated[str]]: > > Indent this way from the start for lesser churn. > OK >> + return [Annotated(f.name, f.ifcond) for f in features] >> >> def _gen_tree(self, name: str, mtype: str, obj: _DObject, >> ifcond: List[str], >> @@ -238,10 +236,10 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject, >> 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: QAPISchemaObjectTypeMember) -> Annotated: >> + member: QAPISchemaObjectTypeMember) -> Annotated[_DObject]: > > Long line. Ty hanging indent. > OK. Admittedly, I hate hanging the return argument, I think it looks bad. Worst part of python types. :( >> obj: _DObject = { >> 'name': member.name, >> 'type': self._use_type(member.type) >> @@ -250,19 +248,19 @@ def _gen_member(self, >> 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: str, >> variants: List[QAPISchemaVariant]) -> _DObject: >> return {'tag': tag_name, >> 'variants': [self._gen_variant(v) for v in variants]} >> >> - def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated: >> + def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]: >> obj: _DObject = { >> '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: str, info: Optional[QAPISourceInfo], >> json_type: str) -> None: >> @@ -272,10 +270,11 @@ def visit_enum_type(self, name: str, info: QAPISourceInfo, >> ifcond: List[str], features: List[QAPISchemaFeature], >> members: List[QAPISchemaEnumMember], >> prefix: Optional[str]) -> None: >> - self._gen_tree(name, 'enum', >> - {'values': [_make_tree(m.name, m.ifcond, None) >> - for m in members]}, >> - ifcond, features) >> + self._gen_tree( >> + name, 'enum', >> + {'values': [Annotated(m.name, m.ifcond) for m in members]}, >> + ifcond, features >> + ) >> >> def visit_array_type(self, name: str, info: Optional[QAPISourceInfo], >> ifcond: List[str], >> @@ -300,12 +299,12 @@ def visit_alternate_type(self, name: str, info: QAPISourceInfo, >> ifcond: List[str], >> features: List[QAPISchemaFeature], >> variants: QAPISchemaVariants) -> None: >> - self._gen_tree(name, 'alternate', >> - {'members': [ >> - _make_tree({'type': self._use_type(m.type)}, >> - 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) > > Long line. Try breaking the line before m.ifcond, or before Annotated. > OK. >> + for m in variants.variants]}, >> + ifcond, features >> + ) >> >> def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str], >> features: List[QAPISchemaFeature],
John Snow <jsnow@redhat.com> writes: > On 11/16/20 5:12 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> This replaces _make_tree with Annotated(). By creating it as a generic >>> container, we can more accurately describe the exact nature of this >>> particular value. i.e., each Annotated object is actually an >>> Annotated<T>, describing its contained value. >>> >>> This adds stricter typing to Annotated nodes and extra annotated >>> information. >> >> Inhowfar? >> > > The Generic[T] trick lets us express the type of the annotated node > itself, which is more specific than Tuple[_something, ...etc...] and > this type can be preserved when we peel the annotations off. > > It's not super crucial, but like you say, the big benefit is the field > names and strict types for the special-purpose structure. I'd lead with a brief description of the data structure you're replacing, how we got there, and why it's ugly. You can steal from my review of PATCH 5. Then explain its replacement, briefly. And only then talk about types. By the time you get to types, I'm nodding along "yes, please", and will be predisposed to accept your typing arguments at face value. If you start with typing arguments, they have to negotiate the "yes, please" bar all by themselves. Harder, because Python typing stuff you have to explain for dummies. >>> It also replaces a check of "isinstance tuple" with the >>> much more explicit "isinstance Annotated" which is guaranteed not to >>> break if a tuple is accidentally introduced into the type tree. (Perhaps >>> as a result of a bad conversion from a list.) >> >> Sure this is worth writing home about? Such accidents seem quite >> unlikely. >> > > We all have our phobias. I find "isinstance(x, > extremely_common_stdlib_type)" to be extremely fragile and likely to > frustrate. You're applying programming-in-the-large reasoning to a programming-in-the-small case. Say you're writing a piece of code you expect to be used in contexts you prudently refuse to predict. The code deals with a bunch of basic Python types. Reserving another basic Python type for internal use may well be unwise then, because it can make your code break confusingly when this other type appears in input. Which it shouldn't, but making your reusable code harder to misuse, and misuses easier to diagnose are laudable goals. This is not such a piece of code. All the users it will ever have are in the same file of 200-something LOC. Your commit message makes the case for your patch. Sometimes, dropping weak arguments strengthens a case. I believe dropping the "It also replaces" argument would strengthen your case. > Maybe what's unlikely is anyone editing this code ever again. You've > mentioned wanting to look into changing how the schema information is > stored in QEMU before, so a lot of this might not matter for too much > longer, who knows. Yes, I expect generating the SchemaInfoList directly would beat generating QLitObject, then converting QLitObject -> QObject -> SchemaInfoList. Whether it's worth the effort is unclear. >> For me, the commit's benefit is making the structure of the annotated >> tree node more explicit (your first paragraph, I guess). It's a bit of >> a pattern in developing Python code: we start with a Tuple because it's >> terse and easy, then things get more complex, terse becomes too terse, >> and we're replacing the Tuple with a class. >> > > Yep. > >>> Signed-off-by: John Snow <jsnow@redhat.com>
On 12/16/20 2:08 AM, Markus Armbruster wrote: >> We all have our phobias. I find "isinstance(x, >> extremely_common_stdlib_type)" to be extremely fragile and likely to >> frustrate. > You're applying programming-in-the-large reasoning to a > programming-in-the-small case. > "Surely, they won't use my proof of concept code in production!" Ah, alas, ... > Say you're writing a piece of code you expect to be used in contexts you > prudently refuse to predict. The code deals with a bunch of basic > Python types. Reserving another basic Python type for internal use may > well be unwise then, because it can make your code break confusingly > when this other type appears in input. Which it shouldn't, but making > your reusable code harder to misuse, and misuses easier to diagnose are > laudable goals. > > This is not such a piece of code. All the users it will ever have are > in the same file of 200-something LOC. > I'm just saying that this type of code has bitten me in the ass before. You're right that it's not likely to bite someone explicitly here, but that's indeed why it came in the "Also, ..." section. I've reworked the commit message a bit by now, but I suspect you'll still want to take the red marker to it a bit. --js
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index a0978cb3adb..a261e402d69 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -13,12 +13,13 @@ from typing import ( Any, Dict, + Generic, + Iterable, List, Optional, Sequence, - Tuple, + TypeVar, Union, - cast, ) from .common import ( @@ -63,50 +64,48 @@ _scalar = Union[str, bool, None] _nonscalar = Union[Dict[str, _stub], List[_stub]] _value = Union[_scalar, _nonscalar] -TreeValue = Union[_value, 'Annotated'] +TreeValue = Union[_value, 'Annotated[_value]'] # This is just an alias for an object in the structure described above: _DObject = Dict[str, object] -# Represents the annotations themselves: -Annotations = Dict[str, object] -# Represents an annotated node (of some kind). -Annotated = Tuple[_value, Annotations] +_AnnoType = TypeVar('_AnnoType', bound=TreeValue) -def _make_tree(obj: Union[_DObject, str], ifcond: List[str], - comment: Optional[str] = None) -> Annotated: - extra: Annotations = { - 'if': ifcond, - 'comment': comment, - } - return (obj, extra) +class Annotated(Generic[_AnnoType]): + """ + 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. + """ + # Remove after 3.7 adds @dataclass: + # pylint: disable=too-few-public-methods + def __init__(self, value: _AnnoType, ifcond: Iterable[str], + comment: Optional[str] = None): + self.value = value + self.comment: Optional[str] = comment + self.ifcond: Sequence[str] = tuple(ifcond) -def _tree_to_qlit(obj: TreeValue, - level: int = 0, +def _tree_to_qlit(obj: TreeValue, level: int = 0, suppress_first_indent: bool = False) -> str: def indent(level: int) -> str: return level * 4 * ' ' - if isinstance(obj, tuple): - ifobj, extra = obj - ifcond = cast(Optional[Sequence[str]], extra.get('if')) - comment = extra.get('comment') - + if isinstance(obj, Annotated): msg = "Comments and Conditionals not implemented for dict values" - assert not (suppress_first_indent and (ifcond or comment)), msg + assert not (suppress_first_indent and (obj.comment or obj.ifcond)), msg ret = '' - if comment: - ret += indent(level) + '/* %s */\n' % comment - if ifcond: - ret += gen_if(ifcond) - ret += _tree_to_qlit(ifobj, level, suppress_first_indent) - 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, suppress_first_indent) + if obj.ifcond: + ret += '\n' + gen_endif(obj.ifcond) return ret ret = '' @@ -153,7 +152,7 @@ def __init__(self, prefix: str, unmask: bool): ' * QAPI/QMP schema introspection', __doc__) self._unmask = unmask self._schema: Optional[QAPISchema] = None - self._trees: List[Annotated] = [] + self._trees: List[Annotated[_DObject]] = [] self._used_types: List[QAPISchemaType] = [] self._name_map: Dict[str, str] = {} self._genc.add(mcgen(''' @@ -219,10 +218,9 @@ def _use_type(self, typ: QAPISchemaType) -> str: return self._name(typ.name) @classmethod - def _gen_features(cls, - features: List[QAPISchemaFeature] - ) -> List[Annotated]: - return [_make_tree(f.name, f.ifcond) for f in features] + def _gen_features( + cls, features: List[QAPISchemaFeature]) -> List[Annotated[str]]: + return [Annotated(f.name, f.ifcond) for f in features] def _gen_tree(self, name: str, mtype: str, obj: _DObject, ifcond: List[str], @@ -238,10 +236,10 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject, 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: QAPISchemaObjectTypeMember) -> Annotated: + member: QAPISchemaObjectTypeMember) -> Annotated[_DObject]: obj: _DObject = { 'name': member.name, 'type': self._use_type(member.type) @@ -250,19 +248,19 @@ def _gen_member(self, 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: str, variants: List[QAPISchemaVariant]) -> _DObject: return {'tag': tag_name, 'variants': [self._gen_variant(v) for v in variants]} - def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated: + def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]: obj: _DObject = { '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: str, info: Optional[QAPISourceInfo], json_type: str) -> None: @@ -272,10 +270,11 @@ def visit_enum_type(self, name: str, info: QAPISourceInfo, ifcond: List[str], features: List[QAPISchemaFeature], members: List[QAPISchemaEnumMember], prefix: Optional[str]) -> None: - self._gen_tree(name, 'enum', - {'values': [_make_tree(m.name, m.ifcond, None) - for m in members]}, - ifcond, features) + self._gen_tree( + name, 'enum', + {'values': [Annotated(m.name, m.ifcond) for m in members]}, + ifcond, features + ) def visit_array_type(self, name: str, info: Optional[QAPISourceInfo], ifcond: List[str], @@ -300,12 +299,12 @@ def visit_alternate_type(self, name: str, info: QAPISourceInfo, ifcond: List[str], features: List[QAPISchemaFeature], variants: QAPISchemaVariants) -> None: - self._gen_tree(name, 'alternate', - {'members': [ - _make_tree({'type': self._use_type(m.type)}, - 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: str, info: QAPISourceInfo, ifcond: List[str], features: List[QAPISchemaFeature],
This replaces _make_tree with Annotated(). By creating it as a generic container, we can more accurately describe the exact nature of this particular value. i.e., each Annotated object is actually an Annotated<T>, describing its contained value. This adds stricter typing to Annotated nodes and extra annotated information. It also replaces a check of "isinstance tuple" with the much more explicit "isinstance Annotated" which is guaranteed not to break if a tuple is accidentally introduced into the type tree. (Perhaps as a result of a bad conversion from a list.) Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/introspect.py | 97 +++++++++++++++++++------------------- 1 file changed, 48 insertions(+), 49 deletions(-)