Skip to content

Conversation

pm-dev
Copy link

@pm-dev pm-dev commented Oct 8, 2025

Given a oneof field with ~600 options, the generated traverse function will fail to compile because swift is unable to check the switch is exhaustive in reasonable time. See #1856

This PR follows similar logic to #904 which allows switch statements to be broken up into multiple switch statements (using a default: break case).

Instead of hardcoding 500, I've added a new GeneratorOption to support setting the "maxCasesInSwitch" which allows the same number to be used in EnumGenerator and OneofGenerator and allows clients to set this number to whatever suits them based on Swift version or other considerations. This number could also be set to something very high to effectively turn off the feature.

The second commit adds tests, I asked claude to help out with this part so please help me in double checking those tests. I was able to run tests successfully locally.

@thomasvl
Copy link
Collaborator

thomasvl commented Oct 8, 2025

Haven't looked at the PR yet, but the comment about the generation option for the size of switches gives be a little pause. I follow the logic on the idea, but I also worry since every option is something we have to continue to support going forward, and to my knowledge, no one has come back with the 500 limit being too large/small yet.

@tbkka any thoughts on exposing this as a generation option vs. say just making it a shared constant between the two places?

@pm-dev
Copy link
Author

pm-dev commented Oct 8, 2025

Would be easy enough to remove the option, but keep it as a constant on GeneratorOptions for sharing.
It did make testing easier because the proto file could be much smaller vs having 500+ oneof options

@tbkka
Copy link
Collaborator

tbkka commented Oct 8, 2025

I too am reluctant to expose unnecessary options.

Copy link
Collaborator

@thomasvl thomasvl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing the OneOf and Enum issues, one thing about the Enum problem is each of the case statements in the switches if a return, i.e. - once we get down to the point that something is matched, the function is exited. But in the current flow, every switch will have to get walked to take the default with the exception of the one that matches. So I wonder if there is anything that can be done about that without getting overly complex. Given it's already serialization code, the small performance hit won't really hurt if we can't, but I figured might as well see if anything could be done.

As far as testing goes, I think for the enum grouping, our "testing" was the reference tests, i.e. - code inspection of how things generate.

I believe the oneof can also get segmented into "chunks" if there is/are any non-oneof fields with a field number that mix in:

message SwitchSplitMessage {
  int32 field_06 = 6;
  oneof value {
    int32 field_02 = 2;
    int32 field_01 = 1;
    int32 field_05 = 5;
    int32 field_03 = 3;
    int32 field_04 = 4;
    int32 field_12 = 12;
    int32 field_08 = 8;
    int32 field_09 = 9;
  }
  int32 field_10 = 10;
}

So you might want to factor that into any test case(s) to things are fully working as intended.

@pm-dev
Copy link
Author

pm-dev commented Oct 9, 2025

I've updated my branch to remove the "MaxCasesInSwitch" option in favor of hardcoding a 500 case limit.

I've also updated tests, which now require oneof with 500+ options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants