On Thu, Mar 14, 2024, 9:01 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@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 <js...@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.
> > + # Cast down to the subtype.
> > + members = cast(List[QAPISchemaObjectTypeMember],
> list(seen.values()))
>
> Let's break the line after the comma.
>
Go for it.
After this series I may send a patchset showing what changes black would
make. I cannot be trusted with aesthetic consistency.
> >
> > if self.variants:
> > self.variants.check(schema, seen)
> > @@ -515,11 +519,9 @@ def is_implicit(self):
> > return self.name.startswith('q_')
> >
> > 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)
> >
> > def c_name(self):
>
>
