Re: [PATCH v5 24/36] qapi/source.py: add type hint annotations

[John Snow]

On 10/7/20 7:55 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > Annotations do not change runtime behavior.
> > This commit *only* adds annotations.
> >
> > A note on typing of __init__: mypy requires init functions with no
> > parameters to document a return type of None to be considered fully
> > typed. In the case when there are input parameters, None may be omitted.
> >
> > Since __init__ may never return any value, it is preferred to omit the
> > return annotation whenever possible.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > Reviewed-by: Cleber Rosa <crosa@redhat.com>
> > Tested-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   scripts/qapi/mypy.ini  |  5 -----
> >   scripts/qapi/source.py | 31 ++++++++++++++++++-------------
> >   2 files changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> > index 8ab9ac52cc4..1b8555dfa39 100644
> > --- a/scripts/qapi/mypy.ini
> > +++ b/scripts/qapi/mypy.ini
> > @@ -34,11 +34,6 @@ disallow_untyped_defs = False
> >   disallow_incomplete_defs = False
> >   check_untyped_defs = False
> >   
> > -[mypy-qapi.source]
> > -disallow_untyped_defs = False
> > -disallow_incomplete_defs = False
> > -check_untyped_defs = False
> > -
> >   [mypy-qapi.types]
> >   disallow_untyped_defs = False
> >   disallow_incomplete_defs = False
> > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> > index e97b9a8e15e..1cc6a5b82dc 100644
> > --- a/scripts/qapi/source.py
> > +++ b/scripts/qapi/source.py
> > @@ -11,37 +11,42 @@
> >   
> >   import copy
> >   import sys
> > +from typing import List, Optional, TypeVar
> >   
> >   
> >   class QAPISchemaPragma:
> > -    def __init__(self):
> > +    def __init__(self) -> None:
> >           # Are documentation comments required?
> >           self.doc_required = False
> >           # Whitelist of commands allowed to return a non-dictionary
> > -        self.returns_whitelist = []
> > +        self.returns_whitelist: List[str] = []
> >           # Whitelist of entities allowed to violate case conventions
> > -        self.name_case_whitelist = []
> > +        self.name_case_whitelist: List[str] = []
> >   
> >   
> >   class QAPISourceInfo:
> > -    def __init__(self, fname, line, parent):
> > +    T = TypeVar('T', bound='QAPISourceInfo')
> > +
> > +    def __init__(self: T, fname: str, line: int, parent: Optional[T]):
>
> More ignorant questions (I'm abusing the review process to learn Python
> type hints)...
>
> Why do you need to annotate self here, but not elsewhere?

This is admittedly me being a little extra, but I thought it was a good 
way to show a pattern for people who maybe hadn't been exposed to it yet.

This is a pattern that allows for subclassing. I am stating that this 
__init__ method takes a parent of the same type as itself, whatever that 
happens to actually be.

T is a TypeVar that we can use for Generics. By declaring the type of 
self as that TypeVar, we bind T to self's type. When we annotate the 
parent as a T, we are enforcing that the parent we receive is of the 
same type as ourselves.

(Yep, we don't actually subclass QAPISourceInfo and I don't have plans 
to. It felt slightly icky to hard-code the class type name, though. I 
try to avoid that whenever I can. I'm not sure I would go so far as to 
call it a code smell or an antipattern, but it's something I tend to 
avoid anyway.)

> Why do you use T instead of QAPISourceInfo?
>
> >           self.fname = fname
> >           self.line = line
> >           self.parent = parent
> > -        self.pragma = parent.pragma if parent else QAPISchemaPragma()
> > -        self.defn_meta = None
> > -        self.defn_name = None
> > +        self.pragma: QAPISchemaPragma = (
> > +            parent.pragma if parent else QAPISchemaPragma()
> > +        )
>
> Type inference fail?

Yes:

qapi/source.py:34: error: Cannot determine type of 'pragma'
Found 1 error in 1 file (checked 14 source files)

> > +        self.defn_meta: Optional[str] = None
> > +        self.defn_name: Optional[str] = None
> >   
> > -    def set_defn(self, meta, name):
> > +    def set_defn(self, meta: str, name: str) -> None:
> >           self.defn_meta = meta
> >           self.defn_name = name
> >   
> > -    def next_line(self):
> > +    def next_line(self: T) -> T:
> >           info = copy.copy(self)
> >           info.line += 1
> >           return info
> >   
> > -    def loc(self):
> > +    def loc(self) -> str:
> >           if self.fname is None:
> >               return sys.argv[0]
> >           ret = self.fname
> > @@ -49,13 +54,13 @@ def loc(self):
> >               ret += ':%d' % self.line
> >           return ret
> >   
> > -    def in_defn(self):
> > +    def in_defn(self) -> str:
> >           if self.defn_name:
> >               return "%s: In %s '%s':\n" % (self.fname,
> >                                             self.defn_meta, self.defn_name)
> >           return ''
> >   
> > -    def include_path(self):
> > +    def include_path(self) -> str:
> >           ret = ''
> >           parent = self.parent
> >           while parent:
> > @@ -63,5 +68,5 @@ def include_path(self):
> >               parent = parent.parent
> >           return ret
> >   
> > -    def __str__(self):
> > +    def __str__(self) -> str:
> >           return self.include_path() + self.in_defn() + self.loc()