Rework the API

[josharian]

If we like it, this

Fixes #2
Fixes #3

[josharian]

I just tried integrating this into an internal-use-only HTTP handler wrapper, to see how it felt. The handler setup is similar to tshttp: It's http.Handler but it has an error return. You can return an error at any point, and it does useful things with that returned error. With some careful thinking around the defer and a little anonymous function, the semantics end up identical: return err === try.E.

	var err error
	var frame runtime.Frame
	func() {
		defer try.Recover(&err, func(f runtime.Frame) {
			frame = f
		})
		err = fn(w, r)
	}()
        // handle err as before

Then in the failure path, I annotate the error message with the frame information before logging it, which makes this noticeably better. Same semantics, easier to use, better error messages.

	// Annotate internalErr with frame information, if present.
        // (Frame information will only be available if the error came via try.E, rather than being returned by value.
	if frame.File != "" {
		internalErr = fmt.Errorf("%s:%d: %w", frame.File, frame.Line, internalErr)
	}

So far, I rather like it. :)

[dsnet]

What are your thoughts on the following:

type Error struct {
    Frame runtime.Frame
    Err Error
}

func (e *Error) Error() string {
    return fmt.Sprintf("%s:%d: %v", frame.File, frame.Line, err)
}
func (e *Error) Unwrap() error {
    return e.Err
}

func Recover(func(*Error))
func Handle(*err)
func HandleF(*err, func())

Instead of the TB helper function, we can do:

func TestFoo(t *testing.T) {
    defer try.Recover(func(err *try.Error) { t.Fatal(err) })
    // use try.E throughout your test
}

[dsnet]

golang/go#21498 would be really nice for this application :)

[josharian]

The exposed structure of type Error looks nice. Less magic. I think we'd probably want:

func Recover(func(Error))

instead of

func Recover(func(*Error))

because no one will already have a try.Error floating around that they want to overwrite. :)

I'm not a big fan of this:

    defer try.Recover(func(err *try.Error) { t.Fatal(err) })

compared to:

    defer try.F(log.Fatalf)

It's a line you end up writing and reading a lot. The latter is obvious and trivial. The former has a bit too much punctuation to skim.

But I guess we could also re-implement try.F (as try.Handle?) on top of try.Recover as

// Handle handles errors by calling fn with exactly one arg, of type Error.
// Sample usage:
//    defer try.Handle(t.Fatal)
func Handle(fn func (...any)) {
  try.Recover(func(err try.Error) { fn(err) })
}

The alternative is to make an adapter, so you call something like:

defer try.Recover(try.Any(t.Fatal)))

but I'd rather make the one-liner as simple as possible. Half the point is convenience and obviousness in short tests and throwaway programs.

[josharian]

So if you buy my comments above, the options are:

func F(f func(string, ...any))
func Handle(errptr *error)
func HandleF(errptr *error, fn func())
func Recover(errptr *error, fn func(runtime.Frame))

or

type Error struct {
    Frame runtime.Frame
    Err Error
}
func (e *Error) Error() string
func (e *Error) Unwrap() error

func Recover(func(Error))
func Handle(*err)
func HandleF(*err, func())
func F(fn func (...any))

Though it's more verbose, on balance, I think I prefer the latter. The former F's fmt-style msg+args reads uncomfortably to me.

Looking again at the latter, if we wanted to shrink the surface area, we could:

• try to type-parameterize Recover/F to unify it...but I think that'd hinder docs readability
• unify Handle and HandleF via a variadic ...func(), as you had before...but again, I'm not sure that's better than the bigger API

Tag, you're it. :)

[josharian]

I looked again. :P

The two APIs in my last comment share:

func Handle(errptr *error)
func HandleF(errptr *error, fn func())

So we have:

func F(f func(string, ...any))
func Recover(errptr *error, fn func(runtime.Frame))

vs

type Error struct {
    Frame runtime.Frame
    Err Error
}
func (e *Error) Error() string
func (e *Error) Unwrap() error

func Recover(func(Error))
func F(fn func (...any))

It's not obvious how much value the exported type Error adds. We could reformulate it without exporting type Error, at which point the API could be:

func Recover(func(err error, frame runtime.Frame))
func F(fn func (...any))

which looks an awful lot like the former API...except perhaps better. F is slightly harder to document without reference to an exported type Error, but given its intended use case, I think examples will suffice.

OK, tag you're it again for reals this time.

[josharian]

Implemented my current favorite and pushed here. Some of the docs might need some polishing, but that can happen once the API is settled. New go doc output:

func E(err error)
func E1[A any](a A, err error) A
func E2[A, B any](a A, b B, err error) (A, B)
func E3[A, B, C any](a A, b B, c C, err error) (A, B, C)
func E4[A, B, C, D any](a A, b B, c C, d D, err error) (A, B, C, D)
func F(fn func(...any))
func Handle(errptr *error)
func HandleF(errptr *error, fn func())
func Recover(fn func(err error, frame runtime.Frame))

Handle/HandleF are the original "Handle" functions you had in mind. F is the adapter to work with tb.Fatal/log.Fatal. And Recover is the DIY hook for things like the HTTP handler wrapper discussed above.

[dsnet]

I've come around to your API. One problem with the Error type is that it's unclear whether:

func Handle(*err)
func HandleF(*err, func())

stores the wrapped Error value, or the original error that was passed to E.
It is better to entirely avoid the possibility of ambiguity through API design rather than just documenting it.

[josharian]

I took another pass through, added some more docs, etc. I think it is ready!

[dsnet]

This is the only usage of fmt, which will cause a transitive dependency on reflect, unicode, sort, strconv, and many other packages.

How about we just change r to take in a func(*wrapError) and directly call fn with the *wrapError. Of course, we'll need to add a wrapError.Error method that does the file:line prefix.

[josharian]

Done; now the dep is only strconv.

[josharian]

One more thought/worry: Should Handle unilaterally overwrite errptr? That is, should the implementation be:

func Handle(errptr *error) {
	r(recover(), func(w wrapError) {
		*errptr = w.error
	})
}

like it is now, or should it instead be:

func Handle(errptr *error) {
	r(recover(), func(w wrapError) {
		if w.error != nil {
			*errptr = w.error
		}
	})
}

The latter seems more correct. Otherwise, consider:

func f() (err error) {
  try.Handle(&err)
  return io.EOF
}

Without the proposed modification, f will always return nil instead of io.EOF.

[dsnet]

Ship it!

[dsnet]

Just saw your comment, let me think.

[josharian]

Oh, it's OK! We only call fn from recover in the case in which we catch an error from E.

I've added a test to lock that in.

[dsnet]

Doesn't:

func f() (err error) {
  try.Handle(&err)
  return io.EOF
}

work as expected?

The recovered value should be nil, so the handle function shouldn't even be called.

We should have a test for this case regardless.

[josharian]

Oh GitHub, such an inefficient way to communicate.

[josharian]

OK, I'm hitting merge! Thanks, Joe, this was fun, as always. :) And now I get to use it!

(Want to tag a release after I merge?)

[dsnet]

I'll tag it as v0.0.1. I'd like to take it for a ride myself a bit and then when we're happy with the ergonomics, we can tag it with v1.0.0.

[dsnet]

Yuck. You didn't squash the PR. Now the history is gross 🤮

[josharian]

Sounds good. Also, I foolishly hit the Merge instead of the Rebase button. So if you want a clean commit history, feel free to edit and force push.

I think it is worth writing a blog post about this once you're happy with it.

[josharian]

It's almost like we don't need to bother communicating?

[dsnet]

I did a force push to clean it up. I assigned you as the author.

[dsnet]

https://github.com/dsnet/try/releases/tag/v0.0.1