Re: [PATCH v2 04/19] qapi/schema: declare type for QAPISchemaObjectTypeMember.type

[John Snow <jsnow@redhat.com>]

On Mon, Jan 15, 2024 at 8:53 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > declare, but don't initialize the type of "type" to be QAPISchemaType -
>
> Declare
>
> > and allow the value to be initialized during check(). This creates a
> > form of delayed initialization for QAPISchemaType objects where the
> > static typing only represents the fully-realized object, which occurs
> > after check() has been called.
> >
> > This avoids the need for several "assert type is not None" statements
> > littered throughout the code by asserting it "will always be set."
>
> Uh, I find "will always be set" confusing.
>
> I think you mean "cannot be None".

Yuh

>
> But "be set" can also be read as "will have a value".

...yuh

>
> It actually doesn't exist until .check(), and once it exists, it cannot
> be None.
>
> > Note that the static typing information for this object will be
> > incorrect prior to check() being called. If this field is accessed
> > before it is initialized in check(), you'll be treated to an
> > AttributeError exception.
>
> We could now enter a philosophical debate on whether the static typing
> is actually incorrect.  Clearly v: T asserts that the value of @v is
> always a T, and the type checker proves this.  Does it also assert that
> @v exists?  The type checker doesn't care, and that's a feature.

*clutches TAPL and cries*

>
> Maybe start with the problem, and then develop the solution:
>
>   A QAPISchemaObjectTypeMember's type gets resolved only during .check().
>   We have QAPISchemaObjectTypeMember.__init__() initialize self.type =
>   None, and .check() assign the actual type.  Using .type before .check()
>   is wrong, and hopefully crashes due to the value being None.  Works.
>
>   However, it makes for awkward typing.  With .type:
>   Optional[QAPISchemaType], mypy is of course unable to see that it's None
>   before .check(), and a QAPISchemaType after.  To help it over the hump,
>   we'd have to assert self.type is not None before all the (valid) uses.
>   The assertion catches invalid uses, but only at run time; mypy can't
>   flag them.
>
>   Instead, declare .type in .__init__() as QAPISchemaType *without*
>   initializing it.  Using .type before .check() now certainly crashes,
>   which is an improvement.  Mypy still can't flag invalid uses, but that's
>   okay.
>

OK.

--js

> > Fixes stuff like this:
> >
> > qapi/schema.py:657: error: "None" has no attribute "alternate_qtype"  [attr-defined]
> > qapi/schema.py:662: error: "None" has no attribute "describe"  [attr-defined]
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/schema.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index e39ed972a80..48a51dcd188 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -794,7 +794,7 @@ def __init__(self, name, info, typ, optional, ifcond=None, features=None):
> >              assert isinstance(f, QAPISchemaFeature)
> >              f.set_defined_in(name)
> >          self._type_name = typ
> > -        self.type = None
> > +        self.type: QAPISchemaType  # set during check()
> >          self.optional = optional
> >          self.features = features or []
>