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

[Markus Armbruster]

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 canary for determining if check has
> completed.

Certain members become valid only after .check().  Two ways to code
that:

1. Assign to such members only in .check().  If you try to use them
before .check(), AttributeError.  Nice.  Drawback: pylint is unhappy,
W0201 attribute-defined-outside-init.

2. Assign None in .__init__(), and the real value in .check().  If you
try to use them before .check(), you get None, which hopefully leads to
an error.  Meh, but pylint is happy.

I picked 2. because pylint's warning made me go "when in Rome..."

With type hints, we can declare in .__init__(), and assign in .check().
Gives me the AttributeError I like, and pylint remains happy.  What do
you think?

Splitting .checked feels like a separate change, though.  I can't quite
see why we need it.  Help me out: what problem does it solve?

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

Yes, but when L is None, it's *invalid*, and for i in L *should* fail
when L is invalid.

I think the actual problem is something else.  By adding the type hint
Optional[List[T]], the valid uses of L become type errors.  We really
want L to be a List[T].  Doesn't mean we have to (or want to) make uses
of invalid L "work".

> RFC: are we guaranteed to have members here? can we just use "if
> members" without adding the new field?

I'm afraid I don't understand these questions.

> 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 164d86c4064..200bc0730d6 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -18,7 +18,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,
> @@ -447,22 +447,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] = []

Can we do

           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

If we assign only in .check(), we can't read self.members to find out
whether it's valid.  We could perhaps mess with .__dict__() instead.
Not sure it's worthwhile.  Dumb down the assertion?

>  
>          seen = OrderedDict()
>          if self._base_name:
> @@ -479,13 +481,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