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

[John Snow <jsnow@redhat.com>]

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