Errorf: support %w anywhere in the format string

jba · jba · commit 9bdfabe68543 · 2019-12-04T19:05:36.000Z
Allow a single %w to appear anywhere in the format string, matched by an operand that satisfies the error interface. This is complicated by a few things: - We don't want to do full-fledged printf verb parsing. We approximate, and do not handle argument indexes on %w like "%[3]w". - There is a messy interaction with the xerrors formatting system (not adopted in Go 1.13). When the %w is at the end we can control the error string so that the wrapped error's message is formatted properly, after the wrapping errors' message. With a %w in the middle, we can't do that. In this CL, we print the wrapped error's message twice: once in its place in the format string, and then again because it's wrapped. Fixes #go/33143. Change-Id: I6192096521664476e82d5a54b80e352e47397cfe Reviewed-on: https://go-review.googlesource.com/c/xerrors/+/186957 Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/fmt.go b/fmt.go
@@ -7,65 +7,143 @@ package xerrors
 import (
 	"fmt"
 	"strings"
+	"unicode"
+	"unicode/utf8"
 
 	"golang.org/x/xerrors/internal"
 )
 
+const percentBangString = "%!"
+
 // Errorf formats according to a format specifier and returns the string as a
 // value that satisfies error.
 //
 // The returned error includes the file and line number of the caller when
 // formatted with additional detail enabled. If the last argument is an error
 // the returned error's Format method will return it if the format string ends
 // with ": %s", ": %v", or ": %w". If the last argument is an error and the
-// format string ends with ": %w", the returned error implements Wrapper
-// with an Unwrap method returning it.
+// format string ends with ": %w", the returned error implements an Unwrap
+// method returning it.
+//
+// If the format specifier includes a %w verb with an error operand in a
+// position other than at the end, the returned error will still implement an
+// Unwrap method returning the operand, but the error's Format method will not
+// return the wrapped error.
+//
+// It is invalid to include more than one %w verb or to supply it with an
+// operand that does not implement the error interface. The %w verb is otherwise
+// a synonym for %v.
 func Errorf(format string, a ...interface{}) error {
-	err, wrap := lastError(format, a)
 	format = formatPlusW(format)
-	if err == nil {
-		return &noWrapError{fmt.Sprintf(format, a...), nil, Caller(1)}
+	// Support a ": %[wsv]" suffix, which works well with xerrors.Formatter.
+	wrap := strings.HasSuffix(format, ": %w")
+	idx, format2, ok := parsePercentW(format)
+	percentWElsewhere := !wrap && idx >= 0
+	if !percentWElsewhere && (wrap || strings.HasSuffix(format, ": %s") || strings.HasSuffix(format, ": %v")) {
+		err := errorAt(a, len(a)-1)
+		if err == nil {
+			return &noWrapError{fmt.Sprintf(format, a...), nil, Caller(1)}
+		}
+		// TODO: this is not entirely correct. The error value could be
+		// printed elsewhere in format if it mixes numbered with unnumbered
+		// substitutions. With relatively small changes to doPrintf we can
+		// have it optionally ignore extra arguments and pass the argument
+		// list in its entirety.
+		msg := fmt.Sprintf(format[:len(format)-len(": %s")], a[:len(a)-1]...)
+		frame := Frame{}
+		if internal.EnableTrace {
+			frame = Caller(1)
+		}
+		if wrap {
+			return &wrapError{msg, err, frame}
+		}
+		return &noWrapError{msg, err, frame}
+	}
+	// Support %w anywhere.
+	// TODO: don't repeat the wrapped error's message when %w occurs in the middle.
+	msg := fmt.Sprintf(format2, a...)
+	if idx < 0 {
+		return &noWrapError{msg, nil, Caller(1)}
+	}
+	err := errorAt(a, idx)
+	if !ok || err == nil {
+		// Too many %ws or argument of %w is not an error. Approximate the Go
+		// 1.13 fmt.Errorf message.
+		return &noWrapError{fmt.Sprintf("%sw(%s)", percentBangString, msg), nil, Caller(1)}
 	}
-
-	// TODO: this is not entirely correct. The error value could be
-	// printed elsewhere in format if it mixes numbered with unnumbered
-	// substitutions. With relatively small changes to doPrintf we can
-	// have it optionally ignore extra arguments and pass the argument
-	// list in its entirety.
-	msg := fmt.Sprintf(format[:len(format)-len(": %s")], a[:len(a)-1]...)
 	frame := Frame{}
 	if internal.EnableTrace {
 		frame = Caller(1)
 	}
-	if wrap {
-		return &wrapError{msg, err, frame}
+	return &wrapError{msg, err, frame}
+}
+
+func errorAt(args []interface{}, i int) error {
+	if i < 0 || i >= len(args) {
+		return nil
 	}
-	return &noWrapError{msg, err, frame}
+	err, ok := args[i].(error)
+	if !ok {
+		return nil
+	}
+	return err
 }
 
 // formatPlusW is used to avoid the vet check that will barf at %w.
 func formatPlusW(s string) string {
 	return s
 }
 
-func lastError(format string, a []interface{}) (err error, wrap bool) {
-	wrap = strings.HasSuffix(format, ": %w")
-	if !wrap &&
-		!strings.HasSuffix(format, ": %s") &&
-		!strings.HasSuffix(format, ": %v") {
-		return nil, false
-	}
-
-	if len(a) == 0 {
-		return nil, false
+// Return the index of the only %w in format, or -1 if none.
+// Also return a rewritten format string with %w replaced by %v, and
+// false if there is more than one %w.
+// TODO: handle "%[N]w".
+func parsePercentW(format string) (idx int, newFormat string, ok bool) {
+	// Loosely copied from golang.org/x/tools/go/analysis/passes/printf/printf.go.
+	idx = -1
+	ok = true
+	n := 0
+	sz := 0
+	var isW bool
+	for i := 0; i < len(format); i += sz {
+		if format[i] != '%' {
+			sz = 1
+			continue
+		}
+		// "%%" is not a format directive.
+		if i+1 < len(format) && format[i+1] == '%' {
+			sz = 2
+			continue
+		}
+		sz, isW = parsePrintfVerb(format[i:])
+		if isW {
+			if idx >= 0 {
+				ok = false
+			} else {
+				idx = n
+			}
+			// "Replace" the last character, the 'w', with a 'v'.
+			p := i + sz - 1
+			format = format[:p] + "v" + format[p+1:]
+		}
+		n++
 	}
+	return idx, format, ok
+}
 
-	err, ok := a[len(a)-1].(error)
-	if !ok {
-		return nil, false
+// Parse the printf verb starting with a % at s[0].
+// Return how many bytes it occupies and whether the verb is 'w'.
+func parsePrintfVerb(s string) (int, bool) {
+	// Assume only that the directive is a sequence of non-letters followed by a single letter.
+	sz := 0
+	var r rune
+	for i := 1; i < len(s); i += sz {
+		r, sz = utf8.DecodeRuneInString(s[i:])
+		if unicode.IsLetter(r) {
+			return i + sz, r == 'w'
+		}
 	}
-
-	return err, wrap
+	return len(s), false
 }
 
 type noWrapError struct {
diff --git a/fmt_test.go b/fmt_test.go
@@ -61,6 +61,10 @@ func TestErrorf(t *testing.T) {
 		xerrors.Errorf("wrapw: %w", chained),
 		chain("wraps:wrapw/path.TestErrorf/path.go:xxx",
 			"chained/somefile.go:xxx"),
+	}, {
+		xerrors.Errorf("wrapw %w middle", chained),
+		chain("wraps:wrapw chained middle/path.TestErrorf/path.go:xxx",
+			"chained/somefile.go:xxx"),
 	}, {
 		xerrors.Errorf("not wrapped: %+v", chained),
 		chain("not wrapped: chained: somefile.go:123/path.TestErrorf/path.go:xxx"),
@@ -156,7 +160,7 @@ func TestErrorFormatter(t *testing.T) {
 		fmt: "%+v",
 		want: "something:" +
 			"\n    golang.org/x/xerrors_test.TestErrorFormatter" +
-			"\n        .+/fmt_test.go:97" +
+			"\n        .+/fmt_test.go:101" +
 			"\n    something more",
 		regexp: true,
 	}, {
diff --git a/fmt_unexported_test.go b/fmt_unexported_test.go
@@ -0,0 +1,51 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package xerrors
+
+import "testing"
+
+func TestParsePrintfVerb(t *testing.T) {
+	for _, test := range []struct {
+		in       string
+		wantSize int
+		wantW    bool
+	}{
+		{"", 0, false},
+		{"%", 1, false},
+		{"%3.1", 4, false},
+		{"%w", 2, true},
+		{"%v", 2, false},
+		{"%3.*[4]d", 8, false},
+	} {
+		gotSize, gotW := parsePrintfVerb(test.in)
+		if gotSize != test.wantSize || gotW != test.wantW {
+			t.Errorf("parsePrintfVerb(%q) = (%d, %t), want (%d, %t)",
+				test.in, gotSize, gotW, test.wantSize, test.wantW)
+		}
+	}
+}
+
+func TestParsePercentW(t *testing.T) {
+	for _, test := range []struct {
+		in         string
+		wantIdx    int
+		wantFormat string
+		wantOK     bool
+	}{
+		{"", -1, "", true},
+		{"%", -1, "%", true},
+		{"%w", 0, "%v", true},
+		{"%w%w", 0, "%v%v", false},
+		{"%3.2s %+q %% %w %#v", 2, "%3.2s %+q %% %v %#v", true},
+		{"%3.2s %w %% %w %#v", 1, "%3.2s %v %% %v %#v", false},
+	} {
+		gotIdx, gotFormat, gotOK := parsePercentW(test.in)
+		if gotIdx != test.wantIdx || gotFormat != test.wantFormat || gotOK != test.wantOK {
+			t.Errorf("parsePercentW(%q) = (%d, %q, %t), want (%d, %q, %t)",
+				test.in, gotIdx, gotFormat, gotOK, test.wantIdx, test.wantFormat, test.wantOK)
+
+		}
+	}
+}
diff --git a/wrap_113_test.go b/wrap_113_test.go
@@ -0,0 +1,28 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//+build go1.13
+
+package xerrors_test
+
+import (
+	"errors"
+	"testing"
+
+	"golang.org/x/xerrors"
+)
+
+func TestErrorsIs(t *testing.T) {
+	var errSentinel = errors.New("sentinel")
+
+	got := errors.Is(xerrors.Errorf("%w", errSentinel), errSentinel)
+	if !got {
+		t.Error("got false, want true")
+	}
+
+	got = errors.Is(xerrors.Errorf("%w: %s", errSentinel, "foo"), errSentinel)
+	if !got {
+		t.Error("got false, want true")
+	}
+}
