diff mbox series

[v2,09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure

Message ID 20201026194251.11075-10-jsnow@redhat.com
State New
Headers show
Series qapi: static typing conversion, pt2 | expand

Commit Message

John Snow Oct. 26, 2020, 7:42 p.m. UTC
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(-)

Comments

Cleber Rosa Nov. 7, 2020, 5:45 a.m. UTC | #1
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>
Markus Armbruster Nov. 16, 2020, 10:12 a.m. UTC | #2
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],
diff mbox series

Patch

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],