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

[John Snow]

On Tue, Feb 20, 2024 at 10:03 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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?

We need it now: to declare members without initializing it, I need to
declare a type. May as well choose the correct one. This assignment
needs to be the correct type.

>
> >
> >          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 :)
>