Repository: kubernetes/klog
Pull request #326 (2022-05-10 14:47:00)

internal: remove unused TrimDuplicates
**What this PR does / why we need it**:

Because the "internal" package is inaccessible for consumers of the klog/v2
API, removing code from it is okay.


**Release note**:
```release-note
NONE
```

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes/klog/pull/326#" title="Author self-approved">pohly</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **dims** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @dims` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes%2Fklog).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes/klog/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["dims"]} -->
serathius: https://github.com/kubernetes/klog/runs/6372237998 WAT?
pohly: I think apidiff complaining about the function removal from `internal` is a false positive. That package should be fair game for any kind of breaking API changes.

@dims: do you agree?

pohly: Bug filed against apidiff: https://github.com/golang/go/issues/52864


Patch:
diff --git a/internal/serialize/keyvalues.go b/internal/serialize/keyvalues.go
index 193796f..14f7ec5 100644
--- a/internal/serialize/keyvalues.go
+++ b/internal/serialize/keyvalues.go
@@ -20,8 +20,6 @@ import (
 	"bytes"
 	"fmt"
 	"strconv"
-
-	"github.com/go-logr/logr"
 )
 
 // WithValues implements LogSink.WithValues. The old key/value pairs are
@@ -46,55 +44,6 @@ func WithValues(oldKV, newKV []interface{}) []interface{} {
 	return kv
 }
 
-// TrimDuplicates deduplicates elements provided in multiple key/value tuple
-// slices, whilst maintaining the distinction between where the items are
-// contained.
-func TrimDuplicates(kvLists ...[]interface{}) [][]interface{} {
-	// maintain a map of all seen keys
-	seenKeys := map[interface{}]struct{}{}
-	// build the same number of output slices as inputs
-	outs := make([][]interface{}, len(kvLists))
-	// iterate over the input slices backwards, as 'later' kv specifications
-	// of the same key will take precedence over earlier ones
-	for i := len(kvLists) - 1; i >= 0; i-- {
-		// initialise this output slice
-		outs[i] = []interface{}{}
-		// obtain a reference to the kvList we are processing
-		// and make sure it has an even number of entries
-		kvList := kvLists[i]
-		if len(kvList)%2 != 0 {
-			kvList = append(kvList, missingValue)
-		}
-
-		// start iterating at len(kvList) - 2 (i.e. the 2nd last item) for
-		// slices that have an even number of elements.
-		// We add (len(kvList) % 2) here to handle the case where there is an
-		// odd number of elements in a kvList.
-		// If there is an odd number, then the last element in the slice will
-		// have the value 'null'.
-		for i2 := len(kvList) - 2 + (len(kvList) % 2); i2 >= 0; i2 -= 2 {
-			k := kvList[i2]
-			// if we have already seen this key, do not include it again
-			if _, ok := seenKeys[k]; ok {
-				continue
-			}
-			// make a note that we've observed a new key
-			seenKeys[k] = struct{}{}
-			// attempt to obtain the value of the key
-			var v interface{}
-			// i2+1 should only ever be out of bounds if we handling the first
-			// iteration over a slice with an odd number of elements
-			if i2+1 < len(kvList) {
-				v = kvList[i2+1]
-			}
-			// add this KV tuple to the *start* of the output list to maintain
-			// the original order as we are iterating over the slice backwards
-			outs[i] = append([]interface{}{k, v}, outs[i]...)
-		}
-	}
-	return outs
-}
-
 // MergeKVs deduplicates elements provided in two key/value slices.
 //
 // Keys in each slice are expected to be unique, so duplicates can only occur
@@ -178,24 +127,6 @@ func KVListFormat(b *bytes.Buffer, keysAndValues ...interface{}) {
 			writeStringValue(b, true, v)
 		case error:
 			writeStringValue(b, true, ErrorToString(v))
-		case logr.Marshaler:
-			value := MarshalerToValue(v)
-			// A marshaler that returns a string is useful for
-			// delayed formatting of complex values. We treat this
-			// case like a normal string. This is useful for
-			// multi-line support.
-			//
-			// We could do this by recursively formatting a value,
-			// but that comes with the risk of infinite recursion
-			// if a marshaler returns itself. Instead we call it
-			// only once and rely on it returning the intended
-			// value directly.
-			switch value := value.(type) {
-			case string:
-				writeStringValue(b, true, value)
-			default:
-				writeStringValue(b, false, fmt.Sprintf("%+v", v))
-			}
 		case []byte:
 			// In https://github.com/kubernetes/klog/pull/237 it was decided
 			// to format byte slices with "%+q". The advantages of that are:
@@ -228,18 +159,6 @@ func StringerToString(s fmt.Stringer) (ret string) {
 	return
 }
 
-// MarshalerToValue invokes a marshaler and catches
-// panics.
-func MarshalerToValue(m logr.Marshaler) (ret interface{}) {
-	defer func() {
-		if err := recover(); err != nil {
-			ret = fmt.Sprintf("<panic: %s>", err)
-		}
-	}()
-	ret = m.MarshalLog()
-	return
-}
-
 // ErrorToString converts an error to a string,
 // handling panics if they occur.
 func ErrorToString(err error) (ret string) {


Test patch:
diff --git a/internal/serialize/keyvalues_test.go b/internal/serialize/keyvalues_test.go
index 7fd95e7..7926009 100644
--- a/internal/serialize/keyvalues_test.go
+++ b/internal/serialize/keyvalues_test.go
@@ -153,106 +153,66 @@ No whitespace.`,
 
 func TestDuplicates(t *testing.T) {
 	for name, test := range map[string]struct {
-		first, second   []interface{}
-		expectedTrimmed [][]interface{}
-		expectedMerged  []interface{}
+		first, second []interface{}
+		expected      []interface{}
 	}{
-		"empty": {
-			expectedTrimmed: [][]interface{}{{}, {}},
-		},
+		"empty": {},
 		"no-duplicates": {
-			first:  makeKV("a", 3),
-			second: makeKV("b", 3),
-			expectedTrimmed: [][]interface{}{
-				makeKV("a", 3),
-				makeKV("b", 3),
-			},
-			expectedMerged: append(makeKV("a", 3), makeKV("b", 3)...),
+			first:    makeKV("a", 3),
+			second:   makeKV("b", 3),
+			expected: append(makeKV("a", 3), makeKV("b", 3)...),
 		},
 		"all-duplicates": {
-			first:  makeKV("a", 3),
-			second: makeKV("a", 3),
-			expectedTrimmed: [][]interface{}{
-				{},
-				makeKV("a", 3),
-			},
-			expectedMerged: makeKV("a", 3),
+			first:    makeKV("a", 3),
+			second:   makeKV("a", 3),
+			expected: makeKV("a", 3),
 		},
 		"start-duplicate": {
-			first:  append([]interface{}{"x", 1}, makeKV("a", 3)...),
-			second: append([]interface{}{"x", 2}, makeKV("b", 3)...),
-			expectedTrimmed: [][]interface{}{
-				makeKV("a", 3),
-				append([]interface{}{"x", 2}, makeKV("b", 3)...),
-			},
-			expectedMerged: append(append(makeKV("a", 3), "x", 2), makeKV("b", 3)...),
+			first:    append([]interface{}{"x", 1}, makeKV("a", 3)...),
+			second:   append([]interface{}{"x", 2}, makeKV("b", 3)...),
+			expected: append(append(makeKV("a", 3), "x", 2), makeKV("b", 3)...),
 		},
 		"subset-first": {
-			first:  append([]interface{}{"x", 1}, makeKV("a", 3)...),
-			second: append([]interface{}{"x", 2}, makeKV("a", 3)...),
-			expectedTrimmed: [][]interface{}{
-				{},
-				append([]interface{}{"x", 2}, makeKV("a", 3)...),
-			},
-			expectedMerged: append([]interface{}{"x", 2}, makeKV("a", 3)...),
+			first:    append([]interface{}{"x", 1}, makeKV("a", 3)...),
+			second:   append([]interface{}{"x", 2}, makeKV("a", 3)...),
+			expected: append([]interface{}{"x", 2}, makeKV("a", 3)...),
 		},
 		"subset-second": {
-			first:  append([]interface{}{"x", 1}, makeKV("a", 1)...),
-			second: append([]interface{}{"x", 2}, makeKV("b", 2)...),
-			expectedTrimmed: [][]interface{}{
-				makeKV("a", 1),
-				append([]interface{}{"x", 2}, makeKV("b", 2)...),
-			},
-			expectedMerged: append(append(makeKV("a", 1), "x", 2), makeKV("b", 2)...),
+			first:    append([]interface{}{"x", 1}, makeKV("a", 1)...),
+			second:   append([]interface{}{"x", 2}, makeKV("b", 2)...),
+			expected: append(append(makeKV("a", 1), "x", 2), makeKV("b", 2)...),
 		},
 		"end-duplicate": {
-			first:  append(makeKV("a", 3), "x", 1),
-			second: append(makeKV("b", 3), "x", 2),
-			expectedTrimmed: [][]interface{}{
-				makeKV("a", 3),
-				append(makeKV("b", 3), "x", 2),
-			},
-			expectedMerged: append(makeKV("a", 3), append(makeKV("b", 3), "x", 2)...),
+			first:    append(makeKV("a", 3), "x", 1),
+			second:   append(makeKV("b", 3), "x", 2),
+			expected: append(makeKV("a", 3), append(makeKV("b", 3), "x", 2)...),
 		},
 		"middle-duplicate": {
-			first:  []interface{}{"a-0", 0, "x", 1, "a-1", 2},
-			second: []interface{}{"b-0", 0, "x", 2, "b-1", 2},
-			expectedTrimmed: [][]interface{}{
-				{"a-0", 0, "a-1", 2},
-				{"b-0", 0, "x", 2, "b-1", 2},
-			},
-			expectedMerged: []interface{}{"a-0", 0, "a-1", 2, "b-0", 0, "x", 2, "b-1", 2},
+			first:    []interface{}{"a-0", 0, "x", 1, "a-1", 2},
+			second:   []interface{}{"b-0", 0, "x", 2, "b-1", 2},
+			expected: []interface{}{"a-0", 0, "a-1", 2, "b-0", 0, "x", 2, "b-1", 2},
 		},
 		"internal-duplicates": {
 			first:  []interface{}{"a", 0, "x", 1, "a", 2},
 			second: []interface{}{"b", 0, "x", 2, "b", 2},
-			expectedTrimmed: [][]interface{}{
-				{"a", 2},
-				{"x", 2, "b", 2},
-			},
 			// This is the case where Merged keeps key/value pairs
 			// that were already duplicated inside the slices, for
 			// performance.
-			expectedMerged: []interface{}{"a", 0, "a", 2, "b", 0, "x", 2, "b", 2},
+			expected: []interface{}{"a", 0, "a", 2, "b", 0, "x", 2, "b", 2},
 		},
 	} {
 		t.Run(name, func(t *testing.T) {
-			t.Run("TrimDuplicates", func(t *testing.T) {
-				actual := serialize.TrimDuplicates(test.first, test.second)
-				expectEqual(t, "trimmed key/value pairs", test.expectedTrimmed, actual)
-			})
-
 			t.Run("Merged", func(t *testing.T) {
 				actual := serialize.MergeKVs(test.first, test.second)
-				expectEqual(t, "merged key/value pairs", test.expectedMerged, actual)
+				expectEqual(t, "merged key/value pairs", test.expected, actual)
 			})
 		})
 	}
 }
 
-// BenchmarkTrimDuplicates checks performance when TrimDuplicates is called with two slices.
+// BenchmarkMergeKVs checks performance when MergeKVs is called with two slices.
 // In practice that is how the function is used.
-func BenchmarkTrimDuplicates(b *testing.B) {
+func BenchmarkMergeKVs(b *testing.B) {
 	for firstLength := 0; firstLength < 10; firstLength++ {
 		firstA := makeKV("a", firstLength)
 		for secondLength := 0; secondLength < 10; secondLength++ {
@@ -262,7 +222,7 @@ func BenchmarkTrimDuplicates(b *testing.B) {
 				// This is the most common case: all key/value pairs are kept.
 				b.Run("no-duplicates", func(b *testing.B) {
 					expected := append(firstA, secondB...)
-					benchTrimDuplicates(b, expected, firstA, secondB)
+					benchMergeKVs(b, expected, firstA, secondB)
 				})
 
 				// Fairly unlikely...
@@ -272,7 +232,7 @@ func BenchmarkTrimDuplicates(b *testing.B) {
 						expected = firstA[secondLength*2:]
 					}
 					expected = append(expected, secondA...)
-					benchTrimDuplicates(b, expected, firstA, secondA)
+					benchMergeKVs(b, expected, firstA, secondA)
 				})
 
 				// First entry is the same.
@@ -282,7 +242,7 @@ func BenchmarkTrimDuplicates(b *testing.B) {
 					second := []interface{}{"x", 1}
 					second = append(second, secondB...)
 					expected := append(firstA, second...)
-					benchTrimDuplicates(b, expected, first, second)
+					benchMergeKVs(b, expected, first, second)
 				})
 
 				// Last entry is the same.
@@ -292,7 +252,7 @@ func BenchmarkTrimDuplicates(b *testing.B) {
 					second := secondB[:]
 					second = append(second, "x", 1)
 					expected := append(firstA, second...)
-					benchTrimDuplicates(b, expected, first, second)
+					benchMergeKVs(b, expected, first, second)
 				})
 			})
 		}
@@ -310,7 +270,7 @@ func makeKV(prefix string, length int) []interface{} {
 	return kv
 }
 
-func benchTrimDuplicates(b *testing.B, expected []interface{}, first, second []interface{}) {
+func benchMergeKVs(b *testing.B, expected []interface{}, first, second []interface{}) {
 	if len(expected) == 0 {
 		expected = nil
 	}
diff --git a/test/output.go b/test/output.go
index 673e40c..b69286b 100644
--- a/test/output.go
+++ b/test/output.go
@@ -358,13 +358,7 @@ I output.go:<LINE>] "test" firstKey=1 secondKey=3
 		"MarshalLog() that panics": {
 			text:   "marshaler panic",
 			values: []interface{}{"obj", faultyMarshaler{}},
-			expectedOutput: `I output.go:<LINE>] "marshaler panic" obj="<panic: fake MarshalLog panic>"
-`,
-		},
-		"MarshalLog() that returns itself": {
-			text:   "marshaler recursion",
-			values: []interface{}{"obj", recursiveMarshaler{}},
-			expectedOutput: `I output.go:<LINE>] "marshaler recursion" obj={}
+			expectedOutput: `I output.go:<LINE>] "marshaler panic" obj={}
 `,
 		},
 	}
@@ -808,15 +802,6 @@ func (f faultyMarshaler) MarshalLog() interface{} {
 
 var _ logr.Marshaler = faultyMarshaler{}
 
-type recursiveMarshaler struct{}
-
-// MarshalLog returns itself, which could cause the logger to recurse infinitely.
-func (r recursiveMarshaler) MarshalLog() interface{} {
-	return r
-}
-
-var _ logr.Marshaler = recursiveMarshaler{}
-
 type faultyError struct{}
 
 // Error always panics.
diff --git a/test/zapr.go b/test/zapr.go
index 32a82e6..56e60e9 100644
--- a/test/zapr.go
+++ b/test/zapr.go
@@ -161,12 +161,8 @@ I output.go:<LINE>] "odd WithValues" keyWithoutValue="(MISSING)"
 `: `{"caller":"test/output.go:<LINE>","msg":"error panic","errError":"PANIC=fake Error panic"}
 `,
 
-		`I output.go:<LINE>] "marshaler panic" obj="<panic: fake MarshalLog panic>"
+		`I output.go:<LINE>] "marshaler panic" obj={}
 `: `{"caller":"test/output.go:<LINE>","msg":"marshaler panic","v":0,"objError":"PANIC=fake MarshalLog panic"}
-`,
-
-		`I output.go:<LINE>] "marshaler recursion" obj={}
-`: `{"caller":"test/output.go:<LINE>","msg":"marshaler recursion","v":0,"obj":{}}
 `,
 
 		// klog.Info