diff mbox series

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

Message ID 20201005195158.2348217-28-jsnow@redhat.com
State New, archived
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Oct. 5, 2020, 7:51 p.m. UTC
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(-)

Comments

Markus Armbruster Oct. 7, 2020, 12:21 p.m. UTC | #1
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?

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

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

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

>          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('''
Markus Armbruster Oct. 7, 2020, 1:20 p.m. UTC | #2
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

PATCH 03 has a similar cleanup.  Are there more?  Perhaps a separate
patch doing just this kind of cleanup would make sense.  Up to you.

[...]
John Snow Oct. 7, 2020, 4:21 p.m. UTC | #3
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('''
John Snow Oct. 7, 2020, 4:50 p.m. UTC | #4
On 10/7/20 9:20 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
> 
> PATCH 03 has a similar cleanup.  Are there more?  Perhaps a separate
> patch doing just this kind of cleanup would make sense.  Up to you.
> 
> [...]
> 

This isn't a cleanup, I am just importing QAPISourceInfo to use for an 
annotation. It's relevant and required for this patch, and doesn't make 
sense on its own.

Patch 03 ... Oh, you mean identifying the correct location of QAPIError. 
Uh... nah? I think that was the only case of that one changing. Not 
worth pulling out or naming, I think.
Markus Armbruster Oct. 8, 2020, 8:44 a.m. UTC | #5
John Snow <jsnow@redhat.com> writes:

> On 10/7/20 9:20 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
>> PATCH 03 has a similar cleanup.  Are there more?  Perhaps a separate
>> patch doing just this kind of cleanup would make sense.  Up to you.
>> [...]
>> 
>
> This isn't a cleanup, I am just importing QAPISourceInfo to use for an
> annotation. It's relevant and required for this patch, and doesn't
> make sense on its own.

I was mistaken.  A case of patch-review-eye...

> Patch 03 ... Oh, you mean identifying the correct location of
> QAPIError. Uh... nah? I think that was the only case of that one
> changing. Not worth pulling out or naming, I think.

Agree.
diff mbox series

Patch

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]):
         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]):
         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):
         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]:
     """
     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:
         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('''