log/slog: remove calls to Value.Resolve from core

Remove calls to Value.Resolve from Record.AddAttrs, Record.Add and Logger.With.
Handlers must resolve values themselves; document that in Handler.

Call Value.Resolve in the built-in handlers.

Updates #59292.

Change-Id: I00ba2131be0b16e3b1a22741249fd6f81c3efde1
Reviewed-on: https://go-review.googlesource.com/c/go/+/486375
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

src/log/slog/handler.go

@@ -50,6 +50,7 @@ type Handler interface {
 	// Handle methods that produce output should observe the following rules:
 	//   - If r.Time is the zero time, ignore the time.
 	//   - If r.PC is zero, ignore it.
+	//   - Attr's values should be resolved.
 	//   - If an Attr's key and value are both the zero value, ignore the Attr.
 	//     This can be tested with attr.Equal(Attr{}).
 	//   - If a group's key is empty, inline the group's Attrs.
@@ -60,7 +61,6 @@ type Handler interface {
 	// WithAttrs returns a new Handler whose attributes consist of
 	// both the receiver's attributes and the arguments.
 	// The Handler owns the slice: it may retain, modify or discard it.
-	// [Logger.With] will resolve the Attrs.
 	WithAttrs(attrs []Attr) Handler
 
 	// WithGroup returns a new Handler with the given group appended to
@@ -443,11 +443,11 @@ func (s *handleState) appendAttr(a Attr) {
 		if s.groups != nil {
 			gs = *s.groups
 		}
-		a = rep(gs, a)
-		// Although all attributes in the Record are already resolved,
-		// This one came from the user, so it may not have been.
+		// Resolve before calling ReplaceAttr, so the user doesn't have to.
 		a.Value = a.Value.Resolve()
+		a = rep(gs, a)
 	}
+	a.Value = a.Value.Resolve()
 	// Elide empty Attrs.
 	if a.isEmpty() {
 		return

src/log/slog/handler_test.go

@@ -234,6 +234,16 @@ func TestJSONAndTextHandlers(t *testing.T) {
 			wantText: "msg=message a=1 name.first=Ren name.last=Hoek b=2",
 			wantJSON: `{"msg":"message","a":1,"name":{"first":"Ren","last":"Hoek"},"b":2}`,
 		},
+		{
+			// Test resolution when there is no ReplaceAttr function.
+			name: "resolve",
+			attrs: []Attr{
+				Any("", &replace{Value{}}), // should be elided
+				Any("name", logValueName{"Ren", "Hoek"}),
+			},
+			wantText: "time=2000-01-02T03:04:05.000Z level=INFO msg=message name.first=Ren name.last=Hoek",
+			wantJSON: `{"time":"2000-01-02T03:04:05Z","level":"INFO","msg":"message","name":{"first":"Ren","last":"Hoek"}}`,
+		},
 		{
 			name:     "with-group",
 			replace:  removeKeys(TimeKey, LevelKey),

src/log/slog/json_handler.go

@@ -142,7 +142,7 @@ func appendJSONValue(s *handleState, v Value) error {
 			return appendJSONMarshal(s.buf, a)
 		}
 	default:
-		panic(fmt.Sprintf("bad kind: %d", v.Kind()))
+		panic(fmt.Sprintf("bad kind: %s", v.Kind()))
 	}
 	return nil
 }

src/log/slog/logger.go

@@ -89,7 +89,7 @@ func (l *Logger) clone() *Logger {
 func (l *Logger) Handler() Handler { return l.handler }
 
 // With returns a new Logger that includes the given arguments, converted to
-// Attrs as in [Logger.Log] and resolved.
+// Attrs as in [Logger.Log].
 // The Attrs will be added to each output from the Logger.
 // The new Logger shares the old Logger's context.
 // The new Logger's handler is the result of calling WithAttrs on the receiver's

src/log/slog/record.go

@@ -87,7 +87,6 @@ func (r Record) NumAttrs() int {
 
 // Attrs calls f on each Attr in the Record.
 // Iteration stops if f returns false.
-// The Attrs are already resolved.
 func (r Record) Attrs(f func(Attr) bool) {
 	for i := 0; i < r.nFront; i++ {
 		if !f(r.front[i]) {
@@ -102,9 +101,7 @@ func (r Record) Attrs(f func(Attr) bool) {
 }
 
 // AddAttrs appends the given Attrs to the Record's list of Attrs.
-// It resolves the Attrs before doing so.
 func (r *Record) AddAttrs(attrs ...Attr) {
-	resolveAttrs(attrs)
 	n := copy(r.front[r.nFront:], attrs)
 	r.nFront += n
 	// Check if a copy was modified by slicing past the end
@@ -120,7 +117,6 @@ func (r *Record) AddAttrs(attrs ...Attr) {
 
 // Add converts the args to Attrs as described in [Logger.Log],
 // then appends the Attrs to the Record's list of Attrs.
-// It resolves the Attrs before doing so.
 func (r *Record) Add(args ...any) {
 	var a Attr
 	for len(args) > 0 {
@@ -154,7 +150,7 @@ const badKey = "!BADKEY"
 
 // argsToAttr turns a prefix of the nonempty args slice into an Attr
 // and returns the unconsumed portion of the slice.
-// If args[0] is an Attr, it returns it, resolved.
+// If args[0] is an Attr, it returns it.
 // If args[0] is a string, it treats the first two elements as
 // a key-value pair.
 // Otherwise, it treats args[0] as a value with a missing key.
@@ -164,12 +160,9 @@ func argsToAttr(args []any) (Attr, []any) {
 		if len(args) == 1 {
 			return String(badKey, x), nil
 		}
-		a := Any(x, args[1])
-		a.Value = a.Value.Resolve()
-		return a, args[2:]
+		return Any(x, args[1]), args[2:]
 
 	case Attr:
-		x.Value = x.Value.Resolve()
 		return x, args[1:]
 
 	default:

src/log/slog/value.go

@@ -438,19 +438,12 @@ const maxLogValues = 100
 
 // Resolve repeatedly calls LogValue on v while it implements LogValuer,
 // and returns the result.
-// If v resolves to a group, the group's attributes' values are also resolved.
+// If v resolves to a group, the group's attributes' values are not recursively
+// resolved.
 // If the number of LogValue calls exceeds a threshold, a Value containing an
 // error is returned.
 // Resolve's return value is guaranteed not to be of Kind KindLogValuer.
-func (v Value) Resolve() Value {
-	v = v.resolve()
-	if v.Kind() == KindGroup {
-		resolveAttrs(v.Group())
-	}
-	return v
-}
-
-func (v Value) resolve() (rv Value) {
+func (v Value) Resolve() (rv Value) {
 	orig := v
 	defer func() {
 		if r := recover(); r != nil {
@@ -491,11 +484,3 @@ func stack(skip, nFrames int) string {
 	}
 	return b.String()
 }
-
-// resolveAttrs replaces the values of the given Attrs with their
-// resolutions.
-func resolveAttrs(as []Attr) {
-	for i, a := range as {
-		as[i].Value = a.Value.Resolve()
-	}
-}

src/log/slog/value_test.go

@@ -175,14 +175,11 @@ func TestLogValue(t *testing.T) {
 		t.Errorf("expected error, got %T", got)
 	}
 
-	// Test Resolve group.
-	r = &replace{GroupValue(
-		Int("a", 1),
-		Group("b", Any("c", &replace{StringValue("d")})),
-	)}
-	v = AnyValue(r)
+	// Groups are not recursively resolved.
+	c := Any("c", &replace{StringValue("d")})
+	v = AnyValue(&replace{GroupValue(Int("a", 1), Group("b", c))})
 	got2 := v.Resolve().Any().([]Attr)
-	want2 := []Attr{Int("a", 1), Group("b", String("c", "d"))}
+	want2 := []Attr{Int("a", 1), Group("b", c)}
 	if !attrsEqual(got2, want2) {
 		t.Errorf("got %v, want %v", got2, want2)
 	}
@@ -196,7 +193,6 @@ func TestLogValue(t *testing.T) {
 	}
 	// The error should provide some context information.
 	// We'll just check that this function name appears in it.
-	fmt.Println(got)
 	if got, want := gotErr.Error(), "TestLogValue"; !strings.Contains(got, want) {
 		t.Errorf("got %q, want substring %q", got, want)
 	}