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

[Markus Armbruster]

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.