zserio icon indicating copy to clipboard operation
zserio copied to clipboard

Smarter choice type

Open reinco opened this issue 2 years ago • 4 comments

Currently the enum defining what is stored in a choice is stored outside the choice definition. The downside of this is that in generated code, the enum has to be set twice: It has to be set in the struct defining the enum and once passed as argument to the choice. This can lead to subtle bugs in code similar to the issues with list structures in zserio that are only discovered at runtime.

Proposed solution: Add a way to make the enum definining what is stored in the choice part of the choice definition itself. The generated bitstream should be exactly the same as if the choice was used as follows: struct mystruct { EnumType tp; ChoiceType ct(tp); };

One way would be to allow for something like: choice ChoiceType on EnumType

Currently zserio only supports: choice ChoiceType( EnumType tp) on tp

Also with both syntaxes we should ensure that one cannot set the EnumType to one value and still set the actual value in the choice to something else. At least this should be checked at runtime (if extensive checking is enabled). But preferably this is checked at code compile time. Example: Enum bit:2 EnumType { GOOD = 1, BAD = 2 }; Choice ChoiceType(EnumType tp) on tp { case GOOD: varuint32 good; case BAD: string bad; }; C++ ChoiceType myvar; myvar.initialize(EnumType::BAD) myvar.setGood(1); Better: ChoiceType myvar(EnumType::BAD, 1); // compile error. (constructor is implemented using template magic). And with the new syntax we should be able to do the following (so not explicitly setting the enum type at all). Because setGood() could just do a setEnumType() under the hood. However, maybe something like this is even possible with the parameterized choice?: ChoiceType myvar; myvar.setGood(1);

Note: the smart choice is different from the union type in the sense that the choice could have two choices of the same underlying type. In such cases the EnumType is needed to differentiate what is stored.

reinco avatar Aug 02 '22 08:08 reinco

I believe that unions can also have two choices of the same type - see simple_union.zs in our language tests. Union has choiceTag method to find out what is actually stored.

However this still can make sense in case that you need to share same enum in multiple choices. The only problem is how to make it clear for users - one syntax means that it's kind of "auto choice" (it was original name for current union), and the other syntax means that it's classic choice which is still also useful.

Note that zserio was originally designed as a WYSIWIG schema language (see language overivew). Since we still extend it with some "auto" features, it's still less and less WYSIWIG than it was intended to be.

Mi-La avatar Aug 04 '22 12:08 Mi-La

The union looks similar to the smarter choice I was looking for. The sharing of the same enum in multiple choices I do not consider a very strong use case. Maybe we can use it more often and specifically use in the RouteSegment type for which I actually raised this issue. The union does however have 3 drawbacks:

  1. using it instead of the current choice will be a backwards incompatible change on bitstream level (But to me at least that is acceptable currently)
  2. one cannot control the size of the metadata needed, leading to more bits used in the stream
  3. One must always add new elements to the end and cannot remove any element of the union later on without causing a backwards incompatible change.

reinco avatar Aug 08 '22 06:08 reinco

Another possible syntax could be:

choice MyAutoChoice
{
    on uint8 selector;
    case 0:
    case 1: /* note that with multiple cases the automatic setting of selector is not straightforward */
        string field;
};

Mi-La avatar Aug 16 '22 08:08 Mi-La

If the main motivation of this issue is to check inconsistencies between fields and parameters, then this issue is valid for all parameterized types which uses fields as parameters:

struct ParamHolder
{
    uint16 parameter = 11;
    Param(parameter) param;
};

struct Param(uint16 parameter)
{
    uint16  value;
    uint32  extraValue if parameter == 11;
};

In this case, it would be better to find out some more generic solution (if possible) instead of solving only one specific use case (choice with enum param).

In the meantime, C++ users can perhaps use initialize() method on the top level structure to set all parameters recursively.

mikir avatar Sep 05 '22 12:09 mikir