Re: [PATCH 1/3] scripts/qapi: enum with conditional first item must be optional

Paolo Bonzini <[email protected]> writes:

> On 7/27/26 11:32, Markus Armbruster wrote:
>> I count 168 enum types including tests, 12 have conditional members, and
>> none of them have a first member that is conditional.  So this is merely
>> a latent issue.  Doesn't mean we shouldn't get rid of it, of course.
>
> Yes, it's latent.  Rust issues (see below) obviously are latent too.
>
>>>> However, I'm not sure your solution fixes this problem completely.
>>>>
>>>> Code providing an explicit default then is still fine:
>>>>
>>>>      if (!arg.has_opt) {
>>>>          arg.opt = ENUM_TYPE_MUMBLE;
>>>>      }
>>>>
>>>> But we often use arg.opt without checking arg.has_opt for brevity.  This
>>>> is an implicit default to zero, whatever zero may mean.  If the optional
>>>> enum's first member is conditional, this default depends on build
>>>> configuration.
>>>>
>>>> Stupidest solution that could possibly work: an enum's first member
>>>> cannot be conditional.
>>>
>>> That would prevent an enum that is entirely compiled out, which seems like
>>> a plausibly desirable feature.
>>>
>>> Honestly I think this is a C problem, not a QAPI problem. Skipping has_xx
>>> for bools that default to false is already borderline; doing it for enums
>>> is well into "you shouldn't do it" territory.
>>
>> Let's review how we represent optional members in C.
>>
>> * Values of absent members are zero-initialized.
[...]
>> * Anything that doesn't map to pointers: we need has_member.
>>
>>   When the default is zero, which is fairly common, there's again no
>>   need to explicitly supply it.  We just use obj.member, without
>>   cluttering the code with .has_member conditionals.
>>
>> In my opinion, this (non-)usage of .has_member is just *fine* as long as
>> the meaning of zero is well-defined at the QAPI level. [...]
>>
>> It's well-defined for enums as long as the enum's first member is
>> unconditional.  The meaning of zero is admittedly less obvious there.
>>
>> We could certainly change the existing code to only read obj.member
>> where guarded by if (obj.has_member).  Inhowfar we'd then succeed at
>> keeping the shorter (and in my opinion more readable) forms from
>> creeping back is less certain.
>
> I'm certain they would creep back, and I guess it's also fine if the
> enum has a 'default' or 'none' value.  But the language doesn't make a
> difference, because the programmer just uses "0" even if the first
> member is conditional.
>
> Instead, the Rust backend won't let you write Default::default() if
> the first item is conditional; it's a compile-time failure.  In that
> sense the C language has a deficiency.
>
>> But what's the benefit?  Enabling "a plausibly desirable feature" we
>> haven't found a use for is one, but I don't think it can justify the
>> change.
>
> The plausibly desirable feature if enums with conditional items
> only. The alternative is to forbid enums that can disappear
> completely.
>
> How desirable is it?  Here are the two that come closest:
>
> { 'enum': 'Qcow2CompressionType',
>    'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>
> { 'enum': 'BlockExportType',
>    'data': [ 'nbd',
>              { 'name': 'vhost-user-blk',
>                'if': 'CONFIG_VHOST_USER_BLK_SERVER' },
>              { 'name': 'fuse', 'if': 'CONFIG_FUSE' },
>              { 'name': 'vduse-blk', 'if': 'CONFIG_VDUSE_BLK_EXPORT' } ] }
>
> Other cases have a 'none' or even a 'default' member
> (DisplayType).

Yes.

Aside: I prefer "absent defaults to an actual value" over "absent means
something else than any value", because I find it simpler.

>                Qcow2CompressionType could grow one in case zlib option
> becomes mandatory.

Do you mean "becomes optional?"

Note that @zlib can't become optional without a compatibility break,
because @compression-type defaults to it in BlockdevCreateOptionsQcow2.

Moreover, Qcow2CompressionType needs members, because @compression-type
is mandatory in ImageInfoSpecificQCow2.

The hard requirement is "need members at build time", i.e. the build
configuration must not compile out all members.  Without that,
query-named-block-nodes cannot be made to work for QCOW2 images: it must
have a value to store in the mandatory @compression-type.  Storing 0
would be cheating, and also a dangerous violation of invariant "enum
variable must contain bits that represent an actual enum value".

We may want the stricter "need members in any build configuration",
i.e. not all members may be conditional.  And once we use that, we can
just as well require the first member to be unconditional.

> BlockExportType is the most potentially problematic, because it does
> not make sense to have 'none' there.

BlockExportType also needs members, because BlockExportOptions and
BlockExportInfo have a mandatory member @type of BlockExportType.

So, these two don't actually come close.  Doesn't mean we'll never
invent something else that does.

>                                       It would still not be usable -
> for example the C visit_type_BlockExportOptions_members() would
> collapse to
>
>     if (!visit_type_q_obj_BlockExportOptions_base_members(...) {
>         return false;
>     }
>     switch (obj->type) {
>     default:
>         abort();
>     }
>
> i.e. basically "return false" because neither
> visit_type_BlockExportType nor
> visit_type_q_obj_BlockExportOptions_base_members can succeed.
>
> However, QAPI schema generation would work, meaning that you _could_
> write the QMP commands generically, even if in practice they're dead
> code.  Bug or feature?

Or excursion into the weeds?

Joking aside, I wouldn't call it a feature, because "feature" implies
"good for something".  "Bug" implies "has some undesirable effect".  If
we volunteer for the mission "don't let the programmer specify useless
interfaces", then this is a bug.  If we go "you're asking for rope, have
some" instead, it's not.

Two common excuses for letting the programmer do stupid things:

1. Preventing stupid things would also prevent them from doing clever
   things.

2. It just isn't worth the bother.

   Preventing something people do and then regret is worth a lot more
   bother than preventing something they have not done in years.

>>> Rust would spell it "Default::default()"; the language can help rejecting
>>> it if you have mandatory string fields (it would recursively default boxed
>>> structs, unlike C) but it cannot do anything about portability; this patch
>>> closes the gap completely for Rust, and does what it can for C.
>>
>> I wouldn't call it a portability problem.  The actual problem is that
>> build-time configuration can have unwanted effects at least in theory.
>> Since different host platforms can require different configuration,
>> porting can trigger the problem.
>
> Yeah, portability in a wider sense---such as different behavior of
> clients when talking to different QEMU binaries.

Exactly.

>> You propose to
>>
>> * Forbid empty enums [PATCH 2].  They are allowed simply because I never
>>   found a compelling reason to forbid them.  Yes, they're useless, but
>>   users can figure that out without the QAPI generator insisting.
>>
>>   Does forbidding them help Rust?
>
> Somewhat; I cannot generate Default::default() for them.  It would be
> easy enough to skip generation of that method, but you'd still get a
> Rust compilation error if you put them in a struct.
>
> IMO this is telling us that empty enums should indeed fail to compile
> even earlier, in the QAPI schema generator.
>
> Empty enum types cannot be created, and neither can any structs or
> commands they contain.  There's simply no way to have code that refers
> to them, unless they're wrapped in something else that lets the code
> pretend they can exist: for example you can make a field optional and
> write "None" for it.
>
> With conditionally empty enums, this at least makes sense as a concept; on the other hand I just don't see the point in unconditionally empty enums, even if they're optional, so I left them out.

If forbidding empty enums makes Rust generation simpler or easier,
that's a compelling reason, isn't it?

You should mention the Rust reason(s) in the commit message then.

>>   You still allow enums whose members are all conditional.  Build
>>   configuration could make these empty, but there are conceivable uses.
>
> I allow them because patch 1 forces them to be optional and then, even
> if *the enum* has no default, "None" is a sensible default for *the
> field*.  In Rust, unlike C, I cannot just access the value of the enum
> if it's "None"; on top of that, optional_field.unwrap_or_default()
> fails to compile because the enum has no default.
>
>> * Require members of enum type to be optional when the enum's first
>>   member is conditional [PATCH 1].
>>
>>   Does this help Rust?
>
> Yes, it lets me cop out of writing a Default::default() implementation
> for them, while letting them have a sensible default ("None", just
> like in C) if embedded in a struct.
>
> Again, the language is IMO telling us that enums with a conditional
> first member are in general a bad idea, and the solution is to catch
> them earlier, in the QAPI schema generator.

I might have banned conditional first members from the start if I had
thought out things back then.

> I didn't ban them outright because IMO the fact that in C you can
> shoot yourself in the foot, and assume a meaning of "zero" for such an
> enum, *is* a language problem.  In particular it is a language problem
> that it is not able to express the relationship between has_foo and
> foo.
>
> But I can be convinced to do it, if you think we need not care about
> cases like BlockExportType.

To me, requiring unconditional first member feels simpler.

Perhaps I'm wrong.  I've been wrong before.  If you'd like to settle the
matter instead of relying on my gut feelings, try documenting both
solutions in docs/devel/qapi-code-gen.rst.

Perhaps we find uses for optional enums with only conditional members
later.  Doesn't feel too likely.  But if we do, we can complicate
matters then rather than now.

Even with more reasons than just helping Rust, you should still mention
the Rust reason(s) in the commit message.

> Paolo
>
>> I believe these rules are mildly bothersome to explain.  Which you
>> sidestepped by not documenting them in docs/devel/qapi-code-gen.rst :)
>>
>> To actually prevent it reliably, we'd have to attack the root of the
>> problem, namely the meaning of enum types' zero value.  The obvious way
>> to do that is to require enums to have an unconditional first member.
>> This is also simpler to document, I think.
>>
>> Drawback: it removes the ability to define an enum whose members are all
>> conditional, usable with optional object members.  Does this matter
>> enough to complicate things?