Re: [PATCH 18/19] qapi/schema: remove unnecessary asserts

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> With strict typing enabled, these runtime statements aren't necessary
> anymore.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/schema.py | 23 -----------------------
>  1 file changed, 23 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 5d19b59def0..b5f377e68b8 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -78,9 +78,7 @@ def __init__(
   class QAPISchemaEntity:
       meta: str

       def __init__(
           self,
           name: str,
           info: Optional[QAPISourceInfo],
           doc: Optional[QAPIDoc],
>          ifcond: Optional[QAPISchemaIfCond] = None,
>          features: Optional[List[QAPISchemaFeature]] = None,
>      ):
> -        assert name is None or isinstance(name, str)

Yup, because name: str.

>          for f in features or []:
> -            assert isinstance(f, QAPISchemaFeature)

Yup, because features: Optional[List[QAPISchemaFeature]].

>              f.set_defined_in(name)
>          self.name = name
>          self._module: Optional[QAPISchemaModule] = None
> @@ -145,7 +143,6 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
>          assert self._checked
>  
>      def describe(self) -> str:
> -        assert self.meta

Yup, because QAPISchemaEntity has meta: str.

>          return "%s '%s'" % (self.meta, self.name)
>  
>  
> @@ -359,7 +356,6 @@ def check(self, schema: QAPISchema) -> None:
>                      f"feature '{feat.name}' is not supported for types")
>  
>      def describe(self) -> str:
> -        assert self.meta

Likewise.

>          return "%s type '%s'" % (self.meta, self.name)
>  
>  
> @@ -368,7 +364,6 @@ class QAPISchemaBuiltinType(QAPISchemaType):
   class QAPISchemaBuiltinType(QAPISchemaType):
       meta = 'built-in'
>  
>      def __init__(self, name: str, json_type: str, c_type: str):
>          super().__init__(name, None, None)
> -        assert not c_type or isinstance(c_type, str)

Yup, because c_type: str.

Odd: the assertion accepts None, but the type doesn't.  Turns out None
was possible until commit 2d21291ae64 (qapi: Pseudo-type '**' is now
unused, drop it).  The assertion should have been adjusted then.

Probably not worth a commit message mention now.

>          assert json_type in ('string', 'number', 'int', 'boolean', 'null',
>                               'value')
>          self._json_type_name = json_type
> @@ -411,9 +406,7 @@ def __init__(
   class QAPISchemaEnumType(QAPISchemaType):
       meta = 'enum'

       def __init__(
           self,
           name: str,
           info: Optional[QAPISourceInfo],
           doc: Optional[QAPIDoc],
           ifcond: Optional[QAPISchemaIfCond],
           features: Optional[List[QAPISchemaFeature]],
           members: List[QAPISchemaEnumMember],
           prefix: Optional[str],
>      ):
>          super().__init__(name, info, doc, ifcond, features)
>          for m in members:
> -            assert isinstance(m, QAPISchemaEnumMember)

Yup, because members: List[QAPISchemaEnumMember].

>              m.set_defined_in(name)
> -        assert prefix is None or isinstance(prefix, str)

Yup, because prefix: Optional[str].

>          self.members = members
>          self.prefix = prefix
>  
> @@ -456,7 +449,6 @@ def __init__(
   class QAPISchemaArrayType(QAPISchemaType):
       meta = 'array'

       def __init__(
>          self, name: str, info: Optional[QAPISourceInfo], element_type: str
>      ):
>          super().__init__(name, info, None)
> -        assert isinstance(element_type, str)

Yup, because element_type: str.

>          self._element_type_name = element_type
>          self._element_type: Optional[QAPISchemaType] = None
>  
> @@ -517,7 +509,6 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
>                                   self.element_type)
>  
>      def describe(self) -> str:
> -        assert self.meta

Yup, because QAPISchemaEntity has meta: str.

>          return "%s type ['%s']" % (self.meta, self._element_type_name)
>  
>  
> @@ -537,12 +528,9 @@ def __init__(
   class QAPISchemaObjectType(QAPISchemaType):
       def __init__(
           self,
           name: str,
           info: Optional[QAPISourceInfo],
           doc: Optional[QAPIDoc],
           ifcond: Optional[QAPISchemaIfCond],
           features: Optional[List[QAPISchemaFeature]],
           base: Optional[str],
           local_members: List[QAPISchemaObjectTypeMember],
           variants: Optional[QAPISchemaVariants],
       ):
           # struct has local_members, optional base, and no variants
>          # union has base, variants, and no local_members
>          super().__init__(name, info, doc, ifcond, features)
>          self.meta = 'union' if variants else 'struct'
> -        assert base is None or isinstance(base, str)

Yup, because base: Optional[str].

>          for m in local_members:
> -            assert isinstance(m, QAPISchemaObjectTypeMember)

Yup, because local_members: List[QAPISchemaObjectTypeMember].

>              m.set_defined_in(name)
>          if variants is not None:
> -            assert isinstance(variants, QAPISchemaVariants)

Yup, because variants: Optional[QAPISchemaVariants]

>              variants.set_defined_in(name)
>          self._base_name = base
>          self.base = None
> @@ -666,7 +654,6 @@ def __init__(
   class QAPISchemaAlternateType(QAPISchemaType):
       meta = 'alternate'

       def __init__(
           self,
           name: str,
           info: QAPISourceInfo,
           doc: Optional[QAPIDoc],
           ifcond: Optional[QAPISchemaIfCond],
           features: List[QAPISchemaFeature],
>          variants: QAPISchemaVariants,
>      ):
>          super().__init__(name, info, doc, ifcond, features)
> -        assert isinstance(variants, QAPISchemaVariants)

Yup, because variants: QAPISchemaVariants.

>          assert variants.tag_member
>          variants.set_defined_in(name)
>          variants.tag_member.set_defined_in(self.name)
> @@ -742,8 +729,6 @@ def __init__(
   class QAPISchemaVariants:
       def __init__(
           self,
           tag_name: Optional[str],
           info: QAPISourceInfo,
           tag_member: Optional[QAPISchemaObjectTypeMember],
           variants: List[QAPISchemaVariant],
       ):
           # Unions pass tag_name but not tag_member.
           # Alternates pass tag_member but not tag_name.
           # After check(), tag_member is always set.
>          assert bool(tag_member) != bool(tag_name)
>          assert (isinstance(tag_name, str) or
>                  isinstance(tag_member, QAPISchemaObjectTypeMember))
> -        for v in variants:
> -            assert isinstance(v, QAPISchemaVariant)

Yup, because variants: List[QAPISchemaVariant].

>          self._tag_name = tag_name
>          self.info = info
>          self._tag_member = tag_member
> @@ -856,7 +841,6 @@ def __init__(
   class QAPISchemaMember:
       """ Represents object members, enum members and features """
       role = 'member'

       def __init__(
           self,
           name: str,
>          info: Optional[QAPISourceInfo],
>          ifcond: Optional[QAPISchemaIfCond] = None,
>      ):
> -        assert isinstance(name, str)

Yup, because name: str.

>          self.name = name
>          self.info = info
>          self.ifcond = ifcond or QAPISchemaIfCond()
> @@ -924,7 +908,6 @@ def __init__(
   class QAPISchemaEnumMember(QAPISchemaMember):
       role = 'value'

       def __init__(
           self,
           name: str,
           info: QAPISourceInfo,
           typ: str,
           optional: bool,
           ifcond: Optional[QAPISchemaIfCond] = None,
           features: Optional[List[QAPISchemaFeature]] = None,
>      ):
>          super().__init__(name, info, ifcond)
>          for f in features or []:
> -            assert isinstance(f, QAPISchemaFeature)

Yup, because features: Optional[List[QAPISchemaFeature]].

>              f.set_defined_in(name)
>          self.features = features or []
>  
> @@ -953,10 +936,7 @@ def __init__(
   class QAPISchemaObjectTypeMember(QAPISchemaMember):
       def __init__(
           self,
           name: str,
           info: QAPISourceInfo,
           typ: str,
           optional: bool,
           ifcond: Optional[QAPISchemaIfCond] = None,
>          features: Optional[List[QAPISchemaFeature]] = None,
>      ):
>          super().__init__(name, info, ifcond)
> -        assert isinstance(typ, str)

Yup, because typ: str.

> -        assert isinstance(optional, bool)

Yup, because optional: bool.

>          for f in features or []:
> -            assert isinstance(f, QAPISchemaFeature)

Yup, because features: Optional[List[QAPISchemaFeature]].

>              f.set_defined_in(name)
>          self._type_name = typ
>          self.type: QAPISchemaType  # set during check(). Kind of hokey.
> @@ -1015,8 +995,6 @@ def __init__(
   class QAPISchemaCommand(QAPISchemaEntity):
       meta = 'command'

       def __init__(
           self,
           name: str,
           info: QAPISourceInfo,
           doc: Optional[QAPIDoc],
           ifcond: QAPISchemaIfCond,
           features: List[QAPISchemaFeature],
           arg_type: Optional[str],
           ret_type: Optional[str],
           gen: bool,
           success_response: bool,
           boxed: bool,
           allow_oob: bool,
           allow_preconfig: bool,
>          coroutine: bool,
>      ):
>          super().__init__(name, info, doc, ifcond, features)
> -        assert not arg_type or isinstance(arg_type, str)

Yup, because arg_type: Optional[str].

> -        assert not ret_type or isinstance(ret_type, str)

Yup, because ret_type: Optional[str].

>          self._arg_type_name = arg_type
>          self.arg_type: Optional[QAPISchemaObjectType] = None
>          self._ret_type_name = ret_type
> @@ -1093,7 +1071,6 @@ def __init__(
   class QAPISchemaEvent(QAPISchemaEntity):
       meta = 'event'

       def __init__(
           self,
           name: str,
           info: QAPISourceInfo,
           doc: Optional[QAPIDoc],
           ifcond: QAPISchemaIfCond,
           features: List[QAPISchemaFeature],
           arg_type: Optional[str],
           boxed: bool,
       ):
           super().__init__(name, info, doc, ifcond, features)
           assert not arg_type or isinstance(arg_type, str)
           self._arg_type_name = arg_type
           self.arg_type: Optional[QAPISchemaObjectType] = None
>          boxed: bool,
>      ):
>          super().__init__(name, info, doc, ifcond, features)
> -        assert not arg_type or isinstance(arg_type, str)

Yup, because arg_type: Optional[str].

>          self._arg_type_name = arg_type
>          self.arg_type: Optional[QAPISchemaObjectType] = None
>          self.boxed = boxed

Reviewed-by: Markus Armbruster <armbru@redhat.com>