Skip to content
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

proposal: errors: replace sentinel errors with typed constants where possible #63602

Open
myaaaaaaaaa opened this issue Oct 17, 2023 · 6 comments
Labels
Milestone

Comments

@myaaaaaaaaa
Copy link

myaaaaaaaaa commented Oct 17, 2023

Sentinel errors are a common error handling pattern involving comparisons with global variables. The oserror package from the standard library has a good example of how these are typically defined:

package oserror

import "errors"

var (
	ErrInvalid    = errors.New("invalid argument")
	ErrPermission = errors.New("permission denied")
	ErrExist      = errors.New("file already exists")
	ErrNotExist   = errors.New("file does not exist")
	ErrClosed     = errors.New("file already closed")
)

They come with all the usual caveats of global variables, namely that they can be reassigned:

package main

import (
	"fmt"
	"os"
)

func main() {
	var nilFile *os.File

	// prints 0, error("invalid argument")
	fmt.Println(nilFile.Read(nil))

	// allowed :(
	os.ErrInvalid = nil

	// segfaults due to an internal error check being bypassed
	fmt.Println(nilFile.Read(nil))
}

An alternative, backwards-compatible(?) way of defining these using typed constants is possible:

package oserror

// no imports needed! :)

// unexported error type means that no other package
// can define an error equal to any of the below constants,
// even if the string values match.
type osError string

func (err osError) Error() string {
	return string(err)
}

// constants can't be reassigned :)
const (
	// errors are unique as long as the strings are unique.
	ErrInvalid    = osError("invalid argument")
	ErrPermission = osError("permission denied")
	ErrExist      = osError("file already exists")
	ErrNotExist   = osError("file does not exist")
	ErrClosed     = osError("file already closed")
)

I haven't thought through the potential downsides or long-term implications of this.

@gopherbot gopherbot added this to the Proposal milestone Oct 17, 2023
@ianlancetaylor
Copy link
Contributor

Thanks, that is a good idea. But we can't make that change now, as it will break the Go 1 compatibility guarantee.

@Jorropo
Copy link
Member

Jorropo commented Oct 18, 2023

I think a better alternative which elide allocations is to use zero sized dedicated types:

type errInvalid struct{}

func (errInvalid) Error() string {
	return "invalid argument"
}

type errPermission struct{}

func (errPermission) Error() string {
	return "permission denied"
}

type errExist struct{}

func (errExist) Error() string {
	return "file already exists"
}

type errNotExist struct{}

func (errNotExist) Error() string {
	return "file does not exist"
}

type errClosed struct{}

func (errClosed) Error() string {
	return "file already closed"
}

var (
	ErrInvalid    errInvalid
	ErrPermission errPermission
	ErrExist      errExist
	ErrNotExist   errNotExist
	ErrClosed     errClosed
)

It technically is possible to reassign them, os.ErrInvalid = os.ErrInvalid is valid code, however that do absolutely nothing so it's fine and avoid an allocation every time someone return or errors.Is the error value. Nvm passing constant strings as interfaces allocate the string header as a global, I still think something like var ErrInvalid errors.Error[osError, "invalid argument"] is better.

Would be nice to find a less verbose way to express this.
If we could pass const as type parameters could do:

type errInvalid errors.Error["invalid argument"]
var ErrInvalid errInvalid

A helper like this would allow to easily create individual errors which wrap a parent error for wildcard checking:

type Error[Wrapping error, Message const string] struct{}

func (Error[Wrapping, Message]) Error() string {
 return Message
}

func (Error[Wrapping, Message]) Unwrap() error {
 var zero Wrapping
 return zero
}

@go101
Copy link

go101 commented Oct 18, 2023

One drawback of using constant string errors is, we can easily invent new errors of the same type:

package main

type osError string

func (err osError) Error() string {
	return string(err)
}

const ErrInvalid = osError("invalid argument")

func main() {
	var myError error = ErrInvalid + " 2"
	println(myError.Error())
}

Maybe, it is a good idea to add a vet rule: find all modifications of global error values (including the operations taking addresses of global error values).

@myaaaaaaaaa
Copy link
Author

myaaaaaaaaa commented Oct 18, 2023

Would be nice to find a less verbose way to express this. If we could pass const as type parameters could do:

type errInvalid errors.Error["invalid argument"]
var ErrInvalid errInvalid

To add to this, there's already a precedent for const type parameters: Arrays

var (
	a [4]int
	b [3]float32
	c [8]string
)

@psnilesh
Copy link

psnilesh commented Oct 19, 2023

Without getting too off track, has Go considered adding a val qualifier to ensure the variable can't be re-assigned post initialization ? I'd be interested in reading a thread about it if there is one. I've read through several proposals about adding immutable types and found myself agreeing with the arguments against it, but it seems like preventing re-assignment and leaving interior mutability to the type (i.e no setters) might be middle ground solution that could apply to more than just errors.

@ianlancetaylor
Copy link
Contributor

@psnilesh There is https://go.dev/issue/21130.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

6 participants