Swap order of encoding.TextUnmarshaler and encoding.BinaryUnmarshaler

[gust1n]

We're transitioning to using the Go 1.18 netip.AddrPort with go-envconfig parsing it from a string. We noticed a small corner case though, that some addresses don't fail on the binary unmarshaling

go-envconfig/envconfig.go

Line 521 in befaf9a

if tu, ok := iface.(encoding.BinaryUnmarshaler); ok {

and therefore we end up with a really weird IPv6 address.

So e.g. "239.255.76.67:7667" would succeed (not error) when unmarshaled as binary (resulting in [3233:392e:3235:352e:3736:2e36:373a:3736%6]:13623 but e.g. 127.0.0.1:11311 would fail and fall back to using the TextUnmarshaler, resulting in a properly decoded IPv4 address.

Could we swap the order so the unmarshaling as text happens before the binary one?

[sethvargo]

Hey @gust1n - thanks for opening an issue. Of course, it's technically possible to switch the order, but I'm worried about how switching the order might impact someone who is behaving on the current behavior (binary before text).

Is there a reason this shouldn't be considered an upstream bug in netip.AddrPort? If the custom binary unmarshaler is misbehaving, it seems like we might want to try and get that fixed upstream.

I know it's not ideal, but you could also wrap a custom type and define EnvDecode on it, since that takes precedence.

type AddrPort netip.AddrPort

func (p *AddrPort) EnvDecode(val string) error {
  return p. UnmarshalText(val)
}

I'm definitely open to the change, but there doesn't seem to be clear guidance from Go on the "order" in which to test unmarshalers, and this feels like a corner case for this particular type.

[gust1n]

Hehe I thought you would say this (not wanna risk others relying on the current order) which totally makes sense. And I can for sure open an upstream issue too!

However, when talking to some colleagues, we figured in the case of this library, it's probably more common to unmarshal text than binary, given that it's main use case is configuration (most often input from humans). Given that reasoning perhaps unmarshaling text should come first?

[sethvargo]

Okay - let's do it. Do you want to put together a PR?

[gust1n]

Sure, will fix!