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

[John Snow]

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