Get rid of a lot of error handling

[stefanhaller]

Error handling in go is cumbersome and noisy, and I'd like to restrict it to cases where real errors can occur (e.g. IO or network errors, or errors from calling external tools, etc.).

Sometimes we have controller code that wants to call 4 things in a row, all of which can return errors. This kind of situation is problematic for two reasons:

• when we simply return each time one of the calls returns an error, we might miss the important call at the end that needs to happen no matter what (e.g. Refresh).
• if we do make sure that the last call happens no matter what, we have to deal with the possibility of getting errors from more than one of the calls, so we have to decide which error is the most important one to report to the user. This leads to code that is more complicated than necessary, and it makes writing new code harder because thinking about this problem is hard.

Now, a lot of functions in the lazygit code base return errors, but these can only occur when the function is called with invalid arguments (e.g. calling SetCurrentView with a view name that doesn't exist). These are programming errors, so it makes little sense to report them to the user; we might as well panic right away. This is closer to an assert call in other languages.

So I'm proposing to get rid of the error return values of most of the UI-related functions like HandleFocus, HandleRender, ContextMgr.Push, etc., turning any error conditions inside these into panics. This should simplify a lot of lazygit's code.

Opinions welcome.

[jesseduffield]

That sounds good to me. So long as we don't end up with a big uptick in run-time panics caused by issues that aren't unrecoverable errors, cos that would be annoying for a user.

[stefanhaller]

In my opinion it's not so much a distinction between recoverable or unrecoverable, but between programming errors and legit runtime errors. An error resulting from a programming mistake could be recoverable, but that's no reason not to panic anyway. It's a good thing that it crashes, so that users report these and we can fix them more quickly.

[jesseduffield]

So long as the programming error is indeed easy to fix! For example if it turned out that we have a bunch of programming errors that lead to intermittent, hard to debug, concurrency issues which causes everybody to start getting panics, that would be bad.

But perhaps I'm using a more broad definition of programming error than you.

[stefanhaller]

But perhaps I'm using a more broad definition of programming error than you.

That's very well possible. I'm mostly thinking about violated preconditions in a design-by-contract situation, e.g. this one.

There's a grey zone, of course. For instance, ContextMgr.Pop() errors when the stack is empty, which I would also call a programming mistake; now, you can imagine this being caused by some concurrency issue (theoretically, maybe it's not the best example).

However, I still think returning an error (and eventually showing it to the user in a panel) is not the right thing to do here. We can decide to be graceful about certain things instead of panicking, but we should never return errors for them.

[jesseduffield]

I agree with that

[stefanhaller]

And sometimes it's not totally clear how to decide this. For example,

func (v *View) SetOriginX(x int) error {
    if x < 0 {
        return ErrInvalidPoint
    }
    v.ox = x
    return nil
}

This could either

• panic
• do nothing
• clamp the value to an allowed one.

What's best? I tend to think clamping is probably best.

[jesseduffield]

I agree. I guess because view-layer stuff is just not very critical.

[stefanhaller]

How do you feel about changing gocui? When I wrote this issue I was mostly thinking about code in lazygit, but cleaning up these would be nice too. That would be a breaking change for other projects using gocui though (lazydocker, lazynpm), and I'm not keen on adapting these...

[jesseduffield]

I'm fine to go and fix up lazydocker next time I make changes to it. And lazynpm is on life support anyway

[stefanhaller]

I made a PR: #3890.

It would be good if we could merge this relatively quickly, as it has a lot of potential to conflict with other PRs. (It conflicts with #3888, for example.)