Bug: Conflict with Size fieldname and Size method name

[awalterschulze]

This issue was originally reported here: #39.

I see that when you have a field named String golang/protobuf generates that fieldname as String_, since this field conflict with the generated String method.

I think this would be a great solution to the Size method and fieldname conflict problem.

[awalterschulze]

I have made this fix on the sizeunderscore branch.
Here is the commit
8d709bd
If someone could please check it out before I merge it into master that would be great.

[awalterschulze]

merged

[dennwc]

Is there something special about the Size method? I mean, is it the part of some stdlib interface, or it's a gogoprotobuf addition?
I'm asking because it seams weird to change the name of Size field for struct in whole codebase to use it with this great library :)

[awalterschulze]

It might be, but not yet.
There is/was a TODO in the golang/protobuf to check for a Size method.
To change the method name now would cause a backwards incompatible change if anyone was calling the Size method.
And this is possible, since Size is a public method.

You can use gogoproto.customname to change the field name again.

[dennwc]

I'm wondering, is it possible to change Size method to SizePb or something like that?
It can be backward-compatible at least for the lib itself. We can just try both interfaces:

type sizer interface{
  SizePb() int
}
func (this *codec) Marshal(v interface{}) ([]byte, error) {
  var n int
  if psz, ok := v.(sizer); ok {
    n = psz.SizePb()
  } else if sz, ok := v.(interface{ Size() int }); ok { // compatibility
    n = sz.Size()
  }
  // ...
}

For lib users we can have one more option to generate Size methods instead of SizePb. It will still work for both variants since we check each interface on Marshal.

[awalterschulze]

That is a lot of effort for very little pay off.
Why is it so important?

[awalterschulze]

This is the TODO I was talking about
https://github.com/gogo/protobuf/blob/master/proto/encode.go#L277

[dennwc]

It's important because Size is a pretty common name for both methods and fields. String method has its interface already defined in Go, so it's ok to rename all user's fields and methods to avoid collisions. But Size is different. There is already interface with a Size method in stdlib with a completely different meaning: Size() int64 in os.FileInfo. So I just can't satisfy it and convert my struct to PB at the same time. I think there are many other examples of the same issue (dropbox@4ceca3a was done for a reason).

The issue can be easily resolved with fork, as Dropbox guys done, but it would be better to have it in official repo, I believe :)

[awalterschulze]

You make a compelling argument, but this is still a breaking change.
You are also not the first to request this, but I don't know how many people's code I will be breaking if I change this.
So the only way would be to add another extension called protosizer or something that generates a ProtoSize() method and then having Marshal check for both methods.
Pretty much like you suggested.

[dennwc]

I agree, another extension is a much better way than just renaming everything at once.
I will fill a PR some time later, if you don't mind.

[awalterschulze]

Ok that will be great.

extensions name: protosizer
method name: ProtoSize

Maybe the marshal plugin could check which of protosizer or sizer is used and then call that method.
If none is used and the user provides its own Size or ProtoSize method. Marshal should type assert something like you have proposed above. What do you think?

[dennwc]

I should look at the code more closely, but it sounds right to check for used extension. I'll post a PR in a few days then. I hope I will get enough time to test it properly :)