[13/37] qapi/common.py: add notational type hints

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/common.py | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

Question on the subject line: what makes a type hint notational?

John Snow <jsnow@redhat.com> writes:

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/common.py | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 4c079755d3..af01348b35 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -12,6 +12,7 @@
>  # See the COPYING file in the top-level directory.
>  
>  import re
> +from typing import Optional, Union, Sequence
>  
>  
>  EATSPACE = '\033EATSPACE.'
> @@ -22,7 +23,7 @@
>  # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
>  # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
>  # ENUM24_Name -> ENUM24_NAME
> -def camel_to_upper(value):
> +def camel_to_upper(value: str) -> str:
>      c_fun_str = c_name(value, False)
>      if value.isupper():
>          return c_fun_str
> @@ -41,7 +42,9 @@ def camel_to_upper(value):
>      return new_name.lstrip('_').upper()
>  
>  
> -def c_enum_const(type_name, const_name, prefix=None):
> +def c_enum_const(type_name: str,
> +                 const_name: str,
> +                 prefix: Optional[str] = None) -> str:
>      if prefix is not None:
>          type_name = prefix
>      return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
> @@ -56,7 +59,7 @@ def c_enum_const(type_name, const_name, prefix=None):
>  # into substrings of a generated C function name.
>  # '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
>  # protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
> -def c_name(name, protect=True):
> +def c_name(name: str, protect: bool = True) -> str:
>      # ANSI X3J11/88-090, 3.1.1
>      c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
>                       'default', 'do', 'double', 'else', 'enum', 'extern',
> @@ -134,24 +137,24 @@ def pop(self, amount: int = 4) -> int:
>  
>  # Generate @code with @kwds interpolated.
>  # Obey INDENT level, and strip EATSPACE.
> -def cgen(code, **kwds):
> +def cgen(code: str, **kwds: Union[str, int]) -> str:

Hmm.

The @kwds values can be anything, provided they match the conversion
specifiers in @code:

>      raw = code % kwds

Your type hint adds a restriction that wasn't there before.

Is there a better way?

>      if INDENT:
>          raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE)
>      return re.sub(re.escape(EATSPACE) + r' *', '', raw)
>  
>  
> -def mcgen(code, **kwds):
> +def mcgen(code: str, **kwds: Union[str, int]) -> str:

Likewise.

>      if code[0] == '\n':
>          code = code[1:]
>      return cgen(code, **kwds)
>  
>  
> -def c_fname(filename):
> +def c_fname(filename: str) -> str:
>      return re.sub(r'[^A-Za-z0-9_]', '_', filename)
>  
>  
> -def guardstart(name):
> +def guardstart(name: str) -> str:
>      return mcgen('''
>  #ifndef %(name)s
>  #define %(name)s
> @@ -160,7 +163,7 @@ def guardstart(name):
>                   name=c_fname(name).upper())
>  
>  
> -def guardend(name):
> +def guardend(name: str) -> str:
>      return mcgen('''
>  
>  #endif /* %(name)s */
> @@ -168,7 +171,7 @@ def guardend(name):
>                   name=c_fname(name).upper())
>  
>  
> -def gen_if(ifcond):
> +def gen_if(ifcond: Sequence[str]) -> str:
>      ret = ''
>      for ifc in ifcond:
>          ret += mcgen('''
> @@ -177,7 +180,7 @@ def gen_if(ifcond):
>      return ret
>  
>  
> -def gen_endif(ifcond):
> +def gen_endif(ifcond: Sequence[str]) -> str:
>      ret = ''
>      for ifc in reversed(ifcond):
>          ret += mcgen('''
> @@ -186,7 +189,9 @@ def gen_endif(ifcond):
>      return ret
>  
>  
> -def build_params(arg_type, boxed, extra=None):
> +def build_params(arg_type,
> +                 boxed: bool,
> +                 extra: Optional[str] = None) -> str:
>      ret = ''
>      sep = ''
>      if boxed:

On 9/17/20 10:32 AM, Markus Armbruster wrote:
> Question on the subject line: what makes a type hint notational?
> 

My cover letter explains that every time I use this phrase, I mean to 
state that "This patch adds exclusively type notations and makes no 
functional changes to the runtime operation whatsoever."

i.e. notations-only.

> John Snow <jsnow@redhat.com> writes:
> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/common.py | 27 ++++++++++++++++-----------
>>   1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index 4c079755d3..af01348b35 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -12,6 +12,7 @@
>>   # See the COPYING file in the top-level directory.
>>   
>>   import re
>> +from typing import Optional, Union, Sequence
>>   
>>   
>>   EATSPACE = '\033EATSPACE.'
>> @@ -22,7 +23,7 @@
>>   # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
>>   # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
>>   # ENUM24_Name -> ENUM24_NAME
>> -def camel_to_upper(value):
>> +def camel_to_upper(value: str) -> str:
>>       c_fun_str = c_name(value, False)
>>       if value.isupper():
>>           return c_fun_str
>> @@ -41,7 +42,9 @@ def camel_to_upper(value):
>>       return new_name.lstrip('_').upper()
>>   
>>   
>> -def c_enum_const(type_name, const_name, prefix=None):
>> +def c_enum_const(type_name: str,
>> +                 const_name: str,
>> +                 prefix: Optional[str] = None) -> str:
>>       if prefix is not None:
>>           type_name = prefix
>>       return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
>> @@ -56,7 +59,7 @@ def c_enum_const(type_name, const_name, prefix=None):
>>   # into substrings of a generated C function name.
>>   # '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
>>   # protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
>> -def c_name(name, protect=True):
>> +def c_name(name: str, protect: bool = True) -> str:
>>       # ANSI X3J11/88-090, 3.1.1
>>       c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
>>                        'default', 'do', 'double', 'else', 'enum', 'extern',
>> @@ -134,24 +137,24 @@ def pop(self, amount: int = 4) -> int:
>>   
>>   # Generate @code with @kwds interpolated.
>>   # Obey INDENT level, and strip EATSPACE.
>> -def cgen(code, **kwds):
>> +def cgen(code: str, **kwds: Union[str, int]) -> str:
> 
> Hmm.
> 
> The @kwds values can be anything, provided they match the conversion
> specifiers in @code:
> 
>>       raw = code % kwds
> 
> Your type hint adds a restriction that wasn't there before.
> 
> Is there a better way?
> 

Maybe there are format-like type annotation tricks you can do to enforce 
this, but I did not research them. I tried to resist "improving" our 
usage of the old % formatter prematurely. I may do a wholesale f-string 
conversion at some point, but not now, it's not important.

In practice, we pass strings and integers. This typing *is* artificially 
restrictive, though. We can declare the type to be "Any" and allow the 
function to fail or succeed at runtime if you'd prefer.

>>       if INDENT:
>>           raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE)
>>       return re.sub(re.escape(EATSPACE) + r' *', '', raw)
>>   
>>   
>> -def mcgen(code, **kwds):
>> +def mcgen(code: str, **kwds: Union[str, int]) -> str:
> 
> Likewise.
> 

Unresearched idea: It's possible that we can subclass the 
string.Formatter class and extend it to perform our special variable 
replacements (chomping EATSPACE, etc.)

And *maybe* because it inherits from the standard formatter, we would 
benefit from any analysis Mypy performs on such things.

Basically, replace mcgen/cgen with class CFormatter(string.Formatter).

(maybe. assume that none of what I just said will work or is feasible.)

--js

On 9/17/20 2:18 PM, John Snow wrote:
> Your type hint adds a restriction that wasn't there before.
> 
> Is there a better way?

I've settled on using the `object` type for now, which is slightly more 
restrictive than `Any`.

`Any` and `object` both allow any type of argument, but `Any` 
effectively creates a gradually typed boundary in which you are 
implicitly casting to whatever typed boundary it enters next.

`object` is an explicit cast to the most abstracted type; any further 
usage that is not supported the base object will be rejected.

This is nice, because if someone adds a call at or below the mcgen 
level, mypy will certainly complain that they are assuming too much 
about the type -- and they would be!

The rest of my series will now take this approach: prefer `object` to `Any`.

--js

John Snow <jsnow@redhat.com> writes:

> On 9/17/20 10:32 AM, Markus Armbruster wrote:
>> Question on the subject line: what makes a type hint notational?
>> 
>
> My cover letter explains that every time I use this phrase, I mean to
> state that "This patch adds exclusively type notations and makes no 
> functional changes to the runtime operation whatsoever."
>
> i.e. notations-only.

By the time I get to PATCH 13, details explained in the cover letter
have been flushed from my memory.  Moreover, the cover letter won't make
it into Git.  Best to repeat them right in the commit message.  Perhaps:

    qapi/common; Add type hints

    Type hints do not change behavior.

>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/common.py | 27 ++++++++++++++++-----------
>>>   1 file changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>> index 4c079755d3..af01348b35 100644
>>> --- a/scripts/qapi/common.py
>>> +++ b/scripts/qapi/common.py
>>> @@ -12,6 +12,7 @@
>>>   # See the COPYING file in the top-level directory.
>>>     import re
>>> +from typing import Optional, Union, Sequence
>>>     
>>>   EATSPACE = '\033EATSPACE.'
>>> @@ -22,7 +23,7 @@
>>>   # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
>>>   # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
>>>   # ENUM24_Name -> ENUM24_NAME
>>> -def camel_to_upper(value):
>>> +def camel_to_upper(value: str) -> str:
>>>       c_fun_str = c_name(value, False)
>>>       if value.isupper():
>>>           return c_fun_str
>>> @@ -41,7 +42,9 @@ def camel_to_upper(value):
>>>       return new_name.lstrip('_').upper()
>>>     
>>> -def c_enum_const(type_name, const_name, prefix=None):
>>> +def c_enum_const(type_name: str,
>>> +                 const_name: str,
>>> +                 prefix: Optional[str] = None) -> str:
>>>       if prefix is not None:
>>>           type_name = prefix
>>>       return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
>>> @@ -56,7 +59,7 @@ def c_enum_const(type_name, const_name, prefix=None):
>>>   # into substrings of a generated C function name.
>>>   # '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
>>>   # protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
>>> -def c_name(name, protect=True):
>>> +def c_name(name: str, protect: bool = True) -> str:
>>>       # ANSI X3J11/88-090, 3.1.1
>>>       c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
>>>                        'default', 'do', 'double', 'else', 'enum', 'extern',
>>> @@ -134,24 +137,24 @@ def pop(self, amount: int = 4) -> int:
>>>     # Generate @code with @kwds interpolated.
>>>   # Obey INDENT level, and strip EATSPACE.
>>> -def cgen(code, **kwds):
>>> +def cgen(code: str, **kwds: Union[str, int]) -> str:
>> Hmm.
>> The @kwds values can be anything, provided they match the conversion
>> specifiers in @code:
>> 
>>>       raw = code % kwds
>> Your type hint adds a restriction that wasn't there before.
>> Is there a better way?
>> 
>
> Maybe there are format-like type annotation tricks you can do to
> enforce this, but I did not research them. I tried to resist
> "improving" our usage of the old % formatter prematurely. I may do a
> wholesale f-string conversion at some point, but not now, it's not
> important.
>
> In practice, we pass strings and integers. This typing *is*
> artificially restrictive, though. We can declare the type to be "Any"
> and allow the function to fail or succeed at runtime if you'd prefer.
>
>>>       if INDENT:
>>>           raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE)
>>>       return re.sub(re.escape(EATSPACE) + r' *', '', raw)
>>>     
>>> -def mcgen(code, **kwds):
>>> +def mcgen(code: str, **kwds: Union[str, int]) -> str:
>> Likewise.
>> 
>
> Unresearched idea: It's possible that we can subclass the
> string.Formatter class and extend it to perform our special variable 
> replacements (chomping EATSPACE, etc.)
>
> And *maybe* because it inherits from the standard formatter, we would
> benefit from any analysis Mypy performs on such things.
>
> Basically, replace mcgen/cgen with class CFormatter(string.Formatter).
>
> (maybe. assume that none of what I just said will work or is feasible.)

Sounds worth exploring.  No need to do it now, of course.

On 9/18/20 7:14 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 9/17/20 10:32 AM, Markus Armbruster wrote:
>>> Question on the subject line: what makes a type hint notational?
>>>
>>
>> My cover letter explains that every time I use this phrase, I mean to
>> state that "This patch adds exclusively type notations and makes no
>> functional changes to the runtime operation whatsoever."
>>
>> i.e. notations-only.
> 
> By the time I get to PATCH 13, details explained in the cover letter
> have been flushed from my memory.  Moreover, the cover letter won't make
> it into Git.  Best to repeat them right in the commit message.  Perhaps:
> 
>      qapi/common; Add type hints
> 
>      Type hints do not change behavior.
> 

ACK, done.

>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    scripts/qapi/common.py | 27 ++++++++++++++++-----------
>>>>    1 file changed, 16 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>>> index 4c079755d3..af01348b35 100644
>>>> --- a/scripts/qapi/common.py
>>>> +++ b/scripts/qapi/common.py
>>>> @@ -12,6 +12,7 @@
>>>>    # See the COPYING file in the top-level directory.
>>>>      import re
>>>> +from typing import Optional, Union, Sequence
>>>>      
>>>>    EATSPACE = '\033EATSPACE.'
>>>> @@ -22,7 +23,7 @@
>>>>    # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
>>>>    # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
>>>>    # ENUM24_Name -> ENUM24_NAME
>>>> -def camel_to_upper(value):
>>>> +def camel_to_upper(value: str) -> str:
>>>>        c_fun_str = c_name(value, False)
>>>>        if value.isupper():
>>>>            return c_fun_str
>>>> @@ -41,7 +42,9 @@ def camel_to_upper(value):
>>>>        return new_name.lstrip('_').upper()
>>>>      
>>>> -def c_enum_const(type_name, const_name, prefix=None):
>>>> +def c_enum_const(type_name: str,
>>>> +                 const_name: str,
>>>> +                 prefix: Optional[str] = None) -> str:
>>>>        if prefix is not None:
>>>>            type_name = prefix
>>>>        return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
>>>> @@ -56,7 +59,7 @@ def c_enum_const(type_name, const_name, prefix=None):
>>>>    # into substrings of a generated C function name.
>>>>    # '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
>>>>    # protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
>>>> -def c_name(name, protect=True):
>>>> +def c_name(name: str, protect: bool = True) -> str:
>>>>        # ANSI X3J11/88-090, 3.1.1
>>>>        c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
>>>>                         'default', 'do', 'double', 'else', 'enum', 'extern',
>>>> @@ -134,24 +137,24 @@ def pop(self, amount: int = 4) -> int:
>>>>      # Generate @code with @kwds interpolated.
>>>>    # Obey INDENT level, and strip EATSPACE.
>>>> -def cgen(code, **kwds):
>>>> +def cgen(code: str, **kwds: Union[str, int]) -> str:
>>> Hmm.
>>> The @kwds values can be anything, provided they match the conversion
>>> specifiers in @code:
>>>
>>>>        raw = code % kwds
>>> Your type hint adds a restriction that wasn't there before.
>>> Is there a better way?
>>>
>>
>> Maybe there are format-like type annotation tricks you can do to
>> enforce this, but I did not research them. I tried to resist
>> "improving" our usage of the old % formatter prematurely. I may do a
>> wholesale f-string conversion at some point, but not now, it's not
>> important.
>>
>> In practice, we pass strings and integers. This typing *is*
>> artificially restrictive, though. We can declare the type to be "Any"
>> and allow the function to fail or succeed at runtime if you'd prefer.
>>

I went with the 'object' type in new revisions.

>>>>        if INDENT:
>>>>            raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE)
>>>>        return re.sub(re.escape(EATSPACE) + r' *', '', raw)
>>>>      
>>>> -def mcgen(code, **kwds):
>>>> +def mcgen(code: str, **kwds: Union[str, int]) -> str:
>>> Likewise.
>>>
>>
>> Unresearched idea: It's possible that we can subclass the
>> string.Formatter class and extend it to perform our special variable
>> replacements (chomping EATSPACE, etc.)
>>
>> And *maybe* because it inherits from the standard formatter, we would
>> benefit from any analysis Mypy performs on such things.
>>
>> Basically, replace mcgen/cgen with class CFormatter(string.Formatter).
>>
>> (maybe. assume that none of what I just said will work or is feasible.)
> 
> Sounds worth exploring.  No need to do it now, of course.
>