encoded: Unalias interfaces

[abhinav]

The go.uber.org/encoded package declares three interfaces as aliases of internal package versions of those interfaces.

package encoded

type DataConverter = internal.DataConverter
type Value = internal.Value
type Values = internal.Values

This is both, unnecessary and undesirable.

It is unnecessary because interfaces in Go are implicit. Unless you're doing something very specialized, where interface identity matters more than interface compliance, you can just redeclare the interfaces in the encoded package and things will continue to work.

package encoded

type Value interface {
  HasValue() bool
  Get(valuePtr interface{}) error
}

Any value that satisfies internal.Value will also satisfy encoded.Value.

You can even add a bidirectional compile-time check to verify that the interface doesn't go out of sync with its internal variant.

var (
  _ Value = (internal.Value)(nil)
  _ internal.Value = (Value)(nil)
)

It is undesirable because it makes it difficult for users to use the interface. They cannot just look at the godocs; they have to dig into the internal packages. It also negatively affects the behavior of tooling like mockgen (https://github.com/golang/mock/issues/244).

---

Given this, is there a strong reason to keep the aliases? Is interface identity necessary or will compliance suffice?

[linzhp]

Another solution maybe removing internval.Value and declaring the interface as encoded.Value. It's OK for an internal package to use a public interface.

[mfateev]

I tried doing this. The problem is that that Go doesn't recognize method signature change:

../client/client.go:403:27: cannot use "go.temporal.io/temporal/internal".NewClient(service, domain, options) (type "go.temporal.io/temporal/internal".Client) as type Client in return argument:
    "go.temporal.io/temporal/internal".Client does not implement Client (wrong type for QueryWorkflow method)
        have QueryWorkflow(context.Context, string, string, string, ...interface {}) (*"go.temporal.io/temporal/internal".EncodedValue, error)
        want QueryWorkflow(context.Context, string, string, string, ...interface {}) (encoded.Value, error)

I verified that internal.EncodedValue implements encoded.Value interface.

[abhinav]

It wouldn't because internal.Foo references type internal.Bar. For such cases, the public version of the interface has to create wrapper implementations that reference the public version. Basically, something similar to #929.