google / protobuf.dart

Runtime library for Dart protobufs
https://pub.dev/packages/protobuf
BSD 3-Clause "New" or "Revised" License
527 stars 183 forks source link

protoc_plugin converts field names to camelCase, protobuf converts it back to snake case #599

Open osa1 opened 2 years ago

osa1 commented 2 years ago

protoc_plugin converts field names to camelCase: https://github.com/google/protobuf.dart/blob/9da84aef9d8126ac3da665f3c163e3cb4af31b6d/protoc_plugin/lib/names.dart#L426-L428 (in _fieldMethodSuffix, which, confusingly, is not really returning a suffix. It's also used as the field name as full).

The library then converts those names back to snake_case: https://github.com/google/protobuf.dart/blob/9da84aef9d8126ac3da665f3c163e3cb4af31b6d/protobuf/lib/src/protobuf/field_info.dart#L60

The problem is conversion from snake_case to camelCase is not invertible. For example, all of these snake_case identifiers are converted to the same camelCase:

So when we construct a FieldInfo without specifying the proto_name argument (it's optional) we'll get wrong protoName for the field in this constructor: https://github.com/google/protobuf.dart/blob/9da84aef9d8126ac3da665f3c163e3cb4af31b6d/protobuf/lib/src/protobuf/field_info.dart#L51-L65

Looking at protoc_compiler, I think we always pass a proto_name argument. If that's true then we should make this argument non-optional as it's otherwise a potential footgun.

osa1 commented 2 years ago

https://github.com/osa1/protobuf.dart/commit/8b22f95aa772074edeadb9e749538c2b6f3a8696 removes Dart name -> proto name conversions by passing proto names of fields in generated code.

Every BuilderInfo call that adds a field gets one more argument so code size grows a little bit (proportional to number of fields).

In runtime we no longer do the unCamelCase business so it should be faster to create the BuilderInfos. This may have an impact on app startups but I don't know how to measure that yet. (proto names are generated lazily so it shouldn't affect app startup)