proposal: protoc-gen-go: generate Setters for fields

[majelbstoat]

Please consider adding setter methods for simple fields, (not just one-offs as requested here: #283), so that we can create partial interfaces for them.

As a specific use-case, when doing event emission, I want to add the current timestamp to most, but not all events. Events are defined as protos:

message EventA {
  string emitted_at = 1;
  string user_id = 2;
}

message EventB {
  string emitted_at = 1;
  string org_id = 2;
}

message EventC {
  string team_id = 1;
}

In a common Emit() method, I would love to use a temporary interface to set the field where necessary:

if e, ok := evt.(interface{SetEmittedAt(emittedAt string)}); ok {
  e.SetEmittedAt(now)
}

Instead, I believe I currently have to use reflection:

	ps := reflect.ValueOf(evt)
	s := ps.Elem()
	if s.Kind() == reflect.Struct {
		f := s.FieldByName("EmittedAt")
		if f.IsValid() && f.CanSet() && f.Kind() == reflect.String {
			f.SetString(now)
		}
	}

Which is kind of brittle, and ugly to boot. (If I don't need to do this, I'd love an alternative instead!)

[puellanivis]

Consider

message Event {
  string emitted_at = 1;

  message A {
    string user_id = 1;
  }
  A a = 2;

  message B {
    string org_id = 1;
  }
  B b = 3;

  message C {
    string team_id = 1;
  }
  C c = 4;
}

[majelbstoat]

@puellanivis: the emitted_at field does not apply to all events. And, while, in this specific case, it could, there are other cases where it wouldn't be semantically correct for the field to be applied to all events. And yes, they're all technically optional, but I don't want the API of a message to include a field that is never used under any circumstance, because that's needlessly confusing (especially if it's then set by some common code similar to the above that doesn't know the field is not used). Also, this introduces an indirection which further complicates matters.

[puellanivis]

OK, but in your given code, how would you propose to safely pass around an arbitrary event?

What happens when I pass in the following protobuf?

message NotAnEventAndShouldNeverBeTreatedAsAnEvent {
  string emitted_at = 1;
}

If you demand that emitted_at not appear in message Event_C, then:

message Event {
  message A {
    string emitted_at = 1;
    string user_id = 2;
  }
  A a = 1;

  message B {
    string emitted_at = 1;
    string org_id = 2;
  }
  B b = 2;

  message C {
    string team_id = 1;
  }
  C c = 3;
}

Or, you could just provide your own Setter methods in a parallel non-generated .go file.

There are a significant number of solutions that do not require complicating everyone’s code for a feature that would only be useful in uncommon situations.

[majelbstoat]

"OK, but in your given code, how would you propose to safely pass around an arbitrary event?"
That's orthogonal to this problem. If I pass a thing that is not an event, it will break, same as today. Some convention is fine. Your updated message structure still requires me to do reflection to see if EmittedAt is settable, because it's not on everything. (The encapsulation of it being an Event message is not what I'm worried about here.)

I know there are lots of solutions to this problem. I already have a solution, documented above :) But there are hundreds of events, and writing hundreds of setters, one per event is painful. Go doesn't have generics, which is fine, because the recommended approach is to use interfaces. Which I can't do in this case. So I have one setter, it uses reflection, and it kind of sucks.

I would hardly say that this counts a complicating everyone's code. Nobody is forced to use it, I'm not advocating removing publicly named fields, and in fact, what I'm describing is idiomatic go: https://golang.org/doc/effective_go.html#Getters

I was legitimately surprised this wasn't already how getting and setting was done.

[puellanivis]

I would hardly say that this counts a complicating everyone's code.

It would add code to every single protobuf package, and in fact a method receiver for every field. If I have a message with hundreds of fields, then it would get hundreds of setters to that message alone.

And in most cases we could expect 99.99% of these setters to never be used.

[majelbstoat]

This is in fact a dupe of #65, which I didn't spot earlier. I don't quite understand yet how #364 will address the needs of #65, but if it does as @dsnet suggests it will, it will probably also solve this. FWIW, I'm not convinced by the code bloat argument. Even for large structs, a simple check and store on a pointer receiver is really not much overhead in the context of Go. But, let's see how API v2 progresses ;)