gogo / protobuf

[Deprecated] Protocol Buffers for Go with Gadgets
Other
5.67k stars 809 forks source link

Improve type safety in generated enums #497

Open rickb777 opened 6 years ago

rickb777 commented 6 years ago

re github.com/gogo/protobuf/protoc-gen-gogo/generator/generator.go

Line 1481 is func generateEnum, which produces two tables, X_name and X_value, both of which are a map. They both use int32, which is weak typing.

The enumeration that is generated has its own type derived from int32. If this were to use this type as the key of the X_name map and as the value of the X_value map, this would give stronger types. Arguably, this would be an improvement.

The benefit of this change is that it would not be necessary to type-convert to/from int32 when using these tables with the enumeration values.

So line 1524 would become

-    g.P("var ", ccTypeName, "_name = map[int32]string{")
+    g.P("var ", ccTypeName, "_name = map[", ccTypeName, "]string{")

And line 1537 would become

-    g.P("var ", ccTypeName, "_value = map[string]int32{")
+    g.P("var ", ccTypeName, "_value = map[string]", ccTypeName, "{")

For an enumeration called, say, ErrorCode, the generated maps would become

- var ErrorCode_name = map[int32]string{ ...
+ var ErrorCode_name = map[ErrorCode]string{ ...

and

- var ErrorCode_value = map[string]int32{ ...
+ var ErrorCode_value = map[string]ErrorCode{ ...
donaldgraham commented 6 years ago

Thanks for the suggestion.

We've spotted a problem here:

https://github.com/gogo/protobuf/blob/master/proto/properties.go#L508

(in the code forked from golang/protobuf),

RegisterEnum() takes map[int32]string

So if we use map[ccTypeName]string, we will have problems registering our enum.