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

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2940 bytes --]

On Thu, Mar 14, 2024, 9:58 AM Markus Armbruster <armbru@redhat.com> wrote:

> 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?
>

Absolutely, of course!


> [...]
>
>

[-- Attachment #2: Type: text/html, Size: 4574 bytes --]