wit-bindgen icon indicating copy to clipboard operation
wit-bindgen copied to clipboard

Go: ergonomics improvement for `Option` and `Result` type

Open Mossaka opened this issue 1 year ago • 49 comments

This issue is created for a discussion on improving the Results and Options type in the Go bindgen.

To see how these types are generated, please read the head of this https://github.com/bytecodealliance/wit-bindgen/issues/612#issue-1801773199.

A summary of what's being proposed: https://github.com/bytecodealliance/wit-bindgen/issues/612#issuecomment-1644691976

Mossaka avatar Jul 20 '23 22:07 Mossaka

Hi @Mossaka,

Thanks for opening this.

My personal preference is the sealed interface. I see in the original issue some concern about allocations related to passing back ints, but unless I'm mistaken, that's not an issue anymore.

Given the following:

type IsRet interface {
   isRet()
}

type Int int64

func (_ Int) isRet() {}

type String string

func (_ String) isRet() {}

Creating Int's for a slice will not cause allocations for a slice like var returns []IsRet. The interface value itself, being the 2 64bit quantities, will store the int itself and the type information. There will be allocation within the slice itself, but no per-value allocation outside of that.

evanphx avatar Jul 24 '23 20:07 evanphx

I've tried reading through all the material and my preference, if it is at all possible, is in line with https://github.com/bytecodealliance/wit-bindgen/issues/612#issuecomment-1634843709 from @jordan-rash. If at all possible, we should avoid interfaces and generics and just flatten to multiple returns.

If there are cases where this is not possible, I think the sealed interface is a better solution than the generic result/option type proposed.

johanbrandhorst avatar Jul 24 '23 21:07 johanbrandhorst

Looking at the proposals again, I'd agree with @johanbrandhorst that if it's possible to (mostly) neatly map them to multiple returns, that's the most idiomatic Go.

evanphx avatar Jul 24 '23 21:07 evanphx

@evanphx

The interface value itself, being the 2 64bit quantities, will store the int itself and the type information.

I previously (briefly) looked for documentation on gc's optimizations trying to figure out if it does this. Any suggestions on where to look to figure out what it will do?

lann avatar Jul 25 '23 13:07 lann

We face a practical problem with result, option, and tuple types: they are parameterized anonymous types with no obvious ergonomic mapping to Go's type system in all cases. For example, given the WIT interface:

interface result-cache {
  // Outer result reflects whether the cache key is set
  // Inner result is the actual cached result
  get-result: func(key: string) -> result<result<string>>
}

We might generate some Go:

type WhatDoWeCallThis {
    Ok string
    Err error
}

interface ResultCache {
    // We can flatten the outer `result` into an idiomatic (value, error) return
    GetResult(key string) (WhatDoWeCallThis, error)
}

As described in https://github.com/bytecodealliance/wit-bindgen/issues/612#issue-1801773199, the current solutions to this problem are generics (Result[T, E] and Option[T]) and unpleasant generated names for tuples (e.g. Tuple4StringU32U64MynamspaceMypackageAV1T).

lann avatar Jul 25 '23 14:07 lann

If we want result error types to implement error we'll need a wrapper type in some cases, which would also be anonymous:

// Errors a la `errno`
write: func(buf: list<u8>) -> result<u64, u16>
type WhatDoWeCallThis u16

func (err WhatDoWeCallThis) Error() string { ... }

func Write(buf []byte) (uint64, error) {
    ...
    if err != 0 {
        return 0, WhatDoWeCallThis(err)
    }
    ...
}

lann avatar Jul 25 '23 14:07 lann

Given the above problems, I think generics are possibly the least-bad solution for results and options (and tuples, though that's a separate issue) [edit: in scenarios where we can't generate nice idiomatic Go interfaces], but I'd be happy to see better alternatives.

lann avatar Jul 25 '23 14:07 lann

@evanphx

The interface value itself, being the 2 64bit quantities, will store the int itself and the type information.

I previously (briefly) looked for documentation on gc's optimizations trying to figure out if it does this. Any suggestions on where to look to figure out what it will do?

It's not an optimization, which is probably why you didn't see anything about it. It's a fundamental aspect of Go's memory model. This talks a bit about the the memory model of interface values: https://www.timr.co/go-interfaces-the-tricky-parts/

evanphx avatar Jul 25 '23 16:07 evanphx

@lann Just not wanting to pick names for things seems like a thin reason to abandon the concept all together. The names, it turns out, rarely matter here because the users will largely be interacting with the values via local type inference: num, res := Write(buf).

I'm not opposed to using generics, just wanted to comment on what is today the least likely to surprise users.

Rather than talk in abstracts, I'd love to get a look at some real wit types that need generating. It seems like mapping write(buf) -> result<u64, error> to func Write(buf []byte) (uint64, error) would be a likely situation that wit would see, and that type of function would be one that Go users fully expect.

There will be the exceptions, no doubt, and perhaps that's where generics could be used. But I think we should have an idea of how much code we're talking about.

evanphx avatar Jul 25 '23 16:07 evanphx

There will be the exceptions, no doubt, and perhaps that's where generics could be used.

Ah, apologies, this is exactly what I was suggesting; using generic forms of e.g. Result[T,E] in edge cases like my (contrived) example -> result<result<...>> which don't map nicely to idiomatic Go. I am very much in favor of generating idiomatic interfaces where they make sense.

lann avatar Jul 25 '23 16:07 lann

Here is the current wasi-preview2 definition of (stream) read, which conveniently includes a problematic return type with an anonymous tuple nested in a result: https://github.com/bytecodealliance/wasmtime/blob/main/crates/wasi/wit/deps/io/streams.wit#L74-L78

-> result<tuple<list<u8>, stream-status>, stream-error>

In practice I would hope we can have an implementation of io.Reader for wasi streams, but the WIT makes for a good general example.

lann avatar Jul 25 '23 17:07 lann

I briefly discussed this exact issue with @Mossaka and agreed that for any wrapping of a result, we'll need an intermediate type (e.g.option<result<...>> which seems to be not too uncommon). My suggestion was to special case the non-wrapped result to a (value, error) tuple, which seems to be the consensus here too. In the wrapped case though, there still seems to be a question of whether to use a generic type or generate a named type based on context. I'm personally in favor of generating a named type and avoiding generics in the initial release. My view is that we can iterate on that by adding generics later if desired, but we can't remove the generics if we start from there. IMO generated type names are (especially for return values) not that big of a deal, as @evanphx suggests.

johanbrandhorst avatar Jul 25 '23 17:07 johanbrandhorst

That is a doozy. Well, look at that file it's clear that tuple<> is a bit part of this.

For a generic tuple, what we'd probably really like is a variadic generic, but those don't exist in Go. Which means for there should be an attempt to special case 'result<tuple<' as much as possible to create idiomatic Go, otherwise the only other option is generic struct types.

For the above read signature, I'd probably expect:

func Read() ([]u8, StreamStatus, error)

evanphx avatar Jul 25 '23 17:07 evanphx

Another relevant example: https://github.com/bytecodealliance/wasmtime/blob/71d0418e4fbf203090e11f9928e36bc12a11437a/crates/wasi/wit/deps/http/types.wit#L47

new-fields: func(entries: list<tuple<string,string>>) -> fields

(note that this doesn't represent a map; http headers can have repeat keys)

lann avatar Jul 25 '23 17:07 lann

I'd be interested to hear more about the desire to avoid generics. The bulk of my Go experience was pre-1.18 and I haven't really used them enough to form my own opinions...

lann avatar Jul 25 '23 17:07 lann

It's a minimalism thing for me, with generics we need type Tuple1[T, ...] etc in our runtime package, but without it we just generate the necessary types in each generated file. Generics don't provide better ergonomics than a generated type, IMO.

johanbrandhorst avatar Jul 25 '23 18:07 johanbrandhorst

@johanbrandhorst type Tuple[T, ...] isn't valid either, there are no varadic templates. So we'll end up with something like:

type Tuple1[T] struct { A T }
type Tuple2[T1, T2] struct { A T1, B T2 }
type Tuple3[T1, T2, T3] struct { A T1, B T2, C T3 }

etc. That's not awful but it's starting to border on if normal struct types should be used instead (the lack of usable field names for both types is awkward regardless).

evanphx avatar Jul 25 '23 18:07 evanphx

My example was poor, I didn't mean variadic, I meant what you posted 😁

johanbrandhorst avatar Jul 25 '23 18:07 johanbrandhorst

The discussion appears to have tapered off. Here's a summary of the points that have been addressed so far:

  1. Special handling for the non-wrapped Result to a (value, error) tuple
  2. For wrapped Result type, we use named types and avoiding generics in the initial release
  3. We want to think about special hanlding for more common wrapped types like result<tuple<

Mossaka avatar Aug 02 '23 02:08 Mossaka

Did we land on anything WRT non-wrapped Option?? I would like to see it return a *value if possible.

Other then that comment, I think your summary looks great to me!

jordan-rash avatar Aug 02 '23 21:08 jordan-rash

Did we land on anything WRT non-wrapped Option?? I would like to see it return a *value if possible.

Ah I missed it. We didn't land it but plan to :)

Mossaka avatar Aug 02 '23 22:08 Mossaka

For a result type, what about using a naked any, with an un-exported errorResult to hold the error value (to address the case where the stored value implements the error interface)?

type Result any

type errorResult error

func ErrorResult(err error) Result {
	return errorResult(err)
}

func Error(r Result) error {
	switch t := r.(type) {
	case errorResult:
		return error(t)
	default:
		return nil
	}
}

func Unwrap(r Result) (any, error) {
	switch t := r.(type) {
	case errorResult:
		return nil, error(t)
	default:
		return t, nil
	}
}

ydnar avatar Aug 07 '23 20:08 ydnar

Interesting, it prevents users from creating their own error results, I guess? I don't love that it doesn't say anything about the use of the type itself though. Maybe Unwrap could be a method on the result type? In any case, it isn't clear to me that we need a "general" result type with the latest proposal - we either flatten it to (resultType, error) or wrap it in a generated type

type SomeOperationResult struct {
    Result resultType
    Error error
} 

func FunctionReturningWrappedResult() (*SomeOperationResult, error)

johanbrandhorst avatar Aug 09 '23 22:08 johanbrandhorst

Unfortunately you can't put methods on an interface receiver.

As @Mossaka pointed out, a WIT may define a function with a result as an argument. This implies the need for a Result type in places where returns couldn't be flattened.

ydnar avatar Aug 09 '23 23:08 ydnar

Couldn't that equally be a generated wrapper type?

type SomeOperationParam1 struct {
    Value resultType
    Error error
} 

func FunctionReturningWrappedResult(param SomeOperationParam1) (error)

johanbrandhorst avatar Aug 09 '23 23:08 johanbrandhorst

If we're going to generate wrappers for results I'd like to understand the naming scheme for those wrappers. It seems quite easy to end up with conflicts, especially with combining two internal types.

lann avatar Aug 10 '23 00:08 lann

Yep, care would have to be taken to use a deterministic and unique naming scheme. I think it's easy to come up with a trivial algorithm, could you share an example of the sort of conflict you're thinking of? My trivial algorithm would be PackageName+FunctionName+ParameterIndex.

johanbrandhorst avatar Aug 10 '23 00:08 johanbrandhorst

My trivial algorithm would be PackageName+FunctionName+ParameterIndex.

WIT result types can appear anywhere any other type can, so we would need at least a fallback algorithm for wrappers that aren't immediately associated with a single function parameter.

The more I think about this the more appealing a single generic Result[T,E] seems to me...

lann avatar Aug 10 '23 14:08 lann

I confess my ignorance here with the complexities of WIT syntax, perhaps a generic Result is the right way to go, but it'd be easier for me and I'm sure others if we had practical examples of the sort of definition that would be impractical using generated wrapper types. In any case, if we do need a type, a generic Result[T, E] would be preferable to me to the suggestion made by Randy which is hard for a user to use without examples.

I guess my view is still "lets implement something and iterate on it".

johanbrandhorst avatar Aug 10 '23 14:08 johanbrandhorst

I don't think anyone has enough experience writing WIT to gauge how practical particular examples might be. :slightly_smiling_face:

Here's one that feels to me like it has a plausible shape:

variant run-result {
  completed(result<...>),
  timed-out,
}
run: func() -> run-result

lann avatar Aug 10 '23 14:08 lann