Re: [PATCH v2 13/19] qapi/schema: split "checked" field into "checking" and "checked"

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> differentiate between "actively in the process of checking" and
> "checking has completed". This allows us to clean up the types of some
> internal fields such as QAPISchemaObjectType's members field which
> currently uses "None" as a test for determining if check has been run
> already or not.
>
> This simplifies the typing from a cumbersome Optional[List[T]] to merely
> a List[T], which is more pythonic: it is safe to iterate over an empty
> list with "for x in []" whereas with an Optional[List[T]] you have to
> rely on the more cumbersome "if L: for x in L: ..."

Does this cumbersome form exist?

> Note that it is valid to have an empty members list, see the internal
> q_empty object as an example.

Yes.

.members becomes valid only in .check().  Before the patch, .__init__()
initializes it to None, and .check() sets it to the real value.  We use
assert .members is not None to catch invalid use.  We can also hope
invalid use without an assert would crash.  for m in .members would.

We've seen this pattern before: PATCH 4+5.  There, we change .__init__()
to declare the attribute without initializing it.  Use before it becomes
valid now certainly crashes, which is an improvement.  Why can't we do
the same here?

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/schema.py | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index eefa58a798b..0d9a70ab4cb 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,22 +457,24 @@ 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):
>          # This calls another type T's .check() exactly when the C
>          # struct emitted by gen_object() contains that T's C struct
>          # (pointers don't count).
> -        if self.members is not None:
> -            # A previous .check() completed: nothing to do
> -            return
> -        if self._checked:
> +        if self._checking:
>              # Recursed: C struct contains itself
>              raise QAPISemError(self.info,
>                                 "object %s contains itself" % self.name)
> +        if self._checked:
> +            # A previous .check() completed: nothing to do
> +            return
>  
> +        self._checking = True
>          super().check(schema)
> -        assert self._checked and self.members is None
> +        assert self._checked and not self.members
>  
>          seen = OrderedDict()
>          if self._base_name:
> @@ -489,13 +491,17 @@ 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.
> +        members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))
>  
>          if self.variants:
>              self.variants.check(schema, seen)
>              self.variants.check_clash(self.info, seen)
>  
> -        self.members = members  # mark completed
> +        self.members = members
> +        self._checking = False  # mark completed
>  
>      # Check that the members of this type do not cause duplicate JSON members,
>      # and update seen to track the members seen so far. Report any errors

I think you missed these:

       def is_empty(self):
           assert self.members is not None
           return not self.members and not self.variants

       def has_conditional_members(self):
           assert self.members is not None
           return any(m.ifcond.is_present() for m in self.members)

The assertions no longer work.  I figure you want to assert .checked
instead.

Consider splitting the patch: first add .checking, and replace use of
.members by use of .checking where possible.  Then change .members.  The
split may or may not be easier to describe and digest.  Use your
judgement.