-
Notifications
You must be signed in to change notification settings - Fork 255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Helper enums ZERO value is incorrectly used #433
Comments
@bogdandrutu I think you are right. I support removing the zero value from enums for bitmasks. |
I think this should be done before 1.0 so adding to the milestone. |
I'm not sure what the problem is with the way things are currently. If a helper enum contains bitmask values then a const (
FLAG_NONE = 0
FLAG_RED = 1
FLAG_BLUE = 2
FLAG_GREEN = 4
)
// 7
value := FLAG_NONE | FLAG_RED | FLAG_BLUE | FLAG_GREEN
// false
isNone := value == FLAG_NONE // or, value == value & FLAG_NONE
// true
isRed := FLAG_RED == value & FLAG_RED
//true
isGreen := FLAG_GREEN == value & FLAG_GREEN
//true
isBlue := FLAG_BLUE == value & FLAG_BLUE While it can allow for nonsensical combinations, this is likely the case for many bitmask-type flag sets and there are still sensible ways to interact with them. Is a user required to utilize the values correctly? Yes. That's the case regardless whether we include helper enums or not, though. Are there concrete examples of these values being used incorrectly that can be pointed out? I think in their absence it will be difficult to progress here. |
What is the meaning of "None" in a bit map? Why do you not offer a "All" (or other subset)? Let's assume you have a 3-state flag somewhere with properties "foo/bar/baz" and the mask will be "6". what None means? |
I think the check It is an incorrect usage because |
I looked into this and unfortunately am unable to fix it. Protobufs require that enums start with a 0 value. If you try to omit it you get a compilation error:
Alternatively, trying to use a dummy name, e.g.:
Fails Objective-C generator:
Also tried declaring it reserved as the doc says should be possible:
but get an error again:
If anyone knows how to fix this please let me know, otherwise I am going to close this issue. |
@open-telemetry/specs-approvers If anyone knows how to fix this please let me know, otherwise I am going to close this issue (see comment above). |
I feel we are going too far. Let's just let there be a FLAG_NONE and carefully document that it is a bitmask value. |
For the record, I think we should keep every existing enum as-is and declare 1.0. |
Resolves open-telemetry#433 The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` To prevent this mistake some additional comments are added and the zero value in the enum is prefixed with an underscore to discourage its use.
Resolves open-telemetry#433 The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` To prevent this mistake some additional comments are added and the zero value in the enum is prefixed with an underscore to discourage its use.
Resolves open-telemetry#433 The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` To prevent this mistake some additional comments are added and the zero value in the enum is prefixed with an underscore to discourage its use.
Resolves open-telemetry#433 The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` To prevent this mistake some additional comments are added and the zero value in the enum is prefixed with an underscore to discourage its use. I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake some additional comments are added and the zero value in the enum is prefixed with an underscore to discourage its use. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
I created a PR that discourages the use of zero values for bitfields: #461 |
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake some additional comments are added and the zero value in the enum is prefixed with an underscore to discourage its use. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake some additional comments are added and the zero value in the enum is prefixed with an underscore to discourage its use. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake the following changes are done: - All bitfield mask enums are suffixed with a _MASK to signify their purpose. - Zero value of the enum is prefixed with an underscore to discourage its use. - Some additional comments are added to explain how bitfields and their enums should be used. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
As others have pointed out, I am not really seeing why
|
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake the following changes are done: - All bitfield mask enums are suffixed with a _MASK to signify their purpose. - Zero value of the enum is prefixed with an underscore to discourage its use. - Some additional comments are added to explain how bitfields and their enums should be used. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake the following changes are done: - All bitfield mask enums are suffixed with a _MASK to signify their purpose. - Zero value of the enum is prefixed with an underscore to discourage its use. - Some additional comments are added to explain how bitfields and their enums should be used. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake the following changes are done: - All bitfield mask enums are suffixed with a _MASK to signify their purpose. - Zero value of the enum is prefixed with an underscore to discourage its use. - Some additional comments are added to explain how bitfields and their enums should be used. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
I think the reasoning is that we have seen enough cases of a mistake described in the first bullet that we think it is OK to sacrifice the convenience you get for the second bullet's case. |
Resolves #433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake the following changes are done: - All bitfield mask enums are suffixed with a _MASK to signify their purpose. - Zero value of the enum is prefixed with an underscore to discourage its use. - Some additional comments are added to explain how bitfields and their enums should be used. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake the following changes are done: - All bitfield mask enums are suffixed with a _MASK to signify their purpose. - Zero value of the enum is prefixed with an underscore to discourage its use. - Some additional comments are added to explain how bitfields and their enums should be used. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake the following changes are done: - All bitfield mask enums are suffixed with a _MASK to signify their purpose. - Zero value of the enum is prefixed with an underscore to discourage its use. - Some additional comments are added to explain how bitfields and their enums should be used. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake the following changes are done: - All bitfield mask enums are suffixed with a _MASK to signify their purpose. - Zero value of the enum is prefixed with an underscore to discourage its use. - Some additional comments are added to explain how bitfields and their enums should be used. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
Resolves open-telemetry#433 ## Problem The zero values defined in helper enums encourage incorrect usage of the bitfields. The typical incorrect code looks like this: ```proto enum DataPointFlags { FLAG_NONE = 0; FLAG_NO_RECORDED_VALUE = 1; // Bits 2-31 are reserved for future use. } ``` ```go if datapoint.Flags == FLAG_NONE { // do something when there is no recorded value } ``` This is incorrect because it does not take into account that the Flags field can be extended in the future to use the reserved bits. The `if` statement above will be incorrect if any other bit is set. The correct code looks like this: ```go if (datapoint.Flags & FLAG_NO_RECORDED_VALUE) == 0 { // do something when there is no recorded value } ``` ## Solution To prevent this mistake the following changes are done: - All bitfield mask enums are suffixed with a _MASK to signify their purpose. - Zero value of the enum is prefixed with an underscore to discourage its use. - Some additional comments are added to explain how bitfields and their enums should be used. ## Alternates Tried I also tried to remove the zero values from enums altogether, but it turned out to be impossible. I tried a few possible approaches and none worked. Protobufs [require that enums start with a 0 value](https://protobuf.dev/programming-guides/proto3/#enum). If you try to omit it you get a compilation error: ``` opentelemetry/proto/metrics/v1/metrics.proto:325:28: The first enum value must be zero in proto3. ``` Alternatively, trying to use a dummy name, e.g.: ``` enum DataPointFlags { _ = 0; FLAG_NO_RECORDED_VALUE = 1; } ``` Fails Objective-C generator: ``` error: got empty string for making TextFormat data, input: "", desired: "_". ``` Also tried declaring it reserved as the doc says should be possible: ``` enum DataPointFlags { reserved 0; FLAG_NO_RECORDED_VALUE = 1; } ``` but get an error again: ``` opentelemetry/proto/metrics/v1/metrics.proto:327:28: The first enum value must be zero in proto3. ```
In the collector before we removed access to the helper enums like
DataPointFlags
(https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/metrics/v1/metrics.proto#L321), I've seen code comparing withFLAG_NONE
or initializing asFLAG_NONE
the value of the flags to check if a value is present.If the helper enums contain "masks" then ZERO value does not make sense, if the enums contain "absolute" values, then zero value makes sense but the other values don't make sense.
My personal suggestion is still to remove them (see #423) but if that is not ok, this inconsistency between masks/absolute value has to be fixed.
Assigning to @tigrannajaryan since he is a big supported of these values.
The text was updated successfully, but these errors were encountered: