[v1] qapi: static typing conversion, pt1.5

[PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond

Posted by John Snow 4 years, 11 months ago

We already assert this in end_if, but that's opaque to mypy. Do it in
_wrap_ifcond instead. Same effect at runtime, but mypy can now infer
the type in _wrap_ifcond's body.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/gen.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b40f18eee3cd..a6dc991b1d03 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
         self._start_if = (ifcond, self._body, self._preamble)
 
     def end_if(self) -> None:
-        assert self._start_if
         self._wrap_ifcond()
         self._start_if = None
 
     def _wrap_ifcond(self) -> None:
+        assert self._start_if
         self._body = _wrap_ifcond(self._start_if[0],
                                   self._start_if[1], self._body)
         self._preamble = _wrap_ifcond(self._start_if[0],
-- 
2.26.2

Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond

Posted by Markus Armbruster 4 years, 11 months ago

John Snow <jsnow@redhat.com> writes:

> We already assert this in end_if, but that's opaque to mypy. Do it in
> _wrap_ifcond instead. Same effect at runtime, but mypy can now infer
> the type in _wrap_ifcond's body.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/gen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index b40f18eee3cd..a6dc991b1d03 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
>          self._start_if = (ifcond, self._body, self._preamble)
>  
>      def end_if(self) -> None:
> -        assert self._start_if
>          self._wrap_ifcond()
>          self._start_if = None
>  
>      def _wrap_ifcond(self) -> None:
> +        assert self._start_if
>          self._body = _wrap_ifcond(self._start_if[0],
>                                    self._start_if[1], self._body)
>          self._preamble = _wrap_ifcond(self._start_if[0],

Drawback: the public method's precondition is now more opaque.  Do we
care?

Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond

Posted by John Snow 4 years, 11 months ago

On 12/16/20 3:26 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> We already assert this in end_if, but that's opaque to mypy. Do it in
>> _wrap_ifcond instead. Same effect at runtime, but mypy can now infer
>> the type in _wrap_ifcond's body.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/gen.py | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index b40f18eee3cd..a6dc991b1d03 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
>>           self._start_if = (ifcond, self._body, self._preamble)
>>   
>>       def end_if(self) -> None:
>> -        assert self._start_if
>>           self._wrap_ifcond()
>>           self._start_if = None
>>   
>>       def _wrap_ifcond(self) -> None:
>> +        assert self._start_if
>>           self._body = _wrap_ifcond(self._start_if[0],
>>                                     self._start_if[1], self._body)
>>           self._preamble = _wrap_ifcond(self._start_if[0],
> 
> Drawback: the public method's precondition is now more opaque.  Do we
> care?
> 

Ish. If you call end_if before start_if, what did you want to have happen?

Or more to the point: do you want the assertion in both places?

--js

Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond

Posted by Markus Armbruster 4 years, 11 months ago

John Snow <jsnow@redhat.com> writes:

> On 12/16/20 3:26 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> We already assert this in end_if, but that's opaque to mypy. Do it in
>>> _wrap_ifcond instead. Same effect at runtime, but mypy can now infer
>>> the type in _wrap_ifcond's body.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/gen.py | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>>> index b40f18eee3cd..a6dc991b1d03 100644
>>> --- a/scripts/qapi/gen.py
>>> +++ b/scripts/qapi/gen.py
>>> @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
>>>           self._start_if = (ifcond, self._body, self._preamble)
>>>   
>>>       def end_if(self) -> None:
>>> -        assert self._start_if
>>>           self._wrap_ifcond()
>>>           self._start_if = None
>>>   
>>>       def _wrap_ifcond(self) -> None:
>>> +        assert self._start_if
>>>           self._body = _wrap_ifcond(self._start_if[0],
>>>                                     self._start_if[1], self._body)
>>>           self._preamble = _wrap_ifcond(self._start_if[0],
>> 
>> Drawback: the public method's precondition is now more opaque.  Do we
>> care?
>> 
>
> Ish. If you call end_if before start_if, what did you want to have happen?

Point.

> Or more to the point: do you want the assertion in both places?

What about inlining QAPIGenCCode._wrap_ifcond() into .end_if()?