`result.Result` is a type that was introduced to enable us to
distinguish between *expected* errors (or *bails* in Pulumi parlance)
and *unexpected* errors or exceptions. Prior to Go 1.13, this made a lot
of sense, since there was no standard story on "wrapping" errors, and
thus no way to answer the question "did I end up here (in an error path)
because I meant to, or did I end up here because of a program bug?".
With the introduction of `%w`, `interface { Unwrap() []error }` and
company in Go 1.13, a simpler interface is available: one can use
`errors.Is` and `errors.As` to ask if an error at any point wraps an
error of a certain type. With this, we can simply ask "is there a bail
at any point in this error tree?" rather than having to track this
explicitly using a type such as `result.Result`. The `IsBail` function
was introduced a while ago to this end, but its rollout was not
completed and several uses of `result.Result` remained.
This commit completes the rollout of this simplified interface,
replacing all uses of `result.Result` with native Go errors that may or
may not wrap bails, and all uses of e.g. `res.IsBail` with
`IsBail(err)`. Doing so allows us to remove `result.Result` entirely.
**Background**
The result.Result type is used by our CLI implementation to communicate
how we want to exit the program.
Most `result.Result` values (built from errors with `result.FromError`)
cause the program to print the message to stderr and exit the program
with exit code -1.
The exception is `result.Bail()`, which indicates that we've already
printed the error message, and we simply need to `exit(-1)` now.
Our CLI command implementation use `cmdutil.RunResultFunc` which takes a
`func(...) result.Result` to implement this logic.
`cmdutil` additionally includes a `cmdutil.RunFunc` which takes a
`func(...) error` and wraps it in `RunResultFunc`, relying on
`result.FromError` for the conversion:
func RunFunc(run func(...) error) func(...) {
return RunResultFunc(func(...) result.Result {
if err := run(...); err != nil {
return result.FromError(err)
}
return nil
})
}
**Problem**
In CLI contexts where we're using an `error`, and we want to print an
error message to the user and exit, it's desirable to use diag.Sink to
print the message to the user with the appropriate level (error,
warning, etc.) and exit without printing anything else.
However, the only way to do that currently is by converting that
function to return `result.Result`, turn all error returns to
`result.FromError`, and then return `result.Bail()`.
**Solution**
This change introduces a `result.BailError` error that gets converted
into a `result.Bail()` when it passes through `result.FromError`.
It allows commands implementations that use `error` to continue
returning errors and still provide an ideal CLI experience.
It relies on `errors.As` for matching, so even if an intermediate layer
wraps the error with `fmt.Errorf("..: %w", ErrBail)`, we'll recognize
the request to bail.
BailError keep track of the internal error that triggered it, which
(when everything is moved off of result and onto error) means we'll
still be able to see the internal errors that triggered a bail during
debugging.
Currently debugging engine tests is pretty horrible because you often
just get back a `result.Result{err:nil}` with no information where in
the engine stack that came from.
**Testing**
Besides unit tests, this includes an end-to-end test for using
RunResultFunc with a bail error.
The test operates by putting the mock behavior in a fake test, and
re-running the test binary to execute *just that test*.
**Demonstration**
This change also ports the following commands to use BailError: cancel,
convert, env, policy rm, stack rm.
These command implementations are simple and were able to switch easily,
without bubbling into a change to a bunch of other code.
Per team discussion, switching to gofumpt.
[gofumpt][1] is an alternative, stricter alternative to gofmt.
It addresses other stylistic concerns that gofmt doesn't yet cover.
[1]: https://github.com/mvdan/gofumpt
See the full list of [Added rules][2], but it includes:
- Dropping empty lines around function bodies
- Dropping unnecessary variable grouping when there's only one variable
- Ensuring an empty line between multi-line functions
- simplification (`-s` in gofmt) is always enabled
- Ensuring multi-line function signatures end with
`) {` on a separate line.
[2]: https://github.com/mvdan/gofumpt#Added-rules
gofumpt is stricter, but there's no lock-in.
All gofumpt output is valid gofmt output,
so if we decide we don't like it, it's easy to switch back
without any code changes.
gofumpt support is built into the tooling we use for development
so this won't change development workflows.
- golangci-lint includes a gofumpt check (enabled in this PR)
- gopls, the LSP for Go, includes a gofumpt option
(see [installation instrutions][3])
[3]: https://github.com/mvdan/gofumpt#installation
This change was generated by running:
```bash
gofumpt -w $(rg --files -g '*.go' | rg -v testdata | rg -v compilation_error)
```
The following files were manually tweaked afterwards:
- pkg/cmd/pulumi/stack_change_secrets_provider.go:
one of the lines overflowed and had comments in an inconvenient place
- pkg/cmd/pulumi/destroy.go:
`var x T = y` where `T` wasn't necessary
- pkg/cmd/pulumi/policy_new.go:
long line because of error message
- pkg/backend/snapshot_test.go:
long line trying to assign three variables in the same assignment
I have included mention of gofumpt in the CONTRIBUTING.md.
I got fed up of assert errors in tests that looked like:
```
Expected nil, but got: &result.simpleResult{err:(*errors.fundamental)(0xc0002fa5d0)}
```
It was very hard to work out at a glance what had gone wrong and I kept
having to hook a debugger just to look at what the error was.
With GoString these now print something like:
```
Expected nil, but got: &simpleResult{err: Unexpected diag message: <{%reset%}>resource violates plan: properties changed: -zed, -baz, -foo<{%reset%}>
}
```
Which is much more useful.