-
Notifications
You must be signed in to change notification settings - Fork 218
Go: ergonomics improvement for Option
and Result
type
#625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
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:
Creating Int's for a slice will not cause allocations for a slice like |
I've tried reading through all the material and my preference, if it is at all possible, is in line with #612 (comment) 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. |
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. |
I previously (briefly) looked for documentation on |
We face a practical problem with 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 #612 (comment), the current solutions to this problem are generics ( |
If we want // 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)
}
...
} |
Given the above problems, I think generics are possibly the least-bad solution for |
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/ |
@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: 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 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. |
Ah, apologies, this is exactly what I was suggesting; using generic forms of e.g. |
Here is the current wasi-preview2 definition of (stream)
In practice I would hope we can have an implementation of |
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. |
That is a doozy. Well, look at that file it's clear that 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:
|
Another relevant example: https://github.com/bytecodealliance/wasmtime/blob/71d0418e4fbf203090e11f9928e36bc12a11437a/crates/wasi/wit/deps/http/types.wit#L47
(note that this doesn't represent a map; http headers can have repeat keys) |
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... |
It's a minimalism thing for me, with generics we need |
@johanbrandhorst
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). |
My example was poor, I didn't mean variadic, I meant what you posted 😁 |
The discussion appears to have tapered off. Here's a summary of the points that have been addressed so far:
|
Did we land on anything WRT non-wrapped Other then that comment, I think your summary looks great to me! |
Ah I missed it. We didn't land it but plan to :) |
For a 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
}
} |
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 type SomeOperationResult struct {
Result resultType
Error error
}
func FunctionReturningWrappedResult() (*SomeOperationResult, error) |
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. |
Couldn't that equally be a generated wrapper type?
|
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. |
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 |
WIT The more I think about this the more appealing a single generic |
I confess my ignorance here with the complexities of WIT syntax, perhaps a generic I guess my view is still "lets implement something and iterate on it". |
I don't think anyone has enough experience writing WIT to gauge how practical particular examples might be. 🙂 Here's one that feels to me like it has a plausible shape: variant run-result {
completed(result<...>),
timed-out,
}
run: func() -> run-result |
Agree with @johanbrandhorst:
One challenge I see is that There’s two halves to this: generating glue code to the WASI ABI ( |
For some context, @ydnar and I have created a post on BCA's zulipchat discussing the possibility of emitting JSON from the description of the WIT that could be consumed by Golang. There are some interesting discussions over there. Do you think it's worth to open another issue on this repo for introducing either a component interface or JSON that consumed by language generators written in their native languages (like |
Personally I don't think there'd be an issue with something like that, and I definitely agree that writing bindings generators in the language they're generating bindings for is almost always the best way to go. The main hurdle currently for writing a bindings generator in Go is parsing a WIT document and dependencies and such as that would require duplicating quite a lot of the Rust library If that's the case then the main thing to focus on would be a The easiest way to do that would be to slap |
If the group is interested, I actually started this effort 2 weeks ago. I was frustrated I needed to learn rust to do a simple edit to the parser. Read this as I dont have the cycles to learn rust right now, not that I think doing it in rust is the wrong answer. None the less, I haven't written a parser since university, so it was a good exercise that I intend on seeing though. |
Example usage here: https://github.com/ydnar/wasm-tools-go/blob/94aed292cc8edc3f8e2efd189c9bbc84f6a02b86/wasi/cli/environment/environment.wit.go#L38 Considerations that drove this design:
|
Similarly, |
I think it would be worthwhile to special-case certain function return type patterns, e.g.: // opt: func() -> option<t>
func Opt() (T, bool) { ... }
// res: func() -> result<t, e>
func Res() (T, E) { ... } ...with a general-purpose representation in any other position, e.g.: // opt-res: func() -> option<result<t, e>>
func OptRes() (Result[T, E], bool) { ... }
// res-opt: func() -> result<option<t>, e>
func ResOpt() (Option[T], E) { ... } |
I think we should be careful about leaning too heavily on this. While it is certainly true that some interfaces will end up being wrapped by libraries, it would be unfortunate if this kind of wrapping was (essentially) mandatory, especially for "business logic" interfaces that are internal to some particular project. In that context, I am skeptical of the linked |
I agree somewhat that having more idiomatic Go interfaces on the caller side of WASI imports could be beneficial to someone who chiefly writes Go code and expects interfaces to follow Go conventions, versus a Go programmer explicitly targeting WASI APIs, who might have different expectations. I’m thinking of a couple different costs: the cost of a programmer reading WIT documentation and mentally translating to how this works in Go, and second, the runtime cost of doing the conversion. Should the caller have control, or should it be mandatory in the generated code? One benefit of using predicable, zero or low-cost abstractions like Ultimately, I think real-world usage will help inform this either way. I figure we’ll see what works and what doesn’t, and adjust accordingly. |
Cross-posted from #626: we now have working Go bindings generated from WIT: https://pkg.go.dev/github.com/ydnar/wasm-tools-go/wasi
wit-bindgen-go generate ./path/to/wasi-cli/wit # if wasm-tools is in path Or: wasm-tools component wit -j ./path/to/wasi-cli/wit | wit-bindgen-go generate These are currently being used in a |
Now that we have something working, I think it’s probably worth exploring whether to special-case certain patterns:
In practice, the use of Noting some tradeoffs: Some of these patterns will move allocations to the heap (namely returning Certain |
I would still argue for returning
This is easy to misinterpret if someone is looking only at the Go signature. |
Could you expand a bit on this? |
Appreciate that feedback. However, when we tried the comma-ok form, it wasn’t great in practice. The func (o *cm.Option[T]) Some() (T, bool) We changed it to: func (o *cm.Option[T]) Some() *T Which let the caller unwrap the option when passing the This has the benefit of not escaping to the heap if the caller holds the |
Let’s say you have a The layout for that struct {
tag uint8
data string
} Problem 1: invalid pointersSince the layout of If the GC needs to grow the stack, and this Problem 2: untracked pointersProblem 1 can be avoided if the representation of the struct {
tag uint8
data [2]uint32 // could be uintptr on wasm32
} However, this creates a new problem: if the This is fine if the caller holds onto the string (say with However, either case requires someone (either the caller, or the CM machinery) to explicitly drop a reference to any pointer used in a Solutions
|
Thanks for the explanation! Is this all scoped to just tinygo? I know tinygo's conservative GC would give a lot more flexibility in recursive stack allocations (like |
Everything above refers to the mainline Go GC, (along with the One TODO I have is to add facilities to the Re: nested |
Tinygo's GC should be fine with your current approach, I think. Its conservative GC treats anything that looks like it could be a pointer as a potential pointer, so the worst case is that a non-pointer accidentally keeps some memory alive that should have been collected (admittedly a lot more likely in wasm...). |
|
Closing this issue since we have deprecated the TinyGo bindgen in this repo (#1194) and moved to a new implementation at bytecodealliance/go-modules |
This issue is created for a discussion on improving the
Results
andOptions
type in the Go bindgen.To see how these types are generated, please read the head of this #612 (comment).
A summary of what's being proposed: #612 (comment)
The text was updated successfully, but these errors were encountered: