Re: [PATCH v4 16/23] qapi/schema: Don't initialize "members" with `None`

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Thu, Mar 14, 2024, 9:01 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/_check_complete fields.)
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  scripts/qapi/schema.py | 12 +++++++-----
>> >  1 file changed, 7 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index 50ebc4f12de..fb30314741a 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,
>> > @@ -449,7 +449,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._check_complete = False
>> >
>> >      def check(self, schema):
>> > @@ -482,7 +482,11 @@ def check(self, schema):
>> >          for m in self.local_members:
>> >              m.check(schema)
>> >              m.check_clash(self.info, seen)
>> > -        members = seen.values()
>> > +
>> > +        # check_clash works in terms of the supertype, but local_members
>> > +        # is asserted to be List[QAPISchemaObjectTypeMember].
>>
>> Do you mean "but self.members is declared as
>> List[QAPISchemaObjectTypeMember]"?
>
> Argh. I meant asserted in the linguistic sense. mypy asserts it to be; not
> a runtime assertion.
>
> I do this a lot, apparently.

Okay to adjust your comment to my version?

[...]