Re: [PATCH v3 14/20] qapi/schema: Don't initialize "members" with `None`

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> Declare, but don't initialize the "members" field with type
> List[QAPISchemaObjectTypeMember].
>
> This simplifies the typing from what would otherwise be
> Optional[List[T]] to merely List[T]. This removes the need to add
> assertions to several callsites that this value is not None - which it
> never will be after the delayed initialization in check() anyway.
>
> The type declaration without initialization trick will cause accidental
> uses of this field prior to full initialization to raise an
> AttributeError.
>
> (Note that it is valid to have an empty members list, see the internal
> q_empty object as an example. For this reason, we cannot use the empty
> list as a replacement test for full initialization and instead rely on
> the _checked/_checking fields.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/schema.py | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index a459016e148..947e7efb1a8 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -20,7 +20,7 @@
>  from collections import OrderedDict
>  import os
>  import re
> -from typing import List, Optional
> +from typing import List, Optional, cast
>  
>  from .common import (
>      POINTER_SUFFIX,
> @@ -457,7 +457,7 @@ def __init__(self, name, info, doc, ifcond, features,
>          self.base = None
>          self.local_members = local_members
>          self.variants = variants
> -        self.members = None
> +        self.members: List[QAPISchemaObjectTypeMember]
>          self._checking = False
>  
>      def check(self, schema):
> @@ -474,7 +474,7 @@ def check(self, schema):
>  
>          self._checking = True
>          super().check(schema)
> -        assert self._checked and self.members is None
> +        assert self._checked

This asserts state is "2. Being checked:.

The faithful update would be

           assert self._checked and self._checking

Or with my alternative patch

           assert self._checked and not self._check_complete
>  
>          seen = OrderedDict()
>          if self._base_name:
> @@ -491,7 +491,10 @@ def check(self, schema):
>          for m in self.local_members:
>              m.check(schema)
>              m.check_clash(self.info, seen)
> -        members = seen.values()
> +
> +        # check_clash is abstract, but local_members is asserted to be
> +        # List[QAPISchemaObjectTypeMember]. Cast to the narrower type.

What do you mean by "check_clash is abstract"?

> +        members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))

Do we actually need this *now*, or only later when we have more type
hints?

>  
>          if self.variants:
>              self.variants.check(schema, seen)
> @@ -524,11 +527,9 @@ def is_implicit(self):
>          return self.name.startswith('q_')
>  
>      def is_empty(self):
> -        assert self.members is not None

This asserts state is "3. Checked".

The faithful update would be

           assert self._checked and not self._checking

Or with my alternative patch

           assert self._check_complete

>          return not self.members and not self.variants
>  
>      def has_conditional_members(self):
> -        assert self.members is not None

Likewise.

>          return any(m.ifcond.is_present() for m in self.members)
>  
>      def c_name(self):

This patch does two things:

1. Replace test of self.members (enabled by the previous patch)

2. Drop initialization of self.members and simplify the typing

Observation, not demand.  Wouldn't *mind* a split, though :)