Proposal: Prefer interfaces to function references

[abhinav]

This one requires some nuance but generally, for public APIs in libraries,
often when a function reference is expected, an interface is preferable.

Demonstration:

For example, in Zap, we have [zapcore.EncoderConfig] with a few EncodeFoo
fields that all accept a FooEncoder type, where FooEncoder is a function
reference.

type EncoderConfig struct {
    // ...
    EncodeCaller CallerEncoder
    // ...
}

type CallerEncoder func(EntryCaller, PrimitiveArrayEncoder)

This is limiting because this means that a CallerEncoder can only ever have
that signature.

If it was, on the other hand, an interface, we would have a non-breaking
upgrade path to change that signature with the help of upcasting.

type CallerEncoder interface {
    EncodeCaller(EntryCaller, PrimitiveArrayEncoder)
}

For example, if we decided to change how we represent caller information, we
could do that by declaring a new interface, and specifying that if the
CallerEncoder implements that interface, we'll use it.

type CallFrame struct{ ... } // theoretical new representation

type CallFrameEncoder interface {
    EncodeCallFrame(CallFrame, PrimitiveArrayEncoder)
}

type EncoderConfig struct {
    // ...
    
    // If this implements CallFrameEncoder, that will be used instead.
    EncodeCaller CallerEncoder
}

---

The above is just a demonstration of a case where this guidance would have
been useful. If we liked this, we would have to turn it into a nuanced
guidance. The guideline could largely be ignored for unexported APIs, and
largely just applies to exported API surfaces.

[prashantv]

I generally agree that interfaces are more powerful as they can be upcast, and it can be more performant too (an existing object can be passed vs functions often alloc when they're not global)

However, upcasting is also more subtle and not explicitly documented by the types. In the above example, a new field EncodeCallFrame func(CallFrame, PrimitiveArrayEncoder) could be added to support a "newer" method, and it's more explicitly part of the API surface.

[peterbourgon]

Interface upgrades effectively prevent the use of the decorator pattern, which is one of the most useful things you can do with the things. Things like http.Hijacker are hacks that should be avoided, not patterns to copy! 😄

[rabbbit]

@peterbourgon could you add an example of what exactly you meant here? :>

[peterbourgon]

If you have some code that takes an e.g. io.Reader and relies on interface upgrades to do something special with it

func Reticulate(r io.Reader) {
    fr, ok := r.(peterb.FastReader)
    ...

then your function signature is lying, you're not actually just consuming an io.Reader, and I can't do things like

a := peterb.GetFastReader()
b := peterb.SomeOtherFastReader()
Reticulate(io.MultiReader(a, b))

because the io.MultiReader layer obscures the underlying types and their capabilities. And if you want to lift the capabilities up through that layer, there's literally no way to do it generally — you have to do some pants-on-head crazy shit to even approximate a solution.

Interface upgrades are kind of like runtime.SetFinalizer or sync.Pools, they're fine if you use them like optimizations, but you can't rely on them for anything. Leveraging them as part of an API evolution strategy is... well, in a pinch, it could be the least-bad option among bad options, but, like package reflect or the empty interface, it's a tool of last resort, not something you should be codifying as good practice.

[rabbbit]

Ack - thx. I do like the example - quite convincing ;)

…

On Tue, 3 Aug 2021 at 06:27, Peter Bourgon ***@***.***> wrote: If you have some code that takes an e.g. io.Reader and relies on interface upgrades to do something special with it func Reticulate(r io.Reader) { fr, ok := r.(peterb.FastReader) ... then your function signature is lying, you're not actually just consuming an io.Reader, and I can't do things like a := peterb.GetFastReader()b := peterb.SomeOtherFastReader()Reticulate(io.MultiReader(a, b)) because io.MultiReader can't be upgraded without some pants-on-head crazy shit <https://github.com/felixge/httpsnoop/blob/42c30f944879059639bcf7b2ca4aba06f4a83198/wrap_generated_gteq_1.8.go#L66-L81> . Interface upgrades are kind of like runtime.SetFinalizer or sync.Pools, they're fine if you use them like optimizations, but you can't rely on them for anything. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#129 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACG3UTLIM5DFUXARXLWQILT27VDJANCNFSM5ABRV5LA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[prashantv]

I don't think the intent of this issue is to recommend interface upgrades as good API, but rather that interfaces allow for flexibility that a func does not. Where possible, explicit APIs are better (as mentioned in my comment earlier, I actually recommend a separate field vs interface upgrade for the initial example).

I think the focus should be on whether a single-method interface should be preferred to using function types. As an example, io.Writer has a single function, so it could be defined as type Writer func(p []byte) (n int, err error), but this guidance recommends an interface instead, which allows for optimizations like StringWriter, and is generally more efficient (or passing existing objects).

Similarly, it's possible to wrap a function to implement an interface (e.g., http.HandlerFunc) efficiently, while passing an interface's method often requires an allocation.

[peterbourgon]

Right, right. And re-reading the OP I think we're basically in agreement 👍 and I just got focused on what are ultimately some minor details.