Avoids OneOf type collision with enums and some nested types #1341
Conversation
jknovi
force-pushed
the
oneof-conflict
branch
2 times, most recently
from
24b47b7 to
22461eb
Compare
When detecting if a oneof type will clash with other types we only compare against nested types, and for those only the snake_case version is validated against the oneof name. This allow room for conflicts when: - The collision would happen with an enum type. - The oneof name is a reserved word that when snaked gets sanitized as r#<word>. In this change the check for collisions is updated to consider enum types and compered against their capitalized camel case version which
jknovi
force-pushed
the
oneof-conflict
branch
from
22461eb to
839517e
Compare
| .any(|nested| to_snake(nested.name()) == descriptor.name()); | ||
| .map(DescriptorProto::name) | ||
| .chain(parent.enum_type.iter().map(EnumDescriptorProto::name)) | ||
| .any(|type_name| to_upper_camel(type_name) == to_upper_camel(descriptor.name())); |
There was a problem hiding this comment.
Just for my understanding: changing from to_snake to to_upper_camel doesn't do anything, right?
The actual change is that you change the case of both types, right?
There was a problem hiding this comment.
The to_snake change was required for oneofs with a reserved keyword as name, for example is oneof name is type this check will miss that case since to_snake("Type") -> "r#type" so the check will compare "type" against "r#type" and hence will produce nested type Type and oneof enum Type, causing the compilation error.
There was a problem hiding this comment.
Comparing to camel case will cover the collision better in these cases.
There was a problem hiding this comment.
LMK if you would like me to add a comment around this in the code or if this is fine resolve the conversation as ACK.
When detecting if a oneof type will clash with other types we only compare against nested types, and for those only the snake_case version is validated against the oneof name. This allow room for conflicts when:
In this change the check for collisions is updated to consider enum types and compered against their capitalized camel case version which is the one that will be used on the type names causing the collision which makes builds using these protos to fail.
Added testcases that would repro these issues on the absence of this fix. Note that the test cases are legal protos that would be codegened without issues on other languanges.