consensus-shipyard/mir

Generate constructors for `mir.struct` protobufs

Opened this issue · 3 comments

Same way as the mir.event-anotated protobuf messages get a generated constructor, mir.struct-annotated protobufs should also have constructor functions for the corresponding generated Go types. Ideally, an event would automatically be as struct as well, and the corresponding types and constructors would be generated too.

For example, the following protobuf

package mypackage

message Foo {
  option (mir.event) = true;

  string some_field = 1;
}

message Bar {
  option (mir.struct) = true;

  string some_field = 1;
}

would generate code such that a programmer can instantiate events and structs like this:

fooEvent := mypackageevents.Foo("Hello")
fooStruct := mypackagetypes.Foo("Hello")

barStruct := mypackagetypes.Bar("Hello")

Here the fooStruct would just be a plain data structure, while evt would wrap it in the corresponding hierarchical event structure. For barStruct, no event exists.

The added value of generating constructor for structs is not just convenience, but also a more robust programming model, as the constructor always forces the programmer to specify all the fields. When, at some point, the definition of a protobuf message changes (e.g. a new field is added), it is very easy to locate its usage in the code (and add the extra value), as the compiler would enforce it. On the other hand, if the objects were constructed as simple struct literals, the compiler would simply implicitly (and silently) fill in the zero value for any new fields and the programmer runs a high risk of not updating all their code properly.

@xosmig does this make sense to you? Intuitively this seems like something very easy to implement by slightly extending the current codegen tools. Is that the case?
This is not to ask you to implement it (unless you want to), just curious what you think.

xosmig commented

Doesn't mypackagetypes.Foo{"hello"} already enforce that you specify all the fields?
This syntax wouldn't work with protoc-generated code because there are some unexported fields but it should work fine for mir-generated structs.

Another note: mypackagetypes.Foo("Hello") would be a name collision. Perhaps, mypackagetypes.NewFoo("Hello")? As a bonus, there would be less confusion with the functions from the events packages.

Intuitively this seems like something very easy to implement by slightly extending the current codegen tools

Yes, I believe it should be relatively easy to implement. I guess github copilot should be able to generate the necessary code :)

Ah yes you're right, mypackagetypes.Foo{"Hello"} does enforce all fields being specified. Didn't realize that as I was used to always use the syntax with named fields mypackagetypes.Foo{SomeField: "Hello"}, which does allow to skip fields and automatically uses the zero value for them. One little drawback still remains though, since the empty struct would always be allowed (mypackagetypes.Foo{}).
As for the name collision, yes, NewFoo would be the obvious solution. One just might need to think about the corner case where the user specifies two protobufs, X and NewX, which would still produce a collision. But the generator could simply check for this case and report an error.