spec: allow range-over-func to omit iteration variables

[rsc]

In discussion during the implementation of #61405 we changed range-over-func to require mentioning iteration variables. The idea is that if we do end up with idioms like:

for line, err := range FileLines("/etc/passwd") {
	...
}

Then we want to diagnose:

for line := range FileLines("/etc/passwd") {
	...
}

as an error. However, this is inconsistent with other range loops and also not what the #61405 text said. Probably we should change it. Starting a separate proposal for that discussion.

[earthboundkid]

What if it's just a vet check that looks for errors not silenced with , _? Then you could still write for key := range mapish.All().

[timothy-king]

I don't think I understand what is being proposed. Doesn't the spec text of #61405 already "allow range-over-func to omit iteration variables"?

The example :

For example if f has type func(yield func(T1, T2) bool) bool any of these are valid:

for x, y := range f { ... }
for x, _ := range f { ... }
for _, y := range f { ... }
for x := range f { ... }
for range f { ... }

The proposed text:

As with range over other types, it is permitted to declare fewer iteration variables
than there are iteration values.

Anyways I am confused about what is being proposed here.

for line, err := range FileLines("/etc/passwd") {

If we think there is a subset of functions like FileSet that need to not ignore one of the values in RangeStmts, this seems like it could fits in well with the unusedresult checker in vet. I am not sure it makes sense to enforce usage at the language level.

[jimmyfrasche]

I don't see how this buys anything other than ceremony.

[jba]

We know that the design of bufio.Scanner, which requires a call to the Err method after iteration, is flawed. We have empirical evidence that people forget the call.

We can guess that range-over-func will become the dominant way to iterate over all sorts of sequences where failure is possible: file lines, DB rows, RPCs that list resources, and so on.

I'm not saying that we should require the iteration variables, but I am saying that if we don't, we need a good story for how to avoid introducing many bugs into Go code. Like a vet check for when the second return type is error that is enabled for testing.

[meling]

I’m in favor of diagnosing it as a compile error to not handle the error or to explicitly ignore it via _, even if this is a special case where the second argument is an error.

[jimmyfrasche]

I agree that not handling the error is a real problem. I do not think that iterators yielding errors solves that problem very well, especially if that means having to immediately buttress that in a way that forces everyone to use range-over-func differently than native iterators. Even if I'm wrong and yielding errors is a boon, then static analysis is more than capable of linting it only in the case when an error is iterated over.

@jba here's https://pkg.go.dev/bufio#example-Scanner.Bytes rewritten to use a hypothetical iterator that returns ([]byte, error): https://go.dev/play/p/G-Wv80AbfqF I don't think having the error handling in the loop is an improvement. One of the alternatives I saw posted was for the iterator factory to take a *error. I didn't much like the idea initially but it has grown on me: https://go.dev/play/p/DdqILPDt9B7

[meling]

The only benefit of passing an *error to the iterator as mentioned @jimmyfrasche is that you must declare and pass an error to the iterator, but you can still forget to check it after the loop. More problematic though is that the pattern prevents you from logging/handling individual errors.

I see a few possible patterns related to errors:

for line, err := range FileLines("/etc/passwd") {
    if err != nil {
        // log or handle error
	break
    }
    ...
}

for line, err := range FileLines("/etc/passwd") {
    if err != nil {
	return err
    }
    ...
}

Both of these are IMO better solutions than handling the error after the loop as with the bufio.Scanner example, as the error handling happens immediately after the call producing the error.

The other possible pattern (if-err-continue) is:

for line, err := range FileLines("/etc/passwd") {
    if err != nil {
        // ignore errors
	continue
    }
    ...
}

If the intent of the developer is to ignore errors, then the above could instead be written as (same semantic meaning):

for line, _ := range FileLines("/etc/passwd") {
    ...
}

The if-err-continue pattern above could be detected by a tool (gopls) and a rewrite to the simple form could be suggested (similar to some other patterns).

However, I would advice against allowing the following pattern if the iterator returns an error, because the _ is a more explicit signal that ignoring the error is intentional:

for line := range FileLines("/etc/passwd") {
    ...
}

If you want to handle/log individual errors and continue, you would still need to use this pattern:

for line, err := range FileLines("/etc/passwd") {
    if err != nil {
        // handle or log error
	continue
    }
    ...
}

Handling and logging individual errors is not possible with the handling after the loop approach.

[earthboundkid]

We have empirical evidence that people forget the call.

Looks like they finally fixed it, but for months the marketing page for Copilot showed code with a missing call to .Err().

[jimmyfrasche]

@meling To clarify:

I fully support yielding (T, error)—when the error is per iteration.

I oppose an error that signals an abnormal end to (or failure to start) the iteration being handled within the loop.

In other words, I think if you can continue on err != nil it's fine, but if you MUST break/return it's not. (Presumably in the latter case the code would still accidentally work correctly if you continued as the iteration would stop but that's not a great argument in the pattern's favor).

[seh]

What about an iterator that's using a context.Context internally to govern its fetching of additional values? If the Context is done, do you think that the iterator should yield the result of Context.Err? If so, should it respect that call to yield returning true and attempt to continue, or should it ignore yield's return value?

I've been writing such an iterator, and I've changed my mind about the answers to these questions several times now.

[AndrewHarrisSPU]

What about an iterator that's using a context.Context internally to govern its fetching of additional values?

This is a really interesting question - to me a practically useful solution has been to delay passing context.Context far enough that it isn't deeply internal. In other words, don't pass around an iterator with an internal context, pass around a func(context.Context) -> iter.Seq and a context.Context as far as possible, so that the base context.Context is in scope where it should be checked. If a computation using iterated values needs to branch, the context should be in scope.