Re: [PATCH 13/19] qapi/schema: fix typing for QAPISchemaVariants.tag_member

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Nov 22, 2023 at 11:02 AM John Snow <jsnow@redhat.com> wrote:
>>
>> On Wed, Nov 22, 2023 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> > John Snow <jsnow@redhat.com> writes:
>> >
>> > > There are two related changes here:
>> > >
>> > > (1) We need to perform type narrowing for resolving the type of
>> > >     tag_member during check(), and
>> > >
>> > > (2) tag_member is a delayed initialization field, but we can hide it
>> > >     behind a property that raises an Exception if it's called too
>> > >     early. This simplifies the typing in quite a few places and avoids
>> > >     needing to assert that the "tag_member is not None" at a dozen
>> > >     callsites, which can be confusing and suggest the wrong thing to a
>> > >     drive-by contributor.
>> > >
>> > > Signed-off-by: John Snow <jsnow@redhat.com>
>> >
>> > Without looking closely: review of PATCH 10 applies, doesn't it?
>> >
>>
>> Yep!
>
> Hm, actually, maybe not quite as cleanly.
>
> The problem is we *are* initializing that field immediately with
> whatever we were passed in during __init__, which means the field is
> indeed Optional. Later, during check(), we happen to eliminate that
> usage of None.

You're right.

QAPISchemaVariants.__init__() takes @tag_name and @tag_member.  Exactly
one of them must be None.  When creating a union's QAPISchemaVariants,
it's tag_member, and when creating an alternate's, it's tag_name.

Why?

A union's tag is an ordinary member selected by name via
'discriminator': TAG_NAME.  We can't resolve the name at this time,
because it may be buried arbitrarily deep in the base type chain.

An alternate's tag is an implicitly created "member" of type 'QType'.
"Member" in scare-quotes, because is special: it exists in C, but not on
the wire, and not in introspection.

Historical note: simple unions also had an implictly created tag member,
and its type was the implicit enum type enumerating the branches.

So _def_union_type() passes TAG_NAME to .__init__(), and
_def_alternate_type() creates and passes the implicit tag member.
Hardly elegant, but it works.

> To remove the use of the @property trick here, we could:
>
> ... declare the field, then only initialize it if we were passed a
> non-None value. But then check() would need to rely on something like
> hasattr to check if it was set or not, which is maybe an unfortunate
> code smell.
> So I think you'd still wind up needing a ._tag_member field which is
> Optional and always gets set during __init__, then setting a proper
> .tag_member field during check().
>
> Or I could just leave this one as-is. Or something else. I think the
> dirt has to get swept somewhere, because we don't *always* have enough
> information to fully initialize it at __init__ time, it's a
> conditional delayed initialization, unlike the others which are
> unconditionally delayed.

Yes.

Here's a possible "something else":

1. Drop parameter .__init__() parameter @tag_member, and leave
.tag_member unset there.

2. Set .tag_member in .check(): if .tag_name, look up that member (no
change).  Else, it's an alternate; create the alternate's implicit tag
member.

Drawback: before, we create AST in just one place, namely
QAPISchema._def_exprs().  Now we also create some in .check().

Here's another "something else":

1. Fuse parameters .__init__() @tag_member and @tag_name.  The type
becomes Union.  Store for .check().

2. Set .tag_member in .check(): if we stored a name, look up that
member, else we must have stored an implicit member, so use that.

3. We check "is this a union?" like if self._tag_name.  Needs
adjustment.

Feels a bit awkward to me.

We can also do nothing, as you said.  We don't *have* to express
".check() resolves unresolved tag member" in the type system.  We can
just live with .tag_member remaining Optional.

Differently awkward, I guess.

Thoughts?