Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate constructors for mir.struct protobufs #409

Open
matejpavlovic opened this issue Apr 17, 2023 · 3 comments
Open

Generate constructors for mir.struct protobufs #409

matejpavlovic opened this issue Apr 17, 2023 · 3 comments
Labels
backlog To address eventually, not necessarily now

Comments

@matejpavlovic
Copy link
Contributor

matejpavlovic commented Apr 17, 2023

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.

@matejpavlovic
Copy link
Contributor Author

@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
Copy link
Contributor

xosmig commented Apr 17, 2023

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 :)

@matejpavlovic
Copy link
Contributor Author

matejpavlovic commented Apr 18, 2023

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.

@matejpavlovic matejpavlovic added the backlog To address eventually, not necessarily now label Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog To address eventually, not necessarily now
Projects
None yet
Development

No branches or pull requests

2 participants