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

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). 
Qcow2CompressionType could grow one in case zlib option becomes mandatory.

BlockExportType is the most potentially problematic, because it does not 
make sense to have 'none' there.  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?

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

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

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

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?