I think this should be named os.SignalError instead of signal.CanceledBySignalError, so that it may be used for cases beyond just signal.Notify. For example, I could envision a caller transforming an exec.ExitError by checking whether its ProcessState indicates termination by signal and returning a SignalError.

Actually, SignalError would be even more useful if exec.ExitError had a convenience Is method for it:

	err := cmd.Wait()
	if errors.Is(err, os.SignalError{Signal: syscall.SIGABRT}) {
		// err indicates termination due to signal SIGABRT — program probably failed an assertion
		…
	}

@bcmills I agree that os.SignalError is a better name, but I have a few comments/questions regarding that approach:

1. The error message of it becomes ambiguous if it's used beyond signal.NotifyContext, e.g. it's no longer necessarily "context canceled by signal ...", I guess we can have a more generic message there and use fmt.Errorf("context canceled by %w", &os.SignalError{...}) in signal.NotifyContext
2. I feel that using errors.Is on it opens some cans of worms we probably don't want to deal with. os.Signal is an interface so there's no guarantee that it will be comparable, and I think in general we prefer pointer errors when the error is a struct to make sure it's comparable (because in that case we only need to compare the pointer address).

But I think this can work if we use a generic error message, and change the errors.Is to errors.As in your example.

os.Signal is an interface so there's no guarantee that it will be comparable

In practice it always is:

~/go/src$ git grep 'func .* Signal() {' .
cmd/vendor/golang.org/x/sys/plan9/syscall_plan9.go:func (n Note) Signal() {}
cmd/vendor/golang.org/x/sys/windows/syscall_windows.go:func (s Signal) Signal() {}
sync/cond.go:func (c *Cond) Signal() {
syscall/syscall_js.go:func (s Signal) Signal() {}
syscall/syscall_plan9.go:func (n Note) Signal() {}
syscall/syscall_unix.go:func (s Signal) Signal() {}
syscall/syscall_wasip1.go:func (s Signal) Signal() {}
syscall/syscall_windows.go:func (s Signal) Signal() {}

syscall.Signal and windows.Signal are each always an integer type when defined. syscall.Note and plan9.Note are string types.

sync.Cond isn't intended to implement os.Signal at all, and doesn't today because it lacks a String method.

So I'm not worried about somebody stuffing an incomparable os.Signal into a SignalError.

I believe that when you use errors.Is(err, os.SignalError{Signal: syscall.SIGABRT}) and err indeed unwraps to os.SignalError type, you need both the unwrapped error and the target to be comparable to not panic. Here you can control the target to be comparable (as you know syscall.SIGABRT is comparable), you have no control what the err is and it's always comparable, so this is still a hanging knife that could panic the code unexpectly.

^ wrong, see https://go.dev/play/p/qpyJXU05CFl

errors.Is already handles comparability:
https://cs.opensource.google/go/go/+/refs/tags/go1.20.5:src/errors/wrap.go;l=48;drc=4a0a2b33dfa3c99250efa222439f2c27d6780e4a

hmm you are right. tested on playground: https://go.dev/play/p/qpyJXU05CFl

but another issue with making the nonpointer type to implement error is that people often don't know which type to use with errors.As, for example they can make this mistake:

var se *os.SignalError // they thought it would be the pointer type
if errors.As(err, &se) {
  // they'd never get here
}

the solution to that is to also implement os.SignalError.As to support both pointer and non-pointer types, but is that something we consider worth doing here?

To be clear, I think it would be ok for the type implementing error to be *SignalError.

(exec.ExitError.Is could follow the pointer explicitly.)

But if we use pointer type, errors.Is will no longer work as you expected here, because it will only return true if the pointers are the same, not the contents: https://go.dev/play/p/VNQ-iy4vA_2

The error type can provide an explicit Is method. 🙃
https://go.dev/play/p/_DxO_e8nZAu

yes but then you need to deal with the comparable check of Signal interface in that Is function (if you just use == blindly you would get unexpected panics)

which kind of goes back to the same question as As in #60756 (comment): is this something we consider worth doing here?

You don't need to check whether the Signal implementation is comparable. Every real implementation is.

That's not really in the "contract" of the API (https://pkg.go.dev/os#Signal definition doesn't guarantee comparability). You are essentially depending on implementation details to not panic.

Although this is very very unlikely to happen in real life, hypothetically someone could define a type implementing os.Signal that wraps some real signals but is uncomparable, and when they try to use it with errors.Is (either as part of the target or trigger the signal into the error) it would cause panic.

@bcmills do you mind if I leave the errors.Is case out of this proposal for now?

Basically the revised proposal would be to add SignalError to os package as:

// TODO: what's a generic doc comment to describe it here that's appropriate?
type SignalError struct {
  Signal Signal
}

func (se *SignalError) Error() string {
  return "received signal " + se.Signal.String() // I think this error message is generic enough, in signal.NotifyContext I can just use wrap the error again with prefix of "context canceled by "
}

@fishy, I started working on this in parallel too. When I saw you worked on it too, I rebased with your changes.

Please take a look at my changes here:
https://go-review.googlesource.com/c/go/+/504215 https://go-review.googlesource.com/c/go/+/579375

Regarding using a pointer for SignalError, I think we shouldn't.
My reasoning is that, as there isn't something as a hard rule on using pointers for structs, we shouldn't use it for SignalError, as we don't need one here for anything.
Using a pointer in this code (that might have good visibility) when we don't need to might make people think they should do the same regardless, and this can hit them with increased garbage collector pressure when they do so on code on a hot path.

/cc @ianlancetaylor (on the original implementation of NotifyContext you shared some thoughts/ideas on how this type could be named; you might want to take a look).

@henvic to reiterate #60756 (comment), the problem of allowing non-pointer type to implement error is that when user try to use errors.As it's very error-prone for them to use the wrong type.

I've created a new change request here https://go-review.googlesource.com/c/go/+/579375

Change https://go.dev/cl/579375 mentions this issue: os/signal: add os.SignalError to expose NotifyContext signal

Change https://go.dev/cl/580695 mentions this issue: os/signal: add os.SignalError to expose NotifyContext signal