| Message ID | 20240201224246.39480-15-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/_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? > > 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 :)
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? > > > > > 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. Do we even need these assertions, though? If members isn't set, it's gonna crash anyway. I don't think you actually need them at all. I think it's fine to leave these changes in this patch and to remove the assertion as it no longer really guards anything. > > > 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 :) > I think maybe one of the assertions can be replaced in the previous patch, but I think everything else does really belong in this patch. --js
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 :) >
John Snow <jsnow@redhat.com> writes: > 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? >> >> > >> > 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. > > Do we even need these assertions, though? If members isn't set, it's > gonna crash anyway. I don't think you actually need them at all. I > think it's fine to leave these changes in this patch and to remove the > assertion as it no longer really guards anything. When I wrote my review comment, my mind was running on "mechanical transformation" rails: "the faithful update would be". The "in state 3" predicate is .members is not None before the patch, and ._check_complete afterwards. You're right, the assertion no longer guards. It can at best aid understanding the code. Feel free to drop it. Would it make sense to explain the transformation of the "in state N" predicates briefly in the commit message? >> > 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 :) >> > > I think maybe one of the assertions can be replaced in the previous > patch, but I think everything else does really belong in this patch. My observation is that this patch could be split in two, not that parts of it belong to the previous patch.
On Wed, Mar 13, 2024, 2:57 AM Markus Armbruster <armbru@redhat.com> wrote: > John Snow <jsnow@redhat.com> writes: > > > 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? > >> > >> > > >> > 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. > > > > Do we even need these assertions, though? If members isn't set, it's > > gonna crash anyway. I don't think you actually need them at all. I > > think it's fine to leave these changes in this patch and to remove the > > assertion as it no longer really guards anything. > > When I wrote my review comment, my mind was running on "mechanical > transformation" rails: "the faithful update would be". The "in state 3" > predicate is .members is not None before the patch, and ._check_complete > afterwards. > > You're right, the assertion no longer guards. It can at best aid > understanding the code. Feel free to drop it. > I did. I felt it was relevant here instead of in the drop assertions patch (at the end of the series) because it wasn't doing type narrowing, it's checking for a value that can truly not occur anymore as of this patch. > Would it make sense to explain the transformation of the "in state N" > predicates briefly in the commit message? > The wha...? Oh, if you'd like your invariant analysis in the commit message, please be my guest and add it with my blessing. > >> > 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 :) > >> > > > > I think maybe one of the assertions can be replaced in the previous > > patch, but I think everything else does really belong in this patch. > > My observation is that this patch could be split in two, not that parts > of it belong to the previous patch. > What I did was change the assertions in the previous patch (including moving one from this patch backwards one) such that comparisons to None are nearly eliminated (the two assertions in is_empty and has_conditional_members remain, however.) then this patch: - changes initialization to declaration by adding the type hint (and removing the None initial value) - adds the cast to the seen dict - removes the two assertions For mypy not to regress mid-series, I *believe* all of those above items cannot be split. The patch has become pretty small... (If you remove the None assertions in the two predicate methods, I think you either run into a type checking problem *or* you just have a commit that technically has a bug. Either way, it seems most cohesive to defer that particular change.) If you believe it is still semantically two changes, you're welcome to adjust the commit message or attempt to split it yourself, but I don't think I see what you do, sorry. --js
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 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. + members = cast(List[QAPISchemaObjectTypeMember], list(seen.values())) 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 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/_checking fields.) Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/schema.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)