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 canary for determining if check has
> completed.
Certain members become valid only after .check(). Two ways to code
that:
1. Assign to such members only in .check(). If you try to use them
before .check(), AttributeError. Nice. Drawback: pylint is unhappy,
W0201 attribute-defined-outside-init.
2. Assign None in .__init__(), and the real value in .check(). If you
try to use them before .check(), you get None, which hopefully leads to
an error. Meh, but pylint is happy.
I picked 2. because pylint's warning made me go "when in Rome..."
With type hints, we can declare in .__init__(), and assign in .check().
Gives me the AttributeError I like, and pylint remains happy. What do
you think?
Splitting .checked feels like a separate change, though. I can't quite
see why we need it. Help me out: what problem does it solve?
> 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: ..."
Yes, but when L is None, it's *invalid*, and for i in L *should* fail
when L is invalid.
I think the actual problem is something else. By adding the type hint
Optional[List[T]], the valid uses of L become type errors. We really
want L to be a List[T]. Doesn't mean we have to (or want to) make uses
of invalid L "work".
> RFC: are we guaranteed to have members here? can we just use "if
> members" without adding the new field?
I'm afraid I don't understand these questions.
> 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 164d86c4064..200bc0730d6 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -18,7 +18,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,
> @@ -447,22 +447,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] = []
Can we do
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
If we assign only in .check(), we can't read self.members to find out
whether it's valid. We could perhaps mess with .__dict__() instead.
Not sure it's worthwhile. Dumb down the assertion?
>
> seen = OrderedDict()
> if self._base_name:
> @@ -479,13 +481,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
