make it possible to pass options to a transport constructor

[marten-seemann]

Background: The libp2p constructor uses reflection magic to figure out the arguments that a Transport requires. This allows us to write

host, err := libp2p.New(
     libp2p.Transport(tcp.NewTCPTransport),
     libp2p.Transport(quic.NewTransport),
)

even though the TCP and the QUIC constructor have completely different signatures.

libp2p/go-tcp-transport#99 now adds TCP options as variadic parameters to the TCP constructor. #1204 added a fix to simply ignore all variadic parameters, making it possible to merge the PR in the TCP repository, but leaving us unable to actually use these options.

This PR now makes it possible to use the option. For example, you can now write

host, err := libp2p.New(
     libp2p.Transport(tcp.NewTCPTransport, tcp.DisableReuseport(), tcp.WithTimeout(42*time.Second)),
)

Variadic function parameters are then passed to the constructor, after we've checked that the constructor actually accepts options and after checking that the type of the parameters passed into libp2p.Transport matches the type expected by the constructor.

@mvdan, can I ask you for a review of this PR?

[mvdan]

I think the design makes sense. My only worry as a user is that opts ...interface{} is very unsafe at a static level, but I don't think you can do any better without generics. Once you had generics, you could do funky stuff like:

package libp2p

type constructor[T any] func() *T

type option[T any] func(*T)

func Transport[T any, TConstructor constructor[T], TOption option[T]] func(ct TConstructor, opts ...TOption) Option

Note that this would not support arbitrary parameters before the variadic options, like the upgrader in func NewTCPTransport(upgrader *tptu.Upgrader, opts ...Option). I don't think 1.18 generics has support for "arbitrary parameters" like that. But perhaps those could be redesigned to fit generics, to perhaps even remove the need for reflection.

Very much getting off-topic here. Just food for thought, in case you considered generics :)

[mvdan]

From the spec:

Otherwise, the value passed is a new slice of type []T with a new underlying array whose successive elements are the actual arguments, which all must be assignable to T.

That is, checking for identical types seems wrong; you should instead use opt.AssignableTo(wantType).

You should add a test case covering this, too. A fairly normal example would be for an Option to be of type type Limit int64, and for you to pass a regular old int64 value. Not identical types, but assignable.

I get that this will likely not happen in practice, as I seem to understand practically all options will be function types. But the named-vs-unnamed distinction could also apply to functions, given that type Option is a named type and not an alias type. And I think it would make more sense to apply the same rules that the Go spec does for variadic arguments.

https://golang.org/ref/spec#Assignability has more details on the rules. Most rules seem like they'd never apply, so we don't need to worry about e.g. the line involving channels. You might want to special case untyped nil and reject it, though, as I think it never makes sense as an option.

[mvdan]

(ah, you already reject untyped nil for incoming options, good!)

[marten-seemann]

Good point. AssignableTo doesn't work though, it's ConvertibleTo that we're looking for here. We then also need to explicitly do the type conversion using reflect.Value.Convert.
Only caveat here: There's a potential failure case when the argument is for example an int32, and you pass a value larger than math.MaxInt32 to the constructor. Go will happily convert this value, leading to an overflow. I'm not very concerned about this corner case in practice though.

[mvdan]

How did you arrive at the conclusion that you want convertible? I'm fairly sure you want assignable, which is what the spec says, and which does not have the problem with lossy conversions that you mention. Assignments are never lossy, because the rules are stricter than explicit conversions.

[marten-seemann]

A type limit int is not AssignableTo an int, but it is ConvertibleTo it.

[mvdan]

You're right, that was a bad example. That follows the rules we want, I think, because int is a named type. I feel like integers are a bit of a red herring here - in practice you're only going to care about function types, I'm pretty sure.

[marten-seemann]

I'm not sure I'm following. With functions, we'd only use the functions defined in the package that defined the Option anyway, so there's no need for converting or assigning anything?

[mvdan]

Right. For functions, checking for type identity via == is enough. I was just saying that, from a Go spec perspective, checking for assignability is more correct and consistent with what people might expect. In practice, I imagine either will be fine.

[marten-seemann]

Ok, I've reverted it back to a == (keeping the other improvements you suggested). We can still change this later, should the need arise.

[mvdan]

SGTM!