Re: [PATCH v5 27/36] qapi/gen.py: add type hint annotations

[John Snow]

On 10/7/20 8:21 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > Annotations do not change runtime behavior.
> > This commit *only* adds annotations.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > Reviewed-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   scripts/qapi/gen.py | 104 ++++++++++++++++++++++++--------------------
> >   1 file changed, 57 insertions(+), 47 deletions(-)
> >
> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > index 1bad37fc06b..d0391cd8718 100644
> > --- a/scripts/qapi/gen.py
> > +++ b/scripts/qapi/gen.py
> > @@ -17,7 +17,13 @@
> >   import errno
> >   import os
> >   import re
> > -from typing import Optional
> > +from typing import (
> > +    Dict,
> > +    Iterator,
> > +    List,
> > +    Optional,
> > +    Tuple,
> > +)
> >   
> >   from .common import (
> >       c_fname,
> > @@ -29,31 +35,31 @@
> >       mcgen,
> >   )
> >   from .schema import QAPISchemaObjectType, QAPISchemaVisitor
> > +from .source import QAPISourceInfo
> >   
> >   
> >   class QAPIGen:
> > -
> > -    def __init__(self, fname):
> > +    def __init__(self, fname: Optional[str]):
>
> I'd expect fname: str.  Can you point me to the spot that passes None?

qapi/commands.py:        self._regy = QAPIGenCCode(None)

Good time to mention again: I am disabling strict none checks for now in 
this conversion.

That means we can pass Optional[T] to functions expecting T and mypy 
will not complain. This should obviously be fixed long-term, but the 
cleanups involve things that are a higher class of finnicky.

If this alarms you, good! We'll fix it in time, but it doesn't break 
anything any worse than it already was.

If this check was disabled, you would be able to edit the Optional[str] 
to str and see what broke. But because I use this affordance for now, 
you would see no difference.

> >           self.fname = fname
> >           self._preamble = ''
> >           self._body = ''
> >   
> > -    def preamble_add(self, text):
> > +    def preamble_add(self, text: str) -> None:
> >           self._preamble += text
> >   
> > -    def add(self, text):
> > +    def add(self, text: str) -> None:
> >           self._body += text
> >   
> > -    def get_content(self):
> > +    def get_content(self) -> str:
> >           return self._top() + self._preamble + self._body + self._bottom()
> >   
> > -    def _top(self):
> > +    def _top(self) -> str:
> >           return ''
> >   
> > -    def _bottom(self):
> > +    def _bottom(self) -> str:
> >           return ''
> >   
> > -    def write(self, output_dir):
> > +    def write(self, output_dir: str) -> None:
> >           # Include paths starting with ../ are used to reuse modules of the 
> > main
> >           # schema in specialised schemas. Don't overwrite the files that are
> >           # already generated for the main schema.
> > @@ -78,7 +84,7 @@ def write(self, output_dir):
> >           f.close()
> >   
> >   
> > -def _wrap_ifcond(ifcond, before, after):
> > +def _wrap_ifcond(ifcond: List[str], before: str, after: str) -> str:
> >       if before == after:
> >           return after   # suppress empty #if ... #endif
> >   
> > @@ -118,40 +124,38 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
> >   
> >   
> >   class QAPIGenCCode(QAPIGen):
> > -
> > -    def __init__(self, fname):
> > +    def __init__(self, fname: Optional[str]):
>
> Likewise.
>
> >           super().__init__(fname)
> > -        self._start_if = None
> > +        self._start_if: Optional[Tuple[List[str], str, str]] = None
> >   
> > -    def start_if(self, ifcond):
> > +    def start_if(self, ifcond: List[str]) -> None:
> >           assert self._start_if is None
> >           self._start_if = (ifcond, self._body, self._preamble)
> >   
> > -    def end_if(self):
> > +    def end_if(self) -> None:
> >           assert self._start_if
> >           self._wrap_ifcond()
> >           self._start_if = None
> >   
> > -    def _wrap_ifcond(self):
> > +    def _wrap_ifcond(self) -> None:
> >           self._body = _wrap_ifcond(self._start_if[0],
> >                                     self._start_if[1], self._body)
> >           self._preamble = _wrap_ifcond(self._start_if[0],
> >                                         self._start_if[2], self._preamble)
> >   
> > -    def get_content(self):
> > +    def get_content(self) -> str:
> >           assert self._start_if is None
> >           return super().get_content()
> >   
> >   
> >   class QAPIGenC(QAPIGenCCode):
> > -
> > -    def __init__(self, fname, blurb, pydoc):
> > +    def __init__(self, fname: str, blurb: str, pydoc: str):
>
> Here it's just str.

That's right.

> >           super().__init__(fname)
> >           self._blurb = blurb
> >           self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
> >                                                     re.MULTILINE))
> >   
> > -    def _top(self):
> > +    def _top(self) -> str:
> >           return mcgen('''
> >   /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> >   
> > @@ -167,7 +171,7 @@ def _top(self):
> >   ''',
> >                        blurb=self._blurb, copyright=self._copyright)
> >   
> > -    def _bottom(self):
> > +    def _bottom(self) -> str:
> >           return mcgen('''
> >   
> >   /* Dummy declaration to prevent empty .o file */
> > @@ -177,16 +181,15 @@ def _bottom(self):
> >   
> >   
> >   class QAPIGenH(QAPIGenC):
> > -
> > -    def _top(self):
> > +    def _top(self) -> str:
> >           return super()._top() + guardstart(self.fname)
> >   
> > -    def _bottom(self):
> > +    def _bottom(self) -> str:
> >           return guardend(self.fname)
> >   
> >   
> >   @contextmanager
> > -def ifcontext(ifcond, *args):
> > +def ifcontext(ifcond: List[str], *args: QAPIGenCCode) -> Iterator[None]:
>
> Oh, the type hint for a *args is QAPIGenCCode, even though args is a
> tuple of excess positional arguments (which are all QAPIGenCCode).

Yeah, it's the way type hints interact with the special '*' syntax. This 
is an Iterable of QAPIGenCCode.

**kwargs works the same way: you annotate the values, and the keys are 
assumed to be str.

> >       """
> >       A with-statement context manager that wraps with `start_if()` / 
> > `end_if()`.
> >   
> > @@ -214,8 +217,11 @@ def ifcontext(ifcond, *args):
> >   
> >   
> >   class QAPISchemaMonolithicCVisitor(QAPISchemaVisitor):
> > -
> > -    def __init__(self, prefix, what, blurb, pydoc):
> > +    def __init__(self,
> > +                 prefix: str,
> > +                 what: str,
> > +                 blurb: str,
> > +                 pydoc: str):
> >           self._prefix = prefix
> >           self._what = what
> >           self._genc = QAPIGenC(self._prefix + self._what + '.c',
> > @@ -223,38 +229,42 @@ def __init__(self, prefix, what, blurb, pydoc):
> >           self._genh = QAPIGenH(self._prefix + self._what + '.h',
> >                                 blurb, pydoc)
> >   
> > -    def write(self, output_dir):
> > +    def write(self, output_dir: str) -> None:
> >           self._genc.write(output_dir)
> >           self._genh.write(output_dir)
> >   
> >   
> >   class QAPISchemaModularCVisitor(QAPISchemaVisitor):
> > -
> > -    def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
> > +    def __init__(self,
> > +                 prefix: str,
> > +                 what: str,
> > +                 user_blurb: str,
> > +                 builtin_blurb: Optional[str],
> > +                 pydoc: str):
> >           self._prefix = prefix
> >           self._what = what
> >           self._user_blurb = user_blurb
> >           self._builtin_blurb = builtin_blurb
> >           self._pydoc = pydoc
> > -        self._genc = None
> > -        self._genh = None
> > -        self._module = {}
> > -        self._main_module = None
> > +        self._genc: Optional[QAPIGenC] = None
> > +        self._genh: Optional[QAPIGenH] = None
> > +        self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
> > +        self._main_module: Optional[str] = None
> >   
> >       @staticmethod
> > -    def _is_user_module(name):
> > +    def _is_user_module(name: Optional[str]) -> bool:
> >           return bool(name and not name.startswith('./'))
> >   
> >       @staticmethod
> > -    def _is_builtin_module(name):
> > +    def _is_builtin_module(name: Optional[str]) -> bool:
> >           return not name
> >   
> > -    def _module_dirname(self, what, name):
> > +    def _module_dirname(self, what: str, name: Optional[str]) -> str:
> >           if self._is_user_module(name):
> >               return os.path.dirname(name)
> >           return ''
> >   
> > -    def _module_basename(self, what, name):
> > +    def _module_basename(self, what: str, name: Optional[str]) -> str:
> >           ret = '' if self._is_builtin_module(name) else self._prefix
> >           if self._is_user_module(name):
> >               basename = os.path.basename(name)
> > @@ -266,27 +276,27 @@ def _module_basename(self, what, name):
> >               ret += re.sub(r'-', '-' + name + '-', what)
> >           return ret
> >   
> > -    def _module_filename(self, what, name):
> > +    def _module_filename(self, what: str, name: Optional[str]) -> str:
> >           return os.path.join(self._module_dirname(what, name),
> >                               self._module_basename(what, name))
> >   
> > -    def _add_module(self, name, blurb):
> > +    def _add_module(self, name: Optional[str], blurb: str) -> None:
> >           basename = self._module_filename(self._what, name)
> >           genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
> >           genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
> >           self._module[name] = (genc, genh)
> >           self._genc, self._genh = self._module[name]
> >   
> > -    def _add_user_module(self, name, blurb):
> > +    def _add_user_module(self, name: str, blurb: str) -> None:
> >           assert self._is_user_module(name)
> >           if self._main_module is None:
> >               self._main_module = name
> >           self._add_module(name, blurb)
> >   
> > -    def _add_system_module(self, name, blurb):
> > +    def _add_system_module(self, name: Optional[str], blurb: str) -> None:
> >           self._add_module(name and './' + name, blurb)
> >   
> > -    def write(self, output_dir, opt_builtins=False):
> > +    def write(self, output_dir: str, opt_builtins: bool = False) -> None:
> >           for name in self._module:
> >               if self._is_builtin_module(name) and not opt_builtins:
> >                   continue
> > @@ -294,13 +304,13 @@ def write(self, output_dir, opt_builtins=False):
> >               genc.write(output_dir)
> >               genh.write(output_dir)
> >   
> > -    def _begin_system_module(self, name):
> > +    def _begin_system_module(self, name: None) -> None:
>
> I figure the type hint None signals a simplifcation opportunity.  No
> need to worry about it now.

Yes, the module name stuff in general has a smell to it. I didn't look 
deeper, but there's an opportunity for cleaning it up. I realize the 
type signature here has parity with _begin_user_module, but I noticed it 
was *never* called with anything that wasn't a literal None, so I added 
the stricter type.

> >           pass
> >   
> > -    def _begin_user_module(self, name):
> > +    def _begin_user_module(self, name: str) -> None:
> >           pass
> >   
> > -    def visit_module(self, name):
> > +    def visit_module(self, name: Optional[str]) -> None:
> >           if name is None:
> >               if self._builtin_blurb:
> >                   self._add_system_module(None, self._builtin_blurb)
> > @@ -314,7 +324,7 @@ def visit_module(self, name):
> >               self._add_user_module(name, self._user_blurb)
> >               self._begin_user_module(name)
> >   
> > -    def visit_include(self, name, info):
> > +    def visit_include(self, name: str, info: QAPISourceInfo) -> None:
> >           relname = os.path.relpath(self._module_filename(self._what, name),
> >                                     os.path.dirname(self._genh.fname))
> >           self._genh.preamble_add(mcgen('''