On Thu, Feb 1, 2024 at 2:41 PM John Snow <js...@redhat.com> wrote: > > On Tue, Jan 16, 2024 at 9:58 AM Markus Armbruster <arm...@redhat.com> wrote: > > > > John Snow <js...@redhat.com> writes: > > > > > differentiate between "actively in the process of checking" and > > > "checking has completed". This allows us to clean up the types of some > > > internal fields such as QAPISchemaObjectType's members field which > > > currently uses "None" as a test for determining if check has been run > > > already or not. > > > > > > This simplifies the typing from a cumbersome Optional[List[T]] to merely > > > a List[T], which is more pythonic: it is safe to iterate over an empty > > > list with "for x in []" whereas with an Optional[List[T]] you have to > > > rely on the more cumbersome "if L: for x in L: ..." > > > > Does this cumbersome form exist? > > I thought it should, but I suppose it shouldn't given that you have > been relying on None to cause a crash. > > Here's where that pattern *would* be used if None was legitimate: > > qapi/schema.py:604: error: Item "None" of > "Optional[List[QAPISchemaObjectTypeMember]]" has no attribute > "__iter__" (not iterable) [union-attr] > qapi/schema.py:626: error: Item "None" of > "Optional[List[QAPISchemaObjectTypeMember]]" has no attribute > "__iter__" (not iterable) [union-attr] > qapi/gen.py:122: error: Item "None" of > "Optional[List[QAPISchemaObjectTypeMember]]" has no attribute > "__iter__" (not iterable) [union-attr] > qapi/commands.py:68: error: Item "None" of > "Optional[List[QAPISchemaObjectTypeMember]]" has no attribute > "__iter__" (not iterable) [union-attr] > qapi/events.py:60: error: Item "None" of > "Optional[List[QAPISchemaObjectTypeMember]]" has no attribute > "__iter__" (not iterable) [union-attr] > > but, I suppose your argument is that it just literally never is. So > never mind the callsite argument - though mypy still wants its > guarantee that it will never be None here. > > > > > > Note that it is valid to have an empty members list, see the internal > > > q_empty object as an example. > > > > Yes. > > > > .members becomes valid only in .check(). Before the patch, .__init__() > > initializes it to None, and .check() sets it to the real value. We use > > assert .members is not None to catch invalid use. We can also hope > > invalid use without an assert would crash. for m in .members would. > > > > Mmm, I see. (I very often just literally don't account for you relying > on invalid types being load-bearing ... Seeing a stack trace where > you're told that "None" does not have such-and-such property usually > feels about as helpful as getting kicked out of a moving car on the > freeway.) > > > We've seen this pattern before: PATCH 4+5. There, we change .__init__() > > to declare the attribute without initializing it. Use before it becomes > > valid now certainly crashes, which is an improvement. Why can't we do > > the same here? > > Didn't occur to me to add a crash on purpose... In the other cases, I > think I didn't have any suitable value to add at all, but in this case > I can use an empty list. > > (I have a kind of distaste for relying on Python-level exceptions like > AttributeError to indicate that a stateful error has occurred - i.e. > that this attribute doesn't exist yet, but it will. I usually aim for > having all of the attributes defined at initialization time. The error > is misleading, semantically. > > In our case, full initialization directly at init time is not ... > quite possible without some vigorous reworking of the known universe, > so I capitulated and allowed some "very late initialization" which > causes some friction for static typing between pre- and post- "delayed > initialization" callsites. It's still very much something I look at > and regard as a code smell. The entire point of declaring all state in > the init method is to centralize that state so it's finite and > knowable, both to static analysis tools and to humans. Punting the > initialization to arbitrary points later in the object's lifetime > feels like lying: we promise this value is here after initialization, > except not really, have a nice day.) > > *cough* that said, I can also use that same trick here. I just want to > whine about it. (I guess I don't want to teach folks what I consider > to be a bad habit just because it makes the linters shut up, but > realize the sausage has to get made.) > > ... I *just* now saw you had more ideas on how to approach this in the > last series, I will go back and read them. I didn't mean to skip past > them. Lemme investigate. > > > > > > Signed-off-by: John Snow <js...@redhat.com> > > > --- > > > scripts/qapi/schema.py | 24 +++++++++++++++--------- > > > 1 file changed, 15 insertions(+), 9 deletions(-) > > > > > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > > index eefa58a798b..0d9a70ab4cb 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,22 +457,24 @@ 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): > > > # This calls another type T's .check() exactly when the C > > > # struct emitted by gen_object() contains that T's C struct > > > # (pointers don't count). > > > - if self.members is not None: > > > - # A previous .check() completed: nothing to do > > > - return > > > - if self._checked: > > > + if self._checking: > > > # Recursed: C struct contains itself > > > raise QAPISemError(self.info, > > > "object %s contains itself" % self.name) > > > + if self._checked: > > > + # A previous .check() completed: nothing to do > > > + return > > > > > > + self._checking = True > > > super().check(schema) > > > - assert self._checked and self.members is None > > > + assert self._checked and not self.members > > > > > > seen = OrderedDict() > > > if self._base_name: > > > @@ -489,13 +491,17 @@ 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) > > > self.variants.check_clash(self.info, seen) > > > > > > - self.members = members # mark completed > > > + self.members = members > > > + self._checking = False # mark completed > > > > > > # Check that the members of this type do not cause duplicate JSON > > > members, > > > # and update seen to track the members seen so far. Report any errors > > > > I think you missed these: > > > > 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) > > > > The assertions no longer work. I figure you want to assert .checked > > instead. > > > > Consider splitting the patch: first add .checking, and replace use of > > .members by use of .checking where possible. Then change .members. The > > split may or may not be easier to describe and digest. Use your > > judgement. > > Ah, oops.
Oh, I got it in the assertions patch, but have since moved it forward. > > (I wish mypy would bark about this, actually: if members is a List, > then surely it can't ever be None, right? It's definitely a smell. I > wonder if there is a reason why it doesn't, or if this is a valid > feature request.) > > --js
