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

[John Snow <jsnow@redhat.com>]

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.
>