[v2,07/11] qapi/introspect.py: Unify return type of _make_tree()

Commit Message

John Snow Oct. 26, 2020, 7:42 p.m. UTC

Returning two different types conditionally can be complicated to
type. Let's always return a tuple for consistency. Prohibit the use of
annotations with dict-values in this circumstance. It can be implemented
later if and when the need for it arises.

Signed-off-by: John Snow <[e-mail]>
---
 scripts/qapi/introspect.py | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Comments

Cleber Rosa Nov. 7, 2020, 5:08 a.m. UTC | #1

On Mon, Oct 26, 2020 at 03:42:47PM -0400, John Snow wrote:
> Returning two different types conditionally can be complicated to
> type. Let's always return a tuple for consistency.

This seems like a standalone change.

> Prohibit the use of
> annotations with dict-values in this circumstance. It can be implemented
> later if and when the need for it arises.

And this seems like another change.

- Cleber.

Markus Armbruster Nov. 16, 2020, 9:46 a.m. UTC | #2

John Snow <[e-mail]> writes:

> Returning two different types conditionally can be complicated to
> type. Let's always return a tuple for consistency. Prohibit the use of
> annotations with dict-values in this circumstance. It can be implemented
> later if and when the need for it arises.
>
> Signed-off-by: John Snow <[e-mail]>
> ---
>  scripts/qapi/introspect.py | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 16282f2634b..ef469b6c06e 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -77,14 +77,12 @@
>  
>  def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>                 extra: Optional[Annotations] = None
> -               ) -> TreeValue:
> +               ) -> Annotated:
>      if extra is None:
>          extra = {}
>      if ifcond:
>          extra['if'] = ifcond
> -    if extra:
> -        return (obj, extra)
> -    return obj
> +    return (obj, extra)

Less efficient, but that's okay.

>  
>  
>  def _tree_to_qlit(obj: TreeValue,
> @@ -98,12 +96,16 @@ def indent(level: int) -> str:
>          ifobj, extra = obj
>          ifcond = cast(Optional[Sequence[str]], extra.get('if'))
>          comment = extra.get('comment')
> +
> +        msg = "Comments and Conditionals not implemented for dict values"
> +        assert not (suppress_first_indent and (ifcond or comment)), msg

What exactly does this assertion guard?

> +
>          ret = ''
>          if comment:
>              ret += indent(level) + '/* %s */\n' % comment
>          if ifcond:
>              ret += gen_if(ifcond)
> -        ret += _tree_to_qlit(ifobj, level)
> +        ret += _tree_to_qlit(ifobj, level, suppress_first_indent)

Why do you need to pass on @suppress_first_indent now?

>          if ifcond:
>              ret += '\n' + gen_endif(ifcond)
>          return ret
> @@ -152,7 +154,7 @@ def __init__(self, prefix: str, unmask: bool):
>              ' * QAPI/QMP schema introspection', __doc__)
>          self._unmask = unmask
>          self._schema: Optional[QAPISchema] = None
> -        self._trees: List[TreeValue] = []
> +        self._trees: List[Annotated] = []
>          self._used_types: List[QAPISchemaType] = []
>          self._name_map: Dict[str, str] = {}
>          self._genc.add(mcgen('''
> @@ -219,7 +221,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>  
>      @classmethod
>      def _gen_features(cls,
> -                      features: List[QAPISchemaFeature]) -> List[TreeValue]:
> +                      features: List[QAPISchemaFeature]
> +                      ) -> List[Annotated]:
>          return [_make_tree(f.name, f.ifcond) for f in features]
>  
>      def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> @@ -239,7 +242,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) -> TreeValue:
> +                    member: QAPISchemaObjectTypeMember) -> Annotated:
>          obj: _DObject = {
>              'name': member.name,
>              'type': self._use_type(member.type)
> @@ -255,7 +258,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) -> TreeValue:
> +    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
>          obj: _DObject = {
>              'case': variant.name,
>              'type': self._use_type(variant.type)

John Snow Dec. 8, 2020, 12:06 a.m. UTC | #3

On 11/16/20 4:46 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> writes:
> 
>> Returning two different types conditionally can be complicated to
>> type. Let's always return a tuple for consistency. Prohibit the use of
>> annotations with dict-values in this circumstance. It can be implemented
>> later if and when the need for it arises.
>>
>> Signed-off-by: John Snow <[e-mail]>
>> ---
>>   scripts/qapi/introspect.py | 21 ++++++++++++---------
>>   1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 16282f2634b..ef469b6c06e 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -77,14 +77,12 @@
>>   
>>   def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>>                  extra: Optional[Annotations] = None
>> -               ) -> TreeValue:
>> +               ) -> Annotated:
>>       if extra is None:
>>           extra = {}
>>       if ifcond:
>>           extra['if'] = ifcond
>> -    if extra:
>> -        return (obj, extra)
>> -    return obj
>> +    return (obj, extra)
> 
> Less efficient, but that's okay.
> 

I have bad news about Python :)

>>   
>>   
>>   def _tree_to_qlit(obj: TreeValue,
>> @@ -98,12 +96,16 @@ def indent(level: int) -> str:
>>           ifobj, extra = obj
>>           ifcond = cast(Optional[Sequence[str]], extra.get('if'))
>>           comment = extra.get('comment')
>> +
>> +        msg = "Comments and Conditionals not implemented for dict values"
>> +        assert not (suppress_first_indent and (ifcond or comment)), msg
> 
> What exactly does this assertion guard?
> 

Something that Eduardo noticed in his review. It's ugly, and I explained 
it poorly.

We don't support annotations on dict *values*, basically. When this 
function is called with suppress_first_indent, we know that we are being 
called to process a dict *value* and not a dict *key*.

What do you do with comments or conditionals on just one half of a key: 
value pair?

Well, break.

>> +
>>           ret = ''
>>           if comment:
>>               ret += indent(level) + '/* %s */\n' % comment
>>           if ifcond:
>>               ret += gen_if(ifcond)
>> -        ret += _tree_to_qlit(ifobj, level)
>> +        ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
> 
> Why do you need to pass on @suppress_first_indent now?
> 

We either never should or we always should have. This is just in the 
case that "suppress first indent" is used on an annotated node. Which, 
err, for the annotations we actually support right now (comment, ifcond) 
-- we will reject in this case.

But it felt precarious...

>>           if ifcond:
>>               ret += '\n' + gen_endif(ifcond)
>>           return ret
>> @@ -152,7 +154,7 @@ def __init__(self, prefix: str, unmask: bool):
>>               ' * QAPI/QMP schema introspection', __doc__)
>>           self._unmask = unmask
>>           self._schema: Optional[QAPISchema] = None
>> -        self._trees: List[TreeValue] = []
>> +        self._trees: List[Annotated] = []
>>           self._used_types: List[QAPISchemaType] = []
>>           self._name_map: Dict[str, str] = {}
>>           self._genc.add(mcgen('''
>> @@ -219,7 +221,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>>   
>>       @classmethod
>>       def _gen_features(cls,
>> -                      features: List[QAPISchemaFeature]) -> List[TreeValue]:
>> +                      features: List[QAPISchemaFeature]
>> +                      ) -> List[Annotated]:
>>           return [_make_tree(f.name, f.ifcond) for f in features]
>>   
>>       def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> @@ -239,7 +242,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) -> TreeValue:
>> +                    member: QAPISchemaObjectTypeMember) -> Annotated:
>>           obj: _DObject = {
>>               'name': member.name,
>>               'type': self._use_type(member.type)
>> @@ -255,7 +258,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) -> TreeValue:
>> +    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
>>           obj: _DObject = {
>>               'case': variant.name,
>>               'type': self._use_type(variant.type)

John Snow Dec. 15, 2020, 12:22 a.m. UTC | #4

On 11/7/20 12:08 AM, Cleber Rosa wrote:
> And this seems like another change.
> 
> - Cleber.

Fair enough.

Markus Armbruster Dec. 16, 2020, 6:35 a.m. UTC | #5

John Snow <[e-mail]> writes:

> On 11/16/20 4:46 AM, Markus Armbruster wrote:
>> John Snow <[e-mail]> writes:
>> 
>>> Returning two different types conditionally can be complicated to
>>> type. Let's always return a tuple for consistency. Prohibit the use of
>>> annotations with dict-values in this circumstance. It can be implemented
>>> later if and when the need for it arises.
>>>
>>> Signed-off-by: John Snow <[e-mail]>
>>> ---
>>>   scripts/qapi/introspect.py | 21 ++++++++++++---------
>>>   1 file changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index 16282f2634b..ef469b6c06e 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -77,14 +77,12 @@
>>>   
>>>   def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>>>                  extra: Optional[Annotations] = None
>>> -               ) -> TreeValue:
>>> +               ) -> Annotated:
>>>       if extra is None:
>>>           extra = {}
>>>       if ifcond:
>>>           extra['if'] = ifcond
>>> -    if extra:
>>> -        return (obj, extra)
>>> -    return obj
>>> +    return (obj, extra)
>> 
>> Less efficient, but that's okay.
>> 
>
> I have bad news about Python :)

Well played, sir!

>>>   
>>>   
>>>   def _tree_to_qlit(obj: TreeValue,
>>> @@ -98,12 +96,16 @@ def indent(level: int) -> str:
>>>           ifobj, extra = obj
>>>           ifcond = cast(Optional[Sequence[str]], extra.get('if'))
>>>           comment = extra.get('comment')
>>> +
>>> +        msg = "Comments and Conditionals not implemented for dict values"
>>> +        assert not (suppress_first_indent and (ifcond or comment)), msg
>> 
>> What exactly does this assertion guard?
>> 
>
> Something that Eduardo noticed in his review. It's ugly, and I explained 
> it poorly.
>
> We don't support annotations on dict *values*, basically. When this 
> function is called with suppress_first_indent, we know that we are being 
> called to process a dict *value* and not a dict *key*.
>
> What do you do with comments or conditionals on just one half of a key: 
> value pair?
>
> Well, break.

Your @msg is a bit misleading then: it suggests comments and
conditionals are not implemented for dict values, but could be.  Not
true for conditionals.

Minimally invasive patch correction: drop @msg.

But the actual impossibility to guard against is even simpler, I
believe:

          if isinstance(obj, tuple):
              assert not suppress_first_indent

But^2, I figure this can be made truly impossible: factor out the part
of _tree_to_qlit() that deals with non-tuple @obj into its own function
(the conditional at its end), then use that for the dict values.

>>> +
>>>           ret = ''
>>>           if comment:
>>>               ret += indent(level) + '/* %s */\n' % comment
>>>           if ifcond:
>>>               ret += gen_if(ifcond)
>>> -        ret += _tree_to_qlit(ifobj, level)
>>> +        ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
>> 
>> Why do you need to pass on @suppress_first_indent now?
>> 
>
> We either never should or we always should have. This is just in the 
> case that "suppress first indent" is used on an annotated node. Which, 
> err, for the annotations we actually support right now (comment, ifcond) 
> -- we will reject in this case.
>
> But it felt precarious...

I suspect the factoring I suggested above will make this less
precarious, too.

>>>           if ifcond:
>>>               ret += '\n' + gen_endif(ifcond)
>>>           return ret
>>> @@ -152,7 +154,7 @@ def __init__(self, prefix: str, unmask: bool):
>>>               ' * QAPI/QMP schema introspection', __doc__)
>>>           self._unmask = unmask
>>>           self._schema: Optional[QAPISchema] = None
>>> -        self._trees: List[TreeValue] = []
>>> +        self._trees: List[Annotated] = []
>>>           self._used_types: List[QAPISchemaType] = []
>>>           self._name_map: Dict[str, str] = {}
>>>           self._genc.add(mcgen('''
>>> @@ -219,7 +221,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>>>   
>>>       @classmethod
>>>       def _gen_features(cls,
>>> -                      features: List[QAPISchemaFeature]) -> List[TreeValue]:
>>> +                      features: List[QAPISchemaFeature]
>>> +                      ) -> List[Annotated]:
>>>           return [_make_tree(f.name, f.ifcond) for f in features]
>>>   
>>>       def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>>> @@ -239,7 +242,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) -> TreeValue:
>>> +                    member: QAPISchemaObjectTypeMember) -> Annotated:
>>>           obj: _DObject = {
>>>               'name': member.name,
>>>               'type': self._use_type(member.type)
>>> @@ -255,7 +258,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) -> TreeValue:
>>> +    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
>>>           obj: _DObject = {
>>>               'case': variant.name,
>>>               'type': self._use_type(variant.type)

Patch

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 16282f2634b..ef469b6c06e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -77,14 +77,12 @@ 
 
 def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
                extra: Optional[Annotations] = None
-               ) -> TreeValue:
+               ) -> Annotated:
     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: TreeValue,
@@ -98,12 +96,16 @@  def indent(level: int) -> str:
         ifobj, extra = obj
         ifcond = cast(Optional[Sequence[str]], extra.get('if'))
         comment = extra.get('comment')
+
+        msg = "Comments and Conditionals not implemented for dict values"
+        assert not (suppress_first_indent and (ifcond or comment)), msg
+
         ret = ''
         if comment:
             ret += indent(level) + '/* %s */\n' % comment
         if ifcond:
             ret += gen_if(ifcond)
-        ret += _tree_to_qlit(ifobj, level)
+        ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
         if ifcond:
             ret += '\n' + gen_endif(ifcond)
         return ret
@@ -152,7 +154,7 @@  def __init__(self, prefix: str, unmask: bool):
             ' * QAPI/QMP schema introspection', __doc__)
         self._unmask = unmask
         self._schema: Optional[QAPISchema] = None
-        self._trees: List[TreeValue] = []
+        self._trees: List[Annotated] = []
         self._used_types: List[QAPISchemaType] = []
         self._name_map: Dict[str, str] = {}
         self._genc.add(mcgen('''
@@ -219,7 +221,8 @@  def _use_type(self, typ: QAPISchemaType) -> str:
 
     @classmethod
     def _gen_features(cls,
-                      features: List[QAPISchemaFeature]) -> List[TreeValue]:
+                      features: List[QAPISchemaFeature]
+                      ) -> List[Annotated]:
         return [_make_tree(f.name, f.ifcond) for f in features]
 
     def _gen_tree(self, name: str, mtype: str, obj: _DObject,
@@ -239,7 +242,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) -> TreeValue:
+                    member: QAPISchemaObjectTypeMember) -> Annotated:
         obj: _DObject = {
             'name': member.name,
             'type': self._use_type(member.type)
@@ -255,7 +258,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) -> TreeValue:
+    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
         obj: _DObject = {
             'case': variant.name,
             'type': self._use_type(variant.type)