deterministic marshaling for non regenerated code

[awalterschulze]

It seems that the Marshal method is called when deterministic is true or false.
https://github.com/golang/protobuf/blob/master/proto/table_marshal.go#L227-L232

This might be a non deterministic generated Marshal method in a library used by a user pulling in the newer version of golang/protobuf, where they are able to set deterministic to true.

Even if we modify this code to rather be

if u.hasmarshaler && !deterministic {
		...
}

We also have to worry about
https://github.com/golang/protobuf/blob/master/proto/table_marshal.go#L310-L314
Which means that if there is a Marshaler the size cache is not set, but the non generated marshal function assumes that it is set.

How do we solve this?

[dsnet]

The unfortunate reality is that we can't reconcile this issue in the present API. If deterministic marshaling is enabled, it will just have to be a best effort as it is impossible to know whether custom implementations are deterministic or not.

The design of the protoiface API is intended to enable to safer way for custom implementations to coexist.

[awalterschulze]

Wouldn't it be more appropriate to tell the user that they are not getting the output by returning an error.

if u.hasmarshaler {
                if deterministic {
                        return nil, fmt.Errorf("deterministic not supported by the Marshal method of %v", u.typ)
                }
		m := ptr.asPointerTo(u.typ).Interface().(Marshaler)
		b1, err := m.Marshal()
		b = append(b, b1...)
		return b, err
}

[awalterschulze]

@dsnet ^^ ?

[dsnet]

Strict erroring for custom unmarshalers sounds good to me. Would you like to submit a PR?

[awalterschulze]

That is great to hear :)

Just checking you mean marshalers and not unmarshalers, right?

[dsnet]

Correct.

[awalterschulze]

@jmarais is working on a pull request.

Thank you so much :)