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

[John Snow]

On Wed, Jan 10, 2024 at 2:53 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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().

I suppose I don't have a concrete argument against this beyond "It
doesn't seem prettier than using the @property getter."

>
> 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.

Yeah, a little. Mechanically simple, though, I think.

>
> 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.

This is the only option I'm sure I don't want - it's misleading to
users of the API for the purposes of new generators using a fully
realized schema object. I think it's important to remove Optional[]
where possible to avoid the question "When will this be set to None?"
if the answer is just "For your purposes, never."

It's an implementation detail of object initialization leaking out.

(Also, I just counted and leaving the field as Optional adds 22 new
type errors; that's a lot of callsites to bandage with new conditions.
nah.)

The *other* way to not do anything is to just leave the @property
solution in O:-)

>
> Differently awkward, I guess.
>
> Thoughts?

Partial to the getter, unless option 1 or 2 leads to simplification of
the check() code, which I haven't really experimented with. If that's
something you'd really rather avoid, I might ask for you to decide on
your preferred alternative - I don't have strong feelings between 'em.

(Not helpful, oops. Thanks for your feedback and review, though.
You've successfully talked me down to a much smaller series over time
:p)

--js