Now that 1.12 is no longer supported, I think we can make this change. What do you think, @jba?

I'm for it. But this is technically breaking change, since anyone since 1.13 who was doing errors.Is on one of these errors could now get a different result. I don't know if that's an OK sort of change.

Hm. I’m not a fan of breaking changes. Will follow up. This needs more investigation.

I don’t think this is a breaking change. It’s a behavioral change, and any behavioral change is of course subject to Hyrum’s Law, but I don’t believe this changes the documented properties of the package’s errors.

@jba @andybons: I’m not sure how this would be interpreted as a breaking change. Because fmt.Errorf returns an *errors.errorString, the only real way to test for equality would be to call its Error method, which will result in the exact same string because of how the %w verb is defined.

If developers are currently using errors.Is or errors.As to check for a wrapped sql.Scanner error, then their code is currently broken because they will never return true. I am unsure if we should really be supporting the use case where Is or As always fails. I argue that changing the fmt.Errorf call to use %w is actually a non-breaking change, because this code has never worked with Is or As.

In fact, with the introduction of errors.Is and errors.As, I was surprised to find that this error was not wrapped. If I had not written comprehensive tests, I may never have caught this and my error handling code would have never handled this correctly.

Someone might argue that these errors shouldn’t be wrapped because this is exposing the internal details of the Scan method: Errors and package APIs. That doesn’t hold water, since the caller of rows.Scan knows which pointers are being passed in and therefore expects certain errors while scanning. If the errors are repackaged without wrapping, then the caller can only examine the Error string heuristicly. The workaround I’m currently using checks strings.Contains(err.Error(), ErrInvalid.Error()) which is kind of terrible.

I'm agreeing with @sfllaw's conclusion. errors.Is and errors.As can't be used against the returned error. Only string level equality can ben tested. If existing code is using string level equality testing, it won't break by switching to the %w verb:

var pgErr = errors.New("playground")

func main() {
	strErr := fmt.Errorf("Hello, %v", pgErr)
	if strings.Contains(strErr.Error(), pgErr.Error()) {
		fmt.Println(strErr)
	}
	
	wrpErr := fmt.Errorf("Hello, %w", pgErr)
	if strings.Contains(wrpErr.Error(), pgErr.Error()) {
		fmt.Println(wrpErr)
	}
	
	if errors.Is(wrpErr, pgErr) {
		fmt.Println(wrpErr)
	}
}

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

One of the affected use cases would be implementers of the sql.Scanner interface, like myself and the OP:

var (
	errJSONEmpty  = errors.New("JSON result empty")
	errJSONAssert = errors.New("JSON assertion error")
)

func (s *jsonScanner) Scan(v interface{}) (err error) {
	if v == nil {
		return errJSONEmpty
	}

	js, ok := v.([]byte)
	if !ok {
		return errJSONAssert
	}

	if s.Value, err = s.p.ParseBytes(js); err != nil {
		return fmt.Errorf("json Scan: %w", err)
	}

	return nil
}

Using the above pattern, I would expect to can do something like:

       	var js jsonScanner

	err = rt.Tx.QueryRowContext(rt.Ctx, query, args...).Scan(&js)
	switch {
	case err == nil:
	case errors.Is(err, errEmptyJSON):
		return []*shop.Article{}, nil
	default:
		return nil, status.Error(codes.Internal, errDB)
	}

@neild @jba what do y’all think? Should we do this early in the 1.16 cycle and see what breaks?

I think we should do this.

I expect that this will break some tests using reflect.DeepEqual on errors returned by Rows.Scan. This is fine, but is best to do early in cycle.

I don't expect any breakage from code using errors.Is, since as @sfllaw and others note this would never have worked in the first place. Even if such code exists, I do not believe this change is a violation of the compatibility guarantee since it makes no backwards-incompatible changes to the documented properties of database/sql's errors.

sgtm. marking as early-in-cycle

This issue is currently labeled as early-in-cycle for Go 1.16.
That time is now, so this is a friendly ping so the issue is looked at again.

I'm working now on a CL, should submit in the next hour or so.

Change https://golang.org/cl/248337 mentions this issue: database/sql: use the %w error verb in Rows.Scan

@muhlemmer Suggestion: we should document how the Scanner's error propagates through the stack.