| Message ID | 20200930043150.1454766-40-jsnow@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
On Wed, Sep 30, 2020 at 12:31:43AM -0400, John Snow wrote: > Returning a *something* or a Tuple of *something* is hard to accurately > type. Let's just always return a tuple for structural consistency. > > Instances of the 'TreeNode' type can be replaced with the slightly more > specific 'AnnotatedNode' type. > > Signed-off-by: John Snow <jsnow@redhat.com> So, the only place where this seems to make a difference is _tree_to_qlit(). We just need to prove that _tree_to_qlit(o, ...) will have exactly the same result as _tree_to_qlit((o, None), ...). For reference, this is the beginning of _tree_to_qlit(): | def _tree_to_qlit(obj: TreeNode, | level: int = 0, | suppress_first_indent: bool = False) -> str: | | def indent(level: int) -> str: | return level * 4 * ' ' | | if isinstance(obj, tuple): | ifobj, extra = obj `obj` is the return value of _make_tree() `ifobj` is the original `obj` argument to _make_tree(). | ifcond = extra.get('if') ifcond will be None. | comment = extra.get('comment') comment will be None | ret = '' | if comment: | ret += indent(level) + '/* %s */\n' % comment nop | if ifcond: | ret += gen_if(ifcond) nop | ret += _tree_to_qlit(ifobj, level) ret will be '', so this is equivalent to: ret = _tree_to_qlit(ifobj, level) which is almost good. The only difference seems to that suppress_first_indent=True will be ignored. We should pass suppress_first_indent as argument in the recursive call above, just in case. The existing code will behave weirdly if there are comments or conditions and suppress_first_indent=True, but I suggest we try to address this issue later. | if ifcond: | ret += '\n' + gen_endif(ifcond) nop | return ret > --- > scripts/qapi/introspect.py | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index 5cbdc7414bd..1c3ba41f1dc 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -53,14 +53,12 @@ > > def _make_tree(obj: Union[_DObject, str], ifcond: List[str], > extra: Optional[Extra] = None > - ) -> Union[TreeNode, AnnotatedNode]: > + ) -> AnnotatedNode: > if extra is None: > extra = {} > if ifcond: > extra['if'] = ifcond > - if extra: > - return (obj, extra) > - return obj > + return (obj, extra) > > > def _tree_to_qlit(obj: TreeNode, > @@ -128,7 +126,7 @@ def __init__(self, prefix: str, unmask: bool): > ' * QAPI/QMP schema introspection', __doc__) > self._unmask = unmask > self._schema: Optional[QAPISchema] = None > - self._trees: List[TreeNode] = [] > + self._trees: List[AnnotatedNode] = [] > self._used_types: List[QAPISchemaType] = [] > self._name_map: Dict[str, str] = {} > self._genc.add(mcgen(''' > @@ -195,7 +193,8 @@ def _use_type(self, typ: QAPISchemaType) -> str: > > @classmethod > def _gen_features(cls, > - features: List[QAPISchemaFeature]) -> List[TreeNode]: > + features: List[QAPISchemaFeature] > + ) -> List[AnnotatedNode]: > return [_make_tree(f.name, f.ifcond) for f in features] > > def _gen_tree(self, name: str, mtype: str, obj: _DObject, > @@ -215,7 +214,7 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject, > self._trees.append(_make_tree(obj, ifcond, extra)) > > def _gen_member(self, > - member: QAPISchemaObjectTypeMember) -> TreeNode: > + member: QAPISchemaObjectTypeMember) -> AnnotatedNode: > obj: _DObject = { > 'name': member.name, > 'type': self._use_type(member.type) > @@ -231,7 +230,7 @@ def _gen_variants(self, tag_name: str, > return {'tag': tag_name, > 'variants': [self._gen_variant(v) for v in variants]} > > - def _gen_variant(self, variant: QAPISchemaVariant) -> TreeNode: > + def _gen_variant(self, variant: QAPISchemaVariant) -> AnnotatedNode: > obj: _DObject = { > 'case': variant.name, > 'type': self._use_type(variant.type) > -- > 2.26.2 > -- Eduardo
On 9/30/20 2:24 PM, Eduardo Habkost wrote: > On Wed, Sep 30, 2020 at 12:31:43AM -0400, John Snow wrote: >> Returning a *something* or a Tuple of *something* is hard to accurately >> type. Let's just always return a tuple for structural consistency. >> >> Instances of the 'TreeNode' type can be replaced with the slightly more >> specific 'AnnotatedNode' type. >> >> Signed-off-by: John Snow <jsnow@redhat.com> > > So, the only place where this seems to make a difference is > _tree_to_qlit(). > > We just need to prove that > _tree_to_qlit(o, ...) > will have exactly the same result as > _tree_to_qlit((o, None), ...). > > For reference, this is the beginning of _tree_to_qlit(): > > | def _tree_to_qlit(obj: TreeNode, > | level: int = 0, > | suppress_first_indent: bool = False) -> str: > | > | def indent(level: int) -> str: > | return level * 4 * ' ' > | > | if isinstance(obj, tuple): > | ifobj, extra = obj > > `obj` is the return value of _make_tree() > > `ifobj` is the original `obj` argument to _make_tree(). > > | ifcond = extra.get('if') > > ifcond will be None. > > | comment = extra.get('comment') > > comment will be None > > | ret = '' > | if comment: > | ret += indent(level) + '/* %s */\n' % comment > > nop > > | if ifcond: > | ret += gen_if(ifcond) > > nop > > | ret += _tree_to_qlit(ifobj, level) > > ret will be '', so this is equivalent to: > > ret = _tree_to_qlit(ifobj, level) > > which is almost good. > > The only difference seems to that suppress_first_indent=True will > be ignored. We should pass suppress_first_indent as argument in > the recursive call above, just in case. > This is a really good spot, and I indeed hadn't considered it at all when I did this. (I simply made the change and observed it worked just fine!) > The existing code will behave weirdly if there are comments or > conditions and suppress_first_indent=True, but I suggest we try > to address this issue later. > > | if ifcond: > | ret += '\n' + gen_endif(ifcond) > > nop > > | return ret > Hm, yes, it's a hypothetical case, but perhaps we can use an assertion to help guard against it if development creates that case later by accident. That ought to be good enough for now to not waste time accommodating a (presently) fictional circumstance? Thanks for the good sleuthing here. --js
On Wed, Sep 30, 2020 at 02:32:49PM -0400, John Snow wrote: > On 9/30/20 2:24 PM, Eduardo Habkost wrote: > > On Wed, Sep 30, 2020 at 12:31:43AM -0400, John Snow wrote: > > > Returning a *something* or a Tuple of *something* is hard to accurately > > > type. Let's just always return a tuple for structural consistency. > > > > > > Instances of the 'TreeNode' type can be replaced with the slightly more > > > specific 'AnnotatedNode' type. > > > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > > > So, the only place where this seems to make a difference is > > _tree_to_qlit(). > > > > We just need to prove that > > _tree_to_qlit(o, ...) > > will have exactly the same result as > > _tree_to_qlit((o, None), ...). > > > > For reference, this is the beginning of _tree_to_qlit(): > > > > | def _tree_to_qlit(obj: TreeNode, > > | level: int = 0, > > | suppress_first_indent: bool = False) -> str: > > | > > | def indent(level: int) -> str: > > | return level * 4 * ' ' > > | > > | if isinstance(obj, tuple): > > | ifobj, extra = obj > > > > `obj` is the return value of _make_tree() > > > > `ifobj` is the original `obj` argument to _make_tree(). > > > > | ifcond = extra.get('if') > > > > ifcond will be None. > > > > | comment = extra.get('comment') > > > > comment will be None > > > > | ret = '' > > | if comment: > > | ret += indent(level) + '/* %s */\n' % comment > > > > nop > > > > | if ifcond: > > | ret += gen_if(ifcond) > > > > nop > > > > | ret += _tree_to_qlit(ifobj, level) > > > > ret will be '', so this is equivalent to: > > > > ret = _tree_to_qlit(ifobj, level) > > > > which is almost good. > > > > The only difference seems to that suppress_first_indent=True will > > be ignored. We should pass suppress_first_indent as argument in > > the recursive call above, just in case. > > > > This is a really good spot, and I indeed hadn't considered it at all when I > did this. > > (I simply made the change and observed it worked just fine!) > > > The existing code will behave weirdly if there are comments or > > conditions and suppress_first_indent=True, but I suggest we try > > to address this issue later. > > > > | if ifcond: > > | ret += '\n' + gen_endif(ifcond) > > > > nop > > > > | return ret > > > > Hm, yes, it's a hypothetical case, but perhaps we can use an assertion to > help guard against it if development creates that case later by accident. > > That ought to be good enough for now to not waste time accommodating a > (presently) fictional circumstance? > > Thanks for the good sleuthing here. With the current code, both ret += _tree_to_qlit(ifobj, level) and ret += _tree_to_qlit(ifobj, level, suppress_first_indent) will behave exactly the same. The former will behave weirdly if we wrap a dictionary value using _tree_node(). We don't do that today. The latter will behave weirdly if there's a comment or ifcond attached in a dictionary value. We don't do that today. I believe the latter is less likely to be triggered by accident. But I'd be happy with either: # _make_tree() shouldn't be use to wrap nodes that # may be printed using suppress_first_indent=True # (in other words, dictionary values shouldn't be wrapped using _make_tree()) assert(not suppress_first_indent) ret += _tree_to_qlit(ifobj, level) or # we can't add ifcond or comments to nodes that may be # printed using suppress_first_indent=True # (in other words, dictionary values can't have ifcond or comments) assert(not suppress_first_indent or (not comment and not ifcond)) ret += _tree_to_qlit(ifobj, level, suppress_first_indent) If we have time to spare later, we could do this: def _value_to_qlit(obj: Union[None, str, Dict[str, object], List[object], bool], level: int = 0, suppress_first_indent: bool = False) -> str: ... if obj is None: ... elif isinstance(obj, str): ... elif isinstance(obj, list): ... ... def _tree_to_qlit(obj: TreeNode, level: int = 0) -> str: if isinstance(obj, AnnotatedNode): ... else: return _value_to_qlit(obj, level) This way, it will be impossible to set suppress_first_indent=True on an annotated node. -- Eduardo
On 9/30/20 2:57 PM, Eduardo Habkost wrote: > On Wed, Sep 30, 2020 at 02:32:49PM -0400, John Snow wrote: >> On 9/30/20 2:24 PM, Eduardo Habkost wrote: >>> On Wed, Sep 30, 2020 at 12:31:43AM -0400, John Snow wrote: >>>> Returning a *something* or a Tuple of *something* is hard to accurately >>>> type. Let's just always return a tuple for structural consistency. >>>> >>>> Instances of the 'TreeNode' type can be replaced with the slightly more >>>> specific 'AnnotatedNode' type. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>> >>> So, the only place where this seems to make a difference is >>> _tree_to_qlit(). >>> >>> We just need to prove that >>> _tree_to_qlit(o, ...) >>> will have exactly the same result as >>> _tree_to_qlit((o, None), ...). >>> >>> For reference, this is the beginning of _tree_to_qlit(): >>> >>> | def _tree_to_qlit(obj: TreeNode, >>> | level: int = 0, >>> | suppress_first_indent: bool = False) -> str: >>> | >>> | def indent(level: int) -> str: >>> | return level * 4 * ' ' >>> | >>> | if isinstance(obj, tuple): >>> | ifobj, extra = obj >>> >>> `obj` is the return value of _make_tree() >>> >>> `ifobj` is the original `obj` argument to _make_tree(). >>> >>> | ifcond = extra.get('if') >>> >>> ifcond will be None. >>> >>> | comment = extra.get('comment') >>> >>> comment will be None >>> >>> | ret = '' >>> | if comment: >>> | ret += indent(level) + '/* %s */\n' % comment >>> >>> nop >>> >>> | if ifcond: >>> | ret += gen_if(ifcond) >>> >>> nop >>> >>> | ret += _tree_to_qlit(ifobj, level) >>> >>> ret will be '', so this is equivalent to: >>> >>> ret = _tree_to_qlit(ifobj, level) >>> >>> which is almost good. >>> >>> The only difference seems to that suppress_first_indent=True will >>> be ignored. We should pass suppress_first_indent as argument in >>> the recursive call above, just in case. >>> >> >> This is a really good spot, and I indeed hadn't considered it at all when I >> did this. >> >> (I simply made the change and observed it worked just fine!) >> >>> The existing code will behave weirdly if there are comments or >>> conditions and suppress_first_indent=True, but I suggest we try >>> to address this issue later. >>> >>> | if ifcond: >>> | ret += '\n' + gen_endif(ifcond) >>> >>> nop >>> >>> | return ret >>> >> >> Hm, yes, it's a hypothetical case, but perhaps we can use an assertion to >> help guard against it if development creates that case later by accident. >> >> That ought to be good enough for now to not waste time accommodating a >> (presently) fictional circumstance? >> >> Thanks for the good sleuthing here. > > With the current code, both > ret += _tree_to_qlit(ifobj, level) > and > ret += _tree_to_qlit(ifobj, level, suppress_first_indent) > will behave exactly the same. > > The former will behave weirdly if we wrap a dictionary value using > _tree_node(). We don't do that today. > > The latter will behave weirdly if there's a comment or ifcond > attached in a dictionary value. We don't do that today. > > I believe the latter is less likely to be triggered by accident. > > But I'd be happy with either: > > # _make_tree() shouldn't be use to wrap nodes that > # may be printed using suppress_first_indent=True > # (in other words, dictionary values shouldn't be wrapped using _make_tree()) > assert(not suppress_first_indent) > ret += _tree_to_qlit(ifobj, level) > > or > > # we can't add ifcond or comments to nodes that may be > # printed using suppress_first_indent=True > # (in other words, dictionary values can't have ifcond or comments) > assert(not suppress_first_indent or (not comment and not ifcond)) > ret += _tree_to_qlit(ifobj, level, suppress_first_indent) > > > If we have time to spare later, we could do this: > > def _value_to_qlit(obj: Union[None, str, Dict[str, object], List[object], bool], > level: int = 0, > suppress_first_indent: bool = False) -> str: > ... > if obj is None: > ... > elif isinstance(obj, str): > ... > elif isinstance(obj, list): > ... > ... > > def _tree_to_qlit(obj: TreeNode, level: int = 0) -> str: > if isinstance(obj, AnnotatedNode): > ... > else: > return _value_to_qlit(obj, level) > > This way, it will be impossible to set suppress_first_indent=True > on an annotated node. > Maybe it's the right thing to separate out container types from leaf types and make this mutually recursive. I debating doing that earlier, but the patches were already so strangled and messy I was afraid of plunging deeper into refactors. Maybe I'll go take a nap and do it when I wake up. :) --js
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 5cbdc7414bd..1c3ba41f1dc 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -53,14 +53,12 @@ def _make_tree(obj: Union[_DObject, str], ifcond: List[str], extra: Optional[Extra] = None - ) -> Union[TreeNode, AnnotatedNode]: + ) -> AnnotatedNode: if extra is None: extra = {} if ifcond: extra['if'] = ifcond - if extra: - return (obj, extra) - return obj + return (obj, extra) def _tree_to_qlit(obj: TreeNode, @@ -128,7 +126,7 @@ def __init__(self, prefix: str, unmask: bool): ' * QAPI/QMP schema introspection', __doc__) self._unmask = unmask self._schema: Optional[QAPISchema] = None - self._trees: List[TreeNode] = [] + self._trees: List[AnnotatedNode] = [] self._used_types: List[QAPISchemaType] = [] self._name_map: Dict[str, str] = {} self._genc.add(mcgen(''' @@ -195,7 +193,8 @@ def _use_type(self, typ: QAPISchemaType) -> str: @classmethod def _gen_features(cls, - features: List[QAPISchemaFeature]) -> List[TreeNode]: + features: List[QAPISchemaFeature] + ) -> List[AnnotatedNode]: return [_make_tree(f.name, f.ifcond) for f in features] def _gen_tree(self, name: str, mtype: str, obj: _DObject, @@ -215,7 +214,7 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject, self._trees.append(_make_tree(obj, ifcond, extra)) def _gen_member(self, - member: QAPISchemaObjectTypeMember) -> TreeNode: + member: QAPISchemaObjectTypeMember) -> AnnotatedNode: obj: _DObject = { 'name': member.name, 'type': self._use_type(member.type) @@ -231,7 +230,7 @@ def _gen_variants(self, tag_name: str, return {'tag': tag_name, 'variants': [self._gen_variant(v) for v in variants]} - def _gen_variant(self, variant: QAPISchemaVariant) -> TreeNode: + def _gen_variant(self, variant: QAPISchemaVariant) -> AnnotatedNode: obj: _DObject = { 'case': variant.name, 'type': self._use_type(variant.type)
Returning a *something* or a Tuple of *something* is hard to accurately type. Let's just always return a tuple for structural consistency. Instances of the 'TreeNode' type can be replaced with the slightly more specific 'AnnotatedNode' type. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/introspect.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)