| Message ID | 20240313044127.49089-17-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
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]"? > + # Cast down to the subtype. > + members = cast(List[QAPISchemaObjectTypeMember], list(seen.values())) Let's break the line after the comma. > > 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):
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. > > + # 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): > >
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? [...]
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! > [...] > >
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]. + # Cast down to the subtype. + members = cast(List[QAPISchemaObjectTypeMember], list(seen.values())) 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):
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(-)