Repository: corazawaf/coraza
Pull request #647 (2023-02-14 07:19:38)

Remove VariablesCount and use an iterator instead
VariablesCount was more useful previously when variables were stored as an array. Now that we just use fields, the only use we have for it is iterating over variables to print them. This ties variables to the `int` type though and it will be good to decouple that for future flexibility (in a followup PR I'll probably change `Variable` from `byte` to `struct` as a proposal.

Writing out the format function is a lot of lines but it's not much different from our other functions and future work can explore codegen for them, such work will only touch internal code.

codecov[bot]: # [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/647?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) Report
Base: **79.49**% // Head: **78.88**% // Decreases project coverage by **`-0.61%`** :warning:
> Coverage data is based on head [(`74aa71c`)](https://codecov.io/gh/corazawaf/coraza/pull/647?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) compared to base [(`0bae380`)](https://codecov.io/gh/corazawaf/coraza/commit/0bae380e3f9935b5510d4427b9bf7ca955078d8f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf).
> Patch coverage: 100.00% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           v3/dev     #647      +/-   ##
==========================================
- Coverage   79.49%   78.88%   -0.61%     
==========================================
  Files         145      145              
  Lines        7533     7603      +70     
==========================================
+ Hits         5988     5998      +10     
- Misses       1312     1372      +60     
  Partials      233      233              
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `74.58% <100.00%> (-0.63%)` | :arrow_down: |
| examples | `30.35% <26.47%> (-0.30%)` | :arrow_down: |
| ftw | `54.73% <26.47%> (-0.53%)` | :arrow_down: |
| ftw-multiphase | `54.82% <26.47%> (-0.53%)` | :arrow_down: |
| tinygo | `72.68% <100.00%> (-0.73%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/corazawaf/coraza/pull/647?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) | Coverage Δ | |
|---|---|---|
| [internal/corazawaf/transaction.go](https://codecov.io/gh/corazawaf/coraza/pull/647?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvY29yYXphd2FmL3RyYW5zYWN0aW9uLmdv) | `77.04% <100.00%> (-3.48%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/corazawaf/coraza/pull/647?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf).

jcchavezs: As of now, playground iterates over the variables so I think we want to allow that https://github.com/corazawaf/coraza-playground/blob/main/internal/processor.go#L116
anuraaga: Ah ok I'll still remove the variable but add the iterator method in this PR then
anuraaga: @jcchavezs Before continuing I'd like to merge #643 as it will conflict
anuraaga: Nevermind - think this is ready. For reference, see discussion on making custom iterables in Go

https://github.com/golang/go/discussions/56413

Don't know if/when that would land but it doesn't hurt to follow the push pattern presented there
jcchavezs: Just left a comment about docs, otherwise it looks great to me.
jcchavezs: Do you mind also updating playground to use this function?
jptosso: My main concern is how could other implementations could iterate through the variables?

Playground won't be able to list collections anymore
anuraaga: @jptosso I updated the PR title, already fixed that :P

Patch:
diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go
index acb56d5..4c6b36a 100644
--- a/internal/corazawaf/transaction.go
+++ b/internal/corazawaf/transaction.go
@@ -1377,11 +1377,7 @@ func (tx *Transaction) String() string {
 	}
 
 	res.WriteString("\n------------------------ DEBUG ------------------------\n")
-	for v := byte(1); v < types.VariablesCount; v++ {
-		vr := variables.RuleVariable(v)
-		col := tx.Collection(vr)
-		fmt.Fprint(&res, col)
-	}
+	tx.variables.format(&res)
 	return res.String()
 }
 
@@ -1395,17 +1391,35 @@ func (tx *Transaction) generateReqbodyError(err error) {
 
 // TransactionVariables has pointers to all the variables of the transaction
 type TransactionVariables struct {
-	// Single Variables
-	urlencodedError          *collections.Single
-	responseContentType      *collections.Single
-	uniqueID                 *collections.Single
+	args                     *collections.ConcatKeyed
 	argsCombinedSize         *collections.SizeCollection
+	argsGet                  *collections.NamedCollection
+	argsGetNames             collection.Collection
+	argsNames                *collections.ConcatCollection
+	argsPath                 *collections.NamedCollection
+	argsPost                 *collections.NamedCollection
+	argsPostNames            collection.Collection
+	duration                 *collections.Single
+	env                      *collections.Map
+	files                    *collections.Map
 	filesCombinedSize        *collections.Single
+	filesNames               *collections.Map
+	filesSizes               *collections.Map
+	filesTmpContent          *collections.Map
+	filesTmpNames            *collections.Map
 	fullRequestLength        *collections.Single
+	geo                      *collections.Map
+	highestSeverity          *collections.Single
 	inboundDataError         *collections.Single
+	inboundErrorData         *collections.Single
 	matchedVar               *collections.Single
 	matchedVarName           *collections.Single
+	matchedVars              *collections.NamedCollection
+	matchedVarsNames         collection.Collection
 	multipartDataAfter       *collections.Single
+	multipartFilename        *collections.Map
+	multipartName            *collections.Map
+	multipartPartHeaders     *collections.Map
 	outboundDataError        *collections.Single
 	queryString              *collections.Single
 	remoteAddr               *collections.Single
@@ -1413,60 +1427,40 @@ type TransactionVariables struct {
 	remotePort               *collections.Single
 	reqbodyError             *collections.Single
 	reqbodyErrorMsg          *collections.Single
+	reqbodyProcessor         *collections.Single
 	reqbodyProcessorError    *collections.Single
 	reqbodyProcessorErrorMsg *collections.Single
-	reqbodyProcessor         *collections.Single
 	requestBasename          *collections.Single
 	requestBody              *collections.Single
 	requestBodyLength        *collections.Single
+	requestCookies           *collections.NamedCollection
+	requestCookiesNames      collection.Collection
 	requestFilename          *collections.Single
+	requestHeaders           *collections.NamedCollection
+	requestHeadersNames      collection.Collection
 	requestLine              *collections.Single
 	requestMethod            *collections.Single
 	requestProtocol          *collections.Single
 	requestURI               *collections.Single
 	requestURIRaw            *collections.Single
+	requestXML               *collections.Map
 	responseBody             *collections.Single
 	responseContentLength    *collections.Single
+	responseContentType      *collections.Single
+	responseHeaders          *collections.NamedCollection
+	responseHeadersNames     collection.Collection
 	responseProtocol         *collections.Single
 	responseStatus           *collections.Single
+	responseXML              *collections.Map
+	rule                     *collections.Map
 	serverAddr               *collections.Single
 	serverName               *collections.Single
 	serverPort               *collections.Single
-	highestSeverity          *collections.Single
 	statusLine               *collections.Single
-	inboundErrorData         *collections.Single
-	// Custom
-	env                  *collections.Map
-	tx                   *collections.Map
-	rule                 *collections.Map
-	duration             *collections.Single
-	args                 *collections.ConcatKeyed
-	argsGet              *collections.NamedCollection
-	argsGetNames         collection.Collection
-	argsPost             *collections.NamedCollection
-	argsPostNames        collection.Collection
-	argsPath             *collections.NamedCollection
-	argsNames            *collections.ConcatCollection
-	filesTmpNames        *collections.Map
-	geo                  *collections.Map
-	files                *collections.Map
-	requestCookies       *collections.NamedCollection
-	requestCookiesNames  collection.Collection
-	requestHeaders       *collections.NamedCollection
-	responseHeadersNames collection.Collection
-	responseHeaders      *collections.NamedCollection
-	requestHeadersNames  collection.Collection
-	multipartName        *collections.Map
-	multipartFilename    *collections.Map
-	matchedVars          *collections.NamedCollection
-	matchedVarsNames     collection.Collection
-	filesSizes           *collections.Map
-	filesNames           *collections.Map
-	filesTmpContent      *collections.Map
-	xml                  *collections.Map
-	requestXML           *collections.Map
-	responseXML          *collections.Map
-	multipartPartHeaders *collections.Map
+	tx                       *collections.Map
+	uniqueID                 *collections.Single
+	urlencodedError          *collections.Single
+	xml                      *collections.Map
 }
 
 func NewTransactionVariables() *TransactionVariables {
@@ -1839,66 +1833,243 @@ func (v *TransactionVariables) ArgsPostNames() collection.Collection {
 	return v.argsPostNames
 }
 
+// All iterates over the variables. We return both variable and its collection, i.e. key/value, to follow
+// general range iteration in Go which always has a key and value (key is int index for slices). Notably,
+// this is consistent with discussions for custom iterable types in a future language version
+// https://github.com/golang/go/discussions/56413
+func (v *TransactionVariables) All(f func(v variables.RuleVariable, col collection.Collection) bool) {
+	if !f(variables.Args, v.args) {
+		return
+	}
+	if !f(variables.ArgsCombinedSize, v.argsCombinedSize) {
+		return
+	}
+	if !f(variables.ArgsGet, v.argsGet) {
+		return
+	}
+	if !f(variables.ArgsGetNames, v.argsGetNames) {
+		return
+	}
+	if !f(variables.ArgsNames, v.argsNames) {
+		return
+	}
+	if !f(variables.ArgsPath, v.argsPath) {
+		return
+	}
+	if !f(variables.ArgsPost, v.argsPost) {
+		return
+	}
+	if !f(variables.ArgsPostNames, v.argsPostNames) {
+		return
+	}
+	if !f(variables.Duration, v.duration) {
+		return
+	}
+	if !f(variables.Env, v.env) {
+		return
+	}
+	if !f(variables.Files, v.files) {
+		return
+	}
+	if !f(variables.FilesCombinedSize, v.filesCombinedSize) {
+		return
+	}
+	if !f(variables.FilesNames, v.filesNames) {
+		return
+	}
+	if !f(variables.FilesSizes, v.filesSizes) {
+		return
+	}
+	if !f(variables.FilesTmpContent, v.filesTmpContent) {
+		return
+	}
+	if !f(variables.FilesTmpNames, v.filesTmpNames) {
+		return
+	}
+	if !f(variables.FullRequestLength, v.fullRequestLength) {
+		return
+	}
+	if !f(variables.Geo, v.geo) {
+		return
+	}
+	if !f(variables.HighestSeverity, v.highestSeverity) {
+		return
+	}
+	if !f(variables.InboundDataError, v.inboundDataError) {
+		return
+	}
+	if !f(variables.InboundErrorData, v.inboundErrorData) {
+		return
+	}
+	if !f(variables.MatchedVar, v.matchedVar) {
+		return
+	}
+	if !f(variables.MatchedVarName, v.matchedVarName) {
+		return
+	}
+	if !f(variables.MatchedVars, v.matchedVars) {
+		return
+	}
+	if !f(variables.MatchedVarsNames, v.matchedVarsNames) {
+		return
+	}
+	if !f(variables.MultipartDataAfter, v.multipartDataAfter) {
+		return
+	}
+	if !f(variables.MultipartFilename, v.multipartFilename) {
+		return
+	}
+	if !f(variables.MultipartName, v.multipartName) {
+		return
+	}
+	if !f(variables.MultipartPartHeaders, v.multipartPartHeaders) {
+		return
+	}
+	if !f(variables.OutboundDataError, v.outboundDataError) {
+		return
+	}
+	if !f(variables.QueryString, v.queryString) {
+		return
+	}
+	if !f(variables.RemoteAddr, v.remoteAddr) {
+		return
+	}
+	if !f(variables.RemoteHost, v.remoteHost) {
+		return
+	}
+	if !f(variables.RemotePort, v.remotePort) {
+		return
+	}
+	if !f(variables.ReqbodyError, v.reqbodyError) {
+		return
+	}
+	if !f(variables.ReqbodyErrorMsg, v.reqbodyErrorMsg) {
+		return
+	}
+	if !f(variables.ReqbodyProcessor, v.reqbodyProcessor) {
+		return
+	}
+	if !f(variables.ReqbodyProcessorError, v.reqbodyProcessorError) {
+		return
+	}
+	if !f(variables.ReqbodyProcessorErrorMsg, v.reqbodyProcessorErrorMsg) {
+		return
+	}
+	if !f(variables.RequestBasename, v.requestBasename) {
+		return
+	}
+	if !f(variables.RequestBody, v.requestBody) {
+		return
+	}
+	if !f(variables.RequestBodyLength, v.requestBodyLength) {
+		return
+	}
+	if !f(variables.RequestCookies, v.requestCookies) {
+		return
+	}
+	if !f(variables.RequestCookiesNames, v.requestCookiesNames) {
+		return
+	}
+	if !f(variables.RequestFilename, v.requestFilename) {
+		return
+	}
+	if !f(variables.RequestHeaders, v.requestHeaders) {
+		return
+	}
+	if !f(variables.RequestHeadersNames, v.requestHeadersNames) {
+		return
+	}
+	if !f(variables.RequestLine, v.requestLine) {
+		return
+	}
+	if !f(variables.RequestMethod, v.requestMethod) {
+		return
+	}
+	if !f(variables.RequestProtocol, v.requestProtocol) {
+		return
+	}
+	if !f(variables.RequestURI, v.requestURI) {
+		return
+	}
+	if !f(variables.RequestURIRaw, v.requestURIRaw) {
+		return
+	}
+	if !f(variables.RequestXML, v.requestXML) {
+		return
+	}
+	if !f(variables.ResponseBody, v.responseBody) {
+		return
+	}
+	if !f(variables.ResponseContentLength, v.responseContentLength) {
+		return
+	}
+	if !f(variables.ResponseContentType, v.responseContentType) {
+		return
+	}
+	if !f(variables.ResponseHeaders, v.responseHeaders) {
+		return
+	}
+	if !f(variables.ResponseHeadersNames, v.responseHeadersNames) {
+		return
+	}
+	if !f(variables.ResponseProtocol, v.responseProtocol) {
+		return
+	}
+	if !f(variables.ResponseStatus, v.responseStatus) {
+		return
+	}
+	if !f(variables.ResponseXML, v.responseXML) {
+		return
+	}
+	if !f(variables.Rule, v.rule) {
+		return
+	}
+	if !f(variables.ServerAddr, v.serverAddr) {
+		return
+	}
+	if !f(variables.ServerName, v.serverName) {
+		return
+	}
+	if !f(variables.ServerPort, v.serverPort) {
+		return
+	}
+	if !f(variables.StatusLine, v.statusLine) {
+		return
+	}
+	if !f(variables.TX, v.tx) {
+		return
+	}
+	if !f(variables.UniqueID, v.uniqueID) {
+		return
+	}
+	if !f(variables.UrlencodedError, v.urlencodedError) {
+		return
+	}
+	if !f(variables.XML, v.xml) {
+		return
+	}
+}
+
+func (v *TransactionVariables) format(res *strings.Builder) {
+	// TODO(anuraaga): Optimize this, currently each println allocates a string that is then
+	// written to res, we should create a function independent from fmt.Stringer interface
+	// that accepts a res to write to.
+
+	v.All(func(_ variables.RuleVariable, col collection.Collection) bool {
+		fmt.Fprintln(res, col)
+		return true
+	})
+}
+
+type resettable interface {
+	Reset()
+}
+
 func (v *TransactionVariables) reset() {
-	v.urlencodedError.Reset()
-	v.responseContentType.Reset()
-	v.uniqueID.Reset()
-	v.filesCombinedSize.Reset()
-	v.fullRequestLength.Reset()
-	v.inboundDataError.Reset()
-	v.matchedVar.Reset()
-	v.matchedVarName.Reset()
-	v.multipartDataAfter.Reset()
-	v.outboundDataError.Reset()
-	v.queryString.Reset()
-	v.remoteAddr.Reset()
-	v.remoteHost.Reset()
-	v.remotePort.Reset()
-	v.reqbodyError.Reset()
-	v.reqbodyErrorMsg.Reset()
-	v.reqbodyProcessorError.Reset()
-	v.reqbodyProcessorErrorMsg.Reset()
-	v.reqbodyProcessor.Reset()
-	v.requestBasename.Reset()
-	v.requestBody.Reset()
-	v.requestBodyLength.Reset()
-	v.requestFilename.Reset()
-	v.requestLine.Reset()
-	v.requestMethod.Reset()
-	v.requestProtocol.Reset()
-	v.requestURI.Reset()
-	v.requestURIRaw.Reset()
-	v.responseBody.Reset()
-	v.responseContentLength.Reset()
-	v.responseProtocol.Reset()
-	v.responseStatus.Reset()
-	v.serverAddr.Reset()
-	v.serverName.Reset()
-	v.serverPort.Reset()
-	v.highestSeverity.Reset()
-	v.statusLine.Reset()
-	v.inboundErrorData.Reset()
-	v.env.Reset()
-	v.tx.Reset()
-	v.rule.Reset()
-	v.duration.Reset()
-	v.argsGet.Reset()
-	v.argsPost.Reset()
-	v.argsPath.Reset()
-	v.filesTmpNames.Reset()
-	v.geo.Reset()
-	v.files.Reset()
-	v.requestCookies.Reset()
-	v.requestHeaders.Reset()
-	v.responseHeaders.Reset()
-	v.multipartName.Reset()
-	v.multipartFilename.Reset()
-	v.matchedVars.Reset()
-	v.filesSizes.Reset()
-	v.filesNames.Reset()
-	v.filesTmpContent.Reset()
-	v.xml.Reset()
-	v.requestXML.Reset()
-	v.responseXML.Reset()
-	v.multipartPartHeaders.Reset()
+	v.All(func(_ variables.RuleVariable, col collection.Collection) bool {
+		if r, ok := col.(resettable); ok {
+			r.Reset()
+		}
+		return true
+	})
 }
diff --git a/internal/variables/generator/main.go b/internal/variables/generator/main.go
index 18f8c8a..07fb0d7 100644
--- a/internal/variables/generator/main.go
+++ b/internal/variables/generator/main.go
@@ -6,7 +6,6 @@ package main
 import (
 	"bytes"
 	_ "embed"
-	"fmt"
 	"go/ast"
 	"go/format"
 	"go/importer"
@@ -23,9 +22,6 @@ import (
 //go:embed variablesmap.go.tmpl
 var variablesMapTmpl string
 
-//go:embed variablescount.go.tmpl
-var variablesCountTmpl string
-
 type VariablesMap struct {
 	Key   string
 	Value string
@@ -58,7 +54,7 @@ func main() {
 		log.Fatal(err) // type error
 	}
 
-	directives := []VariablesMap{}
+	var directives []VariablesMap
 	ast.Inspect(f, func(n ast.Node) bool {
 		switch decl := n.(type) {
 		case *ast.GenDecl:
@@ -116,26 +112,6 @@ func main() {
 	if err != nil {
 		log.Fatal(err)
 	}
-
-	vc, err := os.Create("./variablescount.gen.go")
-	if err != nil {
-		log.Fatal(err)
-	}
-
-	_, err = vc.WriteString(strings.Replace(
-		variablesCountTmpl,
-		"{{ . }}",
-		fmt.Sprintf("%d", len(directives)),
-		1,
-	))
-	if err != nil {
-		log.Fatal(err)
-	}
-
-	err = vc.Close()
-	if err != nil {
-		log.Fatal(err)
-	}
 }
 
 var (
diff --git a/internal/variables/generator/variablescount.go.tmpl b/internal/variables/generator/variablescount.go.tmpl
deleted file mode 100644
index c2668f7..0000000
--- a/internal/variables/generator/variablescount.go.tmpl
+++ /dev/null
@@ -1,10 +0,0 @@
-// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors
-// SPDX-License-Identifier: Apache-2.0
-
-// Code generated by types/variables/generator DO NOT EDIT.
-
-package variables
-
-// VariablesCount contains the number of variables handled by the variables package
-// It is used to create arrays of the correct size
-const VariablesCount = {{ . }}
diff --git a/internal/variables/variablescount.gen.go b/internal/variables/variablescount.gen.go
deleted file mode 100644
index e1a64b5..0000000
--- a/internal/variables/variablescount.gen.go
+++ /dev/null
@@ -1,10 +0,0 @@
-// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors
-// SPDX-License-Identifier: Apache-2.0
-
-// Code generated by types/variables/generator DO NOT EDIT.
-
-package variables
-
-// VariablesCount contains the number of variables handled by the variables package
-// It is used to create arrays of the correct size
-const VariablesCount = 92
diff --git a/rules/transaction.go b/rules/transaction.go
index 2273e7f..0b41c8e 100644
--- a/rules/transaction.go
+++ b/rules/transaction.go
@@ -45,6 +45,10 @@ type TransactionState interface {
 
 // TransactionVariables has pointers to all the variables of the transaction
 type TransactionVariables interface {
+	// All iterates over all the variables in this TransactionVariables, invoking f for each.
+	// Results are passed in no defined order. If f returns false, iteration stops.
+	All(f func(v variables.RuleVariable, col collection.Collection) bool)
+
 	// Simple Variables
 	UrlencodedError() collection.Single
 	ResponseContentType() collection.Single
diff --git a/types/variablescount.go b/types/variablescount.go
deleted file mode 100644
index 4782e1c..0000000
--- a/types/variablescount.go
+++ /dev/null
@@ -1,12 +0,0 @@
-// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors
-// SPDX-License-Identifier: Apache-2.0
-
-package types
-
-import (
-	"github.com/corazawaf/coraza/v3/internal/variables"
-)
-
-// VariablesCount contains the number of variables handled by the variables package
-// It is used to create arrays of the correct size
-const VariablesCount = variables.VariablesCount


Test patch:
diff --git a/internal/corazawaf/transaction_test.go b/internal/corazawaf/transaction_test.go
index fbe93fc..d013a3b 100644
--- a/internal/corazawaf/transaction_test.go
+++ b/internal/corazawaf/transaction_test.go
@@ -1247,3 +1247,38 @@ func TestProcessorsIdempotency(t *testing.T) {
 		})
 	}
 }
+
+func TestIterationStops(t *testing.T) {
+	// This is a valid test of iteration mechanics but is really overkill. We mostly do it for
+	// code coverage.
+
+	waf := NewWAF()
+	tx := waf.NewTransaction()
+
+	// Order doesn't matter, iterate once without stopping to know the order
+	var allVars []variables.RuleVariable
+	tx.Variables().All(func(v variables.RuleVariable, _ collection.Collection) bool {
+		allVars = append(allVars, v)
+		return true
+	})
+
+	for i, stopV := range allVars {
+		t.Run(stopV.Name(), func(t *testing.T) {
+			var haveVars []variables.RuleVariable
+			tx.Variables().All(func(v variables.RuleVariable, _ collection.Collection) bool {
+				haveVars = append(haveVars, v)
+				return v != stopV
+			})
+
+			if want, have := i+1, len(haveVars); want != have {
+				t.Errorf("stopped with unexpected number of variables, want %d, have %d", want, have)
+			}
+
+			for j, v := range haveVars {
+				if want, have := allVars[j], v; want != have {
+					t.Errorf("unexpected variable at index %d, want %s, have %s", j, want.Name(), have.Name())
+				}
+			}
+		})
+	}
+}