proposal: limit external dependencies

[mewmew]

Background (click arrow to expand):

This proposal is in a similar spirit as #73, seeking to limit the external dependencies we rely on for the core of `llir/llvm`. Note, that at least in the initial stage of this proposal, this does not include dependencies required by usage example code (e.g. [asm/example_test.go](https://github.com/llir/llvm/blob/master/asm/example_test.go) depends on `github.com/kr/pretty`). However, for the `llir/llvm` packages we should seek to remove any external dependency that we don't have explicit control over (that is any package not living under `github.com/llir`) or which has not been accepted as canonical packages to use by the Go community. For now, this includes `pkg/errors`, but that is likely to change in Go 1.14 (see #73).

To start with, I'd like to copy/fork/implement natural sorting to remove our external dependency on github.com/rickypai/natsort, firstly because the implementation is fairly trivial (natsort/sort.go is 136 lines of code), and secondly and perhaps more importantly github.com/rickypai/natsort is the GitHub mirror repo for bitbucket.org/zombiezen/cardcpx.

Going forward, I'd also like for us to implement the policy of not adding external dependencies for the project unless carefully motivated.

Food for thought.

edit: The following external dependencies are currently considered by this proposal:

• github.com/rickypai/natsort (see all: replace rickypai/natsort with internal/natsort #94)

As of 2019-06-02, the external dependencies situation is as follows.

To be kept

Used by core during development.

• github.com/mewspring/tools
  • needed for string2enum tool, the inverse of stringer (as used by asm/enum)
• golang.org/x/tools
  • needed for stringer tool (as used by ir/enum and ir/types)

To be kept for test cases

Used by example code and test cases.

• github.comgoogle/go-cmp
  • used for pretty-printing diffs in test cases

To be removed in Go 1.14

Used everywhere.

• github.com/pkg/errors
  • will be removed in Go 1.14, when Go errors contain stack traces (see proposal: drop github.com/pkg/errors as a dependency #73)

---

Done

To be moved? (decided not to move)

Used by core.

• github.com/mewmew/float
  • needed for floating-point representations (used by ir/constant)
  • consider moving mewmew/float to internal/float.
  • decided not to move, as binary representation of floating-point numbers is generally useful for other projects, and having the package be external rather than internal may help us get additional users, and hopefully iron out any few remaining bugs.

To be removed?

Used by example code and test cases.

• github.com/kr/pretty
  • used for pretty-printing data structures in example code
  • decided not to remove as it's useful to give users insight into how these data structures look, and that's easy to do with pretty printed data structures.
• github.com/mewkiz/pkg (removed in favour of google/go-cmp)
  • used for pretty-printing diffs in test cases

[pwaller]

Sounds good to me.

[mewmew]

Consider using github.com/google/go-cmp instead of mewkiz/pkg/diffutil for testing.

[mewmew]

Consider using github.com/google/go-cmp instead of mewkiz/pkg/diffutil for testing.

Done as of commit 5aaf037.

[mewmew]

All dependencies have been updated, replaced or removed, as outlined in #92 (comment)

The only dependency that has not been removed/replaced is github.com/pkg/errors, and the reason is that the Go 1.13/1.14 error handling did not include support for stack traces, which is the main reason we use pkg/errors. So until that time comes, lets keep pkg/errors. (Note: this issue is tracked by #73)

Closing this issue for now. If we start accumulating unsolicited dependencies in the future, feel free to re-open this issue or create a new one.

[sbinet]

If you capture an error value with the '%w' verb and then display the resulting error with '%+v' you do get a stack trace.
And that works with the 'x/xerrors' package (as well as the regular fmt one for display, even for old Go releases)

[mewmew]

If you capture an error value with the '%w' verb and then display the resulting error with '%+v' you do get a stack trace.
And that works with the 'x/xerrors' package (as well as the regular fmt one for display, even for old Go releases)

Oh, great to know. I must have missed this. Thanks @sbinet, I'll definitely look into using x/errors then. In your personal opinion, are there any specific pros/cons with pkg/errors vs x/errors?

[mewmew]

From at least a very quick test, it seems that pkg/errors include the full stack trace, and x/xerrors only include the stack frame of the function creating the error (and if wrapper with other errors, then each of those stack frames). So, pkg/errors still give more information, and knowing the callee location (as included in the full stack trace) is often very useful for debugging errors.

@sbinet, do you know if its possible to include the full stack trace with x/xerrors?

package main

import (
	"log"

	"github.com/pkg/errors"
	//errors "golang.org/x/xerrors"
)

func main() {
	if err := foo(); err != nil {
		log.Fatalf("%+v", err)
	}
}

func foo() error {
	return errors.New("error message")
}

// pkg/errors:
//
//    2019/12/28 12:23:48 error message
//    main.foo
//       /home/u/Desktop/go/src/play/aac/main.go:17
//    main.main
//       /home/u/Desktop/go/src/play/aac/main.go:11
//    runtime.main
//       /home/u/go/src/runtime/proc.go:203
//    runtime.goexit
//       /home/u/go/src/runtime/asm_amd64.s:1375
//    exit status 1

// x/xerrors:
//
//    2019/12/28 12:20:41 error message:
//        main.foo
//            /home/u/Desktop/go/src/play/aac/main.go:17
//    exit status 1

[sbinet]

I believe a better example would be something along these lines:

https://play.golang.org/p/T7w1Hc15JVZ

package main

import (
	"io"
	"log"

	errors "golang.org/x/xerrors"
)

func main() {
	if err := foo(); err != nil {
		log.Fatalf("%+v", err)
	}
}

func foo() error {
	err := f1()
	if err != nil {
		return errors.Errorf("could not foo me: %w", err)
	}
	return nil
}

func f1() error {
	err := f2()
	if err != nil {
		return errors.Errorf("could not do f1: %w", err)
	}
	return nil
}

func f2() error {
	return errors.Errorf("error message: %w", io.EOF)
}

2009/11/10 23:00:00 could not foo me:
    main.foo
        /tmp/sandbox501304043/prog.go:19
  - could not do f1:
    main.f1
        /tmp/sandbox501304043/prog.go:27
  - error message:
    main.f2
        /tmp/sandbox501304043/prog.go:33
  - EOF

that said, I don't know whether it's possible to bend xerrors (or fmt with Go >=1.13) into adding more detailed stack traces.

[mewmew]

I believe a better example would be something along these lines

Yeah, I agree your example is a better showcase of xerrors, as it uses %w to wrap errors.

What I'm missing in xerrors is something analogous to pkg/errors.WithStack which essentially wraps an error to include a detailed stack trace, but without adding any other information. Sometimes you don't want to annotate an error with more user-defined messages, but just want to include the stack information, and that's exactly the use case of pkg/errors.WithStack.

that said, I don't know whether it's possible to bend xerrors (or fmt with Go >=1.13) into adding more detailed stack traces.

Yeah. At least with my current (limited) experience of using xerrors, I still find pkg/errors more useful to troubleshoot errors.

To use the terminology of eris, if all root errors where created with xerrors and xerrors.Frame was extended to include more than just a single stack frame (actually 3, to skip skipPleaseUseCallersFrames), then it would work great and you wouldn't have to annotate errors using something analogous to pkg/errors.WithStack.

When the root error was not created by xerrors, pkg/errors or eris, that is when we need to add stack trace information. And since we don't know if the root error was from the Go standard library (e.g. io.EOF), we always need to add stack trace information.

Creating a new user-defined message to be able to capture more of the stack trace seems like a hack. Sometimes wrapping an error with a user-defined error message is definitely the right thing to do as it adds useful context information. But if the user-defined error message does not add any additional information than the stack trace info (e.g. name of function), then it just adds noise, and limits readability as it becomes more difficult to glance over the code to identify valuable user-defined error messages. As such, I think pkg/errors.WithStack is useful and cannot be replaced with xerrors.Errorf for those cases when user-defined error messages does not contain any additional information than the function name.

At least, that's my take on it. The addition of xerrors and the integration of error wrapping into fmt is a great step forward for error handling in Go. But pkg/errors still has a place in the error handling ecosystem. I hope we will arrive at something similar to Zig error handling, making errors a deep first-class citizen in Go. Until that time, I'll probably keep using pkg/errors :)

Thanks for taking the time to give more background and examples of xerrors @sbinet. Wish you all the best and a good year to come!

Cheers,
Robin