feat: Support `iter.Seq`s in `[Not]Contains` and `[Not]ElementsMatch` by misberner · Pull Request #1685 · stretchr/testify

misberner · 2024-12-12T11:33:51Z

Summary

This PR adds support iter.Seq[...] sequences (introduced in Go 1.23, or earlier via rangefunc experiment) in the Contains and ElementsMatch (+ derived) asserters.

Changes

Introduced a seqToSlice helper that transparently converts an iter.Seq (which is a function of signature func(func(T) bool)) to a corresponding slice of type []T, if support for sequences is enabled. Otherwise, it does nothing. Note: this function is reflection-based and does not introduce any source dependencies on newer language features, thereby respecting the Go version constraint in the go.mod file.
Used the seqToSlice helper in the above-mentioned asserters to contain any inputs from sequence to slice form (this has to happen early on because of the empty check)
Added tests (conditional on the support for sequences) for these matchers on sequences

Motivation

Sequences are a new language feature that as of go1.23 has become standard for obtaining sequences of values w/o requiring to materialize the values into a slice (e.g., maps.Keys(...), maps.Values(...)). Currently, using these sequences in matchers requires materialization via slices.Collect. However, since ElementsMatch and Contains are conceptually applicable to sequences in addition to slices, arrays etc., this should be handled by the testify framework.

Example usage

requires go1.23

import "slices"
import "maps"
...
assert.ElementsMatch(slices.Values([]string{"hello", "world"}), []string{"world", "hello"})
assert.NotContains(maps.Values(map[int]string{1: "foo", 2: "bar"}), "baz")

## Related issues
N/A

assert/seq_supported.go

dolmen

We don't need to guard against Go 1.23. Iterator functions can be implemented in any version of Go, not just the ones with the iter package or the for loops syntactic sugar.

dolmen · 2025-01-07T09:28:54Z

assert/seq_supported.go

+
+	yieldFunc := reflect.MakeFunc(paramType, func(args []reflect.Value) []reflect.Value {
+		result = reflect.Append(result, args[0])
+		return []reflect.Value{reflect.ValueOf(true)}


Allocate []reflect.Value{reflect.ValueOf(true)} out of the yieldFunc to allow reuse.

Sure, will do.

dolmen · 2025-01-07T09:42:20Z

assert/seq_supported.go

+
+	elemType := paramType.In(0)
+	resultType := reflect.SliceOf(elemType)
+	result := reflect.MakeSlice(resultType, 0, 0)


It is not necessary to allocate a zero element slice, as it will be discarded on the first call to append.
Instead a nil slice is just enough:

Suggested change

result := reflect.MakeSlice(resultType, 0, 0)

result := reflect.New(resultType).Elem()

See https://go.dev/play/p/_9VZP__CIS8

Indeed not necessary, thanks for pointing this out. Though I'll go with this to save a new:

Suggested change

result := reflect.MakeSlice(resultType, 0, 0)

result := reflect.Zero(resultType)

dolmen · 2025-01-07T09:48:20Z

assert/seq_supported.go

+	})
+
+	// Call the function with the yield function as the argument
+	xv.Call([]reflect.Value{yieldFunc})


This extracts only the first element of the sequence. From the description of the function I expect the whole sequence to be serialized into the slice using a loop.

This is incorrect, as otherwise obviously tests wouldn't be passing. The iteration itself happens in the function wrapped in xv. A for loop ranging over this function is syntactic sugar/magic for treating this single call as a coroutine, switching control between the function body and the loop head/body. If we call this function w/o the for/coroutine magic, it will just do a full iteration, calling yieldFunc for every element.

dolmen · 2025-01-07T09:51:13Z

assert/seq_supported.go

@@ -0,0 +1,44 @@
+//go:build go1.23 || goexperiment.rangefunc


This compile guard doesn't seem necessary. There is nothing in seqToSlice implementation that requires Go 1.23. Sequences functions can be implemented in Go below 1.23. Go 1.23 only adds syntactic sugar in for loops.

Fair. My thinking was that w/o range over func, there is no such thing as a "sequence" in Go and thus no basis for semantically treating a function of that form as a sequence (I am not aware of any use of the yield func pattern in Go outside of/before range-over-func). But I agree this is pedantry, so fine to change it in the name of simplification.

dolmen · 2025-01-07T10:05:25Z

assert/assertions_seq_test.go

@@ -0,0 +1,179 @@
+//go:build go1.23 || goexperiment.rangefunc


I don't see a reason for this guard (see my other comment in seq_supported.go).

dolmen · 2025-01-07T10:25:24Z

⚠️ This implementation in its current state is not appropriate for Contains: Contains with iterators should be implemented in a lazy way and not consume the whole sequence before the checks.

misberner · 2025-01-08T12:35:34Z

Thanks for your review.

⚠️ This implementation in its current state is not appropriate for Contains: Contains with iterators should be implemented in a lazy way and not consume the whole sequence before the checks.

Could you please elaborate on why? Note that sequences aren't iterators and that sequences may be such that they can only be consumed once, but an iter.Seq is not inherently stateful. Also note that the existing base implementation first checks for emptiness very early on. Since sequences cannot be checked for emptiness other than by retrieving the first element, some iteration has to take place early on, and thus it's impossible to guarantee anything about the sequences post-state in the general case.

However, I believe this is anyway not a realistic ask because of how sequences are implemented. Without range-over-func and the associated coroutine magic (see other comment), there just isn't a way to control the iteration from the caller side (you can abort it by returning false from the yield function but that isn't desirable). Lazily iterating over the sequence requires iter.Pull, which regardless of file-level build constraints isn't available w/o bumping the Go version in go.mod, which I believe isn't desirable either. Well - one could do controlled iterations by using Goroutines and channels instead of coroutines, but to me that hardly seems justified, complexity-wise.

LMK your thoughts.

dolmen · 2025-01-08T13:12:42Z

Note that sequences aren't iterators and that sequences may be such that they can only be consumed once, but an iter.Seq is not inherently stateful.

Sequences may also be almost infinite. For example, a sequence could provide even int64 starting from a given value.

func EvenInc(start int64) func(yield func(int64) bool) {
	if start%1 == 1 {
		start++
	}
	return func(yield func(int64) bool) {
		for {
			if !yield(start) {
				break
			}
			start += 2
		}
	}
}

Full code: https://go.dev/play/p/19ADbf5mf8g

Limiting the set of sequences usable with Contains to the finite sequences would be artificial and would be against the spirit of iterators. Why should the following call be disallowed?

assert.Contains(t, EvenInc(0), int64(42), "42 is in the set of even numbers starting from zero")

dolmen · 2025-01-08T13:17:37Z

Since sequences cannot be checked for emptiness other than by retrieving the first element, some iteration has to take place early on, and thus it's impossible to guarantee anything about the sequences post-state in the general case.

So we should either implement handling of sequences differently to not check for emptyness that early, or just not add support for sequences.

misberner · 2025-01-08T13:30:57Z

Why should the following call be disallowed?

Not saying an implementation should actively disallow it but I think there's a pretty obvious reason for not supporting it: because for the general case of infinite sequences, a Contains check cannot fail, it can only cause non-termination. Consider the call

assert.NotContains(t, EvenInc(0), int64(37), "37 is an odd number and thus not in the set of even numbers starting from zero")

This obviously won't work, and fundamentally cannot be made to work. Hence it seems a very odd case to optimize for, especially if that optimization requires a major rework/limited reuse of existing code.

MichaelMure · 2025-12-16T00:05:11Z

It would be nice to have this.

Originally from upstream: stretchr#1685 NOTE: did not add SeqElementsMatch as the origin PR suggested. Contains is good enough. Signed-off-by: Frederic BIDON <fredbi@yahoo.com>

Support iter.Seqs in [Not]Contains and [Not]ElementsMatch

7f96d99

ccoVeille reviewed Dec 12, 2024

View reviewed changes

assert/seq_supported.go Show resolved Hide resolved

dolmen requested changes Jan 7, 2025

View reviewed changes

dolmen added enhancement on hold generics labels Jan 7, 2025

misberner requested a review from dolmen January 8, 2025 12:37

	result := reflect.MakeSlice(resultType, 0, 0)
	result := reflect.New(resultType).Elem()

	result := reflect.MakeSlice(resultType, 0, 0)
	result := reflect.Zero(resultType)

		@@ -0,0 +1,179 @@
		//go:build go1.23 \|\| goexperiment.rangefunc

Conversation

misberner commented Dec 12, 2024

Summary

Changes

Motivation

Example usage

Uh oh!

Uh oh!

dolmen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

misberner Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dolmen commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

misberner commented Jan 8, 2025

Uh oh!

dolmen commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dolmen commented Jan 8, 2025

Uh oh!

misberner commented Jan 8, 2025

Uh oh!

MichaelMure commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

misberner Jan 8, 2025 •

edited

Loading

dolmen commented Jan 7, 2025 •

edited

Loading

dolmen commented Jan 8, 2025 •

edited

Loading